[PATCH] D40980: Add /DEBUG:GHASH option to LLD to speed up COFF linking

[Rui Ueyama via Phabricator via llvm-commits]

ruiu added inline comments.


================
Comment at: lld/COFF/Config.h:92
   bool DebugDwarf = false;
+  bool DebugGlobalHashes = false;
   unsigned DebugTypes = static_cast<unsigned>(DebugType::None);
----------------
We are using the same name for the command line option and the variable name, so it should be DebugGHashes.


================
Comment at: lld/COFF/PDB.cpp:77-81
+      GlobalTypeTable = llvm::make_unique<GlobalTypeTableBuilder>(Alloc);
+      GlobalIDTable = llvm::make_unique<GlobalTypeTableBuilder>(Alloc);
+    } else {
+      TypeTable = llvm::make_unique<MergingTypeTableBuilder>(Alloc);
+      IDTable = llvm::make_unique<MergingTypeTableBuilder>(Alloc);
----------------
Are these classes expensive to instantiate? If the cost of instantiation is negligible, I'd just create four instances as members of this class.


================
Comment at: lld/COFF/PDB.cpp:176
 
+static bool canUseDebugH(ArrayRef<uint8_t> DebugH) {
+  if (DebugH.size() < sizeof(object::debug_h_header))
----------------
Please add a function comment saying that this is .debug$H is a clang extension and this function checks if that is available.


================
Comment at: lld/COFF/PDB.cpp:182-190
+  if (Header->Magic != COFF::DEBUG_HASHES_SECTION_MAGIC)
+    return false;
+  if (Header->Version != 0)
+    return false;
+  if (Header->HashAlgorithm != uint16_t(GlobalTypeHashAlg::SHA1))
+    return false;
+  if (DebugH.size() % 20 != 0)
----------------
  return Header->Magic == COFF::DEBUG_HASHES_SECTION_MAGIC &&
         Header->Version == 0 &&
         Header->HashAlgorithm == uint16_t(GlobalTypeHashAlg::SHA1) &&
         DebugH.size() % 20 == 0;

is probably a bit more straightforward.


================
Comment at: lld/COFF/PDB.cpp:196
+  if (!Sec)
+    return llvm::None;
+  auto Contents = Sec->getContents();
----------------
You can omit `llvm::`.


================
Comment at: lld/COFF/PDB.cpp:197
+    return llvm::None;
+  auto Contents = Sec->getContents();
+  if (!canUseDebugH(Contents))
----------------
auto -> ArrayRef<uint8_t>


================
Comment at: lld/COFF/PDB.cpp:262
+    std::vector<GloballyHashedType> OwnedHashes;
+    if (auto DebugH = getDebugH(File))
+      Hashes = getHashesFromDebugH(*DebugH);
----------------
auto -> Optional<ArrayRef<uint8_t>>


================
Comment at: llvm/lib/DebugInfo/CodeView/TypeStreamMerger.cpp:103
+  bool hasTypeStream() const {
+    return (UseGlobalHashes) ? (!!DestGlobalTypeStream) : (!!DestTypeStream);
+  }
----------------
Why so many parentheses?


https://reviews.llvm.org/D40980