[LLD][PDB] Implement FIXME: Warn on missing TypeServer PDB rather than error

[colden]

Summary

Instead of fatal-ing out when missing a type server PDB, insead warn and cache the miss.

[rnk]

I think the right approach is to propagate failure out of mergeDebugT so that we can issue the warning there and then return early from PDBLinker::addObjFile. That implements the suggested recovery in the best way possible: linking the object as if it had no debug info.

This is the code in question:

// Before we can process symbol substreams from .debug$S, we need to process
// type information, file checksums, and the string table.  Add type info to
// the PDB first, so that we can get the map from object file type and item
// indices to PDB type and item indices.
CVIndexMap ObjectIndexMap;
const CVIndexMap &IndexMap = mergeDebugT(File, ObjectIndexMap); // --- Make this Expected or Optional, check and return early

// Now do all live .debug$S sections.
DebugStringTableSubsectionRef CVStrTab;
DebugChecksumsSubsectionRef Checksums;
std::vector<ulittle32_t *> StringTableReferences;
for (SectionChunk *DebugChunk : File->getDebugChunks()) {

[colden]

Ok, that makes sense.

I'm 99% done implementing the change, but what I'm seeing is when a lib is missing it's PDB, every single obj inside of it tosses the warning. Is this the behavior we want? Or is there another way to make sure the warning only goes once per file?

With the warning inside PDBLinker::maybeMergeTypeServerPDB, it just warns the first time the file load is attempted.

[rnk]

In D42188#979174, @colden wrote:

Ok, that makes sense.

I'm 99% done implementing the change, but what I'm seeing is when a lib is missing it's PDB, every single obj inside of it tosses the warning. Is this the behavior we want? Or is there another way to make sure the warning only goes once per file?

With the warning inside PDBLinker::maybeMergeTypeServerPDB, it just warns the first time the file load is attempted.

I think MSVC will also warn once for every object. I think it's pretty reasonable, this isn't something you want to ignore for long.

[colden]

Works for me! Update incoming.

[colden]

Changes requested by @rnk. Warning is now handled by addObjFile().

[rnk]

COFF/PDB.cpp

768–769

This is the MSVC warning text:

t.obj : warning LNK4099: PDB 'vc140.pdb' was not found with 't.obj' or at 'C:\src\llvm-project\build\vc140.pdb'; linking object as if no debug info

I think it might be worth including the .obj path or at least the basename and mentioning that we're ignoring its debug info. Maybe something like:

warning: Type server PDB foo.pdb was not found, ignoring debug info from foo.obj

[colden]

Got as close as I could to @rnk's suggested error format, while still using toString(theError).

New error example:
warning: Type server PDB for tmp32.dbg\dso_win32.obj is invalid, ignoring debug info. PDB Error: Type server PDB was not found. F:\external-libs\openssl\openssl-1.0.2k\tmp32.dbg\lib.pdb

[rnk]

Looks good, thanks!

[colden]

Yay, thanks! This is one of the last bugs that's blocking me. At some point I'll have to figure out how to get these changes merged to 6.0...

In the mean time, would you mind submitting this to trunk for me?

[colden]

Rebased on top of the timer change

[colden]

Hey guys, is there anything holding this back? If not, I'd love it if one of you could submit it for me.

COFF/PDB.cpp

   /// caller-provided ObjectIndexMap.
   const CVIndexMap &mergeDebugT(ObjFile *File, CVIndexMap &ObjectIndexMap);
 
-  const CVIndexMap &maybeMergeTypeServerPDB(ObjFile *File,
+  Expected<const CVIndexMap&> maybeMergeTypeServerPDB(ObjFile *File,
                                             TypeServer2Record &TS);
 
   /// Add the section map and section contributions to the PDB.

 
   /// Type index mappings of type server PDBs that we've loaded so far.
   std::map<GUID, CVIndexMap> TypeServerIndexMappings;
+
+  /// List of TypeServer PDBs which cannot be loaded.
+  /// Cached to prevent repeated load attempts.
+  std::set<GUID> MissingTypeServerPDBs;
 };
 }
 

 
   // Look through type servers. If we've already seen this type server, don't
   // merge any type information.
-  if (Optional<TypeServer2Record> TS = maybeReadTypeServerRecord(Types))
-    return maybeMergeTypeServerPDB(File, *TS);
+  if (Optional<TypeServer2Record> TS = maybeReadTypeServerRecord(Types)) {
+    // If loading the PDB succeeds, the result can be returned immediately.
+    auto TypeServerIndexMap = maybeMergeTypeServerPDB(File, *TS);
+    if (TypeServerIndexMap)
+      return *TypeServerIndexMap;
 
+    // If the loading failed, a warning was already thrown.
+    consumeError(TypeServerIndexMap.takeError());
+  }
+
   // This is a /Z7 object. Fill in the temporary, caller-provided
   // ObjectIndexMap.
   if (Config->DebugGHashes) {

   return std::move(NS);
 }
 
-const CVIndexMap &PDBLinker::maybeMergeTypeServerPDB(ObjFile *File,
+Expected<const CVIndexMap&> PDBLinker::maybeMergeTypeServerPDB(ObjFile *File,
                                                      TypeServer2Record &TS) {
-  // First, check if we already loaded a PDB with this GUID. Return the type
+  const GUID& TSId = TS.getGuid();
+  StringRef TSPath = TS.getName();
+
+  // First, check if the PDB has previously failed to load.
+  if (MissingTypeServerPDBs.count(TSId))
+      return make_error<pdb::GenericError>(
+          pdb::generic_error_code::type_server_not_found, TSPath);
+
+  // Second, check if we already loaded a PDB with this GUID. Return the type
   // index mapping if we have it.
-  auto Insertion = TypeServerIndexMappings.insert({TS.getGuid(), CVIndexMap()});
+  auto Insertion = TypeServerIndexMappings.insert({TSId, CVIndexMap()});
   CVIndexMap &IndexMap = Insertion.first->second;
   if (!Insertion.second)
     return IndexMap;

   // Check for a PDB at:
   // 1. The given file path
   // 2. Next to the object file or archive file
-  auto ExpectedSession = tryToLoadPDB(TS.getGuid(), TS.getName());
+  auto ExpectedSession = tryToLoadPDB(TSId, TSPath);
   if (!ExpectedSession) {
     consumeError(ExpectedSession.takeError());
     StringRef LocalPath =

         !File->ParentName.empty() ? File->ParentName : File->getName();
     SmallString<128> Path = sys::path::parent_path(LocalPath);
     sys::path::append(
-        Path, sys::path::filename(TS.getName(), sys::path::Style::windows));
-    ExpectedSession = tryToLoadPDB(TS.getGuid(), Path);
+        Path, sys::path::filename(TSPath, sys::path::Style::windows));
+    ExpectedSession = tryToLoadPDB(TSId, Path);
   }
-  if (auto E = ExpectedSession.takeError())
-    fatal("Type server PDB was not found: " + toString(std::move(E)));
+  if (auto E = ExpectedSession.takeError()) {
+    warn("Type server PDB \"" + TSPath + "\" was not found: " +
+         toString(std::move(E)));
 
+    TypeServerIndexMappings.erase(TSId);
+    MissingTypeServerPDBs.emplace(TSId);
+    return std::move(E);
+  }
+
   auto ExpectedTpi = (*ExpectedSession)->getPDBFile().getPDBTpiStream();
   if (auto E = ExpectedTpi.takeError())
     fatal("Type server does not have TPI stream: " + toString(std::move(E)));

test/COFF/pdb-type-server-missing.yaml

 # This is an object compiled with /Zi (see the LF_TYPESERVER2 record) without an
 # adjacent type server PDB. Test that LLD fails gracefully on it.
 
-# FIXME: Ideally we'd do what MSVC does, which is to warn and drop all debug
-# info in the object with the missing PDB.
-
 # RUN: yaml2obj %s -o %t.obj
-# RUN: not lld-link %t.obj -out:%t.exe -debug -pdb:%t.pdb -nodefaultlib -entry:main 2>&1 | FileCheck %s
+# RUN: lld-link %t.obj -out:%t.exe -debug -pdb:%t.pdb -nodefaultlib -entry:main 2>&1 | FileCheck %s
 
-# CHECK: error: Type server PDB was not found
+# CHECK: warning: Type server PDB "C:\src\llvm-project\build\definitely_not_found_for_sure.pdb" was not found
 
 --- !COFF
 header: