Patchwork Prevent inlining of weak hidden symbols.

login
register
mail settings
Submitter Richard Sandiford
Date July 9, 2010, 4:26 p.m.
Message ID <AANLkTilF6KQS-mv6vcR9GOvwkKXd3anDRn2QS54Uqb0l@mail.gmail.com>
Download mbox | patch
Permalink /patch/58411/
State New
Headers show

Comments

Richard Sandiford - July 9, 2010, 4:26 p.m.
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


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/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;
+}
Richard Sandiford - July 9, 2010, 4:29 p.m.
Um, sorry for the duplication, not used to using the gmail web interface.
I hope it was worth reading twice...

Richard
Richard Guenther - July 9, 2010, 4:32 p.m.
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;
> +}
>

Patch

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