[PATCH] D136762: [LLD][COFF] Survive empty and invalid PCH signature

Mon Oct 31 09:59:29 PDT 2022

rnk accepted this revision.
rnk added a comment.
This revision is now accepted and ready to land.

Thanks, looks good.

In D136762#3892851 <https://reviews.llvm.org/D136762#3892851>, @aganea wrote:

> There was a situation where our `.PCH.OBJ`s were always recompiled on a clean rebuild, but not the `.OBJ`s (they were cached).

I'm imagining some system outside the build system forcing the .pch.obj to be recompiled, but the build system (fastbuild?) has correct and complete knowledge that none of the PCH inputs changed.

================
Comment at: lld/COFF/DebugTypes.cpp:319
+  Optional<PCHMergerInfo> pchInfo;
+  if (auto err = mergeTypeAndIdRecords(m->idTable, m->typeTable,
+                                       indexMapStorage, types, pchInfo))
----------------
Maintaining two codepaths for type merging has significant costs, but I think the ghash code path is too optimized, and too difficult to understand. Perhaps with more work and cleanups, it could be made straightforward enough that we could afford to delete the regular type merging codepath, but for now I guess we should continue maintaining both.

================
Comment at: lld/COFF/DebugTypes.h:116

-protected:
-  /// The ghash index (zero based, not 0x1000-based) of the LF_ENDPRECOMP record
-  /// in this object, if one exists. This is the all ones value otherwise. It is
-  /// recorded here so that it can be omitted from the final ghash table.
-  uint32_t endPrecompGHashIdx = ~0U;
+  /// The index (zero based, not 0x1000-based) of the LF_ENDPRECOMP record in
+  /// this object, if one exists. This is the all ones value otherwise. It is
----------------
Part of me wants to say, make it a TypeIndex to eliminate this subtlety, but the new usage uses it as a count, so I don't think it makes sense.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136762