[PATCH] D122372: [lld][COFF] Fix TypeServerSource lookup on GUID collisions

[Tobias Hieta via Phabricator via llvm-commits]

thieta added inline comments.


================
Comment at: lld/COFF/DebugTypes.cpp:71
+        TypeServerSource *tSrc = (TypeServerSource *)it.first->second;
+        warn("GUID collision between " + file.getFilePath() + " and " +
+             tSrc->pdbInputFile->session->getPDBFile().getFilePath());
----------------
aganea wrote:
> aganea wrote:
> > aganea wrote:
> > > Since the issue is in system libs, I wonder if we can detect that and only warn on non-system PDBs? MSVC link.exe doesn't warn. How many warnings do you actually get when linking a production executable?
> > I wonder if `log()` instead of `warn()` wouldn't be more appropriate here.
> This wasn't addressed in the last update. Any advice either way?
The reason I put it as a warning was that before I added the line below that erases it from the map it would actually lead to bad things happening in the PDB, getting the wrong types etc.

Now that this case is actually handled it could most likely be a Log() call instead. I don't have a strong opinion.


================
Comment at: lld/test/COFF/Inputs/pdb-type-server-guid-collision-a-pdb.yaml:2
+---
+MSF:
+  SuperBlock:
----------------
aganea wrote:
> Optionally, and if you can, you could manually trim down a bit the file. It also helps understanding the relationships between the OBJ and the PDB and in-between records. As an example, the MSF part here isn't needed, it will be re-generated. Same with the UDTs and the build records below, it's probably not needed for you test. Up to you! :)
Good idea - I find it a bit hard to know what parts are needed to be there without a lot of trial-and-error. But I guess it will come from experience in adding more tests like this.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122372