[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;
   };
----------------
pcc wrote:
> 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.
Ok


================
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())
----------------
pcc wrote:
> 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.
Ok, can you just note that in the comment?


================
Comment at: llvm/lib/Bitcode/Reader/BitcodeReader.cpp:5454
+          return SymtabOrErr.takeError();
+        if (F.Symtab.empty())
+          F.Symtab = *SymtabOrErr;
----------------
pcc wrote:
> 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.
Ditto about noting in a comment.


https://reviews.llvm.org/D33974