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

[Alan Modra <amodra at gmail dot 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>.

True.

>  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
users.

-- 
Alan Modra
Australia Development Lab, IBM