[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

public:
   /// If the object file uses a type server PDB (compiled with /Zi), merge TPI
   /// and IPI from the type server PDB and return a map for it. Each unique type
   /// server PDB is merged at most once, so this may return an existing index
   /// mapping.
   ///
   /// If the object does not use a type server PDB (compiled with /Z7), we merge
   /// all the type and item records from the .debug$S stream and fill in the
   /// caller-provided ObjectIndexMap.
-  const CVIndexMap &mergeDebugT(ObjFile *File, CVIndexMap &ObjectIndexMap);
+  Expected<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.
   void addSections(ArrayRef<OutputSection *> OutputSections,
                    ArrayRef<uint8_t> SectionTable);
 
   void addSectionContrib(pdb::DbiModuleDescriptorBuilder &LinkerModule,
                          OutputSection *OS, Chunk *C);
 
private:
   DebugStringTableSubsection PDBStrTab;
 
   llvm::SmallString<128> NativePath;
 
   std::vector<pdb::SecMapEntry> SectionMap;
 
   /// 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;
 };
 }
 
 static SectionChunk *findByName(ArrayRef<SectionChunk *> Sections,
                                 StringRef Name) {
   for (SectionChunk *C : Sections)
     if (C->getSectionName() == Name)
       return C;
maybeReadTypeServerRecord(CVTypeArray &Types) {
   if (Type.kind() != LF_TYPESERVER2)
     return None;
   TypeServer2Record TS;
   if (auto EC = TypeDeserializer::deserializeAs(const_cast<CVType &>(Type), TS))
     fatal("error reading type server record: " + toString(std::move(EC)));
   return std::move(TS);
 }
 
-const CVIndexMap &PDBLinker::mergeDebugT(ObjFile *File,
+Expected<const CVIndexMap&> PDBLinker::mergeDebugT(ObjFile *File,
                                                    CVIndexMap &ObjectIndexMap) {
   ScopedTimer T(TypeMergingTimer);
 
   ArrayRef<uint8_t> Data = getDebugSection(File, ".debug$T");
   if (Data.empty())
     return ObjectIndexMap;
 
   BinaryByteStream Stream(Data, support::little);
   CVTypeArray Types;
tryToLoadPDB(const GUID &GuidFromObj, StringRef TSPath) {
   // must match the GUID specified in the TypeServer2 record.
   if (ExpectedInfo->getGuid() != GuidFromObj)
     return make_error<pdb::GenericError>(
         pdb::generic_error_code::type_server_not_found, TSPath);
 
   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;
 
   // Mark this map as a type server map.
   IndexMap.IsTypeServerMap = true;
 
   // 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()) {
+    TypeServerIndexMappings.erase(TSId);
+    MissingTypeServerPDBs.emplace(TSId);
+    return std::move(E);
   }
-  if (auto E = ExpectedSession.takeError())
-    fatal("Type server PDB was not found: " + toString(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)));
   auto ExpectedIpi = (*ExpectedSession)->getPDBFile().getPDBIpiStream();
   if (auto E = ExpectedIpi.takeError())
     fatal("Type server does not have TPI stream: " + toString(std::move(E)));
 
static ArrayRef<uint8_t> relocateDebugChunk(BumpPtrAllocator &Alloc,
   return consumeDebugMagic(makeArrayRef(Buffer, DebugChunk->getSize()),
                            ".debug$S");
 }
 
 void PDBLinker::addObjFile(ObjFile *File) {
   // Add a module descriptor for every object file. We need to put an absolute
   // path to the object into the PDB. If this is a plain object, we make its
   // path absolute. If it's an object in an archive, we make the archive path
   // absolute.
   bool InArchive = !File->ParentName.empty();
   SmallString<128> Path = InArchive ? File->ParentName : File->getName();
   sys::fs::make_absolute(Path);
   sys::path::native(Path, sys::path::Style::windows);
   StringRef Name = InArchive ? File->getName() : StringRef(Path);
 
   File->ModuleDBI = &ExitOnErr(Builder.getDbiBuilder().addModuleInfo(Name));
   File->ModuleDBI->setObjFileName(Path);
 
   // 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);
+  auto IndexMapResult = mergeDebugT(File, ObjectIndexMap);
+
+  // If the .debug$T sections fail to merge, assume there is no debug info.
+  if (!IndexMapResult) {
+    warn("Type server PDB for " + Name + " is invalid, ignoring debug info. " +
+         toString(IndexMapResult.takeError()));
+    return;
+  }
+
+  const CVIndexMap &IndexMap = *IndexMapResult;
 
   ScopedTimer T(SymbolMergingTimer);
 
   // Now do all live .debug$S sections.
   DebugStringTableSubsectionRef CVStrTab;
   DebugChecksumsSubsectionRef Checksums;
   std::vector<ulittle32_t *> StringTableReferences;
   for (SectionChunk *DebugChunk : File->getDebugChunks()) {

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 was not found
 
 --- !COFF
 header:
   Machine:         IMAGE_FILE_MACHINE_AMD64
   Characteristics: [  ]
 sections:
   - Name:            '.debug$S'
     Characteristics: [ IMAGE_SCN_CNT_INITIALIZED_DATA, IMAGE_SCN_MEM_DISCARDABLE, IMAGE_SCN_MEM_READ ]