This is the mail archive of the
mailing list for the binutils project.
Re: [PATCH] PR ld/21233: Avoid sweeping forced-undefined symbols in section GC
- From: Alan Modra <amodra at gmail dot com>
- To: "Maciej W. Rozycki" <macro at imgtec dot com>
- Cc: Nick Clifton <nickc at redhat dot com>, binutils at sourceware dot org
- Date: Wed, 5 Apr 2017 11:41:17 +0930
- Subject: Re: [PATCH] PR ld/21233: Avoid sweeping forced-undefined symbols in section GC
- Authentication-results: sourceware.org; auth=none
- References: <alpine.DEB.firstname.lastname@example.org> <20170404084648.GD16711@bubble.grove.modra.org> <alpine.DEB.email@example.com>
On Tue, Apr 04, 2017 at 11:43:31PM +0100, Maciej W. Rozycki wrote:
> On Tue, 4 Apr 2017, Alan Modra wrote:
> > > I have identified the cause of this phenomenon to be the reverse order
> > > `elf_gc_sweep' and `_bfd_elf_link_check_relocs' are called in on these
> > > targets, causing `->non_got_ref' to be set for the symbol even though the
> > > reference is later discarded. The possibility to change the order has
> > > been introduced with commit d968975277ba ("Check ELF relocs after opening
> > > all input files"), using CHECK_RELOCS_AFTER_OPEN_INPUT, after a discussion
> > > started at <https://www.sourceware.org/ml/binutils/2016-04/msg00295.html>
> > > which indicates the intent to gradually swap the order for all targets.
> > > After the initial change for x86 this has only been since done for SH (cf
> > > PR ld/17739), i.e. I gather we are still in the middle of the move.
> > Well, powerpc hasn't changed where check_relocs is called, so this
> > isn't the whole story. ie. The powerpc backend code shows that it is
> > possible to get this case right without delaying check_relocs.
> Right, I haven't investigated PowerPC, or indeed any target that already
> passes these test cases, however I suspect that just like MIPS PowerPC
> does not require copy relocations in the first place for simple static
> symbol references (i.e. pointers) from data, which usually just end up
> being deferred to the dynamic load time in the form of dynamic relocations
> (e.g. R_MIPS_REL32 for the MIPS target, whether it uses the original SVR4
> psABI or the PLT/copy-reloc extension). These do not set `->non_got_ref'
> at all.
> I suspect some targets might not have a psABI as simple as that however
> and might require (or just want) a copy relocation even where the only
> reference is a static pointer from data, and these might indeed rely on
> the relative order `elf_gc_sweep' and `_bfd_elf_link_check_relocs' are
> called in.
The test has a reference from .data to "bar" that sets non_got_ref and
pointer_equality_needed, increments plt.refcount in case "bar" turns
out to be a function, and records the need for a possible dynamic
reloc in dyn_relocs, in check_relocs on powerpc64le. .data is then
garbage collected, which removes the dyn_reloc entry and decrements
plt.refcount too (*) in gc_sweep_hook but can't remove flags like
non_got_ref. ppc64_elf_adjust_dynamic_symbol does clear non_got_ref
later, and doesn't make a nonsense entry in .dynbss with copy relocs.
*) plt.refcount isn't decremented in this case on ppc64le. Bug! So
it was worth looking at the testcase. :)
> NB the avoidance of such odd cases is what I gather is implied by your
> very observation made in the thread of discussion referred, specifically
> here: <https://www.sourceware.org/ml/binutils/2016-04/msg00318.html>.
> Of course it might be a plain backend bug too.
Also likely true.
> > > Which brings me a question to our general maintainers: which of the
> > > following 3 options shall I pick for the purpose of this test case:
> > >
> > > 1. Leave the new failures as they are and let maintainers handle them as
> > > they find need or time; there may be more to be done for individual
> > > targets beyond the lone change to CHECK_RELOCS_AFTER_OPEN_INPUT.
> > Like this, I think. Your testcase demonstrate a bug in the affected
> > backends. Best make it visible.
> Committed unchanged then. Any issue with backporting it to 2.28, as per
> the situation with the problem report?
I think backporting should omit the testcase, to avoid unduly worrying
Australia Development Lab, IBM