[lld] [lld][ELF] Demote symbols in discarded sections to Undefined. (PR #68777)

[Bevin Hansson via llvm-commits]

bevin-hansson wrote:

> > > Does GNU ld permit this, or does it result in dangling relocations? If GNU ld does permit this then there's a chance that this might break some programs where the dangling relocations are benign. I guess that we may need to add a command line option to suppress the errors, although maybe not initially.
> > 
> > 
> > When I tested a while back, GNU ld seems to keep the section if there is still a reference to it, even though it was marked /DISCARD/. I don't think this is possible in lld, since the relocation processing is done after the linker script handling, and we've already made the call to keep or not keep the section at that point.
> 
> I had a quick check and GNU ld on my machine at least gives a similar error message to what you are proposing.

Interesting. I'll have to try it again myself.

> > > How does this work with uses of /DISCARD/ in a ld -r relocatable link? I guess a follow up question is what is it supposed to do for ld -r relocatable links.
> > 
> > 
> > I haven't tested -r. I would assume that discarding something in an -r invocation today does not trigger these types of errors, so they wouldn't with this patch either.
> 
> It looks like GNU ld does trigger these errors with ld -r at least with a trivial test case. LLD seems to give a warning in this case and outputs a R_*_NONE relocation in its place.
> 
> > > It would still be user error to /DISCARD/ a section where some other section had relocations to that section, but I don't think undefined symbols are checked for.
> > 
> > 
> > Hm, would it? Could you not discard a section that contains something which is referenced from the current file, and have that reference be resolved from some other object at the final link?
> 
> I think it depends on whether the linker resolves the relocation at relocatable link time, and whether it is a global definition or not. For example if we make a local symbol undefined that is likely to be unrecoverable.

Ah, that's true. Unresolved locals would be a problem.

> > > A possible alternative implementation could be to test each relocation in scanOne to see if the symbol is defined in a discarded section. You could then make a specific error message, or do the work to make the symbol undefined at that point. Not sure how well that would work out, but could be worth a prototype.
> > 
> > 
> > Yeah, it doesn't have to go through this logic. It's just that the logic for emitting these errors already existed (and only worked for Undefined), so I went that path.
> 
> I've found out why the tests are failing anyway. The ELF/linkerscript/discard-section.s contains a local reference that we would explicitly want to catch, so I think this test would need updating. The ARM.exidx tests are failing due a missing test for liveness in scanRelocations. The .ARM.exidx SyntheticSection has a list of input .ARM.exidx sections. When some, but not all of these input sections are discarded we need the liveness check.
> 
> This should fix that:
> 
> ```
> diff --git a/lld/ELF/Relocations.cpp b/lld/ELF/Relocations.cpp
> index 441111616196..f3fb0c71a8b3 100644
> --- a/lld/ELF/Relocations.cpp
> +++ b/lld/ELF/Relocations.cpp
> @@ -1574,7 +1574,8 @@ template <class ELFT> void elf::scanRelocations() {
>          scanner.template scanSection<ELFT>(*sec);
>        if (part.armExidx && part.armExidx->isLive())
>          for (InputSection *sec : part.armExidx->exidxSections)
> -          scanner.template scanSection<ELFT>(*sec);
> +          if (sec->isLive())
> +            scanner.template scanSection<ELFT>(*sec);
>      }
>    });
>  }
> ```
> 
> With this you should be able remove the special case for section symbols. It should be possible to use yaml2obj to make a test case with a relocation to a local symbol, but with the special case removed I don't think you'll need them.

Great, big thanks for finding this! I pushed the fix and test additions, along with some other suggested changes.

https://github.com/llvm/llvm-project/pull/68777