[PATCH] D81784: [ELF] Resolve relocations in .debug_* referencing (discarded symbols or ICF folded section symbols) to tombstone values

[James Henderson via Phabricator via llvm-commits]

jhenderson accepted this revision.
jhenderson added a comment.
This revision is now accepted and ready to land.

LGTM, but I'd want @avl to be happy with it, since he did the original work essentially.



================
Comment at: lld/test/ELF/debug-dead-reloc-icf.s:2-5
+## Test we resolve symbolic relocations in .debug_* sections to a tombstone
+## if the referenced section symbol is folded into another section by ICF.
+## Otherwise, we would leave entries in multiple CUs claiming ownership of the
+## same range of code, which can confuse consumers.
----------------
MaskRay wrote:
> dblaikie wrote:
> > jhenderson wrote:
> > > One issue that I don't recall whether it was brought up previously: if we resolve one to -1, and the other has no debug data (e.g. they came from two different CUs, only one of which was compiled with -g), we could actually lose debug data for the function completely.
> > > 
> > > I don't know what the right thing to do is in this situation, as I don't know whether it's better than the risk of confusing consumers. I don't think we can reasonably attempt to identify whether there is matching debug data for the kept version after all.
> > Yeah, I'd accept that as an existing problem/not one that I think this tombstone work should necessarily address - the old solution or the new solution would be compatible with a linker making a more nuanced approach to this problem.
> > 
> > (but, in that separate discussion: I'm not sure this would be appropriate to fix - fixing this, I think, would mean having debug info influence which version of a function the linker chooses - increasing the risk of heisenbugs)
> https://reviews.llvm.org/D59553#2083857
> 
> My understanding is that: the prevailing definition should have associated debug info. If not, the linker should not do extra work to attach debug info from other copies to the prevailing definition.
FWIW, I looked at our properietary linker and downstream LLD ports, both of which have the -1 patching behaviour implemented, and we don't handle the "some with, some without" case at all, so ICF folded symbols can theoretically lose debug data, same as is implemented in this patch. So from our point of view, there's no regression, just a continued space from improvement.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D81784/new/

https://reviews.llvm.org/D81784