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

[grimar]

Summary

This is an attempt to fix the https://bugs.llvm.org//show_bug.cgi?id=37212.

The issue is about code like:

.section .text.foo,"axG",@progbits,bar,comdat
.Lfoo:

.section .debug_info,"",@progbits
  .quad	.Lfoo

llvm-mc -filetype=obj -triple=x86_64-pc-linux t.s -o t.o
ld t.o t.o -o ld.out

LLD currently ignores the relocations to discarded COMDAT sections and hence out output is different from GNU linkers output.
What LLD do is valid from the spec point of view (https://docs.oracle.com/cd/E19120-01/open.solaris/819-0690/chapter7-11598/index.html),
but unfortunately, modern compilers produce technically incorrect ELF and this patch is an attempt
to work around it. It is a bit hacky but seems to be more or less isolated.

[peter.smith]

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 to discarded local symbols. Relocations from non-dwarf debug sections to discarded sections will be either an error or ignored depending on the use case.

For example
file1.s

        .section .text.1, "axG", %progbits, foo, comdat, unique, 0
foo:    nop

        .section .text.2, "ax", %progbits
        .globl _start
_start:        
        .quad foo

        .section .debug_info
        .quad foo

file2.s

        .section .text.1, "axG", %progbits, foo, comdat, unique, 0
foo:    nop

        .section .text.3, "ax", %progbits
        .globl func
func:
        // This is a reference to a local definition inside group foo, ld.bfd will give an error message, ld.gold a warning, here when group is discarded, LLD will not.
        .quad foo
        nop

        .section .debug_info
        // This is a reference to a local definition inside group foo from a debug section, relocation is resolved to foo in kept group
        .quad foo
        // This is a reference to a local definition inside group foo, but does not exist in group foo in file1.s, resolved to 0.

llvm-mc -triple=x86_64-pc-linux t.s -filetype=obj -o t.o
llvm-mc -triple=x86_64-pc-linux t2.s -filetype=obj -o t2.o
ld.bfd t.o t2.o -o t.exe
.text.1' referenced in section .text.3' of t2.o: defined in discarded section `.text.1[foo]' of t2.o
ld.gold t.o t2.o -o t.exe
t2.o(.text.3+0x0): warning: relocation refers to discarded section
ld.lld t.o t2.o -o t.exe
// no errors

So I think that we are potentially doing two things wrong here:

• I think we should be giving a warning or error when there are relocations from non-debug sections to discarded local symbols.
• If we have a relocation from a debug section to a discarded local symbol, we should use the definition of the symbol from the section in the kept group or 0 if there is no such symbol.

Could this be implemented by something like:

If (relocation to local defined in discarded group section) {
  if (section containing relocation is debug) {
    if (find local definition in kept group)
        use that definition.
    else
        0   
  }
  else
     warn or error

ELF/InputFiles.cpp

439

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.

466

Same comment about number of entries as above.

ELF/InputSection.cpp

298

I'm guessing this is to match the createDiscardedProxy? I'm thinking that it might be better to extract that into a isDiscardedProxy() function to make it clearer. Might be worth checking that Repl is non null as well.

ELF/InputSection.h

53

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.

test/ELF/comdat-debug.s

1

We should add a test for ld -r as well as that could easily get broken in the future.

test/ELF/comdat.s

32

I think this should be an error or warning (see main comment)

[grimar]

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

ELF/InputFiles.cpp

439

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?

ELF/InputSection.h

53

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.

[peter.smith]

In D46502#1094404, @grimar wrote:

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

ELF/InputFiles.cpp

439

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.

[grimar]

Rui, do you have any opinion on that?

[ruiu]

I wonder if we can simply discard debug records that refer discarded sections. If comdat section is discarded because there's another copy of it, there should be debug records for that section, so we have two copies of debug records, thus we can discard debug records for a discarded comdat section, no?

[peter.smith]

In D46502#1094602, @ruiu wrote:

I wonder if we can simply discard debug records that refer discarded sections. If comdat section is discarded because there's another copy of it, there should be debug records for that section, so we have two copies of debug records, thus we can discard debug records for a discarded comdat section, no?

My understanding is that it depends on what action the debugger is trying to do. If it is single-stepping through a function from the selected comdat group there will be at least one set of .debug sections that have the correct PC range so mappings from Address to Source will work. The difficulty is with the Source to Address mapping for the objects where the group was discarded. For example show me all the symbols defined in this source file will not find the comdat group as the PC range won't be correct.

[ruiu]

My understanding is that it depends on what action the debugger is trying to do. If it is single-stepping through a function from the selected comdat group there will be at least one set of .debug sections that have the correct PC range so mappings from Address to Source will work. The difficulty is with the Source to Address mapping for the objects where the group was discarded. For example show me all the symbols defined in this source file will not find the comdat group as the PC range won't be correct.

I don't know if showing all symbols defined by some file is a common operation. Is it?

[grimar]

In D46502#1094602, @ruiu wrote:

I wonder if we can simply discard debug records that refer discarded sections. If comdat section is discarded because there's another copy of it, there should be debug records for that section, so we have two copies of debug records, thus we can discard debug records for a discarded comdat section, no?

Does not that mean we would need to have an additional debug parsing in a linker? (to rebuild the DWARF data).

[grimar]

btw, I think general direction is to emit a single debug data COMDAT group of DWARF sections per -ffunction-section. I think we will have this sooner or later.
Linkers will be able to handle that very fast then (as handling of COMDATs is fast).

I *think* we just want to have some easy-removable hack for now probably. Like this patch do.

[ruiu]

I don't think there's a hack that you can easily remove. Once something lands to the linker's source code, that becomes a part of the implicit ABI, and you can never remove it unless it becomes really obsolete. Basically, you should assume that landing some feature or a workaround is a commitment that we will very likely to keep it for a long period of time.

[grimar]

In D46502#1094663, @ruiu wrote:

I don't think there's a hack that you can easily remove. Once something lands to the linker's source code, that becomes a part of the implicit ABI, and you can never remove it unless it becomes really obsolete. Basically, you should assume that landing some feature or a workaround is a commitment that we will very likely to keep it for a long period of time.

I like this position. Then my question is do we want it? I feel it is a dirty hack. I do not know how much it is important to have/do not have for us though.

[peter.smith]

In D46502#1094633, @ruiu wrote:

My understanding is that it depends on what action the debugger is trying to do. If it is single-stepping through a function from the selected comdat group there will be at least one set of .debug sections that have the correct PC range so mappings from Address to Source will work. The difficulty is with the Source to Address mapping for the objects where the group was discarded. For example show me all the symbols defined in this source file will not find the comdat group as the PC range won't be correct.

I don't know if showing all symbols defined by some file is a common operation. Is it?

I've no idea to be honest, I suspect it would be more common in GUI based debuggers that have a speedbar containing all the files in a program than in something like gdb. It isn't just that operation though; it is anything that requires the debugger to look up something starting at a source location. For example setting breakpoints based on source file and line.

[peter.smith]

In D46502#1094678, @grimar wrote:

In D46502#1094663, @ruiu wrote:

I don't think there's a hack that you can easily remove. Once something lands to the linker's source code, that becomes a part of the implicit ABI, and you can never remove it unless it becomes really obsolete. Basically, you should assume that landing some feature or a workaround is a commitment that we will very likely to keep it for a long period of time.

I like this position. Then my question is do we want it? I feel it is a dirty hack. I do not know how much it is important to have/do not have for us though.

I don't have a strong position, I happened to notice that gold and bfd had implemented some method to patch up relocations to local symbols in groups. I think that this actually started in BFD to work around bugs in gcc linkonce sections : https://sourceware.org/bugzilla/show_bug.cgi?id=233 however it looks to have been altered over time to only affect .debug sections and I haven't been able to find any source code commit messages that say why.

[ruiu]

I don't actually have a strong opinion on which is better. I'm thinking which is easier to implement. In particular, I'm not very happy about the notion of "proxy input section" because it seems like it is too much abstraction. If possible, I want to make this much simpler than this.

[grimar]

In D46502#1094721, @ruiu wrote:

I don't actually have a strong opinion on which is better. I'm thinking which is easier to implement. In particular, I'm not very happy about the notion of "proxy input section" because it seems like it is too much abstraction. If possible, I want to make this much simpler than this.

Note that we have no proxy section. We have a method that creates something that behaves like that. But no specific type for the section. We can trade/play with it. Can it be a section with no name and no data for example?

[ruiu]

Your change might be one of the simplest implementations, but it feels like it is changing too many function signatures, add too much new code and functions. Let me try to see if this can be simplified.

[ruiu]

I tried this patch locally, experimented some other ideas, and do some benchmark. My feeling is that this patch is too much as a workaround for a buggy input. This is too intrusive, and it creates too many objects at runtime. For instance, Firefox contains almost 1 million discarded comdat sections, and creating "proxy" objects for them isn't negligible. You probably should fix the compiler instead of implementing a workaround to the linekr.

[grimar]

In D46502#1095496, @ruiu wrote:

I tried this patch locally, experimented some other ideas, and do some benchmark. My feeling is that this patch is too much as a workaround for a buggy input. This is too intrusive, and it creates too many objects at runtime. For instance, Firefox contains almost 1 million discarded comdat sections, and creating "proxy" objects for them isn't negligible. You probably should fix the compiler instead of implementing a workaround to the linekr.

Thanks for looking, Rui. I agree that it this functionality is too hacky probably.

Let's see for people other opinions. If there will be no strong arguments to do something like that in the linker, I'll abandon this patch then.

[peter.smith]

In D46502#1095700, @grimar wrote:

In D46502#1095496, @ruiu wrote:

I tried this patch locally, experimented some other ideas, and do some benchmark. My feeling is that this patch is too much as a workaround for a buggy input. This is too intrusive, and it creates too many objects at runtime. For instance, Firefox contains almost 1 million discarded comdat sections, and creating "proxy" objects for them isn't negligible. You probably should fix the compiler instead of implementing a workaround to the linekr.

Thanks for looking, Rui. I agree that it this functionality is too hacky probably.

Let's see for people other opinions. If there will be no strong arguments to do something like that in the linker, I'll abandon this patch then.

I suggest asking on the PR as that had quite a few people involved in debug generation on the CC list. They may have a better view on how important it is for a linker to fix up the relocations or not.

A possible alternative, that I haven't thought through fully, that might not require the creation of a proxy section is for each selected group store the tuple <Group Signature Name (as we do today), InputFile*, Group Section Index in InputFile>. With this information could we look up the selected InputSection and store it in Sections[] rather than a proxy section. This would mean having an InputSection from Object A in the Sections array for Object B which could have some unforeseen side effects.

[grimar]

In D46502#1095707, @peter.smith wrote:

I suggest asking on the PR as that had quite a few people involved in debug generation on the CC list. They may have a better view on how important it is for a linker to fix up the relocations or not.

Yes. That what I wanted to do (had issues with VPN as for Russia bugs.llvm.org and svn just does not work without it atm. That's annoying.). Done.

A possible alternative, that I haven't thought through fully, that might not require the creation of a proxy section is for each selected group store the tuple <Group Signature Name (as we do today), InputFile*, Group Section Index in InputFile>. With this information could we look up the selected InputSection and store it in Sections[] rather than a proxy section. This would mean having an InputSection from Object A in the Sections array for Object B which could have some unforeseen side effects.

Side effects are a concern here. When I experimented with approaches for this patch, I tried different solutions, but side effects were annoying.
Also, such way is less isolated and also not clear if we want to change the design of the linker so much for this hack. Let's see the possible comments.

[ruiu]

I'd agree that there might be some alternative way of doing this. I thought that, if there's some way to obtain a replacing comdat section from here

https://github.com/llvm-project/llvm-project-20170507/blob/master/lld/ELF/Symbols.cpp#L54

then we can get an address for a replaced comdat section which might solve the issue. But with the data structure it doesn't seem to be easy thing to do (and that's not lld's fault as it is for a compiler bug workaround).

[grimar]

In D46502#1096122, @ruiu wrote:

I'd agree that there might be some alternative way of doing this. I thought that, if there's some way to obtain a replacing comdat section from here

https://github.com/llvm-project/llvm-project-20170507/blob/master/lld/ELF/Symbols.cpp#L54

then we can get an address for a replaced comdat section which might solve the issue. But with the data structure it doesn't seem to be easy thing to do (and that's not lld's fault as it is for a compiler bug workaround).

Yes. And this place is probably hot as it is called for each relocation. That is why I ended with proxy object finally. We could add one more light base section probably.
Without adding the one more explicit section type. Something like:

class SectionBase;

class SectionProxy {
public:
  // <new comment>
  SectionBase *Repl;

  unsigned SectionKind : 3;

  // The next two bit fields are only used by InputSectionBase, but we
  // put them here so the struct packs better.

  // The garbage collector sets sections' Live bits.
  // If GC is disabled, all sections are considered live by default.
  unsigned Live : 1;

  unsigned Bss : 1;

  unsigned Proxy : 1;
}


class SectionBase : public SectionProxy  { ...

So creating many proxy sections would be much cheaper. I am not sure if such approach for that is OK though.

[peter.smith]

In D46502#1096122, @ruiu wrote:

I'd agree that there might be some alternative way of doing this. I thought that, if there's some way to obtain a replacing comdat section from here

https://github.com/llvm-project/llvm-project-20170507/blob/master/lld/ELF/Symbols.cpp#L54

then we can get an address for a replaced comdat section which might solve the issue. But with the data structure it doesn't seem to be easy thing to do (and that's not lld's fault as it is for a compiler bug workaround).

I can't think of a way that doesn't involve storing a mapping between discarded symbols/sections or just storing the input file of the selected group, but then having to do some potentially non-trivial processing to find the equivalent section for each discarded one. If each InputFile did store a mapping table then in theory copyLocalSymbols could populate the section field of a local symbol defined against a discarded section with that of the selected group.

[echristo]

As a drive by comment: While I think that consumers should be able to handle this case (obviously the bfd/gold workarounds here aren't always effective), I do think that we should probably implement this as well so that the consumers don't have to deal with it in a lot of cases.

[MaskRay]

This has been implemented in a different way in InputSection::relocateNonAlloc to allow tombstone values.

ELF/Driver.cpp

template <class ELFT> void LinkerDriver::link(opt::InputArgList &Args) {
   // Apply symbol renames for -wrap.
   Symtab->applySymbolWrap();
 
   // Now that we have a complete list of input files.
   // Beyond this point, no new files are added.
   // Aggregate all input sections into one place.
   for (InputFile *F : ObjectFiles)
     for (InputSectionBase *S : F->getSections())
-      if (S && S != &InputSection::Discarded)
+      if (S && !isDiscarded(S))
         InputSections.push_back(S);
   for (BinaryFile *F : BinaryFiles)
     for (InputSectionBase *S : F->getSections())
       InputSections.push_back(cast<InputSection>(S));
 
   // We do not want to emit debug sections if --strip-all
   // or -strip-debug are given.
   if (Config->Strip != StripPolicy::None)

ELF/InputFiles.h

 
 public:
   static bool classof(const InputFile *F) { return F->kind() == Base::ObjKind; }
 
   ArrayRef<Symbol *> getLocalSymbols();
   ArrayRef<Symbol *> getGlobalSymbols();
 
   ObjFile(MemoryBufferRef M, StringRef ArchiveName);
-  void parse(llvm::DenseSet<llvm::CachedHashStringRef> &ComdatGroups);
+  void parse(llvm::DenseMap<llvm::CachedHashStringRef, InputSectionBase **>
+                 &ComdatGroups);
 
   Symbol &getSymbol(uint32_t SymbolIndex) const {
     if (SymbolIndex >= this->Symbols.size())
       fatal(toString(this) + ": invalid symbol index");
     return *this->Symbols[SymbolIndex];
   }
 
   template <typename RelT> Symbol &getRelocTargetSym(const RelT &Rel) const {
public:
   uint32_t MipsGp0 = 0;
 
   // Name of source file obtained from STT_FILE symbol value,
   // or empty string if there is no such symbol in object file
   // symbol table.
   StringRef SourceFile;
 
 private:
-  void
-  initializeSections(llvm::DenseSet<llvm::CachedHashStringRef> &ComdatGroups);
+  void initializeSections(llvm::DenseMap<llvm::CachedHashStringRef,
+                                         InputSectionBase **> &ComdatGroups);
   void initializeSymbols();
   void initializeJustSymbols();
   void initializeDwarf();
   InputSectionBase *getRelocTarget(const Elf_Shdr &Sec);
   InputSectionBase *createInputSection(const Elf_Shdr &Sec);
   StringRef getSectionName(const Elf_Shdr &Sec);
 
   bool shouldMerge(const Elf_Shdr &Sec);

 };
 
 class BitcodeFile : public InputFile {
 public:
   BitcodeFile(MemoryBufferRef M, StringRef ArchiveName,
               uint64_t OffsetInArchive);
   static bool classof(const InputFile *F) { return F->kind() == BitcodeKind; }
   template <class ELFT>
-  void parse(llvm::DenseSet<llvm::CachedHashStringRef> &ComdatGroups);
+  void parse(llvm::DenseMap<llvm::CachedHashStringRef, InputSectionBase **>
+                 &ComdatGroups);
   std::unique_ptr<llvm::lto::InputFile> Obj;
 };
 
 // .so file.
 template <class ELFT> class SharedFile : public ELFFileBase<ELFT> {
   typedef ELFFileBase<ELFT> Base;
   typedef typename ELFT::Dyn Elf_Dyn;
   typedef typename ELFT::Shdr Elf_Shdr;

ELF/InputFiles.cpp

template <class ELFT> ArrayRef<Symbol *> ObjFile<ELFT>::getLocalSymbols() {
   return makeArrayRef(this->Symbols).slice(1, this->FirstGlobal - 1);
 }
 
 template <class ELFT> ArrayRef<Symbol *> ObjFile<ELFT>::getGlobalSymbols() {
   return makeArrayRef(this->Symbols).slice(this->FirstGlobal);
 }
 
 template <class ELFT>
-void ObjFile<ELFT>::parse(DenseSet<CachedHashStringRef> &ComdatGroups) {
+void ObjFile<ELFT>::parse(llvm::DenseMap<llvm::CachedHashStringRef,
+                                         InputSectionBase **> &ComdatGroups) {
   // Read a section table. JustSymbols is usually false.
   if (this->JustSymbols)
     initializeJustSymbols();
   else
     initializeSections(ComdatGroups);
 
   // Read a symbol table.
   initializeSymbols();
if (Sec.sh_type != SHT_SYMTAB)
       continue;
     this->initSymtab(ObjSections, &Sec);
     return;
   }
 }
 
 template <class ELFT>
 void ObjFile<ELFT>::initializeSections(
-    DenseSet<CachedHashStringRef> &ComdatGroups) {
+    llvm::DenseMap<llvm::CachedHashStringRef, InputSectionBase **>
+        &ComdatGroups) {
   const ELFFile<ELFT> &Obj = this->getObj();
 
   ArrayRef<Elf_Shdr> ObjSections = CHECK(this->getObj().sections(), this);
   uint64_t Size = ObjSections.size();
   this->Sections.resize(Size);
   this->SectionStringTable =
       CHECK(Obj.getSectionStringTable(ObjSections), this);
 
   for (size_t I = 0, E = ObjSections.size(); I < E; I++) {
-    if (this->Sections[I] == &InputSection::Discarded)
+    if (isDiscarded(this->Sections[I]))
       continue;
     const Elf_Shdr &Sec = ObjSections[I];
 
     // SHF_EXCLUDE'ed sections are discarded by the linker. However,
     // if -r is given, we'll let the final link discard such sections.
     // This is compatible with GNU.
     if ((Sec.sh_flags & SHF_EXCLUDE) && !Config->Relocatable) {
       this->Sections[I] = &InputSection::Discarded;
       continue;
     }
 
     switch (Sec.sh_type) {
     case SHT_GROUP: {
       // De-duplicate section groups by their signatures.
       StringRef Signature = getShtGroupSignature(ObjSections, Sec);
-      bool IsNew = ComdatGroups.insert(CachedHashStringRef(Signature)).second;
       this->Sections[I] = &InputSection::Discarded;
 
       // If it is a new section group, we want to keep group members.
       // Group leader sections, which contain indices of group members, are
       // discarded because they are useless beyond this point. The only
       // exception is the -r option because in order to produce re-linkable
       // object files, we want to pass through basically everything.
-      if (IsNew) {
+      ArrayRef<typename ObjFile<ELFT>::Elf_Word> Entries =
+          getShtGroupEntries(Sec);
+
+      auto It = ComdatGroups.insert({CachedHashStringRef(Signature), nullptr});
+      if (It.second /* IsNew */) {
+        if (Entries.size() == 1)
+          It.first->second = &this->Sections[Entries[0]];
+
         if (Config->Relocatable)
           this->Sections[I] = createInputSection(Sec);
         continue;
       }
 
       // Otherwise, discard group members.
       for (uint32_t SecIndex : getShtGroupEntries(Sec)) {
         if (SecIndex >= Size)
           fatal(toString(this) +
                 ": invalid section index in group: " + Twine(SecIndex));
         this->Sections[SecIndex] = &InputSection::Discarded;
       }
+
+      // Ideally we could finish here, but unfortunately, have one more
+      // special case that we want to support. Modern compilers at the moment
+      // of may/2018 produce technically incorrect ELF when representing the
+      // debug information for COMDAT groups. We want to support resolving the
+      // relocations to deduplicated COMDAT sections. We support only the case
+      // when there is a single COMDAT section in a group because there is no
+      // clean way to perform correct mapping of the sections to the leader
+      // sections as COMDATs are designed to work with a group of sections.
+      // Here we create discarded proxy section that keeps information about the
+      // leader.
+      InputSectionBase **SingleLeaderSec = It.first->second;
+      if (Entries.size() == 1 && SingleLeaderSec) {
+        InputSection *IS = createDiscardedProxy(*SingleLeaderSec);
+        this->Sections[Entries[0]] = IS;
+      }
       break;
     }
     case SHT_SYMTAB:
       this->initSymtab(ObjSections, &Sec);
       break;
     case SHT_SYMTAB_SHNDX:
       this->SymtabSHNDX = CHECK(Obj.getSHNDXTable(Sec, ObjSections), this);
       break;
InputSectionBase *ObjFile<ELFT>::getRelocTarget(const Elf_Shdr &Sec) {
   uint32_t Idx = Sec.sh_info;
   if (Idx >= this->Sections.size())
     fatal(toString(this) + ": invalid relocated section index: " + Twine(Idx));
   InputSectionBase *Target = this->Sections[Idx];
 
   // Strictly speaking, a relocation section must be included in the
   // group of the section it relocates. However, LLVM 3.3 and earlier
   // would fail to do so, so we gracefully handle that case.
-  if (Target == &InputSection::Discarded)
+  if (isDiscarded(Target))
     return nullptr;
 
   if (!Target)
     fatal(toString(this) + ": unsupported relocation reference");
   return Target;
 }
 
 // Create a regular InputSection class that has the same contents
template <class ELFT> Symbol *ObjFile<ELFT>::createSymbol(const Elf_Sym *Sym) {
   }
 
   switch (Binding) {
   default:
     fatal(toString(this) + ": unexpected binding: " + Twine(Binding));
   case STB_GLOBAL:
   case STB_WEAK:
   case STB_GNU_UNIQUE:
-    if (Sec == &InputSection::Discarded)
+    if (isDiscarded(Sec))
       return Symtab->addUndefined<ELFT>(Name, Binding, StOther, Type,
                                         /*CanOmitFromDynSym=*/false, this);
     return Symtab->addRegular(Name, StOther, Type, Value, Size, Binding, Sec,
                               this);
   }
 }
 
 ArchiveFile::ArchiveFile(std::unique_ptr<Archive> &&File)
return Symtab->addCommon(NameRef, ObjSym.getCommonSize(),
                              ObjSym.getCommonAlignment(), Binding, Visibility,
                              STT_OBJECT, F);
 
   return Symtab->addBitcode(NameRef, Binding, Visibility, Type,
                             CanOmitFromDynSym, F);
 }
 
 template <class ELFT>
-void BitcodeFile::parse(DenseSet<CachedHashStringRef> &ComdatGroups) {
+void BitcodeFile::parse(llvm::DenseMap<llvm::CachedHashStringRef,
+                                       InputSectionBase **> &ComdatGroups) {
   std::vector<bool> KeptComdats;
   for (StringRef S : Obj->getComdatTable())
-    KeptComdats.push_back(ComdatGroups.insert(CachedHashStringRef(S)).second);
+    KeptComdats.push_back(
+        ComdatGroups.insert({CachedHashStringRef(S), {}}).second);
 
   for (const lto::InputFile::Symbol &ObjSym : Obj->symbols())
     Symbols.push_back(createBitcodeSymbol<ELFT>(KeptComdats, ObjSym, *this));
 }
 
 static ELFKind getELFKind(MemoryBufferRef MB) {
   unsigned char Size;
   unsigned char Endian;
template <class ELFT> void LazyObjFile::addElfSymbols() {
   }
 }
 
 template void ArchiveFile::parse<ELF32LE>();
 template void ArchiveFile::parse<ELF32BE>();
 template void ArchiveFile::parse<ELF64LE>();
 template void ArchiveFile::parse<ELF64BE>();
 
-template void BitcodeFile::parse<ELF32LE>(DenseSet<CachedHashStringRef> &);
-template void BitcodeFile::parse<ELF32BE>(DenseSet<CachedHashStringRef> &);
-template void BitcodeFile::parse<ELF64LE>(DenseSet<CachedHashStringRef> &);
-template void BitcodeFile::parse<ELF64BE>(DenseSet<CachedHashStringRef> &);
+template void BitcodeFile::parse<ELF32LE>(
+    llvm::DenseMap<llvm::CachedHashStringRef, InputSectionBase **> &);
+template void BitcodeFile::parse<ELF32BE>(
+    llvm::DenseMap<llvm::CachedHashStringRef, InputSectionBase **> &);
+template void BitcodeFile::parse<ELF64LE>(
+    llvm::DenseMap<llvm::CachedHashStringRef, InputSectionBase **> &);
+template void BitcodeFile::parse<ELF64BE>(
+    llvm::DenseMap<llvm::CachedHashStringRef, InputSectionBase **> &);
 
 template void LazyObjFile::parse<ELF32LE>();
 template void LazyObjFile::parse<ELF32BE>();
 template void LazyObjFile::parse<ELF64LE>();
 template void LazyObjFile::parse<ELF64BE>();
 
 template class elf::ELFFileBase<ELF32LE>;
 template class elf::ELFFileBase<ELF32BE>;

ELF/InputSection.h

public:
 
   StringRef Name;
 
   // This pointer points to the "real" instance of this instance.
   // Usually Repl == this. However, if ICF merges two sections,
   // Repl pointer of one section points to another section. So,
   // if you need to get a pointer to this instance, do not use
   // this but instead this->Repl.
+  // We also may use this member to keep information about COMDAT leader
+  // when creating discarded proxy section. See createDiscardedProxy().
   SectionBase *Repl;
 
   unsigned SectionKind : 3;
 
   // The next two bit fields are only used by InputSectionBase, but we
   // put them here so the struct packs better.
 
   // The garbage collector sets sections' Live bits.
   // If GC is disabled, all sections are considered live by default.
private:
   template <class ELFT, class RelTy>
   void copyRelocations(uint8_t *Buf, llvm::ArrayRef<RelTy> Rels);
 
   template <class ELFT> void copyShtGroup(uint8_t *Buf);
 };
 
 // The list of all input sections.
 extern std::vector<InputSectionBase *> InputSections;
+
+InputSection* createDiscardedProxy(InputSectionBase *Target);
+bool isDiscarded(InputSectionBase* IS);
+
 } // namespace elf
 
 std::string toString(const elf::InputSectionBase *);
 } // namespace lld
 
 #endif

ELF/InputSection.cpp

if (auto *D = dyn_cast<Defined>(B))
       if (D->Section == this && D->Value <= Off && Off < D->Value + D->Size)
         return Filename + ":(" + toString(*D) + ")" + Archive;
 
   // If there's no symbol, print out the offset in the section.
   return (Filename + ":(" + Name + "+0x" + utohexstr(Off) + ")" + Archive)
       .str();
 }
 
+InputSection *elf::createDiscardedProxy(InputSectionBase *Target) {
+  InputSection *IS =
+      make<InputSection>(nullptr, 0, 0, 0, ArrayRef<uint8_t>(), "");
+  IS->Repl = Target;
+  return IS;
+}
+
+bool elf::isDiscarded(InputSectionBase *IS) {
+  if (IS == &InputSection::Discarded)
+    return true;
+  return IS && !IS->File && !IS->Flags && !IS->Type && IS->Data.empty() &&
+         IS->Name.empty();
+}
+
 InputSection InputSection::Discarded(nullptr, 0, 0, 0, ArrayRef<uint8_t>(), "");
 
 InputSection::InputSection(InputFile *F, uint64_t Flags, uint32_t Type,
                            uint32_t Alignment, ArrayRef<uint8_t> Data,
                            StringRef Name, Kind K)
     : InputSectionBase(F, Flags, Type,
                        /*Entsize*/ 0, /*Link*/ 0, /*Info*/ 0, Alignment, Data,
                        Name, K) {}
if (Sym.Type == STT_SECTION) {
       // relocation in it pointing to discarded sections with R_*_NONE, which
       // hopefully creates a frame that is ignored at runtime.
       auto *D = dyn_cast<Defined>(&Sym);
       if (!D) {
         error("STT_SECTION symbol should be defined");
         continue;
       }
       SectionBase *Section = D->Section;
-      if (Section == &InputSection::Discarded) {
+      if (isDiscarded(dyn_cast<InputSection>(Section))) {
         P->setSymbolAndType(0, 0, false);
         continue;
       }
 
       if (RelTy::IsRela) {
         P->r_addend =
             Sym.getVA(getAddend<ELFT>(Rel)) - Section->getOutputSection()->Addr;
       } else if (Config->Relocatable) {

ELF/SymbolTable.h

private:
   // but a bit inefficient.
   // FIXME: Experiment with passing in a custom hashing or sorting the symbols
   // once symbol resolution is finished.
   llvm::DenseMap<llvm::CachedHashStringRef, int> SymMap;
   std::vector<Symbol *> SymVector;
 
   // Comdat groups define "link once" sections. If two comdat groups have the
   // same name, only one of them is linked, and the other is ignored. This set
-  // is used to uniquify them.
-  llvm::DenseSet<llvm::CachedHashStringRef> ComdatGroups;
+  // is used to uniquify them. Value contains a pointer to the single comdat
+  // leader section if such exist.
+  llvm::DenseMap<llvm::CachedHashStringRef, InputSectionBase **> ComdatGroups;
 
   // Set of .so files to not link the same shared object file more than once.
   llvm::DenseSet<StringRef> SoNames;
 
   // A map from demangled symbol names to their symbol objects.
   // This mapping is 1:N because two symbols with different versions
   // can have the same name. We use this map to handle "extern C++ {}"
   // directive in version scripts.

ELF/SymbolTable.cpp

if (BitcodeFiles.empty())
     return;
 
   // Compile bitcode files and replace bitcode symbols.
   LTO.reset(new BitcodeCompiler);
   for (BitcodeFile *F : BitcodeFiles)
     LTO->add(*F);
 
   for (InputFile *File : LTO->compile()) {
-    DenseSet<CachedHashStringRef> DummyGroups;
+    llvm::DenseMap<llvm::CachedHashStringRef, InputSectionBase **> DummyGroups;
     auto *Obj = cast<ObjFile<ELFT>>(File);
     Obj->parse(DummyGroups);
     for (Symbol *Sym : Obj->getGlobalSymbols())
       Sym->parseSymbolVersion();
     ObjectFiles.push_back(File);
   }
 }
 

ELF/Writer.cpp

static bool includeInSymtab(const Symbol &B) {
   if (!B.isLocal() && !B.IsUsedInRegularObj)
     return false;
 
   if (auto *D = dyn_cast<Defined>(&B)) {
     // Always include absolute symbols.
     SectionBase *Sec = D->Section;
     if (!Sec)
       return true;
+    if (isDiscarded(dyn_cast<InputSectionBase>(Sec)))
+      return false;
     Sec = Sec->Repl;
     // Exclude symbols pointing to garbage-collected sections.
     if (isa<InputSectionBase>(Sec) && !Sec->Live)
       return false;
     if (auto *S = dyn_cast<MergeInputSection>(Sec))
       if (!S->getSectionPiece(D->Value)->Live)
         return false;
     return true;

test/ELF/comdat-debug.s

+# REQUIRES: x86
+# RUN: llvm-mc -filetype=obj -triple=x86_64-pc-linux %s -o %t.o
+# RUN: ld.lld %t.o %t.o -o %t
+# RUN: llvm-objdump -D %t | FileCheck %s
+
+## Check that we are able to resolve relocation to discarded COMDAT.
+## That violates ELF spec, but unfortunately clang and gcc produce
+## technically incorrect ELF and we have to handle this case to
+## improve debugging experience.
+
+# CHECK:      Disassembly of section .debug_info:
+# CHECK-NEXT: .debug_info:
+# CHECK-NEXT:   00 10
+# CHECK-NEXT:   20 00
+# CHECK-NEXT:   00 10
+# CHECK-NEXT:   20 00
+
+.section .text.foo,"axG",@progbits,bar,comdat
+.Lfoo:
+
+.section .debug_info,"",@progbits
+ .long .Lfoo

test/ELF/comdat.s

 // CHECK-NOT: nop
 
         .section bar, "ax"
         call foo
 
 // CHECK: Disassembly of section bar:
 // CHECK-NEXT: bar:
 // 0x1000 - 0x1001 - 5 = -6
-// 0      - 0x1006 - 5 = -4107
+// 0x1000 - 0x1006 - 5 = -11
 // CHECK-NEXT:   1001:  {{.*}}  callq  -6
-// CHECK-NEXT:   1006:  {{.*}}  callq  -4107
+// CHECK-NEXT:   1006:  {{.*}}  callq  -11
 
         .section .text3,"axG",@progbits,zed,comdat,unique,0
 
 
 // READ:      Name: .text2
 // READ-NEXT: Type: SHT_PROGBITS
 // READ-NEXT: Flags [
 // READ-NEXT:   SHF_ALLOC