[PATCH] D79672: [COFF] Move type merging to TpiSource::mergeDebugT virtual method

[Alexandre Ganea via Phabricator via llvm-commits]

aganea marked an inline comment as done.
aganea added a comment.

Thanks a lot for rebasing this!



================
Comment at: lld/COFF/DebugTypes.cpp:88
+    if (!f->pchSignature || !*f->pchSignature)
+      fatal(toString(f) + " claims to be a precompiled headers object, but "
+                          "does not have a valid signature!");
----------------
rnk wrote:
> aganea wrote:
> > We will need tests for these I think. Same for the other `fatal` below.
> I did this with obj2yaml -> sed -> yaml2obj. It works, but it's fragile.
There's also `Inputs/precomp-invalid.obj` which is a copy of `precomp.obj` (I think) but with a different signature. So maybe this was already covered?


================
Comment at: lld/COFF/DebugTypes.cpp:94
+      fatal(
+          "a PCH object with the same signature has already been provided (" +
+          toString(it.first->second->file) + " and " + toString(file) + ")");
----------------
Could you please ensure this `fatal` codepath is covered? It should be, just to be sure.


================
Comment at: lld/COFF/DebugTypes.cpp:124
 
-TpiSource::TpiSource(TpiKind k, const ObjFile *f) : kind(k), file(f) {}
+static std::vector<std::unique_ptr<TpiSource>> gc;
 
----------------
@santagada reported a 0.5 sec delay at shutdown, for a 800 MB .PDB, just for releasing allocated memory.

Do you think we can attach `gc` to the `PDBLinker` instead instead of it being `static`?

The goal would be after to integrate https://github.com/santagada/llvm-project/commit/5c8a17bbd2d7d7717f9c017e082cd37b0abbe52d


================
Comment at: lld/COFF/DebugTypes.cpp:352
+  if (expectedInfo->getGuid() != typeServerDependency.getGuid() ||
+      expectedInfo->getAge() != typeServerDependency.getAge())
+    return createFileError(
----------------
I'm not sure we need to check the 'Age' here. In a incremental build, the .OBJ pointing to the /Zi .PDB might not change, but the .PDB will change, incrementing its 'Age'. I think the only this that matters is the Guid.


================
Comment at: lld/COFF/PDB.cpp:950
+
+  if (source->kind == TpiSource::PDB)
+    return; // No symbols in TypeServer PDBs
----------------
I was wondering if we could pre-link some libraries to speed-up linking of rarely modified projects. We're building lots of external/third-party libraries compiled from source, because we want to enforce the same settings as the rest of the gamecode (for options like /MT or -flto=thin). However they are cached and very rarely compiled. But we'd still pay the cost to merge symbols and types at link-time.
Maybe each library (or each group of libraries) could come with a /Zi-style .PDB which LLD would use instead of the library's .OBJs. If that ever happens, we'd need to merge symbols here as well.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79672