[LTO] Add SelectionKind to IRSymtab and use it in ld.lld/LLVMgold

[MaskRay]

Summary

In PGO, a C++ external linkage function foo has a private counter
__profc_foo and a private __profd_foo in a comdat nodeduplicate.

A __attribute__((weak)) function foo has a weak hidden counter __profc_foo
and a private __profd_foo in a comdat nodeduplicate.

In ld.lld a.o b.o, say a.o defines an external linkage foo and b.o
defines a weak foo. Currently we treat comdat nodeduplicate as comdat any,
ld.lld will incorrectly consider b.o:__profc_foo non-prevailing. In the worst
case b.o:__profd_foo is retained and b.o:__profc_foo isn't there will be
dangling reference causing an undefined hidden symbol error.

Add SelectionKind to Comdat in IRSymtab and let linkers ignore nodeduplicate comdat.

[tejohnson]

I'm not sure I understand the comdat noduplicates selection kind. The lang ref says it means "The linker requires that only section with this COMDAT key exist.". Does that mean that we should never see more than one section with that COMDAT key, in which case it seems like we shouldn't have the case you describe (2 noduplicates comdat with the key "__profc_foo"), or does it mean something else?

[MaskRay]

In D106228#2887624, @tejohnson wrote:

I'm not sure I understand the comdat noduplicates selection kind. The lang ref says it means "The linker requires that only section with this COMDAT key exist.". Does that mean that we should never see more than one section with that COMDAT key, in which case it seems like we shouldn't have the case you describe (2 noduplicates comdat with the key "__profc_foo"), or does it mean something else?

"The linker requires that only section with this COMDAT key exist." The sentence looks unclear.

The semantics are: (1) Within a module, a section can be the member of at most one ELF section group or PE/COFF comdat. (2) if two groups in two modules have the same key, they are not deduplicated.

One comdat noduplicates may have multiple members. These members will be retained or discarded as a unit. The ability is used by instrumentation to keep some related sections as a unit.

The failure scenario happened for a PGO+ThinLTO case, where two modules have comdat noduplicates of the same name. We should not perform deduplication.

[sbc100]

In D106228#2887667, @MaskRay wrote:

In D106228#2887624, @tejohnson wrote:

I'm not sure I understand the comdat noduplicates selection kind. The lang ref says it means "The linker requires that only section with this COMDAT key exist.". Does that mean that we should never see more than one section with that COMDAT key, in which case it seems like we shouldn't have the case you describe (2 noduplicates comdat with the key "__profc_foo"), or does it mean something else?

"The linker requires that only section with this COMDAT key exist." The sentence looks unclear.

The semantics are: (1) Within a module, a section can be the member of at most one ELF section group or PE/COFF comdat. (2) if two groups in two modules have the same key, they are not deduplicated.

One comdat noduplicates may have multiple members. These members will be retained or discarded as a unit. The ability is used by instrumentation to keep some related sections as a unit.

The failure scenario happened for a PGO+ThinLTO case, where two modules have comdat noduplicates of the same name. We should not perform deduplication.

So it sounds like the meaning of "noduplicates" is "allow duplicates" or maybe "do not de-duplicate" ? Is that right?

[MaskRay]

In D106228#2887760, @sbc100 wrote:

In D106228#2887667, @MaskRay wrote:

In D106228#2887624, @tejohnson wrote:

I'm not sure I understand the comdat noduplicates selection kind. The lang ref says it means "The linker requires that only section with this COMDAT key exist.". Does that mean that we should never see more than one section with that COMDAT key, in which case it seems like we shouldn't have the case you describe (2 noduplicates comdat with the key "__profc_foo"), or does it mean something else?

"The linker requires that only section with this COMDAT key exist." The sentence looks unclear.

The semantics are: (1) Within a module, a section can be the member of at most one ELF section group or PE/COFF comdat. (2) if two groups in two modules have the same key, they are not deduplicated.

One comdat noduplicates may have multiple members. These members will be retained or discarded as a unit. The ability is used by instrumentation to keep some related sections as a unit.

The failure scenario happened for a PGO+ThinLTO case, where two modules have comdat noduplicates of the same name. We should not perform deduplication.

So it sounds like the meaning of "noduplicates" is "allow duplicates" or maybe "do not de-duplicate" ? Is that right?

Right. Other selection kinds (any is the most common; exactmatch/largest/samesize are PE/COFF specific) use the key for duplication.
For noduplicates, the key is just a key to keep several members in the same group; it is not overloaded with the deduplication semantics.

[tejohnson]

In D106228#2887793, @MaskRay wrote:

In D106228#2887760, @sbc100 wrote:

In D106228#2887667, @MaskRay wrote:

In D106228#2887624, @tejohnson wrote:

I'm not sure I understand the comdat noduplicates selection kind. The lang ref says it means "The linker requires that only section with this COMDAT key exist.". Does that mean that we should never see more than one section with that COMDAT key, in which case it seems like we shouldn't have the case you describe (2 noduplicates comdat with the key "__profc_foo"), or does it mean something else?

"The linker requires that only section with this COMDAT key exist." The sentence looks unclear.

Yeah. At a minimum, I think a word is missing (should this be "only *one* section ..." ?). But it still doesn't describe the semantics well.

The semantics are: (1) Within a module, a section can be the member of at most one ELF section group or PE/COFF comdat. (2) if two groups in two modules have the same key, they are not deduplicated.

One comdat noduplicates may have multiple members. These members will be retained or discarded as a unit. The ability is used by instrumentation to keep some related sections as a unit.

The failure scenario happened for a PGO+ThinLTO case, where two modules have comdat noduplicates of the same name. We should not perform deduplication.

So it sounds like the meaning of "noduplicates" is "allow duplicates" or maybe "do not de-duplicate" ? Is that right?

Right. Other selection kinds (any is the most common; exactmatch/largest/samesize are PE/COFF specific) use the key for duplication.
For noduplicates, the key is just a key to keep several members in the same group; it is not overloaded with the deduplication semantics.

The name and description unfortunately don't imply this to me at least. Would you be able to update the langref (in a separate patch) to update the description so it is clearer?

Also, what happens in regular LTO when IR modules with two "noduplicates" comdats with the same key are merged? Is the assumption that the symbols within them would have been deduplicated after symbol resolution, so that we don't end up violating your (1) above?

[MaskRay]

In D106228#2887867, @tejohnson wrote:

In D106228#2887793, @MaskRay wrote:

In D106228#2887760, @sbc100 wrote:

In D106228#2887667, @MaskRay wrote:

In D106228#2887624, @tejohnson wrote:

I'm not sure I understand the comdat noduplicates selection kind. The lang ref says it means "The linker requires that only section with this COMDAT key exist.". Does that mean that we should never see more than one section with that COMDAT key, in which case it seems like we shouldn't have the case you describe (2 noduplicates comdat with the key "__profc_foo"), or does it mean something else?

"The linker requires that only section with this COMDAT key exist." The sentence looks unclear.

Yeah. At a minimum, I think a word is missing (should this be "only *one* section ..." ?). But it still doesn't describe the semantics well.
[...]

The name and description unfortunately don't imply this to me at least. Would you be able to update the langref (in a separate patch) to update the description so it is clearer?

Sounds good! Sent D106300 to clarify noduplicates and misc other things.

Also, what happens in regular LTO when IR modules with two "noduplicates" comdats with the same key are merged? Is the assumption that the symbols within them would have been deduplicated after symbol resolution, so that we don't end up violating your (1) above?

The LTO behavior needs to match the linker. I think LTO needs to retain all members and place all of them into the comdat.

For a case like merging two comdats of different selection kinds, I think the compiler may have to issue an error.

[tejohnson]

In D106228#2888164, @MaskRay wrote:

In D106228#2887867, @tejohnson wrote:

In D106228#2887793, @MaskRay wrote:

In D106228#2887760, @sbc100 wrote:

In D106228#2887667, @MaskRay wrote:

In D106228#2887624, @tejohnson wrote:

I'm not sure I understand the comdat noduplicates selection kind. The lang ref says it means "The linker requires that only section with this COMDAT key exist.". Does that mean that we should never see more than one section with that COMDAT key, in which case it seems like we shouldn't have the case you describe (2 noduplicates comdat with the key "__profc_foo"), or does it mean something else?

"The linker requires that only section with this COMDAT key exist." The sentence looks unclear.

Yeah. At a minimum, I think a word is missing (should this be "only *one* section ..." ?). But it still doesn't describe the semantics well.
[...]

The name and description unfortunately don't imply this to me at least. Would you be able to update the langref (in a separate patch) to update the description so it is clearer?

Sounds good! Sent D106300 to clarify noduplicates and misc other things.

Also, what happens in regular LTO when IR modules with two "noduplicates" comdats with the same key are merged? Is the assumption that the symbols within them would have been deduplicated after symbol resolution, so that we don't end up violating your (1) above?

The LTO behavior needs to match the linker. I think LTO needs to retain all members and place all of them into the comdat.

I assume that any weak symbols of the same name in those comdats would be deduplicated though? For regular LTO what would the resulting comdat group look like in the merged IR? What if the __profc_foo was weak in both modules being linked?

For a case like merging two comdats of different selection kinds, I think the compiler may have to issue an error.

lld/test/ELF/lto/comdat-noduplicates.ll

18 ↗

(On Diff #359587)

I think it would be good to test a few more cases: for non-distributed (no --thinlto-index-only) check the resulting symbols/comdats in the IR dumped by save-temps; test regular LTO as well, since that is presumably affected by this change too.

[MaskRay]

improve test

[tejohnson]

Thanks for the langref and IR patches, that helps my understanding. Have some suggestions below on new/improved comments needed.

lld/COFF/InputFiles.cpp

1069–1070

Can this FIXME be removed now? Or are there more improvements needed?

lld/test/ELF/lto/comdat-noduplicates.ll

19 ↗

(On Diff #359915)

Please add a comment - I'm not completely sure what this value is.

58 ↗

(On Diff #359915)

Some comments here and in the ABC case below about what specifically we are checking for in the resulting summaries would be helpful.

llvm/tools/gold/gold-plugin.cpp

627

Comment needed on how this achieves NoDuplicates effect.

[MaskRay]

improve comments

[MaskRay]

lld/COFF/InputFiles.cpp

1069–1070

Changed the FIXME wording. I'll keep PE/COFF unchanged in this patch.

[tejohnson]

lgtm with a comment follow on request below.

llvm/tools/gold/gold-plugin.cpp

626–629

I guess what I was looking for was more of a comment connecting how not setting comdat_key ensures we don't deduplicate.

[MaskRay]

clarify a comment.

rename noduplicates to the new name nodeduplicate

[tstellar]

I'm seeing test failures on i686 with this patch:

Exit Code: 1

Command Output (stderr):

/usr/bin/ld.gold: error: /builddir/build/BUILD/llvm-13.0.0rc1.src/redhat-linux-build/test/tools/gold/X86/Output/comdat-nodeduplicate.ll.tmp/ab.lto.o: incompatible target

[MaskRay]

In D106228#2927327, @tstellar wrote:

I'm seeing test failures on i686 with this patch:

Exit Code: 1

Command Output (stderr):

/usr/bin/ld.gold: error: /builddir/build/BUILD/llvm-13.0.0rc1.src/redhat-linux-build/test/tools/gold/X86/Output/comdat-nodeduplicate.ll.tmp/ab.lto.o: incompatible target

Cherry picked D107020 into release/13.x

[tstellar]

Thanks you.

[steplong]

llvm/test/tools/gold/X86/comdat-nodeduplicate.ll

40

Hi, I'm seeing a test failure on amd64 with this line. The actual output I'm seeing is
DATA: 0x[[#%x,]] 01000000 00000000 02000000 00000000 ......... I don't know enough about the patch to determine what is going on. I wonder if you have any ideas.

[MaskRay]

llvm/test/tools/gold/X86/comdat-nodeduplicate.ll

40

I suspect it is a bug in older gold.

My llvm-readelf -x .data output with binutils 2.35.2 and (built from git) 2.36.50.2021.0520

Hex dump of section '.data':
  0x00401128 01000000 00000000                   ........

What's your version? If it's not too much a hassle, I hope we just accept it and don't do anything. The test will be good when your gold is upgraded..

[steplong]

llvm/test/tools/gold/X86/comdat-nodeduplicate.ll

40

We are using

$ ld.gold -version
GNU gold (GNU Binutils for Ubuntu 2.24) 1.11

and yup should be ok to just accept it as it is.

lld/COFF/InputFiles.cpp

 FakeSection ltoTextSection(IMAGE_SCN_MEM_EXECUTE);
 FakeSection ltoDataSection(IMAGE_SCN_CNT_INITIALIZED_DATA);
 FakeSectionChunk ltoTextSectionChunk(&ltoTextSection.section);
 FakeSectionChunk ltoDataSectionChunk(&ltoDataSection.section);
 } // namespace
 
 void BitcodeFile::parse() {
   std::vector<std::pair<Symbol *, bool>> comdat(obj->getComdatTable().size());
   for (size_t i = 0; i != obj->getComdatTable().size(); ++i)
-    // FIXME: lto::InputFile doesn't keep enough data to do correct comdat
-    // selection handling.
-    comdat[i] = symtab->addComdat(this, saver.save(obj->getComdatTable()[i]));
+    // FIXME: Check nodeduplicate
+    comdat[i] =
+        symtab->addComdat(this, saver.save(obj->getComdatTable()[i].first));
   for (const lto::InputFile::Symbol &objSym : obj->symbols()) {
     StringRef symName = saver.save(objSym.getName());
     int comdatIndex = objSym.getComdatIndex();
     Symbol *sym;
     SectionChunk *fakeSC = nullptr;
     if (objSym.isExecutable())
       fakeSC = &ltoTextSectionChunk.chunk;
     else
       fakeSC = &ltoDataSectionChunk.chunk;
     if (objSym.isUndefined()) {
       sym = symtab->addUndefined(symName, this, false);
     } else if (objSym.isCommon()) {
       sym = symtab->addCommon(this, symName, objSym.getCommonSize());
     } else if (objSym.isWeak() && objSym.isIndirect()) {
       // Weak external.
       sym = symtab->addUndefined(symName, this, true);
       std::string fallback = std::string(objSym.getCOFFWeakExternalFallback());
       Symbol *alias = symtab->addUndefined(saver.save(fallback));
       checkAndSetWeakAlias(symtab, this, sym, alias);
     } else if (comdatIndex != -1) {
-      if (symName == obj->getComdatTable()[comdatIndex]) {
+      if (symName == obj->getComdatTable()[comdatIndex].first) {
         sym = comdat[comdatIndex].first;
         if (cast<DefinedRegular>(sym)->data == nullptr)
           cast<DefinedRegular>(sym)->data = &fakeSC->repl;
       } else if (comdat[comdatIndex].second) {
         sym = symtab->addRegular(this, symName, nullptr, fakeSC);
       } else {
         sym = symtab->addUndefined(symName, this, false);
       }

lld/ELF/InputFiles.cpp

static Symbol *createBitcodeSymbol(const std::vector<bool> &keptComdats,
   Defined newSym(&f, name, binding, visibility, type, 0, 0, nullptr);
   if (canOmitFromDynSym)
     newSym.exportDynamic = false;
   return symtab->addSymbol(newSym);
 }
 
 template <class ELFT> void BitcodeFile::parse() {
   std::vector<bool> keptComdats;
-  for (StringRef s : obj->getComdatTable())
+  for (std::pair<StringRef, Comdat::SelectionKind> s : obj->getComdatTable()) {
     keptComdats.push_back(
-        symtab->comdatGroups.try_emplace(CachedHashStringRef(s), this).second);
+        s.second == Comdat::NoDeduplicate ||
+        symtab->comdatGroups.try_emplace(CachedHashStringRef(s.first), this)
+            .second);
+  }
 
   for (const lto::InputFile::Symbol &objSym : obj->symbols())
     symbols.push_back(createBitcodeSymbol<ELFT>(keptComdats, objSym, *this));
 
   for (auto l : obj->getDependentLibraries())
     addDependentLibrary(l, this);
 }
 

lld/test/ELF/lto/comdat-nodeduplicate.ll

+; REQUIRES: x86, shell
+
+;; Keep __profd_foo in a nodeduplicate comdat, despite a comdat of the same name
+;; in a previous object file.
+
+;; Regular LTO
+
+; RUN: rm -rf %t && split-file %s %t
+; RUN: llvm-as %t/a.ll -o %t/a.bc
+; RUN: llvm-as %t/b.ll -o %t/b.bc
+; RUN: llvm-as %t/c.ll -o %t/c.bc
+
+; RUN: ld.lld --save-temps -u foo %t/a.bc --start-lib %t/b.bc --end-lib -o %t/ab
+; RUN: FileCheck %s --check-prefix=RESOL_AB < %t/ab.resolution.txt
+; RUN: llvm-readelf -x .data %t/ab | FileCheck %s --check-prefix=DATA
+
+; RESOL_AB: -r={{.*}}b.bc,__profc_foo,pl{{$}}
+
+;; .data contains a.bc:data. b.bc:data and c.bc:data are discarded.
+; DATA: 0x[[#%x,]] 01000000 00000000  ........
+
+;; __profc_foo from c.bc is non-prevailing and thus discarded.
+; RUN: ld.lld --save-temps -u foo -u c %t/a.bc --start-lib %t/b.bc %t/c.bc --end-lib -o %t/abc
+; RUN: FileCheck %s --check-prefix=RESOL_ABC < %t/abc.resolution.txt
+; RUN: llvm-readelf -x .data %t/abc | FileCheck %s --check-prefix=DATA
+
+; RESOL_ABC: -r={{.*}}b.bc,__profc_foo,pl{{$}}
+; RESOL_ABC: -r={{.*}}c.bc,__profc_foo,{{$}}
+
+;; ThinLTO
+
+; RUN: rm -rf %t && split-file %s %t
+; RUN: opt --module-summary %t/a.ll -o %t/a.bc
+; RUN: opt --module-summary %t/b.ll -o %t/b.bc
+; RUN: opt --module-summary %t/c.ll -o %t/c.bc
+
+; RUN: ld.lld --thinlto-index-only --save-temps -u foo %t/a.bc %t/b.bc -o %t/ab
+; RUN: FileCheck %s --check-prefix=RESOL_AB < %t/ab.resolution.txt
+; RUN: (llvm-dis < %t/b.bc && llvm-dis < %t/b.bc.thinlto.bc) | FileCheck %s --check-prefix=IR_AB
+; RUN: ld.lld -u foo %t/a.bc %t/b.bc -o %t/ab
+; RUN: llvm-readelf -x .data %t/ab | FileCheck %s --check-prefix=DATA
+
+; RUN: ld.lld --thinlto-index-only --save-temps -u foo %t/a.bc --start-lib %t/b.bc --end-lib -o %t/ab
+; RUN: FileCheck %s --check-prefix=RESOL_AB < %t/ab.resolution.txt
+; RUN: (llvm-dis < %t/b.bc && llvm-dis < %t/b.bc.thinlto.bc) | FileCheck %s --check-prefix=IR_AB
+; RUN: ld.lld -u foo %t/a.bc --start-lib %t/b.bc --end-lib -o %t/ab
+; RUN: llvm-readelf -x .data %t/ab | FileCheck %s --check-prefix=DATA
+
+; RUN: ld.lld --thinlto-index-only --save-temps -u foo -u c %t/a.bc --start-lib %t/b.bc %t/c.bc --end-lib -o %t/abc
+; RUN: FileCheck %s --check-prefix=RESOL_ABC < %t/abc.resolution.txt
+; RUN: (llvm-dis < %t/b.bc && llvm-dis < %t/b.bc.thinlto.bc) | FileCheck %s --check-prefix=IR_ABC
+; RUN: ld.lld -u foo %t/a.bc --start-lib %t/b.bc %t/c.bc --end-lib -o %t/abc
+; RUN: llvm-readelf -x .data %t/abc | FileCheck %s --check-prefix=DATA
+
+; IR_AB: gv: (name: "__profd_foo", {{.*}} guid = [[PROFD:[0-9]+]]
+; IR_AB: gv: (name: "__profc_foo", {{.*}} guid = [[PROFC:[0-9]+]]
+
+;; Check extra attributes. b.bc:__profc_foo is prevailing, so it can be internalized.
+; IR_AB: gv: (guid: [[PROFD]], summaries: (variable: (module: ^0, flags: (linkage: private, visibility: default, notEligibleToImport: 0, live: 0, dsoLocal: 1, canAutoHide: 0), varFlags: (readonly: 0, writeonly: 0, constant: 0),
+; IR_AB: gv: (guid: [[PROFC]], summaries: (variable: (module: ^0, flags: (linkage: internal, visibility: hidden, notEligibleToImport: 0, live: 1, dsoLocal: 1, canAutoHide: 0), varFlags: (readonly: 0, writeonly: 0, constant: 0))))
+
+; IR_ABC: gv: (name: "__profd_foo", {{.*}} guid = [[PROFD:[0-9]+]]
+; IR_ABC: gv: (name: "__profc_foo", {{.*}} guid = [[PROFC:[0-9]+]]
+
+;; b.bc:__profc_foo prevails c.bc:__profc_foo, so it is exported and therefore not internalized.
+; IR_ABC: gv: (guid: [[PROFD]], summaries: (variable: (module: ^0, flags: (linkage: private, visibility: default, notEligibleToImport: 0, live: 0, dsoLocal: 1, canAutoHide: 0), varFlags: (readonly: 0, writeonly: 0, constant: 0),
+; IR_ABC: gv: (guid: [[PROFC]], summaries: (variable: (module: ^0, flags: (linkage: weak, visibility: hidden, notEligibleToImport: 0, live: 1, dsoLocal: 1, canAutoHide: 0), varFlags: (readonly: 0, writeonly: 0, constant: 0))))
+
+;--- a.ll
+target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+$__profc_foo = comdat nodeduplicate
+@__profc_foo = private global i64 1, comdat, align 8
+@__profd_foo = private global i64* @__profc_foo, comdat($__profc_foo), align 8
+
+declare void @b()
+
+define i64 @foo() {
+  %v = load i64, i64* @__profc_foo
+  %inc = add i64 1, %v
+  store i64 %inc, i64* @__profc_foo
+  ret i64 %inc
+}
+
+define void @_start() {
+  call void @b()
+  ret void
+}
+
+;--- b.ll
+target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+$__profc_foo = comdat nodeduplicate
+@__profc_foo = weak hidden global i64 2, comdat, align 8
+@__profd_foo = private global i64* @__profc_foo, comdat($__profc_foo)
+
+define weak i64 @foo() {
+  %v = load i64, i64* @__profc_foo
+  %inc = add i64 1, %v
+  store i64 %inc, i64* @__profc_foo
+  ret i64 %inc
+}
+
+define void @b() {
+  ret void
+}
+
+;--- c.ll
+target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+$__profc_foo = comdat nodeduplicate
+@__profc_foo = weak hidden global i64 3, comdat, align 8
+@__profd_foo = private global i64* @__profc_foo, comdat($__profc_foo)
+
+define weak i64 @foo() {
+  %v = load i64, i64* @__profc_foo
+  %inc = add i64 1, %v
+  store i64 %inc, i64* @__profc_foo
+  ret i64 %inc
+}
+
+define void @c() {
+  ret void
+}

lld/wasm/InputFiles.cpp

obj = check(lto::InputFile::create(MemoryBufferRef(
       mb.getBuffer(), saver.save(archiveName + mb.getBufferIdentifier()))));
   Triple t(obj->getTargetTriple());
   if (!t.isWasm()) {
     error(toString(this) + ": machine type must be wasm32 or wasm64");
     return;
   }
   checkArch(t.getArch());
   std::vector<bool> keptComdats;
-  for (StringRef s : obj->getComdatTable())
-    keptComdats.push_back(symtab->addComdat(s));
+  // TODO Support nodeduplicate https://bugs.llvm.org/show_bug.cgi?id=50531
+  for (std::pair<StringRef, Comdat::SelectionKind> s : obj->getComdatTable())
+    keptComdats.push_back(symtab->addComdat(s.first));
 
   for (const lto::InputFile::Symbol &objSym : obj->symbols())
     symbols.push_back(createBitcodeSymbol(keptComdats, objSym, *this));
 }
 
 } // namespace wasm
 } // namespace lld

llvm/include/llvm/LTO/LTO.h

private:
   SmallVector<char, 0> Strtab;
   std::vector<Symbol> Symbols;
 
   // [begin, end) for each module
   std::vector<std::pair<size_t, size_t>> ModuleSymIndices;
 
   StringRef TargetTriple, SourceFileName, COFFLinkerOpts;
   std::vector<StringRef> DependentLibraries;
-  std::vector<StringRef> ComdatTable;
+  std::vector<std::pair<StringRef, Comdat::SelectionKind>> ComdatTable;
 
 public:
   ~InputFile();
 
   /// Create an InputFile.
   static Expected<std::unique_ptr<InputFile>> create(MemoryBufferRef Object);
 
   /// The purpose of this class is to only expose the symbol information that an
public:
 
   /// Returns the input file's target triple.
   StringRef getTargetTriple() const { return TargetTriple; }
 
   /// Returns the source file path specified at compile time.
   StringRef getSourceFileName() const { return SourceFileName; }
 
   // Returns a table with all the comdats used by this file.
-  ArrayRef<StringRef> getComdatTable() const { return ComdatTable; }
+  ArrayRef<std::pair<StringRef, Comdat::SelectionKind>> getComdatTable() const {
+    return ComdatTable;
+  }
 
   // Returns the only BitcodeModule from InputFile.
   BitcodeModule &getSingleBitcodeModule();
 
 private:
   ArrayRef<Symbol> module_symbols(unsigned I) const {
     const auto &Indices = ModuleSymIndices[I];
     return {Symbols.data() + Indices.first, Symbols.data() + Indices.second};

llvm/include/llvm/Object/IRSymtab.h

 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_OBJECT_IRSYMTAB_H
 #define LLVM_OBJECT_IRSYMTAB_H
 
 #include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/ADT/iterator_range.h"
+#include "llvm/IR/Comdat.h"
 #include "llvm/IR/GlobalValue.h"
 #include "llvm/Object/SymbolicFile.h"
 #include "llvm/Support/Allocator.h"
 #include "llvm/Support/Endian.h"
 #include "llvm/Support/Error.h"
 #include <cassert>
 #include <cstdint>
 #include <vector>
struct Module {
 
   /// The index of the first Uncommon for this Module.
   Word UncBegin;
 };
 
 /// This is equivalent to an IR comdat.
 struct Comdat {
   Str Name;
+
+  // llvm::Comdat::SelectionKind
+  Word SelectionKind;
 };
 
 /// Contains the information needed by linkers for symbol resolution, as well as
 /// by the LTO implementation itself.
 struct Symbol {
   /// The mangled symbol name.
   Str Name;
 

 };
 
 
 struct Header {
   /// Version number of the symtab format. This number should be incremented
   /// when the format changes, but it does not need to be incremented if a
   /// change to LLVM would cause it to create a different symbol table.
   Word Version;
-  enum { kCurrentVersion = 2 };
+  enum { kCurrentVersion = 3 };
 
   /// The producer's version string (LLVM_VERSION_STRING " " LLVM_REVISION).
   /// Consumers should rebuild the symbol table from IR if the producer's
   /// version does not match the consumer's version due to potential differences
   /// in symbol table format, symbol enumeration order and so on.
   Str Producer;
 
   Range<Module> Modules;
public:
   symbol_range module_symbols(unsigned I) const;
 
   StringRef getTargetTriple() const { return str(header().TargetTriple); }
 
   /// Returns the source file path specified at compile time.
   StringRef getSourceFileName() const { return str(header().SourceFileName); }
 
   /// Returns a table with all the comdats used by this file.
-  std::vector<StringRef> getComdatTable() const {
-    std::vector<StringRef> ComdatTable;
+  std::vector<std::pair<StringRef, llvm::Comdat::SelectionKind>>
+  getComdatTable() const {
+    std::vector<std::pair<StringRef, llvm::Comdat::SelectionKind>> ComdatTable;
     ComdatTable.reserve(Comdats.size());
     for (auto C : Comdats)
-      ComdatTable.push_back(str(C.Name));
+      ComdatTable.push_back({str(C.Name), llvm::Comdat::SelectionKind(
+                                              uint32_t(C.SelectionKind))});
     return ComdatTable;
   }
 
   /// COFF-specific: returns linker options specified in the input file.
   StringRef getCOFFLinkerOpts() const { return str(header().COFFLinkerOpts); }
 
   /// Returns dependent library specifiers
   std::vector<StringRef> getDependentLibraries() const {

llvm/lib/Object/IRSymtab.cpp

if (TT.isOSBinFormatCOFF()) {
       llvm::raw_string_ostream OS(Name);
       Mang.getNameWithPrefix(OS, GV, false);
     } else {
       Name = std::string(C->getName());
     }
 
     storage::Comdat Comdat;
     setStr(Comdat.Name, Saver.save(Name));
+    Comdat.SelectionKind = C->getSelectionKind();
     Comdats.push_back(Comdat);
   }
 
   return P.first->second;
 }
 
 Error Builder::addSymbol(const ModuleSymbolTable &Msymtab,
                          const SmallPtrSet<GlobalValue *, 4> &Used,

llvm/test/LTO/Resolution/X86/symtab.ll

 !llvm.linker.options = !{ !0 }
 
 ; CHECK: D------X _fun
 define i32 @fun() {
   ret i32 0
 }
 
 ; CHECK: D------X @fun2@8
-; CHECK-NEXT: comdat @fun2@8
+; CHECK-NEXT: comdat any @fun2@8
 $fun2 = comdat any
 define x86_fastcallcc i32 @fun2(i32 inreg %a, i32 inreg %b) comdat {
 entry:
   %add = add nsw i32 %b, %a
   ret i32 %add
 }
 
 ; CHECK: H------- _g1

 ; CHECK: D-----T- _g7
 @g7 = thread_local global i32 0
 
 ; CHECK: D-C----- _g8
 ; CHECK-NEXT: size 4 align 8
 @g8 = common global i32 0, align 8
 
 ; CHECK: D------- _g9
-; CHECK-NEXT: comdat _g9
+; CHECK-NEXT: comdat any _g9
 $g9 = comdat any
 @g9 = global i32 0, comdat
 
 ; CHECK-NOT: _g10
 $g10 = comdat any
 @g10 = internal global i32 0, comdat
 
 ; CHECK: D------- _g11
 ; CHECK-NOT: comdat
 @g11 = global i32 0, comdat($g10)
 
+; CHECK: D------- _g12
+; CHECK-NEXT: comdat nodeduplicate _g12
+$g12 = comdat nodeduplicate
+@g12 = global i32 0, comdat
+
 ; CHECK: D--WI--- _a1
-; CHECK-NEXT: comdat _g9
+; CHECK-NEXT: comdat any _g9
 ; CHECK-NEXT: fallback _g9
 @a1 = weak alias i32, i32* @g9

llvm/test/Object/X86/irsymtab.ll

 ; RUN: env LLVM_OVERRIDE_PRODUCER=producer opt -o %t %s
 ; RUN: llvm-bcanalyzer -dump -show-binary-blobs %t | FileCheck --check-prefix=BCA %s
 
 ; Same producer, does not require upgrade.
 ; RUN: env LLVM_OVERRIDE_PRODUCER=producer llvm-lto2 dump-symtab %t | FileCheck --check-prefix=SYMTAB %s
 
 ; Different producer, requires upgrade.
 ; RUN: env LLVM_OVERRIDE_PRODUCER=consumer llvm-lto2 dump-symtab %t | FileCheck --check-prefix=SYMTAB %s
 
 ; BCA:      <SYMTAB_BLOCK
 ; Version stored at offset 0.
-; BCA-NEXT:   <BLOB abbrevid=4/> blob data = '\x02\x00\x00\x00\x06\x00\x00\x00\x08\x00\x00\x00L\x00\x00\x00\x01\x00\x00\x00X\x00\x00\x00\x00\x00\x00\x00X\x00\x00\x00\x02\x00\x00\x00\x88\x00\x00\x00\x00\x00\x00\x00\x0E\x00\x00\x00\x18\x00\x00\x00&\x00\x00\x00\x0B\x00\x00\x00\x03\x00\x00\x00\x00\x00\x00\x00\x88\x00\x00\x00\x02\x00\x00\x00\x00\x00\x00\x00\x02\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x03\x00\x00\x00\x00\x00\x00\x00\x03\x00\x00\x00\xFF\xFF\xFF\xFF\x00$\x00\x00\x03\x00\x00\x00\x03\x00\x00\x00\x03\x00\x00\x00\x03\x00\x00\x00\xFF\xFF\xFF\xFF\x08$\x00\x00\x00\x00\x00\x00\x03\x00\x00\x00\x03\x00\x00\x00\x03\x00\x00\x00'
+; BCA-NEXT:   <BLOB abbrevid=4/> blob data = '\x03\x00\x00\x00\x06\x00\x00\x00\x08\x00\x00\x00L\x00\x00\x00\x01\x00\x00\x00X\x00\x00\x00\x00\x00\x00\x00X\x00\x00\x00\x02\x00\x00\x00\x88\x00\x00\x00\x00\x00\x00\x00\x0E\x00\x00\x00\x18\x00\x00\x00&\x00\x00\x00\x0B\x00\x00\x00\x03\x00\x00\x00\x00\x00\x00\x00\x88\x00\x00\x00\x02\x00\x00\x00\x00\x00\x00\x00\x02\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x03\x00\x00\x00\x00\x00\x00\x00\x03\x00\x00\x00\xFF\xFF\xFF\xFF\x00$\x00\x00\x03\x00\x00\x00\x03\x00\x00\x00\x03\x00\x00\x00\x03\x00\x00\x00\xFF\xFF\xFF\xFF\x08$\x00\x00\x00\x00\x00\x00\x03\x00\x00\x00\x03\x00\x00\x00\x03\x00\x00\x00'
 ; BCA-NEXT: </SYMTAB_BLOCK>
 ; BCA-NEXT: <STRTAB_BLOCK
 ; BCA-NEXT:   <BLOB abbrevid=4/> blob data = 'foobarproducerx86_64-unknown-linux-gnuirsymtab.ll'
 ; BCA-NEXT: </STRTAB_BLOCK>
 
-; SYMTAB:      version: 2
+; SYMTAB:      version: 3
 ; SYMTAB-NEXT: producer: producer
 ; SYMTAB-NEXT: target triple: x86_64-unknown-linux-gnu
 ; SYMTAB-NEXT: source filename: irsymtab.ll
 ; SYMTAB-NEXT: {{^dependent libraries: \"foo\" \"bar\"$}}
 ; SYMTAB-NEXT: D------X foo
 ; SYMTAB-NEXT: DU-----X bar
 
 target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"

llvm/test/tools/gold/X86/comdat-nodeduplicate.ll

+;; Keep __profd_foo in a nodeduplicate comdat, despite a comdat of the same name
+;; in a previous object file.
+
+;; Regular LTO
+
+; RUN: rm -rf %t && split-file %s %t
+; RUN: llvm-as %t/a.ll -o %t/a.bc
+; RUN: llvm-as %t/b.ll -o %t/b.bc
+; RUN: llvm-as %t/c.ll -o %t/c.bc
+
+; RUN: %gold -plugin %llvmshlibdir/LLVMgold%shlibext --plugin-opt=save-temps \
+; RUN:   -u foo %t/a.bc --start-lib %t/b.bc --end-lib -o %t/ab
+
+; RUN: FileCheck %s --check-prefix=RESOL_AB < %t/ab.resolution.txt
+; RUN: llvm-readelf -x .data %t/ab | FileCheck %s --check-prefix=DATA
+
+; RESOL_AB: -r={{.*}}b.bc,__profc_foo,pl{{$}}
+
+;; .data contains a.bc:data. b.bc:data and c.bc:data are discarded.
+; DATA: 0x[[#%x,]] 01000000 00000000  ........
+
+;; __profc_foo from c.bc is non-prevailing and thus discarded.
+; RUN: %gold -plugin %llvmshlibdir/LLVMgold%shlibext --plugin-opt=save-temps \
+; RUN:   -u foo -u c %t/a.bc --start-lib %t/b.bc %t/c.bc --end-lib -o %t/abc
+; RUN: FileCheck %s --check-prefix=RESOL_ABC < %t/abc.resolution.txt
+; RUN: llvm-readelf -x .data %t/abc | FileCheck %s --check-prefix=DATA
+
+; RESOL_ABC: -r={{.*}}b.bc,__profc_foo,pl{{$}}
+; RESOL_ABC: -r={{.*}}c.bc,__profc_foo,l{{$}}
+
+;; ThinLTO
+
+; RUN: rm -rf %t && split-file %s %t
+; RUN: opt --module-summary %t/a.ll -o %t/a.bc
+; RUN: opt --module-summary %t/b.ll -o %t/b.bc
+; RUN: opt --module-summary %t/c.ll -o %t/c.bc
+
+; RUN: %gold -plugin %llvmshlibdir/LLVMgold%shlibext \
+; RUN:   -u foo %t/a.bc --start-lib %t/b.bc %t/c.bc --end-lib -o %t/abc
+; RUN: llvm-readelf -x .data %t/abc | FileCheck %s --check-prefix=DATA
+
+;--- a.ll
+target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+$__profc_foo = comdat nodeduplicate
+@__profc_foo = private global i64 1, comdat, align 8
+@__profd_foo = private global i64* @__profc_foo, comdat($__profc_foo), align 8
+
+declare void @b()
+
+define i64 @foo() {
+  %v = load i64, i64* @__profc_foo
+  %inc = add i64 1, %v
+  store i64 %inc, i64* @__profc_foo
+  ret i64 %inc
+}
+
+define void @_start() {
+  call void @b()
+  ret void
+}
+
+;--- b.ll
+target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+$__profc_foo = comdat nodeduplicate
+@__profc_foo = weak hidden global i64 2, comdat, align 8
+@__profd_foo = private global i64* @__profc_foo, comdat($__profc_foo)
+
+define weak i64 @foo() {
+  %v = load i64, i64* @__profc_foo
+  %inc = add i64 1, %v
+  store i64 %inc, i64* @__profc_foo
+  ret i64 %inc
+}
+
+define void @b() {
+  ret void
+}
+
+;--- c.ll
+target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+$__profc_foo = comdat nodeduplicate
+@__profc_foo = weak hidden global i64 3, comdat, align 8
+@__profd_foo = private global i64* @__profc_foo, comdat($__profc_foo)
+
+define weak i64 @foo() {
+  %v = load i64, i64* @__profc_foo
+  %inc = add i64 1, %v
+  store i64 %inc, i64* @__profc_foo
+  ret i64 %inc
+}
+
+define void @c() {
+  ret void
+}

llvm/tools/gold/gold-plugin.cpp

else if (Sym.isWeak())
       sym.def = LDPK_WEAKDEF;
     else
       sym.def = LDPK_DEF;
 
     sym.size = 0;
     sym.comdat_key = nullptr;
     int CI = Sym.getComdatIndex();
     if (CI != -1) {
-      StringRef C = Obj->getComdatTable()[CI];
-      sym.comdat_key = strdup(C.str().c_str());
+      // Not setting comdat_key for nodeduplicate ensuress we don't deduplicate.
+      std::pair<StringRef, Comdat::SelectionKind> C = Obj->getComdatTable()[CI];
+      if (C.second != Comdat::NoDeduplicate)
+        sym.comdat_key = strdup(C.first.str().c_str());
     }
 
     sym.resolution = LDPR_UNKNOWN;
   }
 
   if (!cf.syms.empty()) {
     if (add_symbols(cf.handle, cf.syms.size(), cf.syms.data()) != LDPS_OK) {
       message(LDPL_ERROR, "Unable to add symbols!");

llvm/tools/llvm-lto2/llvm-lto2.cpp

for (StringRef F : make_range(argv + 1, argv + argc)) {
 
     if (TT.isOSBinFormatELF()) {
       outs() << "dependent libraries:";
       for (auto L : Input->getDependentLibraries())
         outs() << " \"" << L << "\"";
       outs() << '\n';
     }
 
-    std::vector<StringRef> ComdatTable = Input->getComdatTable();
+    ArrayRef<std::pair<StringRef, Comdat::SelectionKind>> ComdatTable =
+        Input->getComdatTable();
     for (const InputFile::Symbol &Sym : Input->symbols()) {
       switch (Sym.getVisibility()) {
       case GlobalValue::HiddenVisibility:
         outs() << 'H';
         break;
       case GlobalValue::ProtectedVisibility:
         outs() << 'P';
         break;
for (const InputFile::Symbol &Sym : Input->symbols()) {
       PrintBool('X', Sym.isExecutable());
       outs() << ' ' << Sym.getName() << '\n';
 
       if (Sym.isCommon())
         outs() << "         size " << Sym.getCommonSize() << " align "
                << Sym.getCommonAlignment() << '\n';
 
       int Comdat = Sym.getComdatIndex();
-      if (Comdat != -1)
-        outs() << "         comdat " << ComdatTable[Comdat] << '\n';
+      if (Comdat != -1) {
+        outs() << "         comdat ";
+        switch (ComdatTable[Comdat].second) {
+        case Comdat::Any:
+          outs() << "any";
+          break;
+        case Comdat::ExactMatch:
+          outs() << "exactmatch";
+          break;
+        case Comdat::Largest:
+          outs() << "largest";
+          break;
+        case Comdat::NoDeduplicate:
+          outs() << "nodeduplicate";
+          break;
+        case Comdat::SameSize:
+          outs() << "samesize";
+          break;
+        }
+        outs() << ' ' << ComdatTable[Comdat].first << '\n';
+      }
 
       if (TT.isOSBinFormatCOFF() && Sym.isWeak() && Sym.isIndirect())
         outs() << "         fallback " << Sym.getCOFFWeakExternalFallback() << '\n';
 
       if (!Sym.getSectionName().empty())
         outs() << "         section " << Sym.getSectionName() << "\n";
     }