Bitcode: Include any strings added to the string table in the module hash.

[tejohnson]

Looks ok but a couple of clarifying questions below.

llvm/lib/Bitcode/Writer/BitcodeWriter.cpp

956 ↗

(On Diff #105349)

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.

llvm/tools/llvm-bcanalyzer/llvm-bcanalyzer.cpp

660 ↗

(On Diff #105349)

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?

[pcc]

llvm/lib/Bitcode/Writer/BitcodeWriter.cpp

956 ↗

(On Diff #105349)

Yes, or when adding other modules to the bitcode file.

llvm/tools/llvm-bcanalyzer/llvm-bcanalyzer.cpp

660 ↗

(On Diff #105349)

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.

[tejohnson]

LGTM

llvm/lib/Bitcode/Writer/BitcodeWriter.cpp

956 ↗

(On Diff #105349)

Ah, forgot about other modules.

llvm/trunk/lib/Bitcode/Writer/BitcodeWriter.cpp

class ModuleBitcodeWriter : public BitcodeWriterBase {
   ValueEnumerator VE;
 
   /// Optional per-module index to write for ThinLTO.
   const ModuleSummaryIndex *Index;
 
   /// True if a module hash record should be written.
   bool GenerateHash;
 
+  SHA1 Hasher;
+
   /// If non-null, when GenerateHash is true, the resulting hash is written
   /// into ModHash. When GenerateHash is false, that specified value
   /// is used as the hash instead of computing from the generated bitcode.
   /// Can be used to produce the same module hash for a minimized bitcode
   /// used just for the thin link as in the regular full bitcode that will
   /// be used in the backend.
   ModuleHash *ModHash;
 
public:
   }
 
   /// Emit the current module to the bitstream.
   void write();
 
 private:
   uint64_t bitcodeStartBit() { return BitcodeStartBit; }
 
+  size_t addToStrtab(StringRef Str);
+
   void writeAttributeGroupTable();
   void writeAttributeTable();
   void writeTypeTable();
   void writeComdats();
   void writeValueSymbolTableForwardDecl();
   void writeModuleInfo();
   void writeValueAsMetadata(const ValueAsMetadata *MD,
                             SmallVectorImpl<uint64_t> &Record);
static unsigned getEncodedUnnamedAddr(const GlobalValue &GV) {
   switch (GV.getUnnamedAddr()) {
   case GlobalValue::UnnamedAddr::None:   return 0;
   case GlobalValue::UnnamedAddr::Local:  return 2;
   case GlobalValue::UnnamedAddr::Global: return 1;
   }
   llvm_unreachable("Invalid unnamed_addr");
 }
 
+size_t ModuleBitcodeWriter::addToStrtab(StringRef Str) {
+  if (GenerateHash)
+    Hasher.update(Str);
+  return StrtabBuilder.add(Str);
+}
+
 void ModuleBitcodeWriter::writeComdats() {
   SmallVector<unsigned, 64> Vals;
   for (const Comdat *C : VE.getComdats()) {
     // COMDAT: [strtab offset, strtab size, selection_kind]
-    Vals.push_back(StrtabBuilder.add(C->getName()));
+    Vals.push_back(addToStrtab(C->getName()));
     Vals.push_back(C->getName().size());
     Vals.push_back(getEncodedComdatSelectionKind(*C));
     Stream.EmitRecord(bitc::MODULE_CODE_COMDAT, Vals, /*AbbrevToUse=*/0);
     Vals.clear();
   }
 }
 
 /// Write a record that will eventually hold the word offset of the
void ModuleBitcodeWriter::writeModuleInfo() {
   // Emit the global variable information.
   for (const GlobalVariable &GV : M.globals()) {
     unsigned AbbrevToUse = 0;
 
     // GLOBALVAR: [strtab offset, strtab size, type, isconst, initid,
     //             linkage, alignment, section, visibility, threadlocal,
     //             unnamed_addr, externally_initialized, dllstorageclass,
     //             comdat, attributes]
-    Vals.push_back(StrtabBuilder.add(GV.getName()));
+    Vals.push_back(addToStrtab(GV.getName()));
     Vals.push_back(GV.getName().size());
     Vals.push_back(VE.getTypeID(GV.getValueType()));
     Vals.push_back(GV.getType()->getAddressSpace() << 2 | 2 | GV.isConstant());
     Vals.push_back(GV.isDeclaration() ? 0 :
                    (VE.getValueID(GV.getInitializer()) + 1));
     Vals.push_back(getEncodedLinkage(GV));
     Vals.push_back(Log2_32(GV.getAlignment())+1);
     Vals.push_back(GV.hasSection() ? SectionMap[GV.getSection()] : 0);
void ModuleBitcodeWriter::writeModuleInfo() {
   }
 
   // Emit the function proto information.
   for (const Function &F : M) {
     // FUNCTION:  [strtab offset, strtab size, type, callingconv, isproto,
     //             linkage, paramattrs, alignment, section, visibility, gc,
     //             unnamed_addr, prologuedata, dllstorageclass, comdat,
     //             prefixdata, personalityfn]
-    Vals.push_back(StrtabBuilder.add(F.getName()));
+    Vals.push_back(addToStrtab(F.getName()));
     Vals.push_back(F.getName().size());
     Vals.push_back(VE.getTypeID(F.getFunctionType()));
     Vals.push_back(F.getCallingConv());
     Vals.push_back(F.isDeclaration());
     Vals.push_back(getEncodedLinkage(F));
     Vals.push_back(VE.getAttributeListID(F.getAttributes()));
     Vals.push_back(Log2_32(F.getAlignment())+1);
     Vals.push_back(F.hasSection() ? SectionMap[F.getSection()] : 0);
for (const Function &F : M) {
     Stream.EmitRecord(bitc::MODULE_CODE_FUNCTION, Vals, AbbrevToUse);
     Vals.clear();
   }
 
   // Emit the alias information.
   for (const GlobalAlias &A : M.aliases()) {
     // ALIAS: [strtab offset, strtab size, alias type, aliasee val#, linkage,
     //         visibility, dllstorageclass, threadlocal, unnamed_addr]
-    Vals.push_back(StrtabBuilder.add(A.getName()));
+    Vals.push_back(addToStrtab(A.getName()));
     Vals.push_back(A.getName().size());
     Vals.push_back(VE.getTypeID(A.getValueType()));
     Vals.push_back(A.getType()->getAddressSpace());
     Vals.push_back(VE.getValueID(A.getAliasee()));
     Vals.push_back(getEncodedLinkage(A));
     Vals.push_back(getEncodedVisibility(A));
     Vals.push_back(getEncodedDLLStorageClass(A));
     Vals.push_back(getEncodedThreadLocalMode(A));
     Vals.push_back(getEncodedUnnamedAddr(A));
     unsigned AbbrevToUse = 0;
     Stream.EmitRecord(bitc::MODULE_CODE_ALIAS, Vals, AbbrevToUse);
     Vals.clear();
   }
 
   // Emit the ifunc information.
   for (const GlobalIFunc &I : M.ifuncs()) {
     // IFUNC: [strtab offset, strtab size, ifunc type, address space, resolver
     //         val#, linkage, visibility]
-    Vals.push_back(StrtabBuilder.add(I.getName()));
+    Vals.push_back(addToStrtab(I.getName()));
     Vals.push_back(I.getName().size());
     Vals.push_back(VE.getTypeID(I.getValueType()));
     Vals.push_back(I.getType()->getAddressSpace());
     Vals.push_back(VE.getValueID(I.getResolver()));
     Vals.push_back(getEncodedLinkage(I));
     Vals.push_back(getEncodedVisibility(I));
     Stream.EmitRecord(bitc::MODULE_CODE_IFUNC, Vals);
     Vals.clear();
static void writeIdentificationBlock(BitstreamWriter &Stream) {
   Stream.EmitRecord(bitc::IDENTIFICATION_CODE_EPOCH, Vals, EpochAbbrev);
   Stream.ExitBlock();
 }
 
 void ModuleBitcodeWriter::writeModuleHash(size_t BlockStartPos) {
   // Emit the module's hash.
   // MODULE_CODE_HASH: [5*i32]
   if (GenerateHash) {
-    SHA1 Hasher;
     uint32_t Vals[5];
     Hasher.update(ArrayRef<uint8_t>((const uint8_t *)&(Buffer)[BlockStartPos],
                                     Buffer.size() - BlockStartPos));
     StringRef Hash = Hasher.result();
     for (int Pos = 0; Pos < 20; Pos += 4) {
       Vals[Pos / 4] = support::endian::read32be(Hash.data() + Pos);
     }
 

llvm/trunk/test/Bitcode/Inputs/module-hash-strtab1.ll

+source_filename = "foo.c"
+
+$com = comdat any
+
+define void @main() comdat($com) {
+  call void @bar()
+  ret void
+}
+
+declare void @bar()

llvm/trunk/test/Bitcode/Inputs/module-hash-strtab2.ll

+source_filename = "foo.c"
+
+$dat = comdat any
+
+define void @main() comdat($dat) {
+  call void @foo()
+  ret void
+}
+
+declare void @foo()

llvm/trunk/test/Bitcode/module-hash-strtab.ll

+; RUN: opt -module-hash %s -o - | llvm-bcanalyzer -dump | grep '<HASH' > %t
+; RUN: opt -module-hash %S/Inputs/module-hash-strtab1.ll -o - | llvm-bcanalyzer -dump | grep '<HASH' >> %t
+; RUN: opt -module-hash %S/Inputs/module-hash-strtab2.ll -o - | llvm-bcanalyzer -dump | grep '<HASH' >> %t
+; RUN: sort %t | uniq | count 3
+
+source_filename = "foo.c"
+
+$com = comdat any
+
+define void @main() comdat($com) {
+  call void @foo()
+  ret void
+}
+
+declare void @foo()

llvm/trunk/test/Bitcode/module_hash.ll

 ; Check per module hash.
-; RUN: opt  -module-hash  %s -o - | llvm-bcanalyzer -dump | FileCheck %s --check-prefix=MOD1
+; RUN: opt  -module-hash  %s -o - | llvm-bcanalyzer -dump -check-hash=foo | FileCheck %s --check-prefix=MOD1
 ; MOD1: <HASH op0={{[0-9]*}} op1={{[0-9]*}} op2={{[0-9]*}} op3={{[0-9]*}} op4={{[0-9]*}} (match)/>
-; RUN: opt  -module-hash  %p/Inputs/module_hash.ll -o - | llvm-bcanalyzer -dump | FileCheck %s --check-prefix=MOD2
+; RUN: opt  -module-hash  %p/Inputs/module_hash.ll -o - | llvm-bcanalyzer -dump -check-hash=bar | FileCheck %s --check-prefix=MOD2
 ; MOD2: <HASH op0={{[0-9]*}} op1={{[0-9]*}} op2={{[0-9]*}} op3={{[0-9]*}} op4={{[0-9]*}} (match)/>
 
 ; Check that the hash matches in the combined index.
 
 ; First regenerate the modules with a summary
 ; RUN: opt  -module-hash -module-summary %s -o %t.m1.bc
 ; RUN: opt  -module-hash -module-summary %p/Inputs/module_hash.ll -o %t.m2.bc
 
 ; Recover the hashes from the modules themselves.
 ; RUN: llvm-bcanalyzer -dump %t.m1.bc | grep '<HASH'  > %t.hash
 ; RUN: llvm-bcanalyzer -dump %t.m2.bc | grep '<HASH'  >> %t.hash
 
 ; Generate the combined index and gather the hashes there.
 ; RUN: llvm-lto --thinlto-action=thinlink -o - %t.m1.bc %t.m2.bc | llvm-bcanalyzer -dump  | grep '<HASH ' >> %t.hash
 
 ; Validate the output now, the hahes in the individual modules and the combined index are in the same file.
 ; RUN: cat %t.hash | FileCheck %s --check-prefix=COMBINED
 
 ; First capture the value of the hash for the two modules.
-; COMBINED: <HASH op0=[[HASH1_1:[0-9]*]] op1=[[HASH1_2:[0-9]*]] op2=[[HASH1_3:[0-9]*]] op3=[[HASH1_4:[0-9]*]] op4=[[HASH1_5:[0-9]*]] (match)/>
-; COMBINED: <HASH op0=[[HASH2_1:[0-9]*]] op1=[[HASH2_2:[0-9]*]] op2=[[HASH2_3:[0-9]*]] op3=[[HASH2_4:[0-9]*]] op4=[[HASH2_5:[0-9]*]] (match)/>
+; COMBINED: <HASH op0=[[HASH1_1:[0-9]*]] op1=[[HASH1_2:[0-9]*]] op2=[[HASH1_3:[0-9]*]] op3=[[HASH1_4:[0-9]*]] op4=[[HASH1_5:[0-9]*]]/>
+; COMBINED: <HASH op0=[[HASH2_1:[0-9]*]] op1=[[HASH2_2:[0-9]*]] op2=[[HASH2_3:[0-9]*]] op3=[[HASH2_4:[0-9]*]] op4=[[HASH2_5:[0-9]*]]/>
 
 ; Validate against the value extracted from the combined index
 ; COMBINED-DAG: <HASH abbrevid={{[0-9]*}} op0=[[HASH1_1]] op1=[[HASH1_2]] op2=[[HASH1_3]] op3=[[HASH1_4]] op4=[[HASH1_5]]/>
 ; COMBINED-DAG: <HASH abbrevid={{[0-9]*}} op0=[[HASH2_1]] op1=[[HASH2_2]] op2=[[HASH2_3]] op3=[[HASH2_4]] op4=[[HASH2_5]]/>
 
 
 ; Need a function for the combined index to be populated.
 define void @foo() {
     ret void
 }

llvm/trunk/tools/llvm-bcanalyzer/llvm-bcanalyzer.cpp

 static cl::opt<std::string>
   BlockInfoFilename("block-info",
                     cl::desc("Use the BLOCK_INFO from the given file"));
 
 static cl::opt<bool>
   ShowBinaryBlobs("show-binary-blobs",
                   cl::desc("Print binary blobs using hex escapes"));
 
+static cl::opt<std::string> CheckHash(
+    "check-hash",
+    cl::desc("Check module hash using the argument as a string table"));
+
 namespace {
 
 /// CurStreamTypeType - A type for CurStreamType
 enum CurStreamTypeType {
   UnknownBitstream,
   LLVMIRBitstream
 };
 
if (DumpRecords) {
             outs() << "match)";
           else
             outs() << "mismatch: " << MetadataIndexOffset << " vs "
                    << RecordStartBit << ")";
         }
       }
 
       // If we found a module hash, let's verify that it matches!
-      if (BlockID == bitc::MODULE_BLOCK_ID && Code == bitc::MODULE_CODE_HASH) {
+      if (BlockID == bitc::MODULE_BLOCK_ID && Code == bitc::MODULE_CODE_HASH &&
+          !CheckHash.empty()) {
         if (Record.size() != 5)
           outs() << " (invalid)";
         else {
           // Recompute the hash and compare it to the one in the bitcode
           SHA1 Hasher;
           StringRef Hash;
+          Hasher.update(CheckHash);
           {
             int BlockSize = (CurrentRecordPos / 8) - BlockEntryPos;
             auto Ptr = Stream.getPointerToByte(BlockEntryPos, BlockSize);
             Hasher.update(ArrayRef<uint8_t>(Ptr, BlockSize));
             Hash = Hasher.result();
           }
           SmallString<20> RecordedHash;
           RecordedHash.resize(20);