Re: [PATCH 5/5] PR ld/20828: Reorder the symbol sweep stage of section GC

["Maciej W. Rozycki" <macro at imgtec dot com>]

On Wed, 25 Jan 2017, Alan Modra wrote:

> >  BTW, in the process of making this change I have discovered that the 
> > linker script PROVIDE keyword causes the symbol requested to be emitted 
> > even in the absence of a local reference if there is an identically named 
> > symbol exported from a DSO present in the link.  This can be reproduced 
> > with the `pr20828-1.so' test and its `pr20828.ld' script modified to use 
> > PROVIDE with its symbols, regardless of the presence of `--gc-sections', 
> > as long as `libpr20828.so' is included in the link.  If the DSO is absent, 
> > then the symbols are not emitted.
> > 
> >  I believe this is due to this piece:
> > 
> >   /* If this symbol is being provided by the linker script, and it is
> >      currently defined by a dynamic object, but not by a regular
> >      object, then mark it as undefined so that the generic linker will
> >      force the correct value.  */
> >   if (provide
> >       && h->def_dynamic
> >       && !h->def_regular)
> >     h->root.type = bfd_link_hash_undefined;
> > 
> > in `bfd_elf_record_link_assignment', the presence of which however 
> > predates our repo history and therefore any justification is hard to track 
> > down.
> 
> This one goes back to
> commit b5279eb6a9672dba08ce9bbef0490f4bf26243f3
> Author: Ian Lance Taylor <ian@airs.com>
> Date:   Tue Jul 4 17:43:05 1995 +0000

 Thanks, though unsurprisingly this doesn't provide any input beyond what 
we already know.  I take it PR 7164 refers to our old GNATS bug tracker or 
maybe even Cygnus's internal records -- has this data been migrated 
anywhere or has it been lost (or perhaps sits catching dust on tapes 
somewhere)?

> The old history is in our git repo but it's a little difficult to go
> beyond 1999-05-03.  You need to pick one of the commits before the
> multiple "Initial creation of sourceware repository" commits..  In
> this case
> 
> git blame 1730ec6b -- bfd/elflink.h
> 
> gets you the old history.

 Thanks, that's really useful indeed!  I didn't realise we had this data.  
I'm not a GIT expert, but for the record:

$ git log --full-history

seems to go past commit 252b5132c753 ("19990502 sourceware import") 
automagically.

> > Our manual however only states that:
> > 
> > "In some cases, it is desirable for a linker script to define a symbol
> > only if it is referenced and is not defined by any object included in
> > the link.  For example, traditional linkers defined the symbol `etext'.
> > However, ANSI C requires that the user be able to use `etext' as a
> > function name without encountering an error.  The `PROVIDE' keyword may
> > be used to define a symbol, such as `etext', only if it is referenced
> > but not defined.  The syntax is `PROVIDE(SYMBOL = EXPRESSION)'."
> > 
> > which given our semantics is I believe at least ambiguous as it does not 
> > tell static and dynamic objects apart -- it merely states "any object".
> > 
> >  Do you or does anyone know what the purpose of this special case is?  It 
> > might be to preempt the DSO symbol so that the DSO uses ours and not its 
> > own, however preemption is not going to happen in a `-shared' link anyway 
> > (and it seems against the spirit of PROVIDE).
> > 
> >  Shall we keep it (presumably yes, owing to its long history) and make it 
> > clear in documentation?
> 
> I think we have to keep the current behaviour.  PROVIDE is likely the
> way it is for symbols like "etext" that might be exported from shared
> libraries but the executable needs its own value as given by the script.

 Right, but why is the symbol PROVIDEd defined even if there's no regular 
reference?  I'm thinking in terms of a change like below, which makes the 
keyword match its description -- in the absence of a regular reference I'd 
expect no regular definition regardless of whether there's a dynamic 
definition present or not.  I wonder if this was a deliberate choice or an 
omission; and either way if we decide to keep the current semantics, then 
I think we need to properly document it to avoid people's confusion.

> A doc patch is preapproved.

 I'm tempted to just s/any object/any static object/ in the paragraph 
quoted above, but let's sort out my concern first.

  Maciej

binutils-bfd-record-link-assignment-provide-unref.diff
Index: binutils/bfd/elflink.c
===================================================================
--- binutils.orig/bfd/elflink.c	2017-01-25 22:35:51.360905740 +0000
+++ binutils/bfd/elflink.c	2017-01-25 23:05:51.315467007 +0000
@@ -667,7 +667,12 @@ bfd_elf_record_link_assignment (bfd *out
   if (provide
       && h->def_dynamic
       && !h->def_regular)
-    h->root.type = bfd_link_hash_undefined;
+    {
+      if (h->ref_regular)
+	h->root.type = bfd_link_hash_undefined;
+      else
+	return TRUE;
+    }
 
   /* If this symbol is not being provided by the linker script, and it is
      currently defined by a dynamic object, but not by a regular object,