[PATCH] D46502: [ELF] - Fix for "LLD can create incorrect debug PC ranges for functions in Comdat groups."

[George Rimar via Phabricator via llvm-commits]

grimar added a comment.

In https://reviews.llvm.org/D46502#1091257, @peter.smith wrote:

> If I've understood correctly (from test/ELF/comdat.s) this will affect relocations from all sections to discarded local symbols. If I'm right I think that this is too wide a use case. My understanding is that in gold and bfd this case is specifically for relocations from Dwarf debug sections


Yes, but should we follow?

Such objects are broken by the specification. For broken objects, it is ok to produce broken output usually.

We (I guess) want to fix the compilers for the case this patch tries to address and have some reasonable workaround in LLD for some time.
I think this patch introduces probably the minimum of required code to do that. And the code is simple and fast.



================
Comment at: ELF/InputFiles.cpp:439
+      if (It.second /* IsNew */) {
+        if (Entries.size() == 1)
+          It.first->second = &this->Sections[Entries[0]];
----------------
peter.smith wrote:
> Unfortunately I don't think that this will work on all targets. For example with --target=armv7a-linux-gnueabihf clang puts the exception table information (.ARM.exdix.name_of_function> and .rel.ARM.exidx.text.name_of_function) in the same group as name_of_function. I think it will be possible to select _a_ candidate section for these groups by choosing the section with SHF_EXECINSTR. 
Ok, thanks for the info. I wonder if it will be enough. Seems gold also matches section name and size.
Should we do the same too for safety?


================
Comment at: ELF/InputSection.h:53
+  // when creating discarded proxy section. See createDiscardedProxy().
   SectionBase *Repl;
 
----------------
peter.smith wrote:
> What happens if the Repl section for a discardedProxy is merged by ICF? Can we have a Repl -> Repl -> Repl needing to follow to the end of the Repl chain to find the candidate section. 
Yes, we will just need `Repl -> Repl -> Repl` at one place I believe (In `getSymVA`). Since it should be an easy fix and can be separate change,
I did not add a test case and fix for ICF in this patch. Question is if we are OK with the whole approach.


https://reviews.llvm.org/D46502