[PATCH, head+2.21] Fix IRONLY visibility estimation in plugin interface, resolves GCC PR

[PATCH, head+2.21] Fix IRONLY visibility estimation in plugin interface, resolves GCC PR

[Dave Korn]

[-- Attachment #1: Type: text/plain, Size: 865 bytes --]


    Hi list,

  In GCC PR46319(*), symbols are being resolved as LDPR_PREVAILING_DEF_IRONLY
when they should in fact not be IRONLY because they are /potentially/ visible
from outside the claimed IR files.  This causes LTRANS to falsely infer that
they aren't referenced at all and optimise the lot away.

  This patch adds the same tests that GOLD uses in this situation when
deciding that a prevailing def is not IRONLY, and resolves the GCC PR.  It's
survived an LTO-enabled bootstrap of GCC on x86_64-unknown-linux-gnu, and
there are no regressions in the binutils testsuite either there or on
i686-pc-cygwin with this patch.

ld/ChangeLog:

2010-11-09  Dave Korn  <...

	* plugin.c (is_visible_from_outside): New function.
	(get_symbols): Use it.

  OK for trunk and branch?

    cheers,
      DaveK
-- 
(*) - http://gcc.gnu.org/bugzilla/show_bug.cgi?id=46319

[-- Attachment #2: pr46319-binutils.diff --]
[-- Type: text/x-c, Size: 4769 bytes --]

Index: ld/plugin.c
===================================================================
RCS file: /cvs/src/src/ld/plugin.c,v
retrieving revision 1.7
diff -p -u -r1.7 plugin.c
--- ld/plugin.c	5 Nov 2010 07:20:07 -0000	1.7
+++ ld/plugin.c	9 Nov 2010 19:04:37 -0000
@@ -382,6 +382,43 @@ release_input_file (const void *handle)
   return LDPS_ERR;
 }
 
+/* Return TRUE if a defined symbol might be reachable from outside the
+   universe of claimed objects.  */
+static inline bfd_boolean
+is_visible_from_outside (struct ld_plugin_symbol *lsym, asection *section,
+			struct bfd_link_hash_entry *blhe)
+{
+  /* Section's owner may be NULL if it is the absolute
+     section, fortunately is_ir_dummy_bfd handles that.  */
+  if (!is_ir_dummy_bfd (section->owner))
+    return TRUE;
+  if (link_info.relocatable)
+    return TRUE;
+  if (link_info.export_dynamic || link_info.shared)
+    {
+      /* Only ELF symbols really have visibility.  */
+      if (bfd_get_flavour (link_info.output_bfd) == bfd_target_elf_flavour)
+	{
+	  struct elf_link_hash_entry *el = (struct elf_link_hash_entry *)blhe;
+	  int vis = ELF_ST_VISIBILITY (el->other);
+	  return vis == STV_DEFAULT || vis == STV_PROTECTED;
+	}
+      /* On non-ELF targets, we can safely make inferences by considering
+         what visibility the plugin would have liked to apply when it first
+	 sent us the symbol.  During ELF symbol processing, visibility only
+	 ever becomes more restrictive, not less, when symbols are merged,
+	 so this is a conservative estimate; it may give false positives,
+	 declaring something visible from outside when it in fact would
+	 not have been, but this will only lead to missed optimisation
+	 opportunities during LTRANS at worst; it will not give false
+	 negatives, which can lead to the disastrous conclusion that the
+	 related symbol is IRONLY.  (See GCC PR46319 for an example.)  */
+      return lsym->visibility == LDPV_DEFAULT
+	  || lsym->visibility == LDPV_PROTECTED;
+    }
+  return FALSE;
+}
+
 /* Get the symbol resolution info for a plugin-claimed input file.  */
 static enum ld_plugin_status
 get_symbols (const void *handle, int nsyms, struct ld_plugin_symbol *syms)
@@ -393,6 +430,7 @@ get_symbols (const void *handle, int nsy
     {
       struct bfd_link_hash_entry *blhe;
       bfd_boolean ironly;
+      asection *owner_sec;
 
       blhe = bfd_link_hash_lookup (link_info.hash, syms[n].name,
 				FALSE, FALSE, TRUE);
@@ -418,17 +456,25 @@ get_symbols (const void *handle, int nsy
 		called_plugin->name, blhe->type);
 	}
 
-      /* We need to know if the sym is referenced from non-IR files.  */
-      ironly = !bfd_hash_lookup (non_ironly_hash, syms[n].name, FALSE, FALSE);
+      /* Find out which section owns the symbol.  Since it's not undef,
+	 it must have an owner; if it's not a common symbol, both defs
+	 and weakdefs keep it in the same place. */
+      owner_sec = (blhe->type == bfd_link_hash_common)
+		? blhe->u.c.p->section
+		: blhe->u.def.section;
+
+      /* We need to know if the sym is referenced from non-IR files.  Or
+         even potentially-referenced, perhaps in a future final link if
+	 this is a partial one, perhaps dynamically at load-time if the
+	 symbol is externally visible.  */
+      ironly = !is_visible_from_outside (&syms[n], owner_sec, blhe)
+	&& !bfd_hash_lookup (non_ironly_hash, syms[n].name, FALSE, FALSE);
 
       /* If it was originally undefined or common, then it has been
          resolved; determine how.  */
       if (syms[n].def == LDPK_UNDEF || syms[n].def == LDPK_WEAKUNDEF
 	  || syms[n].def == LDPK_COMMON)
 	{
-	  asection *owner_sec = (blhe->type == bfd_link_hash_common)
-				? blhe->u.c.p->section
-				: blhe->u.def.section;
 	  if (owner_sec->owner == link_info.output_bfd)
 	    syms[n].resolution = LDPR_RESOLVED_EXEC;
 	  else if (owner_sec->owner == abfd)
@@ -447,9 +493,9 @@ get_symbols (const void *handle, int nsy
       /* Was originally def, or weakdef.  Does it prevail?  If the
          owner is the original dummy bfd that supplied it, then this
 	 is the definition that has prevailed.  */
-      if (blhe->u.def.section->owner == link_info.output_bfd)
+      if (owner_sec->owner == link_info.output_bfd)
 	syms[n].resolution = LDPR_PREEMPTED_REG;
-      else if (blhe->u.def.section->owner == abfd)
+      else if (owner_sec->owner == abfd)
 	{
 	  syms[n].resolution = (ironly)
 				? LDPR_PREVAILING_DEF_IRONLY
@@ -458,7 +504,7 @@ get_symbols (const void *handle, int nsy
 	}
 
       /* Was originally def, weakdef, or common, but has been pre-empted.  */
-      syms[n].resolution = is_ir_dummy_bfd (blhe->u.def.section->owner)
+      syms[n].resolution = is_ir_dummy_bfd (owner_sec->owner)
 				? LDPR_PREEMPTED_IR
 				: LDPR_PREEMPTED_REG;
     }

Re: [PATCH, head+2.21] Fix IRONLY visibility estimation in plugin interface, resolves GCC PR

[Dave Korn]

On 09/11/2010 19:38, Dave Korn wrote:

> ld/ChangeLog:
> 
> 2010-11-09  Dave Korn  <...
> 
> 	* plugin.c (is_visible_from_outside): New function.
> 	(get_symbols): Use it.
> 
>   OK for trunk and branch?

  Ping?

    cheers,
      DaveK

Re: [PATCH, head+2.21] Fix IRONLY visibility estimation in plugin interface, resolves GCC PR

[Dave Korn]

On 15/11/2010 06:39, Dave Korn wrote:
> On 09/11/2010 19:38, Dave Korn wrote:
> 
>> ld/ChangeLog:
>>
>> 2010-11-09  Dave Korn  <...
>>
>> 	* plugin.c (is_visible_from_outside): New function.
>> 	(get_symbols): Use it.
>>
>>   OK for trunk and branch?
> 
>   Ping?
> 

  Just to add a sense of urgency: this patch is required for the release, as
it resolves a PR(*) that will otherwise affect GCC 4.6.0, and is now delaying
Tristan.  I can't self-approve it as it doesn't fall under my area
maintainership.  The full patch is at:

http://sourceware.org/ml/binutils/2010-11/msg00170.html

    cheers,
      DaveK
-- 
(*) - http://gcc.gnu.org/bugzilla/show_bug.cgi?id=46319

Re: [PATCH, head+2.21] Fix IRONLY visibility estimation in plugin interface, resolves GCC PR

[H.J. Lu]

On Thu, Nov 18, 2010 at 10:18 AM, Dave Korn <dave.korn.cygwin@gmail.com> wrote:
> On 15/11/2010 06:39, Dave Korn wrote:
>> On 09/11/2010 19:38, Dave Korn wrote:
>>
>>> ld/ChangeLog:
>>>
>>> 2010-11-09  Dave Korn  <...
>>>
>>>      * plugin.c (is_visible_from_outside): New function.
>>>      (get_symbols): Use it.
>>>
>>>   OK for trunk and branch?
>>
>>   Ping?
>>
>
>  Just to add a sense of urgency: this patch is required for the release, as
> it resolves a PR(*) that will otherwise affect GCC 4.6.0, and is now delaying
> Tristan.  I can't self-approve it as it doesn't fall under my area
> maintainership.  The full patch is at:
>
> http://sourceware.org/ml/binutils/2010-11/msg00170.html
>
>    cheers,
>      DaveK
> --
> (*) - http://gcc.gnu.org/bugzilla/show_bug.cgi?id=46319
>
>

Are there any objections to this patch?


-- 
H.J.

Re: [PATCH, head+2.21] Fix IRONLY visibility estimation in plugin interface, resolves GCC PR

[Cary Coutant]

>>> 2010-11-09  Dave Korn  <...
>>>
>>>      * plugin.c (is_visible_from_outside): New function.
>>>      (get_symbols): Use it.
>>>
>>>   OK for trunk and branch?
>>
>>   Ping?
>>
>
>  Just to add a sense of urgency: this patch is required for the release, as
> it resolves a PR(*) that will otherwise affect GCC 4.6.0, and is now delaying
> Tristan.  I can't self-approve it as it doesn't fall under my area
> maintainership.  The full patch is at:
>
> http://sourceware.org/ml/binutils/2010-11/msg00170.html

I can't approve, but it looks reasonable to me, and seems to match
what gold does.

-cary

Re: [PATCH, head+2.21] Fix IRONLY visibility estimation in plugin interface, resolves GCC PR

[Alan Modra]

On Thu, Nov 18, 2010 at 06:18:40PM +0000, Dave Korn wrote:
> http://sourceware.org/ml/binutils/2010-11/msg00170.html

OK.

-- 
Alan Modra
Australia Development Lab, IBM

Re: [PATCH, head+2.21] Fix IRONLY visibility estimation in plugin interface, resolves GCC PR

[Dave Korn]

On 18/11/2010 21:52, Alan Modra wrote:
> On Thu, Nov 18, 2010 at 06:18:40PM +0000, Dave Korn wrote:
>> http://sourceware.org/ml/binutils/2010-11/msg00170.html
> 
> OK.
> 

  Thanks.  I would happily volunteer to be plugins maintainer for BFD in
general if anyone thinks that's a useful role, but in any case always
appreciate having a second (or more) pair of eyes.

  Will get on with committing.  Thanks for being patient, Tristan.

    cheers,
      DaveK

Re: [PATCH, head+2.21] Fix IRONLY visibility estimation in plugin interface, resolves GCC PR

[Dave Korn]

On 19/11/2010 03:34, Dave Korn wrote:

>   Will get on with committing.  Thanks for being patient, Tristan.

  Committed to head and branch, rebuilt and tested branch (have used head with
this patch for repeated lto-bootstraps of gcc in past couple of weeks), no
FAILs, thank you everyone, from my POV we're good to go!

    cheers,
      DaveK

Re: [PATCH, head+2.21] Fix IRONLY visibility estimation in plugin interface, resolves GCC PR

[H.J. Lu]

On Thu, Nov 18, 2010 at 7:34 PM, Dave Korn <dave.korn.cygwin@gmail.com> wrote:
> On 18/11/2010 21:52, Alan Modra wrote:
>> On Thu, Nov 18, 2010 at 06:18:40PM +0000, Dave Korn wrote:
>>> http://sourceware.org/ml/binutils/2010-11/msg00170.html
>>
>> OK.
>>
>
>  Thanks.  I would happily volunteer to be plugins maintainer for BFD in
> general if anyone thinks that's a useful role, but in any case always
> appreciate having a second (or more) pair of eyes.
>

Here is one for you:

http://sourceware.org/bugzilla/show_bug.cgi?id=12246


-- 
H.J.

Re: [PATCH, head+2.21] Fix IRONLY visibility estimation in plugin interface, resolves GCC PR

[H.J. Lu]

On Sat, Nov 20, 2010 at 7:10 AM, H.J. Lu <hjl.tools@gmail.com> wrote:
> On Thu, Nov 18, 2010 at 7:34 PM, Dave Korn <dave.korn.cygwin@gmail.com> wrote:
>> On 18/11/2010 21:52, Alan Modra wrote:
>>> On Thu, Nov 18, 2010 at 06:18:40PM +0000, Dave Korn wrote:
>>>> http://sourceware.org/ml/binutils/2010-11/msg00170.html
>>>
>>> OK.
>>>
>>
>>  Thanks.  I would happily volunteer to be plugins maintainer for BFD in
>> general if anyone thinks that's a useful role, but in any case always
>> appreciate having a second (or more) pair of eyes.
>>
>
> Here is one for you:
>
> http://sourceware.org/bugzilla/show_bug.cgi?id=12246
>

Another one:

http://sourceware.org/bugzilla/show_bug.cgi?id=12248

-- 
H.J.

Re: [PATCH, head+2.21] Fix IRONLY visibility estimation in plugin interface, resolves GCC PR

[H.J. Lu]

On Thu, Nov 18, 2010 at 10:18 AM, Dave Korn <dave.korn.cygwin@gmail.com> wrote:
> On 15/11/2010 06:39, Dave Korn wrote:
>> On 09/11/2010 19:38, Dave Korn wrote:
>>
>>> ld/ChangeLog:
>>>
>>> 2010-11-09  Dave Korn  <...
>>>
>>>      * plugin.c (is_visible_from_outside): New function.
>>>      (get_symbols): Use it.
>>>
>>>   OK for trunk and branch?
>>
>>   Ping?
>>
>
>  Just to add a sense of urgency: this patch is required for the release, as
> it resolves a PR(*) that will otherwise affect GCC 4.6.0, and is now delaying
> Tristan.  I can't self-approve it as it doesn't fall under my area
> maintainership.  The full patch is at:
>
> http://sourceware.org/ml/binutils/2010-11/msg00170.html
>

FWIW, BFD linker plugin is seriously broken:

http://sourceware.org/bugzilla/show_bug.cgi?id=12248


-- 
H.J.