[LLD][COFF] Fix TypeServerSource matcher with more than one collision

[thieta]

Summary

Follow-up from 98bc304e9faded44f1d8988ffa4c5d8b50c759ec - while that
commit fixed when you had two PDBs colliding on the same Guid it didn't
fix the case where you had more than two PDBs using the same Guid.

This commit fixes that and also tests much more carefully that all
the types are correct no matter the order.

[aganea]

This looks good, but please see my other comment first! https://reviews.llvm.org/D122372#3432711

[thieta]

Fixed an issue and added more careful testing.

[thieta]

@aganea Added more testing - adding a third collision and also probe all the types and in different orders, since the order fooled the testing the first time since the order they where processed in LLD made a difference for the output.

I had to drop the log message since when we know set the entry to nullptr there is nothing informative to log, but I am not convinced the log message was that useful and I really don't think it's worth adding some more complexity to store the paths of all collisions.

WDYT?

[aganea]

In D123185#3433103, @thieta wrote:

@aganea Added more testing - adding a third collision and also probe all the types and in different orders, since the order fooled the testing the first time since the order they where processed in LLD made a difference for the output.

I had to drop the log message since when we know set the entry to nullptr there is nothing informative to log, but I am not convinced the log message was that useful and I really don't think it's worth adding some more complexity to store the paths of all collisions.

WDYT?

I agree, you can leave out the logging.

lld/test/COFF/pdb-type-server-guid-collision-invalid.test

9 ↗

(On Diff #420870)

Are you able to repro the issue with this test, but with the old code from rG98bc304e9faded44f1d8988ffa4c5d8b50c759ec? If so, all good!

[thieta]

lld/test/COFF/pdb-type-server-guid-collision-invalid.test

9 ↗

(On Diff #420870)

Yes - I wrote the test for the old rev and made sure it failed first.

[aganea]

LGTM.

lld/test/COFF/pdb-type-server-guid-collision-invalid.test

9 ↗

(On Diff #420870)

Cool - thanks! ;-)

[thieta]

lld/test/COFF/pdb-type-server-guid-collision-invalid.test

9 ↗

(On Diff #420870)

Thank you! I totally missed this case the last time around - so it was a great catch.

[saudi]

LGTM
I was curious whether it would be good to remove the notion of GUID validity from the tests, since we now have support for collision whatever the GUIDs are.
I just noticed it, but it was already the case in D122372, so it might be for a separate patch if we want that.

[thieta]

Yeah I think the tests can be improved a bit - but I'll submit a separate patch for that when I have time to dig in. For now I will merge this so it ends up in 14.0.1.

lld/COFF/DebugTypes.cpp

explicit TypeServerSource(COFFLinkerContext &ctx, PDBInputFile *f)
     auto it = ctx.typeServerSourceMappings.emplace(Guid, this);
     if (!it.second) {
       // If we hit here we have collision on Guid's in two PDB files.
       // This can happen if the PDB Guid is invalid or if we are really
       // unlucky. This should fall back on stright file-system lookup.
       TypeServerSource *tSrc = (TypeServerSource *)it.first->second;
       log("GUID collision between " + file.getFilePath() + " and " +
           tSrc->pdbInputFile->session->getPDBFile().getFilePath());
-      ctx.typeServerSourceMappings.erase(Guid);
+      it.first->second = nullptr;
     }
   }
 
   Error mergeDebugT(TypeMerger *m) override;
 
   void loadGHashes() override;
   void remapTpiWithGHashes(GHashState *g) override;
 
Error TypeServerSource::mergeDebugT(TypeMerger *m) {
 
   return Error::success();
 }
 
 Expected<TypeServerSource *> UseTypeServerSource::getTypeServerSource() {
   const codeview::GUID &tsId = typeServerDependency.getGuid();
   StringRef tsPath = typeServerDependency.getName();
 
-  TypeServerSource *tsSrc;
+  TypeServerSource *tsSrc = nullptr;
   auto it = ctx.typeServerSourceMappings.find(tsId);
   if (it != ctx.typeServerSourceMappings.end()) {
     tsSrc = (TypeServerSource *)it->second;
-  } else {
+  }
+
+  // check for nullptr since we insert nullptr if we encounter a collision
+  if (tsSrc == nullptr) {
     // The file failed to load, lookup by name
     PDBInputFile *pdb = PDBInputFile::findFromRecordPath(ctx, tsPath, file);
     if (!pdb)
       return createFileError(tsPath, errorCodeToError(std::error_code(
                                          ENOENT, std::generic_category())));
     // If an error occurred during loading, throw it now
     if (pdb->loadErr && *pdb->loadErr)
       return createFileError(tsPath, std::move(*pdb->loadErr));