[PATCH, RFC] ELF/LD: Always consider STB_LOCAL symbols local

[PATCH, RFC] ELF/LD: Always consider STB_LOCAL symbols local

[Maciej W. Rozycki]

Do not require forced local (STB_LOCAL) symbols to have a definition in 
a regular file to be considered to resolve local to the current module, 
matching `elf_link_renumber_local_hash_table_dynsyms'.  In the absence 
of a regular definition any reference to a STB_LOCAL symbol will have to 
be garbage collected along with the undefined symbol itself, or the link
will eventually fail.  Either way the symbol concerned is not going to 
be external.

	bfd/
	* elflink.c (_bfd_elf_symbol_refs_local_p): Always return TRUE 
	if forced local.
---
Alan,

 No regressions against the usual targets.

 I have come across this peculiarity while investigating a possible 
solution for PR ld/21375 and then realised it was the immediate cause of 
assertion failures seen with PR ld/21233.  The thing is 
`_bfd_elf_symbol_refs_local_p' is called via SYMBOL_CALLS_LOCAL or 
SYMBOL_REFERENCES_LOCAL as appropriate by `mips_use_local_got_p' to 
determine which part of the GOT, local or external, a symbol has to be 
assigned to if it needs an entry there:

  /* Symbols that bind locally can (and in the case of forced-local
     symbols, must) live in the local GOT.  */
  if (h->got_only_for_calls
      ? SYMBOL_CALLS_LOCAL (info, &h->root)
      : SYMBOL_REFERENCES_LOCAL (info, &h->root))
    return TRUE;

 And as it happened in the course of section garbage collection done in PR 
ld/21233 an unused reference which would ultimately be dropped has been 
forced local.  That made it considered locally-binding by 
`elf_link_renumber_local_hash_table_dynsyms', however being a reference 
without a static definition it has been contrariwise considered external 
by `_bfd_elf_symbol_refs_local_p' and consequently assigned an external 
GOT entry by `mips_use_local_got_p'.

 This has then led to an inconsistency in `mips_elf_sort_hash_table' 
triggering an assertion failure.

 So my question is: what was the intent for `_bfd_elf_symbol_refs_local_p' 
to consider forced local symbols without a static definition external?  
Or in other words: is there a legitimate situation where a symbol that has 
been forced local has to be treated as if it bound externally?  Perhaps 
considering a situation where there's actually no definition at all, 
meaning that any references will be garbage-collected or the link will 
eventually fail?

 This check was introduced long ago, with a change of yours:

commit f6c52c13681f9c80b3e9cbf20464766c7d29e2e3
Author: Alan Modra <amodra@gmail.com>
Date:   Mon Jul 21 00:24:10 2003 +0000

which updated SYMBOL_CALLS_LOCAL/SYMBOL_REFERENCES_LOCAL to use 
`_bfd_elf_symbol_refs_local_p' rather than `_bfd_elf_dynamic_symbol_p',
which as at commit f6c52c13681f used to have this:

  /* If it was forced local, then clearly it's not dynamic.  */
  if (h->dynindx == -1)
    return FALSE;
  if (h->elf_link_hash_flags & ELF_LINK_FORCED_LOCAL)
    return FALSE;

(and effectively still has), and ahead of these checks added this:

  /* If we don't have a definition in a regular file, then we can't
     resolve locally.  The sym is either undefined or dynamic.  */
  if ((h->elf_link_hash_flags & ELF_LINK_HASH_DEF_REGULAR) == 0)
    return FALSE;

in a reference to 
<https://sourceware.org/ml/binutils/2003-07/msg00375.html>, however with 
no specific link scenario (or indeed test case) mentioned there, so I 
can't figure out whether the swapping of the two conditions is indeed safe 
for all targets, and I fear no regressions in testing may mean no test 
coverage rather than no problem.

 You made a further update with:

commit 89a2ee5a089604df716321acbc40137ef408afe8
Author: Alan Modra <amodra@gmail.com>
Date:   Sat Aug 28 04:04:16 2010 +0000

in a reference to 
<https://sourceware.org/ml/binutils/2010-08/msg00356.html>, however 
again with no actual test case and only the mention of undefined weak 
symbols.  However (defined or not) STB_WEAK is mutually exclusive with 
STB_LOCAL, so a weak symbol shouldn't have its `->forced_local' marking 
set in the first place (i.e. `bfd_link_hash_undefweak' should have been 
changed to `bfd_link_hash_undefined' at the time the marking was set), 
or perhaps the `->forced_local' marking should take precedence instead 
over the weak binding type (if for example we want to keep the marking 
reversible).

 However if the conditions indeed cannot be swapped, then I think 
`elf_link_renumber_local_hash_table_dynsyms' has to be updated instead, to 
treat qualifying symbols (or perhaps all `!_bfd_elf_symbol_refs_local_p' 
indeed) as external, so that the two functions are consistent with each 
other as far as local vs external symbol classification is concerned.

 NB I could paper it over in `mips_use_local_got_p', but I'd rather not.

 Thoughts?

  Maciej

binutils-bfd-elf-symbol-refs-local-forced-local.diff
Index: binutils/bfd/elflink.c
===================================================================
--- binutils.orig/bfd/elflink.c	2017-04-11 11:15:29.634115195 +0100
+++ binutils/bfd/elflink.c	2017-04-13 02:20:30.649746903 +0100
@@ -3020,10 +3020,10 @@ _bfd_elf_dynamic_symbol_p (struct elf_li
    to resolve local to the current module, and false otherwise.  Differs
    from (the inverse of) _bfd_elf_dynamic_symbol_p in the treatment of
    undefined symbols.  The two functions are virtually identical except
-   for the place where forced_local and dynindx == -1 are tested.  If
-   either of those tests are true, _bfd_elf_dynamic_symbol_p will say
-   the symbol is local, while _bfd_elf_symbol_refs_local_p will say
-   the symbol is local only for defined symbols.
+   for the place where dynindx == -1 is tested.  If that test is true,
+   _bfd_elf_dynamic_symbol_p will say the symbol is local, while
+   _bfd_elf_symbol_refs_local_p will say the symbol is local only for
+   defined symbols.
    It might seem that _bfd_elf_dynamic_symbol_p could be rewritten as
    !_bfd_elf_symbol_refs_local_p, except that targets differ in their
    treatment of undefined weak symbols.  For those that do not make
@@ -3046,6 +3046,10 @@ _bfd_elf_symbol_refs_local_p (struct elf
       || ELF_ST_VISIBILITY (h->other) == STV_INTERNAL)
     return TRUE;
 
+  /* Forced local symbols resolve locally.  */
+  if (h->forced_local)
+    return TRUE;
+
   /* Common symbols that become definitions don't get the DEF_REGULAR
      flag set, so test it first, and don't bail out.  */
   if (ELF_COMMON_DEF_P (h))
@@ -3055,11 +3059,7 @@ _bfd_elf_symbol_refs_local_p (struct elf
   else if (!h->def_regular)
     return FALSE;
 
-  /* Forced local symbols resolve locally.  */
-  if (h->forced_local)
-    return TRUE;
-
-  /* As do non-dynamic symbols.  */
+  /* Non-dynamic symbols resolve locally.  */
   if (h->dynindx == -1)
     return TRUE;
 

Re: [PATCH, RFC] ELF/LD: Always consider STB_LOCAL symbols local

[Alan Modra]

On Thu, Apr 13, 2017 at 10:26:05AM +0100, Maciej W. Rozycki wrote:
> Do not require forced local (STB_LOCAL) symbols to have a definition in 
> a regular file to be considered to resolve local to the current module, 
> matching `elf_link_renumber_local_hash_table_dynsyms'.  In the absence 
> of a regular definition any reference to a STB_LOCAL symbol will have to 
> be garbage collected along with the undefined symbol itself, or the link
> will eventually fail.  Either way the symbol concerned is not going to 
> be external.
> 
> 	bfd/
> 	* elflink.c (_bfd_elf_symbol_refs_local_p): Always return TRUE 
> 	if forced local.

OK.  I can't remember why I put the forced_local test after the
def_regular test in the original code.

-- 
Alan Modra
Australia Development Lab, IBM

Re: [PATCH, RFC] ELF/LD: Always consider STB_LOCAL symbols local

[Maciej W. Rozycki]

On Thu, 13 Apr 2017, Alan Modra wrote:

> > Do not require forced local (STB_LOCAL) symbols to have a definition in 
> > a regular file to be considered to resolve local to the current module, 
> > matching `elf_link_renumber_local_hash_table_dynsyms'.  In the absence 
> > of a regular definition any reference to a STB_LOCAL symbol will have to 
> > be garbage collected along with the undefined symbol itself, or the link
> > will eventually fail.  Either way the symbol concerned is not going to 
> > be external.
> > 
> > 	bfd/
> > 	* elflink.c (_bfd_elf_symbol_refs_local_p): Always return TRUE 
> > 	if forced local.
> 
> OK.  I can't remember why I put the forced_local test after the
> def_regular test in the original code.

 Some of the old scattered checks folded into SYMBOL_REFERENCES_LOCAL 
and SYMBOL_CALLS_LOCAL with commit 586119b38f88 had an outer logical AND 
with ELF_LINK_HASH_DEF_REGULAR being set, possibly because the case 
concerned here didn't matter for them.  This might have been the origin 
of the resulting code arrangement.

 Applied then, thanks for your review.

  Maciej