Revert "[clang][modules] Move UNHASHED_CONTROL_BLOCK up in the AST file"

This reverts commit 7d15657.

Author: jansvoboda11

clang/include/clang/Basic/Module.h

@@ -81,12 +81,6 @@ struct ASTFileSignature : std::array<uint8_t, 20> {
     return Sentinel;
   }
 
-  static ASTFileSignature createDummy() {
-    ASTFileSignature Dummy;
-    Dummy.fill(0x00);
-    return Dummy;
-  }
-
   template <typename InputIt>
   static ASTFileSignature create(InputIt First, InputIt Last) {
     assert(std::distance(First, Last) == size &&

clang/include/clang/Serialization/ASTBitCodes.h

@@ -41,7 +41,7 @@ namespace serialization {
 /// Version 4 of AST files also requires that the version control branch and
 /// revision match exactly, since there is no backward compatibility of
 /// AST files at this time.
-const unsigned VERSION_MAJOR = 29;
+const unsigned VERSION_MAJOR = 28;
 
 /// AST file minor version number supported by this version of
 /// Clang.

clang/include/clang/Serialization/ASTWriter.h

@@ -128,17 +128,10 @@ class ASTWriter : public ASTDeserializationListener,
   /// The module we're currently writing, if any.
   Module *WritingModule = nullptr;
 
-  /// The byte range representing all the UNHASHED_CONTROL_BLOCK.
-  std::pair<uint64_t, uint64_t> UnhashedControlBlockRange;
-  /// The bit offset of the AST block hash blob.
-  uint64_t ASTBlockHashOffset = 0;
-  /// The bit offset of the signature blob.
-  uint64_t SignatureOffset = 0;
-
-  /// The bit offset of the first bit inside the AST_BLOCK.
+  /// The offset of the first bit inside the AST_BLOCK.
   uint64_t ASTBlockStartOffset = 0;
 
-  /// The byte range representing all the AST_BLOCK.
+  /// The range representing all the AST_BLOCK.
   std::pair<uint64_t, uint64_t> ASTBlockRange;
 
   /// The base directory for any relative paths we emit.
@@ -502,11 +495,12 @@ class ASTWriter : public ASTDeserializationListener,
                          StringRef isysroot);
 
   /// Write out the signature and diagnostic options, and return the signature.
-  void writeUnhashedControlBlock(Preprocessor &PP, ASTContext &Context);
-  ASTFileSignature backpatchSignature();
+  ASTFileSignature writeUnhashedControlBlock(Preprocessor &PP,
+                                             ASTContext &Context);
 
   /// Calculate hash of the pcm content.
-  std::pair<ASTFileSignature, ASTFileSignature> createSignature() const;
+  static std::pair<ASTFileSignature, ASTFileSignature>
+  createSignature(StringRef AllBytes, StringRef ASTBlockBytes);
 
   void WriteInputFiles(SourceManager &SourceMgr, HeaderSearchOptions &HSOpts);
   void WriteSourceManagerBlock(SourceManager &SourceMgr,

clang/lib/Serialization/ASTReader.cpp

@@ -4734,6 +4734,12 @@ ASTReader::ReadASTCore(StringRef FileName,
       ShouldFinalizePCM = true;
       return Success;
 
+    case UNHASHED_CONTROL_BLOCK_ID:
+      // This block is handled using look-ahead during ReadControlBlock.  We
+      // shouldn't get here!
+      Error("malformed block record in AST file");
+      return Failure;
+
     default:
       if (llvm::Error Err = Stream.SkipBlock()) {
         Error(std::move(Err));
@@ -4853,18 +4859,13 @@ ASTReader::ASTReadResult ASTReader::readUnhashedControlBlockImpl(
     }
     switch ((UnhashedControlBlockRecordTypes)MaybeRecordType.get()) {
     case SIGNATURE:
-      if (F) {
-        F->Signature = ASTFileSignature::create(Blob.begin(), Blob.end());
-        assert(F->Signature != ASTFileSignature::createDummy() &&
-               "Dummy AST file signature not backpatched in ASTWriter.");
-      }
+      if (F)
+        F->Signature = ASTFileSignature::create(Record.begin(), Record.end());
       break;
     case AST_BLOCK_HASH:
-      if (F) {
-        F->ASTBlockHash = ASTFileSignature::create(Blob.begin(), Blob.end());
-        assert(F->ASTBlockHash != ASTFileSignature::createDummy() &&
-               "Dummy AST block hash not backpatched in ASTWriter.");
-      }
+      if (F)
+        F->ASTBlockHash =
+            ASTFileSignature::create(Record.begin(), Record.end());
       break;
     case DIAGNOSTIC_OPTIONS: {
       bool Complain = (ClientLoadCapabilities & ARR_OutOfDate) == 0;
@@ -5166,12 +5167,9 @@ static ASTFileSignature readASTFileSignature(StringRef PCH) {
       consumeError(MaybeRecord.takeError());
       return ASTFileSignature();
     }
-    if (SIGNATURE == MaybeRecord.get()) {
-      auto Signature = ASTFileSignature::create(Blob.begin(), Blob.end());
-      assert(Signature != ASTFileSignature::createDummy() &&
-             "Dummy AST file signature not backpatched in ASTWriter.");
-      return Signature;
-    }
+    if (SIGNATURE == MaybeRecord.get())
+      return ASTFileSignature::create(Record.begin(),
+                                      Record.begin() + ASTFileSignature::size);
   }
 }
 

clang/lib/Serialization/ASTWriter.cpp

@@ -1120,97 +1120,50 @@ adjustFilenameForRelocatableAST(const char *Filename, StringRef BaseDir) {
 }
 
 std::pair<ASTFileSignature, ASTFileSignature>
-ASTWriter::createSignature() const {
-  StringRef AllBytes(Buffer.data(), Buffer.size());
-
+ASTWriter::createSignature(StringRef AllBytes, StringRef ASTBlockBytes) {
   llvm::SHA1 Hasher;
-  Hasher.update(AllBytes.slice(ASTBlockRange.first, ASTBlockRange.second));
+  Hasher.update(ASTBlockBytes);
   ASTFileSignature ASTBlockHash = ASTFileSignature::create(Hasher.result());
 
-  // Add the remaining bytes:
-  //  1. Before the unhashed control block.
-  Hasher.update(AllBytes.slice(0, UnhashedControlBlockRange.first));
-  //  2. Between the unhashed control block and the AST block.
+  // Add the remaining bytes (i.e. bytes before the unhashed control block that
+  // are not part of the AST block).
+  Hasher.update(
+      AllBytes.take_front(ASTBlockBytes.bytes_end() - AllBytes.bytes_begin()));
   Hasher.update(
-      AllBytes.slice(UnhashedControlBlockRange.second, ASTBlockRange.first));
-  //  3. After the AST block.
-  Hasher.update(AllBytes.slice(ASTBlockRange.second, StringRef::npos));
+      AllBytes.take_back(AllBytes.bytes_end() - ASTBlockBytes.bytes_end()));
   ASTFileSignature Signature = ASTFileSignature::create(Hasher.result());
 
   return std::make_pair(ASTBlockHash, Signature);
 }
 
-ASTFileSignature ASTWriter::backpatchSignature() {
-  if (!WritingModule ||
-      !PP->getHeaderSearchInfo().getHeaderSearchOpts().ModulesHashContent)
-    return {};
-
-  // For implicit modules, write the hash of the PCM as its signature.
-
-  auto BackpatchSignatureAt = [&](const ASTFileSignature &S, uint64_t BitNo) {
-    using WordT = unsigned;
-    std::array<WordT, sizeof(ASTFileSignature) / sizeof(WordT)> Words;
-    static_assert(sizeof(Words) == sizeof(S));
-    std::memcpy(Words.data(), S.data(), sizeof(ASTFileSignature));
-    for (WordT Word : Words) {
-      Stream.BackpatchWord(BitNo, Word);
-      BitNo += sizeof(WordT) * 8;
-    }
-  };
-
-  ASTFileSignature ASTBlockHash;
-  ASTFileSignature Signature;
-  std::tie(ASTBlockHash, Signature) = createSignature();
-
-  BackpatchSignatureAt(ASTBlockHash, ASTBlockHashOffset);
-  BackpatchSignatureAt(Signature, SignatureOffset);
-
-  return Signature;
-}
-
-void ASTWriter::writeUnhashedControlBlock(Preprocessor &PP,
-                                          ASTContext &Context) {
+ASTFileSignature ASTWriter::writeUnhashedControlBlock(Preprocessor &PP,
+                                                      ASTContext &Context) {
   using namespace llvm;
 
   // Flush first to prepare the PCM hash (signature).
   Stream.FlushToWord();
-  UnhashedControlBlockRange.first = Stream.GetCurrentBitNo() >> 3;
+  auto StartOfUnhashedControl = Stream.GetCurrentBitNo() >> 3;
 
   // Enter the block and prepare to write records.
   RecordData Record;
   Stream.EnterSubblock(UNHASHED_CONTROL_BLOCK_ID, 5);
 
   // For implicit modules, write the hash of the PCM as its signature.
+  ASTFileSignature Signature;
   if (WritingModule &&
       PP.getHeaderSearchInfo().getHeaderSearchOpts().ModulesHashContent) {
-    // At this point, we don't know the actual signature of the file or the AST
-    // block - we're only able to compute those at the end of the serialization
-    // process. Let's store dummy signatures for now, and replace them with the
-    // real ones later on.
-    // The bitstream VBR-encodes record elements, which makes backpatching them
-    // really difficult. Let's store the signatures as blobs instead - they are
-    // guaranteed to be word-aligned, and we control their format/encoding.
-    auto Dummy = ASTFileSignature::createDummy();
-    SmallString<128> Blob{Dummy.begin(), Dummy.end()};
-
-    auto Abbrev = std::make_shared<BitCodeAbbrev>();
-    Abbrev->Add(BitCodeAbbrevOp(AST_BLOCK_HASH));
-    Abbrev->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Blob));
-    unsigned ASTBlockHashAbbrev = Stream.EmitAbbrev(std::move(Abbrev));
-
-    Abbrev = std::make_shared<BitCodeAbbrev>();
-    Abbrev->Add(BitCodeAbbrevOp(SIGNATURE));
-    Abbrev->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Blob));
-    unsigned SignatureAbbrev = Stream.EmitAbbrev(std::move(Abbrev));
-
-    Record.push_back(AST_BLOCK_HASH);
-    Stream.EmitRecordWithBlob(ASTBlockHashAbbrev, Record, Blob);
-    ASTBlockHashOffset = Stream.GetCurrentBitNo() - Blob.size() * 8;
+    ASTFileSignature ASTBlockHash;
+    auto ASTBlockStartByte = ASTBlockRange.first >> 3;
+    auto ASTBlockByteLength = (ASTBlockRange.second >> 3) - ASTBlockStartByte;
+    std::tie(ASTBlockHash, Signature) = createSignature(
+        StringRef(Buffer.begin(), StartOfUnhashedControl),
+        StringRef(Buffer.begin() + ASTBlockStartByte, ASTBlockByteLength));
+
+    Record.append(ASTBlockHash.begin(), ASTBlockHash.end());
+    Stream.EmitRecord(AST_BLOCK_HASH, Record);
     Record.clear();
-
-    Record.push_back(SIGNATURE);
-    Stream.EmitRecordWithBlob(SignatureAbbrev, Record, Blob);
-    SignatureOffset = Stream.GetCurrentBitNo() - Blob.size() * 8;
+    Record.append(Signature.begin(), Signature.end());
+    Stream.EmitRecord(SIGNATURE, Record);
     Record.clear();
   }
 
@@ -1279,7 +1232,7 @@ void ASTWriter::writeUnhashedControlBlock(Preprocessor &PP,
 
   // Leave the options block.
   Stream.ExitBlock();
-  UnhashedControlBlockRange.second = Stream.GetCurrentBitNo() >> 3;
+  return Signature;
 }
 
 /// Write the control block.
@@ -4737,7 +4690,6 @@ ASTFileSignature ASTWriter::WriteASTCore(Sema &SemaRef, StringRef isysroot,
 
   ASTContext &Context = SemaRef.Context;
   Preprocessor &PP = SemaRef.PP;
-  writeUnhashedControlBlock(PP, Context);
 
   collectNonAffectingInputFiles();
 
@@ -4886,7 +4838,7 @@ ASTFileSignature ASTWriter::WriteASTCore(Sema &SemaRef, StringRef isysroot,
 
   // Write the remaining AST contents.
   Stream.FlushToWord();
-  ASTBlockRange.first = Stream.GetCurrentBitNo() >> 3;
+  ASTBlockRange.first = Stream.GetCurrentBitNo();
   Stream.EnterSubblock(AST_BLOCK_ID, 5);
   ASTBlockStartOffset = Stream.GetCurrentBitNo();
 
@@ -5239,13 +5191,13 @@ ASTFileSignature ASTWriter::WriteASTCore(Sema &SemaRef, StringRef isysroot,
   Stream.EmitRecord(STATISTICS, Record);
   Stream.ExitBlock();
   Stream.FlushToWord();
-  ASTBlockRange.second = Stream.GetCurrentBitNo() >> 3;
+  ASTBlockRange.second = Stream.GetCurrentBitNo();
 
   // Write the module file extension blocks.
   for (const auto &ExtWriter : ModuleFileExtensionWriters)
     WriteModuleFileExtension(SemaRef, *ExtWriter);
 
-  return backpatchSignature();
+  return writeUnhashedControlBlock(PP, Context);
 }
 
 void ASTWriter::WriteDeclUpdatesBlocks(RecordDataImpl &OffsetsRecord) {

clang/lib/Serialization/GlobalModuleIndex.cpp

@@ -697,12 +697,9 @@ llvm::Error GlobalModuleIndexBuilder::loadModuleFile(const FileEntry *File) {
     }
 
     // Get Signature.
-    if (State == DiagnosticOptionsBlock && Code == SIGNATURE) {
-      auto Signature = ASTFileSignature::create(Blob.begin(), Blob.end());
-      assert(Signature != ASTFileSignature::createDummy() &&
-             "Dummy AST file signature not backpatched in ASTWriter.");
-      getModuleFileInfo(File).Signature = Signature;
-    }
+    if (State == DiagnosticOptionsBlock && Code == SIGNATURE)
+      getModuleFileInfo(File).Signature = ASTFileSignature::create(
+          Record.begin(), Record.begin() + ASTFileSignature::size);
 
     // We don't care about this record.
   }

clang/test/Modules/ASTSignature.c

@@ -1,25 +1,24 @@
 // RUN: rm -rf %t
-// RUN: %clang_cc1             -iquote %S/Inputs/ASTHash/ -fsyntax-only \
-// RUN:   -fmodules -fimplicit-module-maps -fmodules-strict-context-hash \
+// RUN: %clang_cc1 -iquote %S/Inputs/ASTHash/ -fsyntax-only -fmodules \
+// RUN:   -fimplicit-module-maps -fmodules-strict-context-hash \
 // RUN:   -fmodules-cache-path=%t -fdisable-module-hash %s
 // RUN: cp %t/MyHeader2.pcm %t1.pcm
 // RUN: rm -rf %t
 // RUN: %clang_cc1 -nostdinc++ -iquote %S/Inputs/ASTHash/ -fsyntax-only \
 // RUN:   -fmodules -fimplicit-module-maps -fmodules-strict-context-hash \
 // RUN:   -fmodules-cache-path=%t -fdisable-module-hash %s
 // RUN: cp %t/MyHeader2.pcm %t2.pcm
-// RUN: llvm-bcanalyzer --dump --disable-histogram --show-binary-blobs %t1.pcm > %t1.dump
-// RUN: llvm-bcanalyzer --dump --disable-histogram --show-binary-blobs %t2.pcm > %t2.dump
+// RUN: llvm-bcanalyzer --dump --disable-histogram %t1.pcm > %t1.dump
+// RUN: llvm-bcanalyzer --dump --disable-histogram %t2.pcm > %t2.dump
 // RUN: cat %t1.dump %t2.dump | FileCheck %s
 
 #include "my_header_2.h"
 
 my_int var = 42;
 
-// CHECK:     <AST_BLOCK_HASH abbrevid={{[0-9]+}}/> blob data = '[[AST_BLOCK_HASH:.*]]'
-// CHECK:     <SIGNATURE abbrevid={{[0-9]+}}/> blob data = '[[SIGNATURE:.*]]'
-// CHECK:     <AST_BLOCK_HASH abbrevid={{[0-9]+}}/> blob data = '[[AST_BLOCK_HASH]]'
-// CHECK-NOT: <SIGNATURE abbrevid={{[0-9]+}}/> blob data = '[[SIGNATURE]]'
-// The modules built by this test are designed to yield the same AST but distinct AST files.
-// If this test fails, it means that either the AST block has become non-relocatable,
-// or the file signature stopped hashing some parts of the AST file.
+// CHECK: [[AST_BLOCK_HASH:<AST_BLOCK_HASH .*>]]
+// CHECK: [[SIGNATURE:<SIGNATURE .*>]]
+// CHECK: [[AST_BLOCK_HASH]]
+// CHECK-NOT: [[SIGNATURE]]
+// The modules built by this test are designed to yield the same AST. If this
+// test fails, it means that the AST block is has become non-relocatable.