[PATCH] D35037: Bitcode: Include any strings added to the string table in the module hash.

[Peter Collingbourne via Phabricator via llvm-commits]

pcc added inline comments.


================
Comment at: llvm/lib/Bitcode/Writer/BitcodeWriter.cpp:956
+  if (GenerateHash)
+    Hasher.update(Str);
+  return StrtabBuilder.add(Str);
----------------
tejohnson wrote:
> Out of curiosity, couldn't the Hasher be updated with the contents of the StrtabBuilder when we writeModuleHash? I guess that would require finalizing the StrtabBuilder a little earlier - ah, we need to add to the StrtabBuilder when building the symtab. 
> 
Yes, or when adding other modules to the bitcode file.


================
Comment at: llvm/tools/llvm-bcanalyzer/llvm-bcanalyzer.cpp:660
+      if (BlockID == bitc::MODULE_BLOCK_ID && Code == bitc::MODULE_CODE_HASH &&
+          !CheckHash.empty()) {
         if (Record.size() != 5)
----------------
tejohnson wrote:
> It's unfortunate that we need to explicitly pass in the expected strtab contents rather than just using the STRTAB_BLOB, but I guess here too the issue is that the final strtab included strings that were in the symtab that we didn't have when writing the module hash?
Right. We could also in principle teach llvm-bcanalyzer about the various records that may refer to the string table, and have it include the hashes of those strings in the hash it computes here, but that seems more complicated than called for here (for one thing, it would require multiple passes over the bitcode file).

I guess that requiring the user to provide the string table also has the side benefit of having the test verify that the string table contents are as expected.


https://reviews.llvm.org/D35037