[LLD][COFF] Survive empty and invalid PCH signature

[aganea]

Summary

While trying to use LLD with the Unreal Engine - which happens to heavily use PCH files - it was failing with "error: xxx.obj claims to be a PCH object, but does not have a valid signature". It seems recent versions of MSVC (or at least the one we use) do not generate the PCH signature in the S_OBJNAME record anymore. Instead we need to parse the entire .PCH.OBJ file until we reach the LF_ENDPRECOMP record, which contains a valid PCH signature. The code already was able to survive without that, by falling back to name lookup, but it's more reliable to use the precompSourceMappings map for matching the corresponding PCH file.

[rnk]

So, the signature moved from S_OBJNAME to LF_ENDPRECOMP, but everything else more or less stays the same.

Sounds good to me.

[thieta]

LGTM

[aganea]

Thanks for the review!

In D136762#3885821, @rnk wrote:

So, the signature moved from S_OBJNAME to LF_ENDPRECOMP, but everything else more or less stays the same.

Sounds good to me.

Actually the code supports both now, depending on the version of the MSVC compiler.

I'll hold on this for a bit, I'm still seeing weird warnings and the .PDB is only 450 MiB with LLD -- instead of 2 GiB with link.exe. I think the issues are unrelated, but i'd like to get to the bottom of it.

[aganea]

I finally found some time to look at this. The issue is more complicated than I thought, and the solution is not the same for GHASH than non-GHASH.

Another issue was that .PCH.OBJs are non-deterministic by default. Example:

>cat precomp.cpp
#include "precomp.h"

>cl /Yc /Z7 /c precomp.cpp /nologo
precomp.cpp

>llvm-pdbutil dump --all precomp.obj > dump.txt

>cl /Yc /Z7 /c precomp.cpp /nologo
precomp.cpp

>llvm-pdbutil dump --all precomp.obj > dump2.txt

>diff dump.txt dump2.txt
17991c17991
< 0x2382 | LF_ENDPRECOMP [size = 8] signature = 0x522700AA
---
> 0x2382 | LF_ENDPRECOMP [size = 8] signature = 0x2E1E4F09
18020c18020
<        0 | S_OBJNAME [size = 40] sig=1378287786, `C:\git\llvm-project\precomp.obj`
---
>        0 | S_OBJNAME [size = 40] sig=773738249, `C:\git\llvm-project\precomp.obj`

There was a situation where our .PCH.OBJs were always recompiled on a clean rebuild, but not the .OBJs (they were cached). MSVC link.exe was able to link without warnings or errors. When I first implemented the PCH support, I initially thought the PCH signature was a hard requirement, but it seems it's just a hint for the runtime. I relaxed that requirement in LLD, but now we'd still fail if the LF_ENDPRECOMP index doesn't line up with the data in LF_PRECOMP.

Also worth noting that using MSVC's cl /Yc /experimental:deterministic .. makes the PCH signature deterministic.

Would you mind taking a second look please?

[rnk]

Thanks, looks good.

In D136762#3892851, @aganea wrote:

There was a situation where our .PCH.OBJs were always recompiled on a clean rebuild, but not the .OBJs (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.

lld/COFF/DebugTypes.cpp

318

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.

lld/COFF/DebugTypes.h

116 ↗

(On Diff #471621)

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.

[thieta]

LGTM

lld/COFF/DebugTypes.cpp

 
 // This class represents the debug type stream of a Microsoft precompiled
 // headers OBJ (PCH OBJ). This OBJ kind needs to be merged first in the output
 // PDB, before any other OBJs that depend on this. Note that only MSVC generate
 // such files, clang does not.
 class PrecompSource : public TpiSource {
 public:
   PrecompSource(COFFLinkerContext &ctx, ObjFile *f) : TpiSource(ctx, PCH, f) {
-    if (!f->pchSignature || !*f->pchSignature)
-      fatal(toString(f) +
-            " claims to be a PCH object, but does not have a valid signature");
-    auto it = ctx.precompSourceMappings.emplace(*f->pchSignature, this);
-    if (!it.second)
-      fatal("a PCH object with the same signature has already been provided (" +
-            toString(it.first->second->file) + " and " + toString(file) + ")");
+    // If the S_OBJNAME record contains the PCH signature, we'll register this
+    // source file right away.
+    registerMapping();
   }
 
+  Error mergeDebugT(TypeMerger *m) override;
+
   void loadGHashes() override;
 
   bool isDependency() const override { return true; }
+
+private:
+  void registerMapping();
+
+  bool registered = false;
 };
 
 // This class represents the debug type stream of an OBJ file that depends on a
 // Microsoft precompiled headers OBJ (see PrecompSource).
 class UsePrecompSource : public TpiSource {
 public:
   UsePrecompSource(COFFLinkerContext &ctx, ObjFile *f, PrecompRecord precomp)
       : TpiSource(ctx, UsingPCH, f), precompDependency(precomp) {}
Error TpiSource::mergeDebugT(TypeMerger *m) {
   BinaryStreamReader reader(file->debugTypes, support::little);
   cantFail(reader.readArray(types, reader.getLength()));
 
   // When dealing with PCH.OBJ, some indices were already merged.
   unsigned nbHeadIndices = indexMapStorage.size();
 
   if (auto err = mergeTypeAndIdRecords(
           m->idTable, m->typeTable, indexMapStorage, types, file->pchSignature))
     fatal("codeview::mergeTypeAndIdRecords failed: " +
           toString(std::move(err)));
 
   // In an object, there is only one mapping for both types and items.
   tpiMap = indexMapStorage;
   ipiMap = indexMapStorage;
 
   if (config->showSummary) {
     nbTypeRecords = indexMapStorage.size() - nbHeadIndices;
Error UsePrecompSource::mergeDebugT(TypeMerger *m) {
   // object are referencing data in the precompiled headers object, so we need
   // both to be loaded.
   if (Error e = mergeInPrecompHeaderObj())
     return e;
 
   return TpiSource::mergeDebugT(m);
 }
 
+Error PrecompSource::mergeDebugT(TypeMerger *m) {
+  if (Error e = TpiSource::mergeDebugT(m))
+    return e;
+
+  // In some cases, the S_OBJNAME record doesn't contain the PCH signature.
+  // The signature comes later with the LF_ENDPRECOMP record, so we first need
+  // to merge in all the .PCH.OBJ file type records.
+  registerMapping();
+
+  return Error::success();
+}
+
+void PrecompSource::registerMapping() {
+  if (registered)
+    return;
+  if (file->pchSignature && *file->pchSignature) {
+    auto it = ctx.precompSourceMappings.emplace(*file->pchSignature, this);
+    if (!it.second)
+      fatal("a PCH object with the same signature has already been provided (" +
+            toString(it.first->second->file) + " and " + toString(file) + ")");
+    registered = true;
+  }
+}
+
 //===----------------------------------------------------------------------===//
 // Parellel GHash type merging implementation.
 //===----------------------------------------------------------------------===//
 
 void TpiSource::loadGHashes() {
   if (Optional<ArrayRef<uint8_t>> debugH = getDebugH(file)) {
     ghashes = getHashesFromDebugH(*debugH);
     ownedGHashes = false;
void PrecompSource::loadGHashes() {
   }
 
   uint32_t ghashIdx = 0;
   std::vector<GloballyHashedType> hashVec;
   forEachTypeChecked(file->debugTypes, [&](const CVType &ty) {
     // Remember the index of the LF_ENDPRECOMP record so it can be excluded from
     // the PDB. There must be an entry in the list of ghashes so that the type
     // indexes of the following records in the /Yc PCH object line up.
-    if (ty.kind() == LF_ENDPRECOMP)
+    if (ty.kind() == LF_ENDPRECOMP) {
+      if (!registered) {
+        EndPrecompRecord endPrecomp = cantFail(
+            TypeDeserializer::deserializeAs<EndPrecompRecord>(ty.data()));
+        file->pchSignature.emplace(endPrecomp.getSignature());
+        registerMapping();
+      }
       endPrecompGHashIdx = ghashIdx;
+    }
 
     hashVec.push_back(GloballyHashedType::hashType(ty, hashVec, hashVec));
     isItemIndex.push_back(isIdRecord(ty.kind()));
     ++ghashIdx;
   });
   assignGHashesFromVector(std::move(hashVec));
 }
 

lld/test/COFF/precomp-link.test

 error. Edit out this record from the yaml-ified object:
   - Kind:            S_OBJNAME
     ObjNameSym:
       Signature:       545589255
       ObjectName:      'F:\svn\lld\test\COFF\precomp\precomp.obj'
 
 RUN: obj2yaml %S/Inputs/precomp.obj | grep -v 'SectionData: *04000000' > %t.precomp.yaml
 RUN: sed '/S_OBJNAME/,/ObjectName:/d' < %t.precomp.yaml > precomp-no-objname.yaml
-RUN: sed 's/Signature: *545589255/Signature: 0/' < %t.precomp.yaml > precomp-zero-sig.yaml
+RUN: sed '16,19s/Signature: *545589255/Signature: 0/' < %t.precomp.yaml > precomp-zero-sig.yaml
 RUN: yaml2obj precomp-no-objname.yaml -o %t.precomp-no-objname.obj
 RUN: yaml2obj precomp-zero-sig.yaml -o %t.precomp-zero-sig.obj
 
-RUN: env LLD_IN_TEST=1 not lld-link %t.precomp-no-objname.obj %S/Inputs/precomp-a.obj %S/Inputs/precomp-b.obj /nodefaultlib /entry:main /debug /pdb:%t.pdb /out:%t.exe 2>&1 | FileCheck %s -check-prefix FAILURE-NO-SIGNATURE
+RUN: env LLD_IN_TEST=1 lld-link %t.precomp-no-objname.obj %S/Inputs/precomp-a.obj %S/Inputs/precomp-b.obj /nodefaultlib /entry:main /debug /pdb:%t.pdb /out:%t.exe 2>&1
 
-RUN: env LLD_IN_TEST=1 not lld-link %t.precomp-zero-sig.obj %S/Inputs/precomp-a.obj %S/Inputs/precomp-b.obj /nodefaultlib /entry:main /debug /pdb:%t.pdb /out:%t.exe 2>&1 | FileCheck %s -check-prefix FAILURE-NO-SIGNATURE
-
-FAILURE-NO-SIGNATURE: error: {{.*}}.obj claims to be a PCH object, but does not have a valid signature
+RUN: env LLD_IN_TEST=1 lld-link %t.precomp-zero-sig.obj %S/Inputs/precomp-a.obj %S/Inputs/precomp-b.obj /nodefaultlib /entry:main /debug /pdb:%t.pdb /out:%t.exe 2>&1
 
 Check that two PCH objs with duplicate signatures are an error.
 
 RUN: cp %S/Inputs/precomp.obj %t.precomp-dup.obj
 
 RUN: env LLD_IN_TEST=1 not lld-link %S/Inputs/precomp.obj %t.precomp-dup.obj %S/Inputs/precomp-a.obj %S/Inputs/precomp-b.obj /nodefaultlib /entry:main /debug /pdb:%t.pdb /out:%t.exe 2>&1 | FileCheck %s -check-prefix FAILURE-DUP-SIGNATURE
 
 FAILURE-DUP-SIGNATURE: error: a PCH object with the same signature has already been provided ({{.*precomp.obj and .*precomp-dup.obj.*}})