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

[Peter Collingbourne via Phabricator via llvm-commits]

pcc added inline comments.


================
Comment at: llvm/include/llvm/Bitcode/BitcodeReader.h:112
     std::vector<BitcodeModule> Mods;
+    StringRef Symtab, StrtabForSymtab;
   };
----------------
tejohnson wrote:
> Why not just "Strtab"?
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.


================
Comment at: llvm/lib/Bitcode/Reader/BitcodeReader.cpp:5443
+        // Similarly, the string table is used by every preceding symbol table;
+        // normally there will be just one.
+        if (!F.Symtab.empty() && F.StrtabForSymtab.empty())
----------------
tejohnson wrote:
> Should this assert if we have a string table but StrtabForSymtab is already set, rather than silently skip it?
If the bitcode file was created by binary concatenation, there may be more than one string table after the symbol table.


================
Comment at: llvm/lib/Bitcode/Reader/BitcodeReader.cpp:5454
+          return SymtabOrErr.takeError();
+        if (F.Symtab.empty())
+          F.Symtab = *SymtabOrErr;
----------------
tejohnson wrote:
> Similarly, should this assert that the Symtab is empty? Otherwise we are just silently ignoring this symtab, which seems wrong.
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.


================
Comment at: llvm/test/Object/X86/irsymtab.ll:8
+; Different producer, requires upgrade.
+; RUN: env LLVM_OVERRIDE_PRODUCER=consumer llvm-lto2 dump-symtab %t | FileCheck --check-prefix=SYMTAB %s
+
----------------
tejohnson wrote:
> If the dumper also emitted the producer, you could check that the producer string is as expected for the two cases.
Yeah. I'll see if I can extend the dumper here.


https://reviews.llvm.org/D33974