Message ID | AANLkTilF6KQS-mv6vcR9GOvwkKXd3anDRn2QS54Uqb0l@mail.gmail.com |
---|---|
State | New |
Headers | show |
Um, sorry for the duplication, not used to using the gmail web interface. I hope it was worth reading twice... Richard
On Fri, Jul 9, 2010 at 6:26 PM, Richard Sandiford <rdsandiford@googlemail.com> wrote: > This patch fixes a wrong-code bug in the compilation of gPXE. gPXE has > a weak symbol called pxe_menu_boot whose default implementation is > defined in the same file as (one of?) the callers. However, this > implementation can be overridden by earlier objects on the link line. > > gPXE has only a single module -- there's no symbol preemption -- > so as an optimisation, it also marks every symbol as hidden. > This causes GCC to think it can inline pxe_menu_boot, something > it wouldn't do for default-visiblity weak symbols. > > The problem is that our notion of "replaceability" is based on whether > the definition binds locally to the current _module_, not to the current > TU. That's fine for most cases, but not for weak symbols. > > Bootstrapped & regression-tested on x86_64-linux-gnu. OK to install? > > Richard > > > This patch fixes a wrong-code bug in the compilation of gPXE. gPXE has > a weak symbol called pxe_menu_boot whose default implementation is > defined in the same file as (one of?) the callers. However, this > implementation can be overridden by earlier objects on the link line. > > gPXE has only a single module -- there's no symbol preemption -- > so as an optimisation, it also marks every symbol as hidden. > This causes GCC to think it can inline pxe_menu_boot, something > it wouldn't do for default-visiblity weak symbols. > > The problem is that our notion of "replaceability" is based on whether > the definition binds locally to the current _module_, not to the current > TU. That's fine for most cases, but not for weak symbols. > > Bootstrapped & regression-tested on x86_64-linux-gnu. OK to install? > > Richard Ok. Thanks, Richard. Ok. Thanks, Richard. ;) > > gcc/ > * tree.h (DECL_REPLACEABLE_P): Strengthen check for weak symbols. > > gcc/testsuite/ > * gcc.dg/attr-weak-hidden-1.c, gcc.dg/attr-weak-hidden-1a.c: New test. > > Index: gcc/tree.h > =================================================================== > --- gcc/tree.h 2010-07-09 09:44:27.000000000 +0100 > +++ gcc/tree.h 2010-07-09 12:59:26.000000000 +0100 > @@ -3012,6 +3012,11 @@ #define DECL_ONE_ONLY(NODE) (DECL_COMDAT > not be treated as replaceable so that use of C++ template > instantiations is not penalized. > > + In other respects, the condition is usually equivalent to whether > + the function binds to the current module (shared library or executable). > + However, weak functions can always be overridden by earlier TUs > + in the same module, even if they bind locally to that module. > + > For example, DECL_REPLACEABLE is used to determine whether or not a > function (including a template instantiation) which is not > explicitly declared "inline" can be inlined. If the function is > @@ -3020,7 +3025,7 @@ #define DECL_ONE_ONLY(NODE) (DECL_COMDAT > function that is not DECL_REPLACEABLE can be inlined, since all > versions of the function will be functionally identical. */ > #define DECL_REPLACEABLE_P(NODE) \ > - (!DECL_COMDAT (NODE) && !targetm.binds_local_p (NODE)) > + (!DECL_COMDAT (NODE) && (DECL_WEAK (NODE) || !targetm.binds_local_p (NODE))) > > /* The name of the object as the assembler will see it (but before any > translations made by ASM_OUTPUT_LABELREF). Often this is the same > Index: gcc/testsuite/gcc.dg/attr-weak-hidden-1.c > =================================================================== > --- /dev/null 2010-06-01 09:29:56.413951209 +0100 > +++ gcc/testsuite/gcc.dg/attr-weak-hidden-1.c 2010-07-09 > 12:58:03.000000000 +0100 > @@ -0,0 +1,6 @@ > +/* { dg-do run } */ > +/* { dg-require-weak "" } */ > +/* { dg-require-visibility "" } */ > +/* { dg-options "-O2" } */ > +/* { dg-additional-sources "attr-weak-hidden-1a.c" } */ > +int __attribute__((weak, visibility("hidden"))) foo (void) { return 0; } > Index: gcc/testsuite/gcc.dg/attr-weak-hidden-1a.c > =================================================================== > --- /dev/null 2010-06-01 09:29:56.413951209 +0100 > +++ gcc/testsuite/gcc.dg/attr-weak-hidden-1a.c 2010-07-09 > 13:00:53.000000000 +0100 > @@ -0,0 +1,9 @@ > +void abort (void); > +int __attribute__((weak, visibility("hidden"))) foo (void) { return 1; } > +int > +main (void) > +{ > + if (foo ()) > + abort (); > + return 0; > +} >
Index: gcc/tree.h =================================================================== --- gcc/tree.h 2010-07-09 09:44:27.000000000 +0100 +++ gcc/tree.h 2010-07-09 12:59:26.000000000 +0100 @@ -3012,6 +3012,11 @@ #define DECL_ONE_ONLY(NODE) (DECL_COMDAT not be treated as replaceable so that use of C++ template instantiations is not penalized. + In other respects, the condition is usually equivalent to whether + the function binds to the current module (shared library or executable). + However, weak functions can always be overridden by earlier TUs + in the same module, even if they bind locally to that module. + For example, DECL_REPLACEABLE is used to determine whether or not a function (including a template instantiation) which is not explicitly declared "inline" can be inlined. If the function is @@ -3020,7 +3025,7 @@ #define DECL_ONE_ONLY(NODE) (DECL_COMDAT function that is not DECL_REPLACEABLE can be inlined, since all versions of the function will be functionally identical. */ #define DECL_REPLACEABLE_P(NODE) \ - (!DECL_COMDAT (NODE) && !targetm.binds_local_p (NODE)) + (!DECL_COMDAT (NODE) && (DECL_WEAK (NODE) || !targetm.binds_local_p (NODE))) /* The name of the object as the assembler will see it (but before any translations made by ASM_OUTPUT_LABELREF). Often this is the same