Object: Improve COFF irsymtab comdat representation.

[pcc]

Summary

Change the representation of COFF comdats so that a COFF linker
is able to accurately resolve comdats between IR and native object
files. Specifically, apply name mangling to comdat names consistently
with native object files, and do not export comdats with an internal
leader because they do not affect symbol resolution.

[pcc]

• Fix use after free

[rnk]

llvm/lib/Object/IRSymtab.cpp

160

Every time I see getNameWithPrefix called without a GlobalValue, I consider it a bit buggy because it won't add the @N byte suffix for __stdcall functions. However, clang always mangles extern "C" names itself for functions with non-standard conventions, so there's no way to exercise this bug from C, it has to come from LLVM IR.

Do we care? Should we fatal error in AsmPrinter when a symbol in a comdat would require a suffix? Most languages that aren't C++ aren't going to care about this.

[rnk]

Didn't mean to push "accept"

[pcc]

llvm/lib/Object/IRSymtab.cpp

160

Thanks. For me that raised the question of how AsmPrinter mangles comdat names. I thought it just called Mangler::getNameByPrefix on the name as I did here, but it looks like in fact we look up the leader symbol using the name of the comdat and mangle its name [1].

That seems really weird to me, but for now it seems best to be consistent with that here. We might consider making a change along the lines you suggest, but that's probably an orthogonal change.

[1] http://llvm-cs.pcc.me.uk/lib/CodeGen/TargetLoweringObjectFileImpl.cpp#1027

[pcc]

• Re-implement Builder::getComdatIndex to be consistent with AsmPrinter and add error handling

[rnk]

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

70

We should be able to test it with this IR:

$f = comdat any
define weak_odr x86_fastcallcc i32 @f(i32 inreg %a, i32 inreg %b) comdat {
entry:
  %add = add nsw i32 %b, %a
  ret i32 %add
}

I guess the symtab would say @f@8. =/

[pcc]

• Add test

[rnk]

lgtm

llvm/lib/Object/IRSymtab.cpp

struct Builder {
 
   // This ctor initializes a StringSaver using the passed in BumpPtrAllocator.
   // The StringTableBuilder does not create a copy of any strings added to it,
   // so this provides somewhere to store any strings that we create.
   Builder(SmallVector<char, 0> &Symtab, StringTableBuilder &StrtabBuilder,
           BumpPtrAllocator &Alloc)
       : Symtab(Symtab), StrtabBuilder(StrtabBuilder), Saver(Alloc) {}
 
-  DenseMap<const Comdat *, unsigned> ComdatMap;
+  DenseMap<const Comdat *, int> ComdatMap;
   Mangler Mang;
   Triple TT;
 
   std::vector<storage::Comdat> Comdats;
   std::vector<storage::Module> Mods;
   std::vector<storage::Symbol> Syms;
   std::vector<storage::Uncommon> Uncommons;
 
   std::string COFFLinkerOpts;
   raw_string_ostream COFFLinkerOptsOS{COFFLinkerOpts};
 
   void setStr(storage::Str &S, StringRef Value) {
     S.Offset = StrtabBuilder.add(Value);
     S.Size = Value.size();
   }
 
   template <typename T>
   void writeRange(storage::Range<T> &R, const std::vector<T> &Objs) {
     R.Offset = Symtab.size();
     R.Size = Objs.size();
     Symtab.insert(Symtab.end(), reinterpret_cast<const char *>(Objs.data()),
                   reinterpret_cast<const char *>(Objs.data() + Objs.size()));
   }
 
+  Expected<int> getComdatIndex(const Comdat *C, const Module *M);
+
   Error addModule(Module *M);
   Error addSymbol(const ModuleSymbolTable &Msymtab,
                   const SmallPtrSet<GlobalValue *, 8> &Used,
                   ModuleSymbolTable::Symbol Sym);
 
   Error build(ArrayRef<Module *> Mods);
 };
 
Error Builder::addModule(Module *M) {
 
   for (ModuleSymbolTable::Symbol Msym : Msymtab.symbols())
     if (Error Err = addSymbol(Msymtab, Used, Msym))
       return Err;
 
   return Error::success();
 }
 
+Expected<int> Builder::getComdatIndex(const Comdat *C, const Module *M) {
+  auto P = ComdatMap.insert(std::make_pair(C, Comdats.size()));
+  if (P.second) {
+    std::string Name;
+    if (TT.isOSBinFormatCOFF()) {
+      const GlobalValue *GV = M->getNamedValue(C->getName());
+      if (!GV)
+        return make_error<StringError>("Could not find leader",
+                                       inconvertibleErrorCode());
+      // Internal leaders do not affect symbol resolution, therefore they do not
+      // appear in the symbol table.
+      if (GV->hasLocalLinkage()) {
+        P.first->second = -1;
+        return -1;
+      }
+      llvm::raw_string_ostream OS(Name);
+      Mang.getNameWithPrefix(OS, GV, false);
+    } else {
+      Name = C->getName();
+    }
+
+    storage::Comdat Comdat;
+    setStr(Comdat.Name, Saver.save(Name));
+    Comdats.push_back(Comdat);
+  }
+
+  return P.first->second;
+}
+
 Error Builder::addSymbol(const ModuleSymbolTable &Msymtab,
                          const SmallPtrSet<GlobalValue *, 8> &Used,
                          ModuleSymbolTable::Symbol Msym) {
   Syms.emplace_back();
   storage::Symbol &Sym = Syms.back();
   Sym = {};
 
   storage::Uncommon *Unc = nullptr;
if (Flags & object::BasicSymbolRef::SF_Common) {
     Uncommon().CommonAlign = GV->getAlignment();
   }
 
   const GlobalObject *Base = GV->getBaseObject();
   if (!Base)
     return make_error<StringError>("Unable to determine comdat of alias!",
                                    inconvertibleErrorCode());
   if (const Comdat *C = Base->getComdat()) {
-    auto P = ComdatMap.insert(std::make_pair(C, Comdats.size()));
-    Sym.ComdatIndex = P.first->second;
-
-    if (P.second) {
-      storage::Comdat Comdat;
-      setStr(Comdat.Name, C->getName());
-      Comdats.push_back(Comdat);
-    }
+    Expected<int> ComdatIndexOrErr = getComdatIndex(C, GV->getParent());
+    if (!ComdatIndexOrErr)
+      return ComdatIndexOrErr.takeError();
+    Sym.ComdatIndex = *ComdatIndexOrErr;
   }
 
   if (TT.isOSBinFormatCOFF()) {
     emitLinkerFlagsForGlobalCOFF(COFFLinkerOptsOS, GV, TT, Mang);
 
     if ((Flags & object::BasicSymbolRef::SF_Weak) &&
         (Flags & object::BasicSymbolRef::SF_Indirect)) {
       auto *Fallback = dyn_cast<GlobalValue>(

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

 !0 = !{!"/include:foo"}
 !llvm.linker.options = !{ !0 }
 
 ; CHECK: D------X _fun
 define i32 @fun() {
   ret i32 0
 }
 
+; CHECK: D------X @fun2@8
+; CHECK-NEXT: comdat @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
 @g1 = hidden global i32 0
 
 ; CHECK: P------- _g2
 @g2 = protected global i32 0
 
 ; CHECK: D------- _g3
 @g3 = global i32 0

 ; 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 _g9
 $g9 = comdat any
 @g9 = global i32 0, comdat
 
-; CHECK: D--WI--- _g10
-; CHECK-NEXT: comdat g9
+; 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--WI--- _a1
+; CHECK-NEXT: comdat _g9
 ; CHECK-NEXT: fallback _g9
-@g10 = weak alias i32, i32* @g9
+@a1 = weak alias i32, i32* @g9