/* { dg-do compile } */ /* { dg-options "-O2" } */ 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); } issues bogus sorry diagnostics, while simpler testcases are handled just fine: 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); } When a gnu_inline (or extern inline gnu89) function has a real is redefined, we don't want any inlining for that function even if it is always_inline, at least GCC always behaved that way and even on the simpler testcase behaves that way. Even the original testcase works just fine with GCC 3.2.x, so this is a regression. On the trunk it is enough to e.g. remove one of the 3 baz calls in inl and it compiles just fine (none of the baz calls is inlined as expected), but e.g. on 4.1 branch the compilation still fails. arts hits this problem when building against recent glibcs.
Confirmed.
This is result of ugly hack handling redefined extern inline functions. The most sane semantics (not what 3.2.x did) would be that function if inlined should get the extern inline body, but the body from redefined non-extern inline function should be output off-line and used when called. Cgraph code can deal with this via cgraph->global->inline_decl. The problem is that frontend currently behave in a way parsing extern inline body and finalizing it in a cgraph, but before unit is finished and cgraph has chance to use it, the redefinition overwrite the body. If frontend was updated to produce 2 separate declarations, one for the extern inline body, one for the offline body and set the inline_decl of the second declaration to point to first, things should just work. There is one extra problem to this - extern inline functions are declared as public and handled so by quite large part of compiler. This means that when doing --combine (or future LTO), we end up having multiple copies of global extern inline function and we end up not inlining them at all too. I think frontend should be told to make those functions static (or we need to update rest of fronend code to special case this) Honza
This patch should disable the sorry message in this case, but I would argue that outputting sorry here is right - we fail to inline alway_inline function because of implementation limitation. So I would claim 3.2.0 being buggy rather than mainline. Honza Index: tree-inline.c =================================================================== *** tree-inline.c (revision 129072) --- tree-inline.c (working copy) *************** expand_call_inline (basic_block bb, tree *** 2560,2565 **** --- 2560,2571 ---- if (!cgraph_inline_p (cg_edge, &reason)) { if (lookup_attribute ("always_inline", DECL_ATTRIBUTES (fn)) + /* For extern inline functions that get redefined we always + silently ignored alway_inline flag. Better behaviour would + be to be able to keep both bodies and use extern inline body + for inlining, but we can't do that because frontends overwrite + the body. */ + && !cg_edge->callee->local.redefined_extern_inline /* Avoid warnings during early inline pass. */ && (!flag_unit_at_a_time || cgraph_global_info_ready)) {
At least for gcc-4_1-branch and gcc-4_2-branch your patch is IMHO the best thing we can do. I agree that with cgraph it shouldn't be very hard to actually register two bodies, one for extern inline fn and one for the real definition, but am not sure if such changes are appropriate this late in 4.3 development, so perhaps it should be applied also for 4.3 and early in 4.4 we should come up with the real fixes.
I would suggest to go with the patch in comment #3.
So do I and even am using that patch for quite some time in our gcc packages. But Mark objected to that: http://gcc.gnu.org/ml/gcc-patches/2007-11/msg00589.html The current state of things is bad, because with glibc 2.7+ several valid programs don't compile. And this late in stage3 is probably dangerous to play with two different bodies in cgraph.
Subject: Re: [4.1/4.2/4.3 Regression] Bogus inlining failed in call to `xxx': redefined extern inline functions are not considered for inlining On Tue, 8 Jan 2008, jakub at gcc dot gnu dot org wrote: > ------- Comment #6 from jakub at gcc dot gnu dot org 2008-01-08 17:25 ------- > So do I and even am using that patch for quite some time in our gcc packages. So do I. > But Mark objected to that: > http://gcc.gnu.org/ml/gcc-patches/2007-11/msg00589.html > The current state of things is bad, because with glibc 2.7+ several valid > programs don't compile. And this late in stage3 is probably dangerous to play > with two different bodies in cgraph. Right, this is something for stage1. Richard.
I can definitely commit the patch to silence the (IMO valid) diagnostics. However, why programs are using always_inline and extern inline combination at all? Just extern inline should be enough. /* For GNU C extern inline functions disregard inline limits. */ if (DECL_EXTERNAL (fndecl) && DECL_DECLARED_INLINE_P (fndecl)) DECL_DISREGARD_INLINE_LIMITS (fndecl) = 1; So perhaps just modifying glibc headers to not do both would do the trick? I agree that solving this problem correctly via two functions (not only bodies, but simply two declarations) is probably bit tricky for stage3. We however have related PR34609 and PR31529 demonstrating same problem in different context. Honza
Subject: Re: [4.1/4.2/4.3 Regression] Bogus inlining failed in call to `xxx': redefined extern inline functions are not considered for inlining On Sat, 12 Jan 2008, hubicka at gcc dot gnu dot org wrote: > ------- Comment #8 from hubicka at gcc dot gnu dot org 2008-01-12 13:55 ------- > I can definitely commit the patch to silence the (IMO valid) diagnostics. > However, why programs are using always_inline and extern inline combination at > all? Just extern inline should be enough. > /* For GNU C extern inline functions disregard inline limits. */ > if (DECL_EXTERNAL (fndecl) > && DECL_DECLARED_INLINE_P (fndecl)) > DECL_DISREGARD_INLINE_LIMITS (fndecl) = 1; > > So perhaps just modifying glibc headers to not do both would do the trick? > > I agree that solving this problem correctly via two functions (not only bodies, > but simply two declarations) is probably bit tricky for stage3. We however have > related PR34609 and PR31529 demonstrating same problem in different context. Can you give it a try? If it looks reasonably safe I would certainly prefer such a solution. Richard.
4.2.3 is being released now, changing milestones of open bugs to 4.2.4.
I must say that I don't see it that safe: we can't clone the functions because at the time fronend decide to rewrite the body by new one we are not having functions gimplified yet. (well we can clone, but we want to get rid of non-gimple inlining code too). Keeping the unclonned body around seems difficult in combination with one declaration rule. But perhaps it is just my limited knowledge of C and C++ frontends here..
4.2.4 is being released, changing milestones to 4.2.5.
Closing 4.1 branch.
The reason for using always_inline attribute in glibc headers is that whether these (tiny) wrappers are inlined or not is a security matter for the program (if they are inlined, -D_FORTIFY_SOURCE checking is performed, if they are not, then no checking is done), and that checking shouldn't be affected by how many functions were already inlined into some function etc. As this isn't going to be fixed in 4.4 either, could we get the sorry quieted up?
Closing 4.2 branch.
GCC 4.3.4 is being released, adjusting target milestone.
Note on the trunk it fails at -O1 but not -O2.
GCC 4.3.5 is being released, adjusting target milestone.
Still fails to inline baz (not a regression) and diagnoses this (this is the regression). > When a gnu_inline (or extern inline gnu89) function has a real is redefined, we > don't want any inlining for that function even if it is always_inline, at least > GCC always behaved that way and even on the simpler testcase behaves that way. I don't think this is the desired behavior (nor is it documented). If it is then we should simply drop the always-inline attribute when merging the function decls (basically throw away the inline definition).
> I don't think this is the desired behavior (nor is it documented). If it is > then we should simply drop the always-inline attribute when merging the > function decls (basically throw away the inline definition). Well, the desired behaviour IMO is to get one body for inline copies, other body for offline copy (this is also only consistent interpretation with LTO. Current way of inlining them early and then droppipng is bit weird). This is quite easy to do at cgraph level once frontend is able to produce both function bodies at two different declarations (one static inline, other external) tells so to the backend. No one volunteed to do this at frontend side yet :( Honza
An example from libhugetlbfs. Note the original call wasn't recursive, following is reduced code I got from delta. $ cat gethugepagesizes.c extern __inline __attribute__ ((__always_inline__)) int open (__const char *__path, int __oflag, ...) { } void cleanup (void); open (const char *file, int flags, ...) { char fname[4096 + 1]; int fd; cleanup (); fd = open (fname, 01 | 0100); } void cleanup (void) { cleanup_fake_data (); } $ ~/install/gcc/trunk/bin/gcc -O3 -c gethugepagesizes.c gethugepagesizes.c: In function 'open': gethugepagesizes.c:7:1: sorry, unimplemented: inlining failed in call to 'open': redefined extern inline functions are not considered for inlining gethugepagesizes.c:12:6: sorry, unimplemented: called from here
4.3 branch is being closed, moving to 4.4.7 target.
Shorter testcase extern __inline __attribute__ ((__always_inline__,__gnu_inline__)) void open () { } void open () { open (); } fails on trunk like > ./cc1 -quiet t.c -O t.c: In function 'open': t.c:5:6: error: inlining failed in call to always_inline 'open': function not inlinable t.c:7:8: error: called from here and creates wrong code at -O0 (and probably would, at -On): open: .LFB1: .cfi_startproc pushq %rbp .cfi_def_cfa_offset 16 .cfi_offset 6, -16 movq %rsp, %rbp .cfi_def_cfa_register 6 movl $0, %eax call open popq %rbp .cfi_def_cfa 7, 8 ret as the always-inline body was not inlined. Note that the initial callgraph is already wrong: Initial callgraph: open/0 @0x7ffff5a2b6c0 (asm: open) analyzed needed reachable body finalized redefined_extern_inline called by: open/0 (1.00 per call) calls: open/0 (1.00 per call) References: Refering this function: variable pool: as said, the C frontend needs to create two decls for open for this to properly work. OTOH, it is time to deprecate this extension and warn about it (after all we miscompile this since quite some time, GCC 3.3 and 4.1 already produce the recursive open - how was this intended to work? ...) I don't have 3.2 (and 2.95 does not have always_inline). The "proper" way to write the testcase is extern __inline __attribute__ ((__always_inline__,__gnu_inline__)) void open () { } void open_1 () asm("open"); void open_1 () { open (); } So, Joseph - would you be fine with changing behavior for this testcase from ICEing at -On, miscompiling at -O0 to rejecting it? Thus, Index: c-decl.c =================================================================== --- c-decl.c (revision 183121) +++ c-decl.c (working copy) @@ -1855,21 +1855,10 @@ diagnose_mismatched_decls (tree newdecl, { if (DECL_INITIAL (olddecl)) { - /* If both decls are in the same TU and the new declaration - isn't overriding an extern inline reject the new decl. + /* If both decls are in the same TU 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)) + if (same_translation_unit_p (newdecl, olddecl)) { error ("redefinition of %q+D", newdecl); locate_old_decl (olddecl);
> OTOH, it is time to deprecate this extension and warn about it (after > all we miscompile this since quite some time, GCC 3.3 and 4.1 already produce > the recursive open - how was this intended to work? ...) I don't have > 3.2 (and 2.95 does not have always_inline). pre cgraph compilers handled it in a way that inline body was kept after parsing extern inline version and inlined into every new parsed function until the offline version was reached. Then the function was marked uninlinable, offline body was produced and all subsequentely parsed calls was not inlined (including calls in the offline body). I think extern inlines are sadly rather common to be deprecated... Honza
On Thu, 12 Jan 2012, hubicka at ucw dot cz wrote: > http://gcc.gnu.org/bugzilla/show_bug.cgi?id=33763 > > --- Comment #24 from Jan Hubicka <hubicka at ucw dot cz> 2012-01-12 14:22:52 UTC --- > > OTOH, it is time to deprecate this extension and warn about it (after > > all we miscompile this since quite some time, GCC 3.3 and 4.1 already produce > > the recursive open - how was this intended to work? ...) I don't have > > 3.2 (and 2.95 does not have always_inline). > > pre cgraph compilers handled it in a way that inline body was kept after > parsing > extern inline version and inlined into every new parsed function until > the offline version was reached. Then the function was marked uninlinable, > offline body was produced and all subsequentely parsed calls was not inlined > (including calls in the offline body). > > I think extern inlines are sadly rather common to be deprecated... Well, not deprecating extern inlines but re-definitions in the same TU which miscompiles once the extern inline is used in that TU (ok, so you can say in 99% of all cases that won't happen). Thus, another workaround would be to avoid transitioning the attribute to the redefinition in merge_decls (under the exact same condition as we allow the redefinition). Richard.
The patch fails to bootstrap in libquadmath btw: /space/rguenther/src/svn/trunk/libquadmath/math/cimagq.c:24:1: error: redefinition of 'cimagq' In file included from /space/rguenther/src/svn/trunk/libquadmath/quadmath-imp.h:26:0, from /space/rguenther/src/svn/trunk/libquadmath/math/cimagq.c:21: /space/rguenther/src/svn/trunk/libquadmath/quadmath.h:175:1: note: previous definition of 'cimagq' was here make[3]: *** [math/cimagq.lo] Error 1 which seems to make heavy use of gnu-inline always-inline.
(In reply to comment #25) > On Thu, 12 Jan 2012, hubicka at ucw dot cz wrote: > > > http://gcc.gnu.org/bugzilla/show_bug.cgi?id=33763 > > > > --- Comment #24 from Jan Hubicka <hubicka at ucw dot cz> 2012-01-12 14:22:52 UTC --- > > > OTOH, it is time to deprecate this extension and warn about it (after > > > all we miscompile this since quite some time, GCC 3.3 and 4.1 already produce > > > the recursive open - how was this intended to work? ...) I don't have > > > 3.2 (and 2.95 does not have always_inline). > > > > pre cgraph compilers handled it in a way that inline body was kept after > > parsing > > extern inline version and inlined into every new parsed function until > > the offline version was reached. Then the function was marked uninlinable, > > offline body was produced and all subsequentely parsed calls was not inlined > > (including calls in the offline body). > > > > I think extern inlines are sadly rather common to be deprecated... > > Well, not deprecating extern inlines but re-definitions in the > same TU which miscompiles once the extern inline is used in that > TU (ok, so you can say in 99% of all cases that won't happen). > > Thus, another workaround would be to avoid transitioning the > attribute to the redefinition in merge_decls (under the exact > same condition as we allow the redefinition). Only a workaround for the ICE, of course, not the miscompile (can anyone check if we ever did not miscompile this case?)
On Thu, 12 Jan 2012, rguenther at suse dot de wrote: > > I think extern inlines are sadly rather common to be deprecated... > > Well, not deprecating extern inlines but re-definitions in the > same TU which miscompiles once the extern inline is used in that > TU (ok, so you can say in 99% of all cases that won't happen). I haven't tested it, but I'd expect extern inline + redefinition to be occur in various cases building glibc, or any other library with extern inlines in its headers. Though those cases may be less likely then to use the redefined function in the same TU.
Btw, GCC 3.2.3 produces for extern __inline __attribute__ ((__always_inline__)) void open () { } void open () { open (); } open: pushl %ebp movl %esp, %ebp call open popl %ebp ret so it's "broken" as well.
Of course the question is what we should really do here wrt name-lookup.
Ok, so the following patch makes extern __inline __attribute__ ((__always_inline__)) void open () { } void bar () { open (); } void open () { open (); } "work" as in the extern inline variant is used for the callin bar() and the call in open () is recursive and all following calls would refer to it. That means, we end up with two cgraph nodes (but do not magically use one for inlining and one for the offline copy - that would need to be fixed in the C frontend by _not_ changing name-lookup to lookup the 2nd version, but I'm not sure that is desired). At least you can say the behavior makes sense and it avoids the ICEs, too. I'm going to test this. Index: gcc/c-decl.c =================================================================== --- gcc/c-decl.c (revision 183121) +++ gcc/c-decl.c (working copy) @@ -2513,6 +2513,24 @@ duplicate_decls (tree newdecl, tree oldd return false; } + /* If we have a redeclared extern inline function simply drop olddecl + on the floor instead of merging it with newdecl. */ + if (TREE_CODE (newdecl) == FUNCTION_DECL + && DECL_INITIAL (newdecl) + && DECL_INITIAL (olddecl) + && !(!(DECL_DECLARED_INLINE_P (olddecl) + && DECL_EXTERNAL (olddecl)) + || (DECL_DECLARED_INLINE_P (newdecl) + && DECL_EXTERNAL (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)))))) + return false; + merge_decls (newdecl, olddecl, newtype, oldtype); return true; }
4.4 branch is being closed, moving to 4.5.4 target.
The 4.5 branch is being closed, adjusting target milestone.
(In reply to comment #31) Just so you know, the proposed patch would break glibc builds. Here's a reduced test case that reproduces an issue compiling glibc's s_isnan.c on amd64... extern int foo() __attribute__((__const__, __nothrow__)); extern int foo() __asm__("__GI_foo") __attribute__ ((visibility("hidden"))); extern __inline int __attribute__((__always_inline__)) foo() { return 0; } int foo() { return 0; } extern __typeof__(foo) __EI_foo __asm__("foo") __attribute__((alias("__GI_foo"))); On Debian, gcc-4.6 compiles this code OK, while gcc-4.7 (which has this patch applied) errors out with: /tmp/pr33763_broken.c:9:24: error: '__EI_foo' aliased to external symbol '__GI_foo'
(In reply to comment #34) > (In reply to comment #31) > Just so you know, the proposed patch would break glibc builds. Here's a > reduced test case that reproduces an issue compiling glibc's s_isnan.c on > amd64... > > extern int foo() __attribute__((__const__, __nothrow__)); > extern int foo() __asm__("__GI_foo") __attribute__ ((visibility("hidden"))); > extern __inline int __attribute__((__always_inline__)) foo() { > return 0; > } > int foo() { > return 0; > } > extern __typeof__(foo) __EI_foo __asm__("foo") > __attribute__((alias("__GI_foo"))); > > On Debian, gcc-4.6 compiles this code OK, while gcc-4.7 (which has this patch > applied) errors out with: > /tmp/pr33763_broken.c:9:24: error: '__EI_foo' aliased to external symbol > '__GI_foo' Yes, I know - the patch had fallout in the testsuite and I dropped the ball on this bugreport.
Author: jakub Date: Fri Oct 5 11:43:38 2012 New Revision: 192119 URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=192119 Log: PR tree-optimization/33763 * tree-inline.c (expand_call_inline): Silently ignore always_inline attribute for redefined extern inline functions. * c-c++-common/pr33763.c: New test. Added: trunk/gcc/testsuite/c-c++-common/pr33763.c Modified: trunk/gcc/ChangeLog trunk/gcc/testsuite/ChangeLog trunk/gcc/tree-inline.c
Author: jakub Date: Fri Oct 5 11:58:46 2012 New Revision: 192121 URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=192121 Log: PR tree-optimization/33763 * tree-inline.c (expand_call_inline): Silently ignore always_inline attribute for redefined extern inline functions. * c-c++-common/pr33763.c: New test. Added: branches/gcc-4_7-branch/gcc/testsuite/c-c++-common/pr33763.c Modified: branches/gcc-4_7-branch/gcc/ChangeLog branches/gcc-4_7-branch/gcc/testsuite/ChangeLog branches/gcc-4_7-branch/gcc/tree-inline.c
Author: jakub Date: Fri Oct 5 12:01:59 2012 New Revision: 192122 URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=192122 Log: PR tree-optimization/33763 * tree-inline.c (expand_call_inline): Silently ignore always_inline attribute for redefined extern inline functions. * c-c++-common/pr33763.c: New test. Added: branches/gcc-4_6-branch/gcc/testsuite/c-c++-common/pr33763.c Modified: branches/gcc-4_6-branch/gcc/ChangeLog branches/gcc-4_6-branch/gcc/testsuite/ChangeLog branches/gcc-4_6-branch/gcc/tree-inline.c
Is there still a bug here, after the commits of comment #36 to #38?
Fixed.
Maybe this failure with a newly built gcc 4.8 is related to this bug. Built gcc today from commit a846b696b9f3e9b9caab7e43ef8450cbded2715c git-svn-id: svn+ssh://gcc.gnu.org/svn/gcc/branches/gcc-4_8-branch@202762 138bc75d-0d04-0410-961f-82ee72b ~/gcc48/bin$ ./gcc -v Using built-in specs. COLLECT_GCC=./gcc COLLECT_LTO_WRAPPER=/home/kaltsi/gcc48/libexec/gcc/x86_64-unknown-linux-gnu/4.8.2/lto-wrapper Target: x86_64-unknown-linux-gnu Configured with: ../gcc/configure --prefix=/home/kaltsi/gcc48 --disable-bootstrap --with-tune=generic --disable-multilib --enable-checking=release --with-system-zlib --enable-__cxa_atexit --disable-libunwind-exceptions --enable-gnu-unique-object --enable-lto --enable-linker-build-id --enable-languages=c,c++ --enable-threads=posix --enable-shared --disable-libgcj Thread model: posix gcc version 4.8.2 20130920 (prerelease) (GCC) Compiling the following simple program with these options on an Ubuntu 12.04 causes a failure: $ gcc -Wp,-D_FORTIFY_SOURCE=2 -O2 -D_GNU_SOURCE=1 -- foo.c -- #include <sys/socket.h> extern ssize_t recvfrom(int s, void *buf, size_t len, int flags, struct sockaddr *from, socklen_t *fromlen); int main() { int s, flags; void *buf; size_t len; struct sockaddr *from; socklen_t* fromlen; return(recvfrom(s, buf, len, flags, from, fromlen)); } -- foo.c -- ~/gcc48/bin$ ./gcc -Wp,-D_FORTIFY_SOURCE=2 -O2 -D_GNU_SOURCE=1 foo.c In file included from /usr/include/x86_64-linux-gnu/sys/socket.h:251:0, from foo.c:5: foo.c: In function ‘main’: /usr/include/x86_64-linux-gnu/bits/socket2.h:65:1: error: inlining failed in call to always_inline ‘recvfrom’: mismatched arguments recvfrom (int __fd, void *__restrict __buf, size_t __n, int __flags, ^ foo.c:12:18: error: called from here return(recvfrom(s, buf, len, flags, from, fromlen)); ^
(In reply to Juha Kallioinen from comment #41) > Maybe this failure with a newly built gcc 4.8 is related to this bug. No, that is just a buggy testcase. Redefining a prototype, especially if you have no idea how the system function prototype looks like, is a bug. In the testcaseif you redefine the prototype to something different and obviously it then fails to inline it. Don't do that.
(In reply to Jakub Jelinek from comment #42) > (In reply to Juha Kallioinen from comment #41) > > Maybe this failure with a newly built gcc 4.8 is related to this bug. > > No, that is just a buggy testcase. Redefining a prototype, especially if > you have no idea how the system function prototype looks like, is a bug. > In the testcaseif you redefine the prototype to something different and > obviously it then fails to inline it. Don't do that. This was just a reduction of a piece of code that started to fail with gcc 4.8, but compiles with earlier versions. I'll try to understand what is different in the prototype redefinition.