[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:67
+    // a path lookup fallback which works fine - but is slower.
+    if (Guid != InvalidGuid) {
+      auto it = ctx.typeServerSourceMappings.emplace(Guid, this);
----------------
saudi wrote:
> aganea wrote:
> > 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.
> With this approach, the idea is that we still trust the unicity of Guids with just one exception for the special case encountered in Microsoft's system PDBs;  I tend to think it's valid.  
> 
> Except for that bug in those PDBs, I think that it's still safe to assume that Guids are really unique, and that if we find two PDBs with different paths but same Guid, it would be safe to assume it was just a pdb that was copied, which can very well happen.  
We could do this instead:


```
    auto it = ctx.typeServerSourceMappings.emplace(Guid, this);
    if (!it.second) {
      TypeServerSource *tSrc = (TypeServerSource *)it.first->second;
      if (Guid != InvalidGuid)
      {
        warn("GUID collision between " + file.getFilePath() + " and " +
              tSrc->pdbInputFile->session->getPDBFile().getFilePath());
      }
      ctx.typeServerSourceMappings.erase(Guid);
```

This only handle the special case of logging when the PDB is invalid. This cuts down the error messages.

Check if it's system libraries seems complicated and adds a lot of code here or even filesystem checks.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122372