Re: [PATCH] PR ld/21233: Avoid sweeping forced-undefined symbols in section GC

[Christophe Lyon <christophe dot lyon at linaro dot org>]

On 5 April 2017 at 00:43, Maciej W. Rozycki <macro@imgtec.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.
>>
>> OK.
>
>  Committed, thanks for your review!
>

Hi,

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
(--require-defined)
./ld/ld.sum:FAIL: PR ld/21233 dynamic symbols with section GC (EXTERN)

Is it a known problem?

Thanks,

Christophe


>> >  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?
>
>   Maciej