On Mon, 16 Jan 2012, Richard Guenther wrote:
> On Mon, 16 Jan 2012, Jakub Jelinek wrote:
>
> > On Mon, Jan 16, 2012 at 09:35:08AM +0100, Richard Guenther wrote:
> > > But that's of course intended. Attributes or redirection on the
> > > extern inline variant are completely meaningless.
> > >
> > > > If you want to keep olddecl as is, then IMHO we should add a new bool
> > > > argument to merge_decls and if the flag is set, make sure we only ever
> > > > modify newdecl and not olddecl.
> > >
> > > I don't think that is necesary nor warranted. What legitimate use
> > > would break you think?
> >
> > The extern inline decl gets also all the previous decl attributes
> > accumulated. Asm redirection on the extern inline is meaningless.
> > Consider:
> > /* foo.h */
> > extern int foo (int, int) __attribute__((regparm (2))) __asm ("bar");
> > ...
> > /* foo-inlines.h */
> > extern inline __attribute__((gnu_inline, always_inline)) void
> > foo (int x, int y)
> > {
> > return something (x, y);
> > }
> > ...
> > /* foo.c */
> > int
> > foo (int x, int y)
> > {
> > return something (x, y);
> > }
> >
> > You really want to have __asm ("bar") on the foo out-of-line definition,
> > wouldn't surprise me if this broke glibc or other packages that heavily
> > use asm redirection (in glibc e.g. LFS). And the regparm attribute
> > is needed too. The gnu_inline attribute isn't needed, but can be safely
> > copied over and ignored, the always_inline attribute and maybe artificial
> > attributes are the only ones that should be not copied over to the
> > out-of-line definition.
>
> Ok, I suppose that requires even more adjustments of the scope/lookup
> stuff for extern inlines when we see the redefinition - thus, lookup
> a previous decl and merge with that instead of doing no merging.
> We don't seem to have a testcase for this at least. I'll add yours
> as follows (works with 4.6, fails with my patch)
>
> extern int foo (int, int) __asm__ ("bar");
> extern inline __attribute__((gnu_inline, always_inline)) int
> foo (int x, int y)
> {
> return 0;
> }
> int
> foo (int x, int y)
> {
> return 0;
> }
> int main()
> {
> return bar ();
> }
>
> Before I spend too much time on this age-old bug - Joseph, can you
> agree with the proposed partial fix sofar and then with looking
> through the I_SYMBOL_BINDING chain to skip the extern inline decl
> to find the decl to merge with?
The following makes all testcases collected sofar pass.
Richard.
2012-01-13 Richard Guenther <rguent...@suse.de>
PR c/33763
* c-decl.c (redeclared_extern_inline_fn_p): New function.
(duplicate_decls): Do not merge re-declared extern
inline function decls with their re-declaration. Do not
merge extern inline functions with any declarations either.
(pushdecl): Do not put extern inlines into external_scope.
* gcc.dg/torture/pr33763-1.c: New testcase.
* gcc.dg/torture/pr33763-2.c: Likewise.
* gcc.dg/torture/pr33763-3.c: Likewise.
* gcc.dg/torture/pr33763-4.c: Likewise.
Index: gcc/testsuite/gcc.dg/torture/pr33763-1.c
===================================================================
*** gcc/testsuite/gcc.dg/torture/pr33763-1.c (revision 0)
--- gcc/testsuite/gcc.dg/torture/pr33763-1.c (revision 0)
***************
*** 0 ****
--- 1,43 ----
+ /* { dg-do run } */
+
+ extern void abort (void);
+
+ extern __inline __attribute__ ((__always_inline__))
+ int foo ()
+ {
+ return 1;
+ }
+ int test1 ()
+ {
+ /* Name-lookup should find foo that returns 1. */
+ return foo ();
+ }
+ int foo ()
+ {
+ return 0;
+ }
+
+ extern __inline __attribute__ ((__always_inline__))
+ int bar ()
+ {
+ return 1;
+ }
+ int bar ()
+ {
+ return 0;
+ }
+ int test2 ()
+ {
+ /* Name-lookup should find bar that returns 0. */
+ return bar ();
+ }
+
+ int
+ main()
+ {
+ if (test1 () != 1)
+ abort ();
+ if (test2 () != 0)
+ abort ();
+ return 0;
+ }
Index: gcc/testsuite/gcc.dg/torture/pr33763-2.c
===================================================================
*** gcc/testsuite/gcc.dg/torture/pr33763-2.c (revision 0)
--- gcc/testsuite/gcc.dg/torture/pr33763-2.c (revision 0)
***************
*** 0 ****
--- 1,30 ----
+ /* { dg-do compile } */
+
+ extern int foo (const char *, int);
+ extern int baz (const char *, int);
+
+ extern inline __attribute__ ((always_inline, gnu_inline)) int
+ baz (const char *x, int y)
+ {
+ return 2;
+ }
+
+ int
+ baz (const char *x, int y)
+ {
+ return 1;
+ }
+
+ int xa, xb;
+
+ static int
+ inl (const char *x, int y)
+ {
+ return baz (x, y);
+ }
+
+ int
+ foo (const char *x, int y)
+ {
+ return inl (x, y);
+ }
Index: gcc/testsuite/gcc.dg/torture/pr33763-3.c
===================================================================
*** gcc/testsuite/gcc.dg/torture/pr33763-3.c (revision 0)
--- gcc/testsuite/gcc.dg/torture/pr33763-3.c (revision 0)
***************
*** 0 ****
--- 1,58 ----
+ /* { dg-do compile } */
+
+ typedef struct
+ {
+ void *a;
+ void *b;
+ } T;
+ extern void *foo (const char *, const char *);
+ extern void *bar (void *, const char *, T);
+ extern int baz (const char *, int);
+
+ extern inline __attribute__ ((always_inline, gnu_inline)) int
+ baz (const char *x, int y)
+ {
+ return 2;
+ }
+
+ int
+ baz (const char *x, int y)
+ {
+ return 1;
+ }
+
+ int xa, xb;
+
+ static void *
+ inl (const char *x, const char *y)
+ {
+ T t = { &xa, &xb };
+ int *f = (int *) __builtin_malloc (sizeof (int));
+ const char *z;
+ int o = 0;
+ void *r = 0;
+
+ for (z = y; *z; z++)
+ {
+ if (*z == 'r')
+ o |= 1;
+ if (*z == 'w')
+ o |= 2;
+ }
+ if (o == 1)
+ *f = baz (x, 0);
+ if (o == 2)
+ *f = baz (x, 1);
+ if (o == 3)
+ *f = baz (x, 2);
+
+ if (o && *f > 0)
+ r = bar (f, "w", t);
+ return r;
+ }
+
+ void *
+ foo (const char *x, const char *y)
+ {
+ return inl (x, y);
+ }
Index: gcc/testsuite/gcc.dg/torture/pr33763-4.c
===================================================================
*** gcc/testsuite/gcc.dg/torture/pr33763-4.c (revision 0)
--- gcc/testsuite/gcc.dg/torture/pr33763-4.c (revision 0)
***************
*** 0 ****
--- 1,17 ----
+ /* { dg-do run } */
+
+ extern int foo (int, int) __asm__ ("bar");
+ extern inline __attribute__((gnu_inline, always_inline)) int
+ foo (int x, int y)
+ {
+ return 0;
+ }
+ int
+ foo (int x, int y)
+ {
+ return 0;
+ }
+ int main()
+ {
+ return bar ();
+ }
Index: gcc/c-decl.c
===================================================================
*** gcc/c-decl.c (revision 183205)
--- gcc/c-decl.c (working copy)
*************** locate_old_decl (tree decl)
*** 1627,1632 ****
--- 1627,1652 ----
inform (input_location, "previous declaration of %q+D was here", decl);
}
+ #define DECL_EXTERN_INLINE(DECL) (DECL_DECLARED_INLINE_P (DECL) \
+ && DECL_EXTERNAL (DECL))
+
+ /* Return whether NEWDECL is a redeclaration of an extern inline declared
+ function OLDDECL. */
+
+ static bool
+ redeclared_extern_inline_fn_p (tree olddecl, tree newdecl)
+ {
+ return !(!DECL_EXTERN_INLINE (olddecl)
+ || DECL_EXTERN_INLINE (newdecl)
+ || (!flag_gnu89_inline
+ && (!DECL_DECLARED_INLINE_P (olddecl)
+ || !lookup_attribute ("gnu_inline",
+ DECL_ATTRIBUTES (olddecl)))
+ && (!DECL_DECLARED_INLINE_P (newdecl)
+ || !lookup_attribute ("gnu_inline",
+ DECL_ATTRIBUTES (newdecl)))));
+ }
+
/* Subroutine of duplicate_decls. Compare NEWDECL to OLDDECL.
Returns true if the caller should proceed to merge the two, false
if OLDDECL should simply be discarded. As a side effect, issues
*************** diagnose_mismatched_decls (tree newdecl,
*** 1644,1652 ****
bool warned = false;
bool retval = true;
- #define DECL_EXTERN_INLINE(DECL) (DECL_DECLARED_INLINE_P (DECL) \
- && DECL_EXTERNAL (DECL))
-
/* If we have error_mark_node for either decl or type, just discard
the previous decl - we're in an error cascade already. */
if (olddecl == error_mark_node || newdecl == error_mark_node)
--- 1664,1669 ----
*************** diagnose_mismatched_decls (tree newdecl,
*** 1858,1873 ****
isn't overriding an extern inline reject the new decl.
In c99, no overriding is allowed in the same translation
unit. */
! if ((!DECL_EXTERN_INLINE (olddecl)
! || DECL_EXTERN_INLINE (newdecl)
! || (!flag_gnu89_inline
! && (!DECL_DECLARED_INLINE_P (olddecl)
! || !lookup_attribute ("gnu_inline",
! DECL_ATTRIBUTES (olddecl)))
! && (!DECL_DECLARED_INLINE_P (newdecl)
! || !lookup_attribute ("gnu_inline",
! DECL_ATTRIBUTES (newdecl))))
! )
&& same_translation_unit_p (newdecl, olddecl))
{
error ("redefinition of %q+D", newdecl);
--- 1875,1881 ----
isn't overriding an extern inline reject the new decl.
In c99, no overriding is allowed in the same translation
unit. */
! if (!redeclared_extern_inline_fn_p (olddecl, newdecl)
&& same_translation_unit_p (newdecl, olddecl))
{
error ("redefinition of %q+D", newdecl);
*************** diagnose_mismatched_decls (tree newdecl,
*** 2132,2142 ****
if (warned || pedwarned)
locate_old_decl (olddecl);
- #undef DECL_EXTERN_INLINE
-
return retval;
}
/* Subroutine of duplicate_decls. NEWDECL has been found to be
consistent with OLDDECL, but carries new information. Merge the
new information into OLDDECL. This function issues no
--- 2140,2150 ----
if (warned || pedwarned)
locate_old_decl (olddecl);
return retval;
}
+ #undef DECL_EXTERN_INLINE
+
/* Subroutine of duplicate_decls. NEWDECL has been found to be
consistent with OLDDECL, but carries new information. Merge the
new information into OLDDECL. This function issues no
*************** duplicate_decls (tree newdecl, tree oldd
*** 2512,2517 ****
--- 2520,2544 ----
return false;
}
+ if (TREE_CODE (newdecl) == FUNCTION_DECL)
+ {
+ /* If we have a redeclared extern inline function simply drop olddecl
+ on the floor instead of merging it with newdecl. */
+ if (DECL_INITIAL (newdecl)
+ && DECL_INITIAL (olddecl)
+ && redeclared_extern_inline_fn_p (olddecl, newdecl))
+ return false;
+
+ /* Do not merge an extern inline decl with any other decl either. */
+ if ((DECL_DECLARED_INLINE_P (newdecl)
+ && DECL_EXTERNAL (newdecl))
+ || (!flag_gnu89_inline
+ && (DECL_DECLARED_INLINE_P (newdecl)
+ || lookup_attribute ("gnu_inline",
+ DECL_ATTRIBUTES (newdecl)))))
+ return false;
+ }
+
merge_decls (newdecl, olddecl, newtype, oldtype);
return true;
}
*************** pushdecl (tree x)
*** 2775,2781 ****
nested = true;
x = visdecl;
}
! else
{
bind (name, x, external_scope, /*invisible=*/true,
/*nested=*/false, locus);
--- 2802,2810 ----
nested = true;
x = visdecl;
}
! else if (TREE_CODE (x) != FUNCTION_DECL
! || !(DECL_DECLARED_INLINE_P (x)
! && DECL_EXTERNAL (x)))
{
bind (name, x, external_scope, /*invisible=*/true,
/*nested=*/false, locus);
