Object: Have the irsymtab builder take a string table builder. NFCI.

[pcc]

Summary

This will be needed in order to share the irsymtab string table with
the bitcode string table.

Depends on D33969

[tejohnson]

llvm/lib/Object/IRSymtab.cpp

52 ↗

(On Diff #101651)

Why pass in the BumpPtrAllocator as well? I don't see that being shared in this or any follow-on patches?

55 ↗

(On Diff #101651)

Move this data member up above constructor that now sets it, adjacent to the other members initialized by the constructor.

241 ↗

(On Diff #101651)

Why this change? We had a StringSaver before, and weren't using it here.

[pcc]

llvm/lib/Object/IRSymtab.cpp

52 ↗

(On Diff #101651)

The BumpPtrAllocator owns any strings that are created by this code. This is necessary because the StringTableBuilder does not take ownership of any strings that are added to it. This change makes it possible for the lifetime of the strings to extend past the lifetime of the Builder.

55 ↗

(On Diff #101651)

Will do

241 ↗

(On Diff #101651)

Previously the lifetime of StrtabBuilder was effectively [1] shorter than that of the COFFLinkerOpts string, so StrtabBuilder was able to share ownership of the string with COFFLinkerOpts. Now that StrtabBuilder outlives the string, we need to save it in the StringSaver.

[1] "Effectively" meaning that we were done with StrtabBuilder before returning from build(). Of course StrtabBuilder's actual lifetime was longer because it is a field. I guess we were just getting lucky that its destructor presumably didn't need to access the string.

[tejohnson]

LGTM with the suggested comment and move of the StringSaver declaration.

llvm/lib/Object/IRSymtab.cpp

52 ↗

(On Diff #101651)

Ok, can you document this on the constructor? It would probably be clearer to have a single data structure that owns all these things that need the same lifetime (the StringTableBuilder, BumpPtrAllocator and possibly the StringSaver for simplicity), but that can be considered for a follow on refactoring.

llvm/trunk/include/llvm/Object/IRSymtab.h

 #include "llvm/Support/Error.h"
 #include <cassert>
 #include <cstdint>
 #include <vector>
 
 namespace llvm {
 
 struct BitcodeFileContents;
+class StringTableBuilder;
 
 namespace irsymtab {
 
 namespace storage {
 
 // The data structures in this namespace define the low-level serialization
 // format. Clients that just want to read a symbol table should use the
 // irsymtab::Reader class.
struct Header {
   Str TargetTriple, SourceFileName;
 
   /// COFF-specific: linker directives.
   Str COFFLinkerOpts;
 };
 
 } // end namespace storage
 
-/// Fills in Symtab and Strtab with a valid symbol and string table for Mods.
+/// Fills in Symtab and StrtabBuilder with a valid symbol and string table for
+/// Mods.
 Error build(ArrayRef<Module *> Mods, SmallVector<char, 0> &Symtab,
-            SmallVector<char, 0> &Strtab);
+            StringTableBuilder &StrtabBuilder, BumpPtrAllocator &Alloc);
 
 /// This represents a symbol that has been read from a storage::Symbol and
 /// possibly a storage::Uncommon.
 struct Symbol {
   // Copied from storage::Symbol.
   StringRef Name, IRName;
   int ComdatIndex;
   uint32_t Flags;

llvm/trunk/lib/Object/IRSymtab.cpp

 using namespace llvm;
 using namespace irsymtab;
 
 namespace {
 
 /// Stores the temporary state that is required to build an IR symbol table.
 struct Builder {
   SmallVector<char, 0> &Symtab;
-  SmallVector<char, 0> &Strtab;
+  StringTableBuilder &StrtabBuilder;
+  StringSaver Saver;
 
-  Builder(SmallVector<char, 0> &Symtab, SmallVector<char, 0> &Strtab)
-      : Symtab(Symtab), Strtab(Strtab) {}
-
-  StringTableBuilder StrtabBuilder{StringTableBuilder::RAW};
-
-  BumpPtrAllocator Alloc;
-  StringSaver Saver{Alloc};
+  // 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;
   Mangler Mang;
   Triple TT;
 
   std::vector<storage::Comdat> Comdats;
   std::vector<storage::Module> Mods;
   std::vector<storage::Symbol> Syms;
Error Builder::build(ArrayRef<Module *> IRMods) {
   setStr(Hdr.SourceFileName, IRMods[0]->getSourceFileName());
   TT = Triple(IRMods[0]->getTargetTriple());
 
   for (auto *M : IRMods)
     if (Error Err = addModule(M))
       return Err;
 
   COFFLinkerOptsOS.flush();
-  setStr(Hdr.COFFLinkerOpts, COFFLinkerOpts);
+  setStr(Hdr.COFFLinkerOpts, Saver.save(COFFLinkerOpts));
 
   // We are about to fill in the header's range fields, so reserve space for it
   // and copy it in afterwards.
   Symtab.resize(sizeof(storage::Header));
   writeRange(Hdr.Modules, Mods);
   writeRange(Hdr.Comdats, Comdats);
   writeRange(Hdr.Symbols, Syms);
   writeRange(Hdr.Uncommons, Uncommons);
 
   *reinterpret_cast<storage::Header *>(Symtab.data()) = Hdr;
-
-  raw_svector_ostream OS(Strtab);
-  StrtabBuilder.finalizeInOrder();
-  StrtabBuilder.write(OS);
-
   return Error::success();
 }
 
 } // end anonymous namespace
 
 Error irsymtab::build(ArrayRef<Module *> Mods, SmallVector<char, 0> &Symtab,
-                      SmallVector<char, 0> &Strtab) {
-  return Builder(Symtab, Strtab).build(Mods);
+                      StringTableBuilder &StrtabBuilder,
+                      BumpPtrAllocator &Alloc) {
+  return Builder(Symtab, StrtabBuilder, Alloc).build(Mods);
 }
 
 // Upgrade a vector of bitcode modules created by an old version of LLVM by
 // creating an irsymtab for them in the current format.
 static Expected<FileContents> upgrade(ArrayRef<BitcodeModule> BMs) {
   FileContents FC;
 
   LLVMContext Ctx;
   std::vector<Module *> Mods;
   std::vector<std::unique_ptr<Module>> OwnedMods;
   for (auto BM : BMs) {
     Expected<std::unique_ptr<Module>> MOrErr =
         BM.getLazyModule(Ctx, /*ShouldLazyLoadMetadata*/ true,
                          /*IsImporting*/ false);
     if (!MOrErr)
       return MOrErr.takeError();
 
     Mods.push_back(MOrErr->get());
     OwnedMods.push_back(std::move(*MOrErr));
   }
 
-  if (Error E = build(Mods, FC.Symtab, FC.Strtab))
+  StringTableBuilder StrtabBuilder(StringTableBuilder::RAW);
+  BumpPtrAllocator Alloc;
+  if (Error E = build(Mods, FC.Symtab, StrtabBuilder, Alloc))
     return std::move(E);
 
+  StrtabBuilder.finalizeInOrder();
+  FC.Strtab.resize(StrtabBuilder.getSize());
+  StrtabBuilder.write((uint8_t *)FC.Strtab.data());
+
   FC.TheReader = {{FC.Symtab.data(), FC.Symtab.size()},
                   {FC.Strtab.data(), FC.Strtab.size()}};
   return std::move(FC);
 }
 
 Expected<FileContents> irsymtab::readBitcode(const BitcodeFileContents &BFC) {
   if (BFC.Mods.empty())
     return make_error<StringError>("Bitcode file does not contain any modules",
                                    inconvertibleErrorCode());
 
   // Right now we have no on-disk representation of symbol tables, so we always
   // upgrade.
   return upgrade(BFC.Mods);
 }