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

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

Differential Revision: https://reviews.llvm.org/D106228

include/llvm/LTO/LTO.h

@@ -119,7 +119,7 @@ private:
 
   StringRef TargetTriple, SourceFileName, COFFLinkerOpts;
   std::vector<StringRef> DependentLibraries;
-  std::vector<StringRef> ComdatTable;
+  std::vector<std::pair<StringRef, Comdat::SelectionKind>> ComdatTable;
 
 public:
   ~InputFile();
@@ -172,7 +172,9 @@ public:
   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();

include/llvm/Object/IRSymtab.h

@@ -26,6 +26,7 @@
 #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"
@@ -80,6 +81,9 @@ struct Module {
 /// 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
@@ -132,7 +136,7 @@ struct Header {
   /// 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
@@ -280,11 +284,13 @@ public:
   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<std::pair<StringRef, llvm::Comdat::SelectionKind>>
-    std::vector<StringRef> ComdatTable;
+  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;
   }
 

lib/Object/IRSymtab.cpp

@@ -199,6 +199,7 @@ Expected<int> Builder::getComdatIndex(const Comdat *C, const Module *M) {
 
     storage::Comdat Comdat;
     setStr(Comdat.Name, Saver.save(Name));
+    Comdat.SelectionKind = C->getSelectionKind();
     Comdats.push_back(Comdat);
   }
 

test/LTO/Resolution/X86/symtab.ll

@@ -18,7 +18,7 @@ define i32 @fun() {
 }
 
 ; 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:
@@ -52,7 +52,7 @@ entry:
 @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
 
@@ -64,7 +64,12 @@ $g10 = comdat any
 ; 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

test/Object/X86/irsymtab.ll

@@ -9,13 +9,13 @@
 
 ; 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

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

@@ -0,0 +1,100 @@
+;; 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
+}

tools/gold/gold-plugin.cpp

@@ -623,8 +623,10 @@ static ld_plugin_status claim_file_hook(const ld_plugin_input_file *file,
     sym.comdat_key = nullptr;
     int CI = Sym.getComdatIndex();
     if (CI != -1) {
-      StringRef C = Obj->getComdatTable()[CI];
+      // Not setting comdat_key for nodeduplicate ensuress we don't deduplicate.
-      sym.comdat_key = strdup(C.str().c_str());
+      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;

tools/llvm-lto2/llvm-lto2.cpp

@@ -418,7 +418,8 @@ static int dumpSymtab(int argc, char **argv) {
       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:
@@ -447,8 +448,27 @@ static int dumpSymtab(int argc, char **argv) {
                << Sym.getCommonAlignment() << '\n';
 
       int Comdat = Sym.getComdatIndex();
-      if (Comdat != -1)
+      if (Comdat != -1) {
-        outs() << "         comdat " << ComdatTable[Comdat] << '\n';
+        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';