From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 32663 invoked by alias); 9 Jul 2010 16:32:47 -0000 Received: (qmail 32654 invoked by uid 22791); 9 Jul 2010 16:32:46 -0000 X-SWARE-Spam-Status: No, hits=-1.8 required=5.0 tests=AWL,BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,FREEMAIL_FROM,RCVD_IN_DNSWL_NONE,TW_PX X-Spam-Check-By: sourceware.org Received: from mail-ww0-f51.google.com (HELO mail-ww0-f51.google.com) (74.125.82.51) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Fri, 09 Jul 2010 16:32:40 +0000 Received: by wwb31 with SMTP id 31so1062653wwb.8 for ; Fri, 09 Jul 2010 09:32:37 -0700 (PDT) MIME-Version: 1.0 Received: by 10.227.7.34 with SMTP id b34mr7874237wbb.200.1278693157381; Fri, 09 Jul 2010 09:32:37 -0700 (PDT) Received: by 10.216.179.76 with HTTP; Fri, 9 Jul 2010 09:32:37 -0700 (PDT) In-Reply-To: References: Date: Fri, 09 Jul 2010 16:32:00 -0000 Message-ID: Subject: Re: Prevent inlining of weak hidden symbols. From: Richard Guenther To: Richard Sandiford Cc: gcc-patches Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable X-IsSubscribed: yes Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org X-SW-Source: 2010-07/txt/msg00797.txt.bz2 On Fri, Jul 9, 2010 at 6:26 PM, Richard Sandiford wrote: > This patch fixes a wrong-code bug in the compilation of gPXE. =A0gPXE has > a weak symbol called pxe_menu_boot whose default implementation is > defined in the same file as (one of?) the callers. =A0However, 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. =A0That's fine for most cases, but not for weak symbols. > > Bootstrapped & regression-tested on x86_64-linux-gnu. =A0OK to install? > > Richard > > > This patch fixes a wrong-code bug in the compilation of gPXE. =A0gPXE has > a weak symbol called pxe_menu_boot whose default implementation is > defined in the same file as (one of?) the callers. =A0However, 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. =A0That's fine for most cases, but not for weak symbols. > > Bootstrapped & regression-tested on x86_64-linux-gnu. =A0OK to install? > > Richard Ok. Thanks, Richard. Ok. Thanks, Richard. ;) > > gcc/ > =A0 =A0 =A0 =A0* tree.h (DECL_REPLACEABLE_P): Strengthen check for weak s= ymbols. > > gcc/testsuite/ > =A0 =A0 =A0 =A0* gcc.dg/attr-weak-hidden-1.c, gcc.dg/attr-weak-hidden-1a.= c: New test. > > Index: gcc/tree.h > =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D > --- gcc/tree.h =A02010-07-09 09:44:27.000000000 +0100 > +++ gcc/tree.h =A02010-07-09 12:59:26.000000000 +0100 > @@ -3012,6 +3012,11 @@ #define DECL_ONE_ONLY(NODE) (DECL_COMDAT > =A0 =A0not be treated as replaceable so that use of C++ template > =A0 =A0instantiations is not penalized. > > + =A0 In other respects, the condition is usually equivalent to whether > + =A0 the function binds to the current module (shared library or executa= ble). > + =A0 However, weak functions can always be overridden by earlier TUs > + =A0 in the same module, even if they bind locally to that module. > + > =A0 =A0For example, DECL_REPLACEABLE is used to determine whether or not a > =A0 =A0function (including a template instantiation) which is not > =A0 =A0explicitly declared "inline" can be inlined. =A0If the function is > @@ -3020,7 +3025,7 @@ #define DECL_ONE_ONLY(NODE) (DECL_COMDAT > =A0 =A0function that is not DECL_REPLACEABLE can be inlined, since all > =A0 =A0versions of the function will be functionally identical. =A0*/ > =A0#define DECL_REPLACEABLE_P(NODE) \ > - =A0(!DECL_COMDAT (NODE) && !targetm.binds_local_p (NODE)) > + =A0(!DECL_COMDAT (NODE) && (DECL_WEAK (NODE) || !targetm.binds_local_p = (NODE))) > > =A0/* The name of the object as the assembler will see it (but before any > =A0 =A0translations made by ASM_OUTPUT_LABELREF). =A0Often this is the sa= me > Index: gcc/testsuite/gcc.dg/attr-weak-hidden-1.c > =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D > --- /dev/null =A0 2010-06-01 09:29:56.413951209 +0100 > +++ gcc/testsuite/gcc.dg/attr-weak-hidden-1.c =A0 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 > =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D > --- /dev/null =A0 2010-06-01 09:29:56.413951209 +0100 > +++ gcc/testsuite/gcc.dg/attr-weak-hidden-1a.c =A02010-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) > +{ > + =A0if (foo ()) > + =A0 =A0abort (); > + =A0return 0; > +} >