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

[Reid Kleckner via Phabricator via llvm-commits]

rnk marked 5 inline comments as done.
rnk added a subscriber: blackhole12.
rnk added inline comments.


================
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!");
----------------
aganea wrote:
> 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?
That one seems to cover a slightly different edge case. It has a valid S_OBJNAME record and signature, so we don't come to this `fatal` call.


================
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) + ")");
----------------
aganea wrote:
> Could you please ensure this `fatal` codepath is covered? It should be, just to be sure.
Yep, it's already covered. The test just `cp`s the PCH object and links it twice.


================
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;
 
----------------
aganea wrote:
> @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
Hm, this reminds me, I seem to recall that there is a user who complains every time we add these global lists. Oh yeah: D70378 @blackhole12

In any case, I can confirm I saw @santagada's issues, I made this change recently:
rG2868ee5b3273e203d3b5d170531ecba764b2d610
We were destroying the BumpPtrAllocator used for PDB linking, instead of using the global one, which is not destroyed.


================
Comment at: lld/COFF/DebugTypes.cpp:352
+  if (expectedInfo->getGuid() != typeServerDependency.getGuid() ||
+      expectedInfo->getAge() != typeServerDependency.getAge())
+    return createFileError(
----------------
aganea wrote:
> 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.
OK, I'll remove it. I noticed this was a functional change, I had to update one of the tests.


================
Comment at: lld/COFF/PDB.cpp:950
+
+  if (source->kind == TpiSource::PDB)
+    return; // No symbols in TypeServer PDBs
----------------
aganea wrote:
> 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.
True.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79672