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

[Peter Smith via Phabricator via llvm-commits]

peter.smith added a comment.

In https://reviews.llvm.org/D46502#1094404, @grimar wrote:

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


Personally I think we should as output that can fail at runtime is much worse than output that doesn't have a correct debug illusion. My understanding is that compilers will not generated code that makes illegal references into comdat groups, if one exists it is a serious bug. If there is an illegal reference from assembly code I think users would want to know about it and fix it. My understanding is that debug sections are considered a special case as a bad reference won't result in runtime failure.

> We first of all (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.

I don't think that this would make LLD any worse; references to local symbols from non-debug sections are often just as broken if they are resolved to 0 (current behaviour) as if they are resolved to a symbol. Given that it is likely to be some time before compilers change their behaviour and for the ecosystem to pick up the latest versions; I personally would prefer to restrict to just debug sections. This isn't a very strong opinion though so I'm happy to go with the consensus.



================
Comment at: ELF/InputFiles.cpp:439
+      if (It.second /* IsNew */) {
+        if (Entries.size() == 1)
+          It.first->second = &this->Sections[Entries[0]];
----------------
grimar wrote:
> 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?
I think it is the right thing to do. If a file is compiled with the same compiler and the same flags the same source code must/should be the same size with the same section name. If files are compiled at different times, with different compilers it is probably safer to not to try and substitute debug information that could be wrong. 


https://reviews.llvm.org/D46502