[PATCH] D80383: Add AST_SIGNATURE record to unhashed control block of PCM files

[Daniel Grumberg via Phabricator via cfe-commits]

dang marked an inline comment as done.
dang added inline comments.


================
Comment at: clang/lib/Serialization/ASTWriter.cpp:1084
       PP.getHeaderSearchInfo().getHeaderSearchOpts().ModulesHashContent) {
-    Signature = createSignature(StringRef(Buffer.begin(), StartOfUnhashedControl));
+    Record.append(ASTSignature.begin(), ASTSignature.end());
+    Stream.EmitRecord(AST_SIGNATURE, Record);
----------------
dang wrote:
> dexonsmith wrote:
> > The default abbreviation (7-VBR) isn't great for hashes. Given that we're going to have two of these records, I suggest specifying a custom abbreviation.
> > 
> > If you were to split out a prep commit as suggested above to change `SIGNATURE` to be an array of 8-bit values (instead of the current array of 32-bit values), that would be a good opportunity to add an abbreviation.
> I think encoding the signature as a blob makes the most sense if we make it an array of 20 8-bit values, which is what the hasher gives us. However it is not quite so simple, as it would need a big change in the way direct imports are serialized. The way it is currently done embeds multiple strings and signatures within a single unabbreviated record. I can replace the unabbreviated record with a sub-block that keeps track of a string table and a record for each import containing offsets inside the string table and blobs for the signature.
But I do think it is outside the scope of this change to do this.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D80383/new/

https://reviews.llvm.org/D80383