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: Christophe Lyon <christophe dot lyon at linaro dot org>
- To: "Maciej W. Rozycki" <macro at imgtec dot com>
- Cc: Alan Modra <amodra at gmail dot com>, Nick Clifton <nickc at redhat dot com>, binutils <binutils at sourceware dot org>
- Date: Wed, 19 Apr 2017 14:02:39 +0200
- Subject: Re: [PATCH] PR ld/21233: Avoid sweeping forced-undefined symbols in section GC
- Authentication-results: sourceware.org; auth=none
- References: <alpine.DEB.email@example.com> <20170404084648.GD16711@bubble.grove.modra.org> <alpine.DEB.firstname.lastname@example.org>
On 5 April 2017 at 00:43, Maciej W. Rozycki <email@example.com> wrote:
> On Tue, 4 Apr 2017, Alan Modra wrote:
>> > PR ld/21233
>> > * ldlang.c (insert_undefined): Set `mark' for ELF symbols.
>> > * testsuite/ld-elf/pr21233.sd: New test.
>> > * testsuite/ld-elf/pr21233-l.sd: New test.
>> > * testsuite/ld-elf/pr21233.ld: New test linker script.
>> > * testsuite/ld-elf/pr21233-e.ld: New test linker script.
>> > * testsuite/ld-elf/pr21233.s: New test source.
>> > * testsuite/ld-elf/pr21233-l.s: New test source.
>> > * testsuite/ld-elf/shared.exp: Run the new tests.
> Committed, thanks for your review!
I've noticed that these tests fail on aarch64 and arm targets:
./ld/ld.sum:FAIL: PR ld/21233 dynamic symbols with section GC (--undefined)
./ld/ld.sum:FAIL: PR ld/21233 dynamic symbols with section GC
./ld/ld.sum:FAIL: PR ld/21233 dynamic symbols with section GC (EXTERN)
Is it a known problem?
>> > 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.
> 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.
>> > 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?