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

[Alexandre Ganea via Phabricator via llvm-commits]

aganea added a comment.

In D122372#3413297 <https://reviews.llvm.org/D122372#3413297>, @thieta wrote:

> Consolidated the test-inputs and used SED instead. I tried using available input but couldn't get it to collide the same way this one did.

Oh yeah to fully repro the bug, you'd probably need to insert and extra record in the second PDB to shift the offsets to the LF_FUNC_ID. Seems good otherwise, just two (and a half) 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:
> > 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?


================
Comment at: lld/COFF/DebugTypes.cpp:67
+    // a path lookup fallback which works fine - but is slower.
+    if (Guid != InvalidGuid) {
+      auto it = ctx.typeServerSourceMappings.emplace(Guid, this);
----------------
I'm starting to wonder if we really need this `if`. The fact that the bug is raised with a `FF FF...` guid is just an implementation artefact. The issue would still happen if two PDBs with different paths but same GUIDs (like in my example) are loaded during a link session.


================
Comment at: lld/test/COFF/Inputs/pdb-type-server-guid-collision-a-pdb.yaml:2
+---
+MSF:
+  SuperBlock:
----------------
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! :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122372