Object: Teach irsymtab::read() to try to use the irsymtab that we wrote to disk.

[pcc]

Summary

Fixes PR27551.

Depends on D33973

[tejohnson]

llvm/include/llvm/Bitcode/BitcodeReader.h

112 ↗

(On Diff #101656)

Why not just "Strtab"?

llvm/lib/Bitcode/Reader/BitcodeReader.cpp

5443 ↗

(On Diff #101656)

Should this assert if we have a string table but StrtabForSymtab is already set, rather than silently skip it?

5454 ↗

(On Diff #101656)

Similarly, should this assert that the Symtab is empty? Otherwise we are just silently ignoring this symtab, which seems wrong.

llvm/test/Object/X86/irsymtab.ll

8 ↗

(On Diff #101656)

If the dumper also emitted the producer, you could check that the producer string is as expected for the two cases.

[pcc]

llvm/include/llvm/Bitcode/BitcodeReader.h

112 ↗

(On Diff #101656)

Because a bitcode file may contain multiple string tables if it was created by binary concatenation. The name reflects that it is the string table for the symbol table, not some other string table.

llvm/lib/Bitcode/Reader/BitcodeReader.cpp

5443 ↗

(On Diff #101656)

If the bitcode file was created by binary concatenation, there may be more than one string table after the symbol table.

5454 ↗

(On Diff #101656)

Again, that would assert with binary concatenation.

It's fine to silently ignore the symbol table here in that case because this is a low-level function. Later (in IRSymtab.cpp:342) we will realise that the symbol table that we chose has the wrong number of modules, so we will regenerate it.

llvm/test/Object/X86/irsymtab.ll

8 ↗

(On Diff #101656)

Yeah. I'll see if I can extend the dumper here.

[tejohnson]

llvm/include/llvm/Bitcode/BitcodeReader.h

112 ↗

(On Diff #101656)

Ok

llvm/lib/Bitcode/Reader/BitcodeReader.cpp

5443 ↗

(On Diff #101656)

Ok, can you just note that in the comment?

5454 ↗

(On Diff #101656)

Ditto about noting in a comment.

[pcc]

• Address review comments

[tejohnson]

llvm/test/Object/X86/irsymtab.ll

8 ↗

(On Diff #104299)

Wouldn't the producer string in this second case be "consumer" since it would have been upgraded?

[pcc]

llvm/test/Object/X86/irsymtab.ll

8 ↗

(On Diff #104299)

The idea is that llvm-lto2 prints the producer string from before the upgrade. That seems more useful than printing the producer from after an upgrade since that's basically equivalent to "print the current producer string".

[tejohnson]

LGTM

llvm/test/Object/X86/irsymtab.ll

8 ↗

(On Diff #104299)

Ok true

llvm/trunk/include/llvm/Bitcode/BitcodeReader.h

public:
     /// Parse the specified bitcode buffer and merge its module summary index
     /// into CombinedIndex.
     Error readSummary(ModuleSummaryIndex &CombinedIndex, StringRef ModulePath,
                       uint64_t ModuleId);
   };
 
   struct BitcodeFileContents {
     std::vector<BitcodeModule> Mods;
+    StringRef Symtab, StrtabForSymtab;
   };
 
-  /// Returns the contents of a bitcode file.
+  /// Returns the contents of a bitcode file. This includes the raw contents of
+  /// the symbol table embedded in the bitcode file. Clients which require a
+  /// symbol table should prefer to use irsymtab::read instead of this function
+  /// because it creates a reader for the irsymtab and handles upgrading bitcode
+  /// files without a symbol table or with an old symbol table.
   Expected<BitcodeFileContents> getBitcodeFileContents(MemoryBufferRef Buffer);
 
   /// Returns a list of modules in the specified bitcode buffer.
   Expected<std::vector<BitcodeModule>>
   getBitcodeModuleList(MemoryBufferRef Buffer);
 
   /// Read the header of the specified bitcode buffer and prepare for lazy
   /// deserialization of function bodies. If ShouldLazyLoadMetadata is true,

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

public:
 
   using symbol_range = iterator_range<object::content_iterator<SymbolRef>>;
 
   /// Returns the symbol table for the entire bitcode file.
   /// The symbols enumerated by this method are ephemeral, but they can be
   /// copied into an irsymtab::Symbol object.
   symbol_range symbols() const;
 
+  size_t getNumModules() const { return Modules.size(); }
+
   /// Returns a slice of the symbol table for the I'th module in the file.
   /// The symbols enumerated by this method are ephemeral, but they can be
   /// copied into an irsymtab::Symbol object.
   symbol_range module_symbols(unsigned I) const;
 
   StringRef getTargetTriple() const { return str(header().TargetTriple); }
 
   /// Returns the source file path specified at compile time.

llvm/trunk/lib/Bitcode/Reader/BitcodeReader.cpp

 } // end anonymous namespace
 
 static ManagedStatic<BitcodeErrorCategoryType> ErrorCategory;
 
 const std::error_category &llvm::BitcodeErrorCategory() {
   return *ErrorCategory;
 }
 
-static Expected<StringRef> readStrtab(BitstreamCursor &Stream) {
-  if (Stream.EnterSubBlock(bitc::STRTAB_BLOCK_ID))
+static Expected<StringRef> readBlobInRecord(BitstreamCursor &Stream,
+                                            unsigned Block, unsigned RecordID) {
+  if (Stream.EnterSubBlock(Block))
     return error("Invalid record");
 
   StringRef Strtab;
   while (1) {
     BitstreamEntry Entry = Stream.advance();
     switch (Entry.Kind) {
     case BitstreamEntry::EndBlock:
       return Strtab;
 
     case BitstreamEntry::Error:
       return error("Malformed block");
 
     case BitstreamEntry::SubBlock:
       if (Stream.SkipBlock())
         return error("Malformed block");
       break;
 
     case BitstreamEntry::Record:
       StringRef Blob;
       SmallVector<uint64_t, 1> Record;
-      if (Stream.readRecord(Entry.ID, Record, &Blob) == bitc::STRTAB_BLOB)
+      if (Stream.readRecord(Entry.ID, Record, &Blob) == RecordID)
         Strtab = Blob;
       break;
     }
   }
 }
 
 //===----------------------------------------------------------------------===//
 // External interface
case BitstreamEntry::SubBlock: {
         F.Mods.push_back({Stream.getBitcodeBytes().slice(
                               BCBegin, Stream.getCurrentByteNo() - BCBegin),
                           Buffer.getBufferIdentifier(), IdentificationBit,
                           ModuleBit});
         continue;
       }
 
       if (Entry.ID == bitc::STRTAB_BLOCK_ID) {
-        Expected<StringRef> Strtab = readStrtab(Stream);
+        Expected<StringRef> Strtab =
+            readBlobInRecord(Stream, bitc::STRTAB_BLOCK_ID, bitc::STRTAB_BLOB);
         if (!Strtab)
           return Strtab.takeError();
         // This string table is used by every preceding bitcode module that does
         // not have its own string table. A bitcode file may have multiple
         // string tables if it was created by binary concatenation, for example
         // with "llvm-cat -b".
         for (auto I = F.Mods.rbegin(), E = F.Mods.rend(); I != E; ++I) {
           if (!I->Strtab.empty())
             break;
           I->Strtab = *Strtab;
         }
+        // Similarly, the string table is used by every preceding symbol table;
+        // normally there will be just one unless the bitcode file was created
+        // by binary concatenation.
+        if (!F.Symtab.empty() && F.StrtabForSymtab.empty())
+          F.StrtabForSymtab = *Strtab;
+        continue;
+      }
+
+      if (Entry.ID == bitc::SYMTAB_BLOCK_ID) {
+        Expected<StringRef> SymtabOrErr =
+            readBlobInRecord(Stream, bitc::SYMTAB_BLOCK_ID, bitc::SYMTAB_BLOB);
+        if (!SymtabOrErr)
+          return SymtabOrErr.takeError();
+
+        // We can expect the bitcode file to have multiple symbol tables if it
+        // was created by binary concatenation. In that case we silently
+        // ignore any subsequent symbol tables, which is fine because this is a
+        // low level function. The client is expected to notice that the number
+        // of modules in the symbol table does not match the number of modules
+        // in the input file and regenerate the symbol table.
+        if (F.Symtab.empty())
+          F.Symtab = *SymtabOrErr;
         continue;
       }
 
       if (Stream.SkipBlock())
         return error("Malformed block");
       continue;
     }
     case BitstreamEntry::Record:

llvm/trunk/lib/Object/IRSymtab.cpp

static Expected<FileContents> upgrade(ArrayRef<BitcodeModule> BMs) {
   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.
+  if (BFC.StrtabForSymtab.empty() ||
+      BFC.Symtab.size() < sizeof(storage::Header))
     return upgrade(BFC.Mods);
+
+  // We cannot use the regular reader to read the version and producer, because
+  // it will expect the header to be in the current format. The only thing we
+  // can rely on is that the version and producer will be present as the first
+  // struct elements.
+  auto *Hdr = reinterpret_cast<const storage::Header *>(BFC.Symtab.data());
+  unsigned Version = Hdr->Version;
+  StringRef Producer = Hdr->Producer.get(BFC.StrtabForSymtab);
+  if (Version != storage::Header::kCurrentVersion ||
+      Producer != kExpectedProducerName)
+    return upgrade(BFC.Mods);
+
+  FileContents FC;
+  FC.TheReader = {{BFC.Symtab.data(), BFC.Symtab.size()},
+                  {BFC.StrtabForSymtab.data(), BFC.StrtabForSymtab.size()}};
+
+  // Finally, make sure that the number of modules in the symbol table matches
+  // the number of modules in the bitcode file. If they differ, it may mean that
+  // the bitcode file was created by binary concatenation, so we need to create
+  // a new symbol table from scratch.
+  if (FC.TheReader.getNumModules() != BFC.Mods.size())
+    return upgrade(std::move(BFC.Mods));
+
+  return std::move(FC);
 }

llvm/trunk/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 = '\x00\x00\x00\x00\x06\x00\x00\x00\x08\x00\x00\x00D\x00\x00\x00\x01\x00\x00\x00P\x00\x00\x00\x00\x00\x00\x00P\x00\x00\x00\x02\x00\x00\x00\x80\x00\x00\x00\x00\x00\x00\x00\x0E\x00\x00\x00\x18\x00\x00\x00&\x00\x00\x00\x0B\x00\x00\x001\x00\x00\x00\x00\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'
 ; 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: 0
+; SYMTAB-NEXT: producer: producer
+; SYMTAB-NEXT: target triple: x86_64-unknown-linux-gnu
+; SYMTAB-NEXT: source filename: irsymtab.ll
+; SYMTAB-NEXT: D------X foo
+; SYMTAB-NEXT: DU-----X bar
+
 target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
 target triple = "x86_64-unknown-linux-gnu"
 source_filename = "irsymtab.ll"
 
 define void @foo() {
   ret void
 }
 
 declare void @bar()

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

 // link-time optimization according to the provided symbol resolutions using the
 // resolution-based LTO interface, and outputs one or more object files.
 //
 // This program is intended to eventually replace llvm-lto which uses the legacy
 // LTO interface.
 //
 //===----------------------------------------------------------------------===//
 
-#include "llvm/LTO/Caching.h"
+#include "llvm/Bitcode/BitcodeReader.h"
 #include "llvm/CodeGen/CommandFlags.h"
 #include "llvm/IR/DiagnosticPrinter.h"
+#include "llvm/LTO/Caching.h"
 #include "llvm/LTO/LTO.h"
 #include "llvm/Support/CommandLine.h"
 #include "llvm/Support/FileSystem.h"
 #include "llvm/Support/TargetSelect.h"
 #include "llvm/Support/Threading.h"
 
 using namespace llvm;
 using namespace lto;
static int run(int argc, char **argv) {
 
   check(Lto.run(AddStream, Cache), "LTO::run failed");
   return 0;
 }
 
 static int dumpSymtab(int argc, char **argv) {
   for (StringRef F : make_range(argv + 1, argv + argc)) {
     std::unique_ptr<MemoryBuffer> MB = check(MemoryBuffer::getFile(F), F);
+    BitcodeFileContents BFC = check(getBitcodeFileContents(*MB), F);
+
+    if (BFC.Symtab.size() >= sizeof(irsymtab::storage::Header)) {
+      auto *Hdr = reinterpret_cast<const irsymtab::storage::Header *>(
+          BFC.Symtab.data());
+      outs() << "version: " << Hdr->Version << '\n';
+      if (Hdr->Version == irsymtab::storage::Header::kCurrentVersion)
+        outs() << "producer: " << Hdr->Producer.get(BFC.StrtabForSymtab)
+               << '\n';
+    }
+
     std::unique_ptr<InputFile> Input =
         check(InputFile::create(MB->getMemBufferRef()), F);
 
     outs() << "target triple: " << Input->getTargetTriple() << '\n';
     Triple TT(Input->getTargetTriple());
 
     outs() << "source filename: " << Input->getSourceFileName() << '\n';