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

[Alexandre Ganea via Phabricator via llvm-commits]

aganea accepted this revision.
aganea marked an inline comment as done.
aganea added a comment.
This revision is now accepted and ready to land.

LGTM, thanks again! (minor comment below)

In D79672#2035215 <https://reviews.llvm.org/D79672#2035215>, @blackhole12 wrote:

> It looks like this time around, the global list that was added has been properly cleaned up in link(), so this should be fine. Of course, the fixes that were mentioned <https://reviews.llvm.org/D70378> still haven't been merged, so as of right now LLVM 10's LLD linker crashes on windows when used multiple times, but apparently nobody cares about that right now. I can attempt to test this modification on my fork to see if it actually works, if people are willing to commit to actually fixing the memory cleanup errors.


I do care. I fixed a few similar issues on our end. We have a similar scenario where the LLD excutable is loaded as a DLL and repeatly called from multiple threads by the build system (see this <https://www.youtube.com/watch?v=usPL_DROn4k> for more info, especially the last part). However in addition to the issues you're seeing, in our case the linker can run with different inputs, in several threads. Any `static` is a no-go. I concur with your suggestion of making everything as part of a context, and not have statics.



================
Comment at: lld/COFF/DebugTypes.cpp:370
+  for (auto kv : PrecompSource::mappings) {
+    StringRef currentFileName = sys::path::filename(kv.second->file->getName());
+
----------------
@rnk : If we're using `sys::path::Style::windows` below, we should do the same here?


================
Comment at: lld/COFF/PDB.cpp:950
+
+  if (source->kind == TpiSource::PDB)
+    return; // No symbols in TypeServer PDBs
----------------
santagada wrote:
> rnk wrote:
> > 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.
> What I would like is to generate static libraries that are more than just a tar of obj, but actually have a merged type and possibly symbol tables. That would make linking much faster as you can distribute those libs and reuse most of them. Ar files have a place for these, maybe we could put a empty .obj with a specific name on a .lib and read from there when available?
@santagada : Yeah that's what `cl /Zi` does, it stamps a .PDB redirection into the .OBJ type stream:
```
0x1000 : Length = 58, Leaf = 0x1515 LF_TYPESERVER2
                GUID={2760BCA6-C486-455B-833A-13B67CA4FA9A}, age = 0x00000001, PDB name = 'F:\llvm-project\__test\vc140.pdb
```
Except that the .PDB only contains the type stream, not the symbol stream, which remains in the .OBJ. The type stream is self-standing, whereas symbols need references to other sections in the .OBJ. MSVC does this with an external application (`mspdbsrv.exe`), so it would be a bit complicated to merge the symbols on the fly, multithreaded.

In our case, this would be a deferred step, not a 'live' step like MSVC. We could call lld-link in a special pre-link mode. At least that's the idea.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79672