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

Wed Apr 6 06:38:25 PDT 2022

aganea added a subscriber: tstellar.
aganea added inline comments.

================
Comment at: lld/COFF/DebugTypes.cpp:66
+          tSrc->pdbInputFile->session->getPDBFile().getFilePath());
+      ctx.typeServerSourceMappings.erase(Guid);
+    }
----------------
thieta wrote:
> aganea wrote:
> > thieta wrote:
> > > aganea wrote:
> > > > I'm having some doubts regarding this line (L66). If we have an odd number of PDBs queued with bad GUIDs, the first one will `ctx.typeServerSourceMappings.emplace()`, the second will do `ctx.typeServerSourceMappings.erase()` and the third will `ctx.typeServerSourceMappings.emplace()` again. I think we need a different strategy here to handle an arbitrary number of PDBs with bad (FFFF..) GUIDs. Any suggestions? I would be tempted to keep the entry in the map and not erase it -- that could help later with multithreading, if we ever do that. What about doing `it.first->second = nullptr;` when there's a collision, and then handle this case later in `UseTypeServerSource::getTypeServerSource()` to fallback to the lookup by path?
> > > Yeah this is much nicer - I actually had the thought before and thought it didn't matter that much - but it's definitely the better solution. Posted a follow-up diff here: https://reviews.llvm.org/D123185
> > I was thinking in term of correctness. This current patch as it stands could still exhibit the issue if a third "bad" PDB is inserted again in `typeServerSourceMappings`? The code in `UseTypeServerSource::getTypeServerSource()` will not fallback to path lookup, and all OBJs with 0xFFFF.. `LF_TYPESERVER2` will fetch the wrong PDB, leading most likely to [[ https://github.com/llvm/llvm-project/issues/54500 | PR54500 ]]? Could you please confirm @thieta ?
> > 
> > If we want to backport this (D122372), we'll have to also backport D123185. I'm just wondering if you should just re-open this current patch, and re-land everything atomically? 
> Ah I think you are correct. I think I need to change the testing here a bit to catch this.
> 
> I don't know about reverting and re-adding - is that normally how things are handled? 
@tstellar Regarding https://github.com/llvm/llvm-project/issues/54487#issuecomment-1089361869, please see above, we'll need to land another patch (D123185) first and backport both. Should we do that, or revert this current patch, and reland it by including D123185 ?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122372