* [PATCH, RFC] ELF/LD: Always consider STB_LOCAL symbols local
@ 2017-04-13 9:26 Maciej W. Rozycki
2017-04-13 14:09 ` Alan Modra
0 siblings, 1 reply; 3+ messages in thread
From: Maciej W. Rozycki @ 2017-04-13 9:26 UTC (permalink / raw)
To: Alan Modra; +Cc: binutils
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;
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH, RFC] ELF/LD: Always consider STB_LOCAL symbols local
2017-04-13 9:26 [PATCH, RFC] ELF/LD: Always consider STB_LOCAL symbols local Maciej W. Rozycki
@ 2017-04-13 14:09 ` Alan Modra
2017-04-20 15:38 ` Maciej W. Rozycki
0 siblings, 1 reply; 3+ messages in thread
From: Alan Modra @ 2017-04-13 14:09 UTC (permalink / raw)
To: Maciej W. Rozycki; +Cc: binutils
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
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH, RFC] ELF/LD: Always consider STB_LOCAL symbols local
2017-04-13 14:09 ` Alan Modra
@ 2017-04-20 15:38 ` Maciej W. Rozycki
0 siblings, 0 replies; 3+ messages in thread
From: Maciej W. Rozycki @ 2017-04-20 15:38 UTC (permalink / raw)
To: Alan Modra; +Cc: binutils
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
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2017-04-20 15:38 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-04-13 9:26 [PATCH, RFC] ELF/LD: Always consider STB_LOCAL symbols local Maciej W. Rozycki
2017-04-13 14:09 ` Alan Modra
2017-04-20 15:38 ` Maciej W. Rozycki
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).