Make ASTFileSignature an array of 20 uint8_t instead of 5 uint32_t

[dang]

clang/include/clang/Basic/Module.h

90

I couldn't find anywhere in the code base that checks the bit-pattern so I left it as is. If anyone knows if someone could point me to the code that checks this (if any code does check it) that would be nice.

[aprantl]

I was going to ask why make this change, but looking at the patch, it's pretty obvious :-)

clang/include/clang/Basic/Module.h

29

Can you move the code that depends on algorithm into a .cpp file?

90

The PCM hash is reused in CGDebugInfo as the DW_AT_dwo_id hash for the -gmodules debug info. This value is checked by dsymutil and LLDB.

[dexonsmith]

LGTM once @aprantl is happy.

[dang]

Address @aprantl's comments

[dang]

Fix bug

clang/include/clang/Basic/Module.h

29

That code is the call to std::copy in the template function so it can't really be moved out... But I removed the include because STLExtras.h already includes it, so I think it is ok to remove the include.

90

DW_AT_dwo_id is 64 bit so using 0xFF everywhere is fine.

[dang]

Missed a case where an ASTFileSignature was created using an explicit std::copy

[dblaikie]

In D81347#2080217, @aprantl wrote:

I was going to ask why make this change, but looking at the patch, it's pretty obvious :-)

Might be worth writing it down for everyone else - isn't exactly obvious to me (though not a part of the codebase I'm particularly familiar with).

clang/include/clang/Basic/Module.h

57

Looks like "Base" is the more common name for this purpose (rather than "BaseT") across the LLVM project. BaseT makes me think (when I'm reading the rest of the code) that's it's a template parameter.

61

Bit of a big structure to pass by value - should it be by const ref instead? (existing problem with the old code anyway, so no big deal either way)

(also the std::move is a no-op here because std::array<uint32_t> has a move that's the same as copy, though I don't mind it out of habit/consistency)

clang/include/clang/Basic/Module.h

 #ifndef LLVM_CLANG_BASIC_MODULE_H
 #define LLVM_CLANG_BASIC_MODULE_H
 
 #include "clang/Basic/SourceLocation.h"
 #include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/DenseSet.h"
 #include "llvm/ADT/Optional.h"
 #include "llvm/ADT/PointerIntPair.h"
+#include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/SetVector.h"
 #include "llvm/ADT/SmallVector.h"
-#include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/StringMap.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/ADT/iterator_range.h"
 #include <array>
 #include <cassert>
 #include <cstdint>
 #include <ctime>
+#include <iterator>
 #include <string>
 #include <utility>
 #include <vector>
 
 namespace llvm {
 
 class raw_ostream;
 
 } // namespace llvm
 
 namespace clang {
 
 class DirectoryEntry;
 class FileEntry;
 class FileManager;
 class LangOptions;
 class TargetInfo;
 
 /// Describes the name of a module.
 using ModuleId = SmallVector<std::pair<std::string, SourceLocation>, 2>;
 
 /// The signature of a module, which is a hash of the AST content.
-struct ASTFileSignature : std::array<uint32_t, 5> {
-  ASTFileSignature(std::array<uint32_t, 5> S = {{0}})
-      : std::array<uint32_t, 5>(std::move(S)) {}
+struct ASTFileSignature : std::array<uint8_t, 20> {
+  using BaseT = std::array<uint8_t, 20>;
+
+  static constexpr size_t size = std::tuple_size<BaseT>::value;
 
-  explicit operator bool() const {
-    return *this != std::array<uint32_t, 5>({{0}});
+  ASTFileSignature(BaseT S = {{0}}) : BaseT(std::move(S)) {}
+
+  explicit operator bool() const { return *this != BaseT({{0}}); }
+
+  static ASTFileSignature create(StringRef Bytes) {
+    return create(Bytes.bytes_begin(), Bytes.bytes_end());
+  }
+
+  static ASTFileSignature createDISentinel() {
+    ASTFileSignature Sentinel;
+    Sentinel.fill(0xFF);
+    return Sentinel;
+  }
+
+  template <typename InputIt>
+  static ASTFileSignature create(InputIt First, InputIt Last) {
+    assert(std::distance(First, Last) == size &&
+           "Wrong amount of bytes to create an ASTFileSignature");
+
+    ASTFileSignature Signature;
+    std::copy(First, Last, Signature.begin());
+    return Signature;
   }
 };
 
 /// Describes a module or submodule.
 class Module {
 public:
   /// The name of this module.
   std::string Name;
 
   /// The location of the module definition.
   SourceLocation DefinitionLoc;
 
   enum ModuleKind {
     /// This is a module that was defined by a module map and built out
     /// of header files.
     ModuleMapModule,

clang/lib/CodeGen/CGDebugInfo.cpp

auto RemapPath = [this](StringRef Path) -> std::string {
 
     return Relative.str();
   };
 
   if (CreateSkeletonCU && IsRootModule && !Mod.getASTFile().empty()) {
     // PCH files don't have a signature field in the control block,
     // but LLVM detects skeleton CUs by looking for a non-zero DWO id.
     // We use the lower 64 bits for debug info.
-    uint64_t Signature =
-        Mod.getSignature()
-            ? (uint64_t)Mod.getSignature()[1] << 32 | Mod.getSignature()[0]
-            : ~1ULL;
+
+    uint64_t Signature = 0;
+    if (const auto &ModSig = Mod.getSignature()) {
+      for (unsigned I = 0; I != sizeof(Signature); ++I)
+        Signature |= (uint64_t)ModSig[I] << (I * 8);
+    } else {
+      Signature = ~1ULL;
+    }
     llvm::DIBuilder DIB(CGM.getModule());
     SmallString<0> PCM;
     if (!llvm::sys::path::is_absolute(Mod.getASTFile()))
       PCM = Mod.getPath();
     llvm::sys::path::append(PCM, Mod.getASTFile());
     DIB.createCompileUnit(
         TheCU->getSourceLanguage(),
         // TODO: Support "Source" from external AST providers?

clang/lib/CodeGen/ObjectFilePCHContainerOperations.cpp

void Initialize(ASTContext &Context) override {
     M.reset(new llvm::Module(MainFileName, *VMContext));
     M->setDataLayout(Ctx->getTargetInfo().getDataLayout());
     Builder.reset(new CodeGen::CodeGenModule(
         *Ctx, HeaderSearchOpts, PreprocessorOpts, CodeGenOpts, *M, Diags));
 
     // Prepare CGDebugInfo to emit debug info for a clang module.
     auto *DI = Builder->getModuleDebugInfo();
     StringRef ModuleName = llvm::sys::path::filename(MainFileName);
-    DI->setPCHDescriptor({ModuleName, "", OutputFileName,
-                          ASTFileSignature{{{~0U, ~0U, ~0U, ~0U, ~1U}}}});
+    DI->setPCHDescriptor(
+        {ModuleName, "", OutputFileName, ASTFileSignature::createDISentinel()});
     DI->setModuleMap(MMap);
   }
 
   bool HandleTopLevelDecl(DeclGroupRef D) override {
     if (Diags.hasErrorOccurred())
       return true;
 
     // Collect debug info for all decls in this group.

clang/lib/Serialization/ASTReader.cpp

case IMPORTS: {
         ModuleKind ImportedKind = (ModuleKind)Record[Idx++];
         // The import location will be the local one for now; we will adjust
         // all import locations of module imports after the global source
         // location info are setup, in ReadAST.
         SourceLocation ImportLoc =
             ReadUntranslatedSourceLocation(Record[Idx++]);
         off_t StoredSize = (off_t)Record[Idx++];
         time_t StoredModTime = (time_t)Record[Idx++];
-        ASTFileSignature StoredSignature = {
-            {{(uint32_t)Record[Idx++], (uint32_t)Record[Idx++],
-              (uint32_t)Record[Idx++], (uint32_t)Record[Idx++],
-              (uint32_t)Record[Idx++]}}};
+        auto FirstSignatureByte = Record.begin() + Idx;
+        ASTFileSignature StoredSignature = ASTFileSignature::create(
+            FirstSignatureByte, FirstSignatureByte + ASTFileSignature::size);
+        Idx += ASTFileSignature::size;
 
         std::string ImportedName = ReadString(Record, Idx);
         std::string ImportedFile;
 
         // For prebuilt and explicit modules first consult the file map for
         // an override. Note that here we don't search prebuilt module
         // directories, only the explicit name to file mappings. Also, we will
         // still verify the size/signature making sure it is essentially the
while (true) {
     Expected<unsigned> MaybeRecordType = Stream.readRecord(Entry.ID, Record);
     if (!MaybeRecordType) {
       // FIXME this drops the error.
       return Failure;
     }
     switch ((UnhashedControlBlockRecordTypes)MaybeRecordType.get()) {
     case SIGNATURE:
       if (F)
-        std::copy(Record.begin(), Record.end(), F->Signature.data());
+        F->Signature = ASTFileSignature::create(Record.begin(), Record.end());
       break;
     case DIAGNOSTIC_OPTIONS: {
       bool Complain = (ClientLoadCapabilities & ARR_OutOfDate) == 0;
       if (Listener && ValidateDiagnosticOptions &&
           !AllowCompatibleConfigurationMismatch &&
           ParseDiagnosticOptions(Record, Complain, *Listener))
         Result = OutOfDate; // Don't return early.  Read the signature.
       break;
while (true) {
     StringRef Blob;
     Expected<unsigned> MaybeRecord = Stream.readRecord(Entry.ID, Record, &Blob);
     if (!MaybeRecord) {
       // FIXME this drops the error on the floor.
       consumeError(MaybeRecord.takeError());
       return ASTFileSignature();
     }
     if (SIGNATURE == MaybeRecord.get())
-      return {{{(uint32_t)Record[0], (uint32_t)Record[1], (uint32_t)Record[2],
-                (uint32_t)Record[3], (uint32_t)Record[4]}}};
+      return ASTFileSignature::create(Record.begin(),
+                                      Record.begin() + ASTFileSignature::size);
   }
 }
 
 /// Retrieve the name of the original source file name
 /// directly from the AST file, without actually loading the AST
 /// file.
 std::string ASTReader::getOriginalSourceFile(
     const std::string &ASTFileName, FileManager &FileMgr,
while (!DoneWithControlBlock) {
 
     case IMPORTS: {
       if (!NeedsImports)
         break;
 
       unsigned Idx = 0, N = Record.size();
       while (Idx < N) {
         // Read information about the AST file.
-        Idx += 1+1+1+1+5; // Kind, ImportLoc, Size, ModTime, Signature
+        Idx +=
+            1 + 1 + 1 + 1 +
+            ASTFileSignature::size; // Kind, ImportLoc, Size, ModTime, Signature
         std::string ModuleName = ReadString(Record, Idx);
         std::string Filename = ReadString(Record, Idx);
         ResolveImportedPath(Filename, ModuleDir);
         Listener.visitImport(ModuleName, Filename);
       }
       break;
     }
 

clang/lib/Serialization/ASTWriter.cpp

 }
 
 ASTFileSignature ASTWriter::createSignature(StringRef Bytes) {
   // Calculate the hash till start of UNHASHED_CONTROL_BLOCK.
   llvm::SHA1 Hasher;
   Hasher.update(ArrayRef<uint8_t>(Bytes.bytes_begin(), Bytes.size()));
   auto Hash = Hasher.result();
 
-  // Convert to an array [5*i32].
-  ASTFileSignature Signature;
-  auto LShift = [&](unsigned char Val, unsigned Shift) {
-    return (uint32_t)Val << Shift;
-  };
-  for (int I = 0; I != 5; ++I)
-    Signature[I] = LShift(Hash[I * 4 + 0], 24) | LShift(Hash[I * 4 + 1], 16) |
-                   LShift(Hash[I * 4 + 2], 8) | LShift(Hash[I * 4 + 3], 0);
-
-  return Signature;
+  return ASTFileSignature::create(Hash);
 }
 
 ASTFileSignature ASTWriter::writeUnhashedControlBlock(Preprocessor &PP,
                                                       ASTContext &Context) {
   // Flush first to prepare the PCM hash (signature).
   Stream.FlushToWord();
   auto StartOfUnhashedControl = Stream.GetCurrentBitNo() >> 3;
 

clang/lib/Serialization/GlobalModuleIndex.cpp

if (State == ControlBlock && Code == IMPORTS) {
         ++Idx;
 
         // Load stored size/modification time.
         off_t StoredSize = (off_t)Record[Idx++];
         time_t StoredModTime = (time_t)Record[Idx++];
 
         // Skip the stored signature.
         // FIXME: we could read the signature out of the import and validate it.
-        ASTFileSignature StoredSignature = {
-            {{(uint32_t)Record[Idx++], (uint32_t)Record[Idx++],
-              (uint32_t)Record[Idx++], (uint32_t)Record[Idx++],
-              (uint32_t)Record[Idx++]}}};
+        auto FirstSignatureByte = Record.begin() + Idx;
+        ASTFileSignature StoredSignature = ASTFileSignature::create(
+            FirstSignatureByte, FirstSignatureByte + ASTFileSignature::size);
+        Idx += ASTFileSignature::size;
 
         // Skip the module name (currently this is only used for prebuilt
         // modules while here we are only dealing with cached).
         Idx += Record[Idx] + 1;
 
         // Retrieve the imported file name.
         unsigned Length = Record[Idx++];
         SmallString<128> ImportedFile(Record.begin() + Idx,
if (State == ASTBlock && Code == IDENTIFIER_TABLE && Record[0] > 0) {
           InterestingIdentifiers[Ident.first].push_back(ID);
         else
           (void)InterestingIdentifiers[Ident.first];
       }
     }
 
     // Get Signature.
     if (State == DiagnosticOptionsBlock && Code == SIGNATURE)
-      getModuleFileInfo(File).Signature = {
-          {{(uint32_t)Record[0], (uint32_t)Record[1], (uint32_t)Record[2],
-            (uint32_t)Record[3], (uint32_t)Record[4]}}};
+      getModuleFileInfo(File).Signature = ASTFileSignature::create(
+          Record.begin(), Record.begin() + ASTFileSignature::size);
 
     // We don't care about this record.
   }
 
   return llvm::Error::success();
 }
 
 namespace {