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

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson added inline comments.


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


================
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())
----------------
Should this assert if we have a string table but StrtabForSymtab is already set, rather than silently skip it?


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


================
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
+
----------------
If the dumper also emitted the producer, you could check that the producer string is as expected for the two cases.


https://reviews.llvm.org/D33974