Make the LTO comdat api more symbol table friendly

[• rafael]

Summary

In an IR symbol table I would expect the comdats to be represented as:

• A table of strings, one for each comdat name.
• Each symbol has an optional index into that table.

The natural api for accessing that would be

InputFile:
ArrayRef<StringRef> getComdats() const;

Symbol:
int getComdat() const;

This patch implements an API as close to that as possible. The implementation on top of the current IRObjectFile is a bit hackish, but should map just fine over a symbol table and is very convenient to use (I will upload the lld patch).

[mehdi_amini]

While the symbols in the "on disk" representation would have an int pointing to a table, it is not clear to me why you couldn't return directly a StringRef from the symbol?

[davide]

lib/LTO/LTO.cpp

231

Can you please document this API?
e.g. we return if the symbol has no GlobalValue associated etc..

[davide]

Ideally all the APIs in this file should be documented (at least the non-trivial ones).
We don't return a comdat pointer but a comdat handle/identifier, so, maybe the API should be renamed accordingly to getComdatIndex/Identifier or something?

[• rafael]

Use better names and add comments.

[davide]

Thank you for taking care of my comments. I'm happy with it. Mehdi, what do you think?

include/llvm/LTO/LTO.h

185–188

Uh, this is not ideal. Hopefully we can get rid of it once we migrate to a bitcode symtab.

[mehdi_amini]

Fine with me!

[mehdi_amini]

There are no changes in llvm-lto2, that makes me worried on the coverage of these code path.

include/llvm/LTO/LTO.h

188

The FIXME is not very clear "the way aliases are implemented" -> I have no idea what this is about.

[mehdi_amini]

lib/LTO/LTO.cpp

239

The error looks like the caller is not supposed to call this API under some circumstances, but it is not clear to me how it is supposed to figure, or recover.

[mehdi_amini]

I didn't look at the state before, most of this is mechanically translated.

The fact that llvm-lto2 is not testing this API is still an issue, but shouldn't block this.

If you can improve the FIXME to better describe the error situation that'd be great.

LGTM.

[mehdi_amini]

Was this committed? If yes can you add the revision here?

[mehdi_amini]

(From Rafael on the mailing list) r285061

include/llvm/LTO/LTO.h

 class InputFile {
   // FIXME: Remove LTO class friendship once we have bitcode symbol tables.
   friend LTO;
   InputFile() = default;
 
   // FIXME: Remove the LLVMContext once we have bitcode symbol tables.
   LLVMContext Ctx;
   std::unique_ptr<object::IRObjectFile> Obj;
+  std::vector<StringRef> Comdats;
+  DenseMap<const Comdat *, unsigned> ComdatMap;
 
 public:
   /// Create an InputFile.
   static Expected<std::unique_ptr<InputFile>> create(MemoryBufferRef Object);
 
   class symbol_iterator;
 
   /// This is a wrapper for object::basic_symbol_iterator that exposes only the
   /// information that an LTO client should need in order to do symbol
   /// resolution.
   ///
   /// This object is ephemeral; it is only valid as long as an iterator obtained
   /// from symbols() refers to it.
   class Symbol {
     friend symbol_iterator;
     friend LTO;
 
     object::basic_symbol_iterator I;
+    const InputFile *File;
     const GlobalValue *GV;
     uint32_t Flags;
     SmallString<64> Name;
 
     bool shouldSkip() {
       return !(Flags & object::BasicSymbolRef::SF_Global) ||
              (Flags & object::BasicSymbolRef::SF_FormatSpecific);
     }
void skip() {
       {
         raw_svector_ostream OS(Name);
         I->printName(OS);
       }
       GV = cast<object::IRObjectFile>(Obj)->getSymbolGV(I->getRawDataRefImpl());
     }
 
   public:
-    Symbol(object::basic_symbol_iterator I) : I(I) { skip(); }
+    Symbol(object::basic_symbol_iterator I, const InputFile *File)
+        : I(I), File(File) {
+      skip();
+    }
 
     StringRef getName() const { return Name; }
     StringRef getIRName() const {
       if (GV)
         return GV->getName();
       return StringRef();
     }
     uint32_t getFlags() const { return Flags; }
     GlobalValue::VisibilityTypes getVisibility() const {
       if (GV)
         return GV->getVisibility();
       return GlobalValue::DefaultVisibility;
     }
     bool canBeOmittedFromSymbolTable() const {
       return GV && llvm::canBeOmittedFromSymbolTable(GV);
     }
     bool isTLS() const {
       // FIXME: Expose a thread-local flag for module asm symbols.
       return GV && GV->isThreadLocal();
     }
 
-    Expected<StringRef> getComdat() const {
-      if (!GV)
-        return "";
-      const GlobalObject *GO;
-      if (auto *GA = dyn_cast<GlobalAlias>(GV)) {
-        GO = GA->getBaseObject();
-        if (!GO)
-          return make_error<StringError>("Unable to determine comdat of alias!",
-                                         inconvertibleErrorCode());
-      } else {
-        GO = cast<GlobalObject>(GV);
-      }
-      if (const Comdat *C = GO->getComdat())
-        return C->getName();
-      return "";
-    }
+    // Returns the index of the comdat this symbol is in or -1 if the symbol
+    // is not in a comdat.
+    // FIXME: We have to return Expected<int> because of the way aliases are
+    // implemented and the lack of a IR symbol table.
+    Expected<int> getComdatIndex() const;
 
     uint64_t getCommonSize() const {
       assert(Flags & object::BasicSymbolRef::SF_Common);
       if (!GV)
         return 0;
       return GV->getParent()->getDataLayout().getTypeAllocSize(
           GV->getType()->getElementType());
     }
     unsigned getCommonAlignment() const {
       assert(Flags & object::BasicSymbolRef::SF_Common);
       if (!GV)
         return 0;
       return GV->getAlignment();
     }
   };
 
   class symbol_iterator {
     Symbol Sym;
 
   public:
-    symbol_iterator(object::basic_symbol_iterator I) : Sym(I) {}
+    symbol_iterator(object::basic_symbol_iterator I, const InputFile *File)
+        : Sym(I, File) {}
 
     symbol_iterator &operator++() {
       ++Sym.I;
       Sym.skip();
       return *this;
     }
 
     symbol_iterator operator++(int) {
       symbol_iterator I = *this;
       ++*this;
       return I;
     }
 
     const Symbol &operator*() const { return Sym; }
     const Symbol *operator->() const { return &Sym; }
 
     bool operator!=(const symbol_iterator &Other) const {
       return Sym.I != Other.Sym.I;
     }
   };
 
   /// A range over the symbols in this InputFile.
   iterator_range<symbol_iterator> symbols() {
-    return llvm::make_range(symbol_iterator(Obj->symbol_begin()),
-                            symbol_iterator(Obj->symbol_end()));
+    return llvm::make_range(symbol_iterator(Obj->symbol_begin(), this),
+                            symbol_iterator(Obj->symbol_end(), this));
   }
 
   StringRef getDataLayoutStr() const {
     return Obj->getModule().getDataLayoutStr();
   }
 
   StringRef getSourceFileName() const {
     return Obj->getModule().getSourceFileName();
   }
 
   MemoryBufferRef getMemoryBufferRef() const {
     return Obj->getMemoryBufferRef();
   }
+
+  // Returns a table with all the comdats used by this file.
+  ArrayRef<StringRef> getComdatTable() const { return Comdats; }
 };
 
 /// This class wraps an output stream for a native object. Most clients should
 /// just be able to return an instance of this base class from the stream
 /// callback, but if a client needs to perform some action after the stream is
 /// written to, that can be done by deriving from this class and overriding the
 /// destructor.
 class NativeObjectStream {

lib/LTO/LTO.cpp

Expected<std::unique_ptr<InputFile>> InputFile::create(MemoryBufferRef Object) {
   if (!Msg.empty())
     return make_error<StringError>(Msg, inconvertibleErrorCode());
   if (!IRObj)
     return errorCodeToError(IRObj.getError());
   File->Obj = std::move(*IRObj);
 
   File->Ctx.setDiagnosticHandler(nullptr, nullptr);
 
+  for (const auto &C : File->Obj->getModule().getComdatSymbolTable()) {
+    auto P =
+        File->ComdatMap.insert(std::make_pair(&C.second, File->Comdats.size()));
+    assert(P.second);
+    File->Comdats.push_back(C.first());
+  }
+
   return std::move(File);
 }
 
+Expected<int> InputFile::Symbol::getComdatIndex() const {
+  if (!GV)
+    return -1;
+  const GlobalObject *GO;
+  if (auto *GA = dyn_cast<GlobalAlias>(GV)) {
+    GO = GA->getBaseObject();
+    if (!GO)
+      return make_error<StringError>("Unable to determine comdat of alias!",
+                                     inconvertibleErrorCode());
+  } else {
+    GO = cast<GlobalObject>(GV);
+  }
+  if (const Comdat *C = GO->getComdat()) {
+    auto I = File->ComdatMap.find(C);
+    assert(I != File->ComdatMap.end());
+    return I->second;
+  }
+  return -1;
+}
+
 LTO::RegularLTOState::RegularLTOState(unsigned ParallelCodeGenParallelismLevel,
                                       Config &Conf)
     : ParallelCodeGenParallelismLevel(ParallelCodeGenParallelismLevel),
       Ctx(Conf) {}
 
 LTO::ThinLTOState::ThinLTOState(ThinBackend Backend) : Backend(Backend) {
   if (!Backend)
     this->Backend =
Error LTO::addRegularLTO(std::unique_ptr<InputFile> Input,
   std::vector<GlobalValue *> Keep;
 
   for (GlobalVariable &GV : M.globals())
     if (GV.hasAppendingLinkage())
       Keep.push_back(&GV);
 
   auto ResI = Res.begin();
   for (const InputFile::Symbol &Sym :
-       make_range(InputFile::symbol_iterator(Obj->symbol_begin()),
-                  InputFile::symbol_iterator(Obj->symbol_end()))) {
+       make_range(InputFile::symbol_iterator(Obj->symbol_begin(), nullptr),
+                  InputFile::symbol_iterator(Obj->symbol_end(), nullptr))) {
     assert(ResI != Res.end());
     SymbolResolution Res = *ResI++;
     addSymbolToGlobalRes(Obj.get(), Used, Sym, Res, 0);
 
     GlobalValue *GV = Obj->getSymbolGV(Sym.I->getRawDataRefImpl());
     if (Sym.getFlags() & object::BasicSymbolRef::SF_Undefined)
       continue;
     if (Res.Prevailing && GV) {

tools/gold/gold-plugin.cpp

if (Symflags & object::BasicSymbolRef::SF_Undefined) {
       sym.def = LDPK_COMMON;
     else if (Symflags & object::BasicSymbolRef::SF_Weak)
       sym.def = LDPK_WEAKDEF;
     else
       sym.def = LDPK_DEF;
 
     sym.size = 0;
     sym.comdat_key = nullptr;
-    StringRef C = check(Sym.getComdat());
-    if (!C.empty())
+    int CI = check(Sym.getComdatIndex());
+    if (CI != -1) {
+      StringRef C = Obj->getComdatTable()[CI];
       sym.comdat_key = strdup(C.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!");
       return LDPS_ERR;