[PDB] Emit old fpo data to the PDB

[zturner]

Summary

D51951 tried to fix https://bugs.llvm.org/show_bug.cgi?id=38847 by emitting the new fpo data stream to the PDB file. While I confirmed that that did indeed fix one known repro of this symptom, it turns out it didn't fix the one in the reporting bug.

The reason is that FPO data is emitted in two different places into the PDB, each with its own different format. The DBI stream contains a "new fpo" data stream and an "fpo" data stream (which I refer to as the "old" fpo stream). The New Fpo data stream is populated via the contents of the individual object files' DEBUG_S_FRAMEDATA subsections that appear inside of .debug$S sections. This is what modern compilers (both clang-cl and cl) emit. However, the bug in the report is about what happens if a crash occurs inside of memcpy. memcpy is written in assembler, and if you disassemble memcpy.obj from the CRT you find that it does not have a DEBUG_S_FRAMEDATA subsection in its object file. Instead, it contains a .debug$F section, which is something that until now there has not been a single reference to in the entire LLVM, Clang, or LLD codebase. However, some investigation into this shows that it is a trivial format, and is simply a list of FPO_DATA records. Furthermore, this section contains relocations, so unlike the manual relocating we needed to do with the DEBUG_S_FRAMEDATA contents, we just need to apply relocations to this section and copy it through.

But, as mentioned the format is slightly different and it does not go to the new fpo data stream, it goes to its own place.

With this patch I confirmed that the problem in the original bug report no longer reproduces. ChildFunction correctly shows up in my stack trace and I can switch to it and view local variables.

[rnk]

lld/COFF/PDB.cpp

899–906 ↗

(On Diff #164996)

You can check if the section is empty before you relocate it by checking DebugChunk->getSize() == 0. Then you can sink the calls to relocateDebugChunk into the .debug$S handling and only check the section name once.

957 ↗

(On Diff #164996)

I would handle .debug$F first and continue early to reduce indentation. It's the less complicated special case.

[zturner]

I might have gone overboard here, but I wanted to kind of put all .debug$S handling into its own function to make the loop simpler and easier to follow. Since resolving .debug$S requires state to be maintained across multiple .debug$S sections in a single object file, I made a class out of it and moved a lot of the local state into the class. .debug$F is simpler and doesn't require state across runs so I just handle that directly in the loop.

[rnk]

Needs tests? Maybe they just aren't in the diff?

lld/COFF/PDB.cpp

845–846 ↗

(On Diff #165106)

I think we can raise this into handleDebugS and then we don't need the HasMagic parameter.

COFF/PDB.cpp

 /// Map from type index and item index in a type server PDB to the
 /// corresponding index in the destination PDB.
 struct CVIndexMap {
   SmallVector<TypeIndex, 0> TPIMap;
   SmallVector<TypeIndex, 0> IPIMap;
   bool IsTypeServerMap = false;
 };
 
+class DebugSHandler;
+
 class PDBLinker {
+  friend DebugSHandler;
+
 public:
   PDBLinker(SymbolTable *Symtab)
       : Alloc(), Symtab(Symtab), Builder(Alloc), TypeTable(Alloc),
         IDTable(Alloc), GlobalTypeTable(Alloc), GlobalIDTable(Alloc) {
     // This isn't strictly necessary, but link.exe usually puts an empty string
     // as the first "valid" string in the string table, so we do the same in
     // order to maintain as much byte-for-byte compatibility as possible.
     PDBStrTab.insert("");
private:
 
   /// 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::map<GUID, std::string> MissingTypeServerPDBs;
 };
+
+class DebugSHandler {
+  PDBLinker &Linker;
+
+  /// The object file whose .debug$S sections we're processing.
+  ObjFile &File;
+
+  /// The result of merging type indices.
+  const CVIndexMap &IndexMap;
+
+  /// The DEBUG_S_STRINGTABLE subsection.  These strings are referred to by
+  /// index from other records in the .debug$S section.  All of these strings
+  /// need to be added to the global PDB string table, and all references to
+  /// these strings need to have their indices re-written to refer to the
+  /// global PDB string table.
+  DebugStringTableSubsectionRef CVStrTab;
+
+  /// The DEBUG_S_FILECHKSMS subsection.  As above, these are referred to
+  /// by other records in the .debug$S section and need to be merged into the
+  /// PDB.
+  DebugChecksumsSubsectionRef Checksums;
+
+  /// The DEBUG_S_FRAMEDATA subsection(s).  There can be more than one of
+  /// these and they need not appear in any specific order.  However, they
+  /// contain string table references which need to be re-written, so we
+  /// collect them all here and re-write them after all subsections have been
+  /// discovered and processed.
+  std::vector<DebugFrameDataSubsectionRef> NewFpoFrames;
+
+  /// Pointers to raw memory that we determine have string table references
+  /// that need to be re-written.  We first process all .debug$S subsections
+  /// to ensure that we can handle subsections written in any order, building
+  /// up this list as we go.  At the end, we use the string table (which must
+  /// have been discovered by now else it is an error) to re-write these
+  /// references.
+  std::vector<ulittle32_t *> StringTableReferences;
+
+public:
+  DebugSHandler(PDBLinker &Linker, ObjFile &File, const CVIndexMap &IndexMap)
+      : Linker(Linker), File(File), IndexMap(IndexMap) {}
+
+  void handleDebugS(lld::coff::SectionChunk &DebugS);
+  void finish();
+};
 }
 
 static SectionChunk *findByName(ArrayRef<SectionChunk *> Sections,
                                 StringRef Name) {
   for (SectionChunk *C : Sections)
     if (C->getSectionName() == Name)
       return C;
   return nullptr;
auto EC = forEachCodeViewRecord<CVSymbol>(
         // Add the symbol to the module.
         if (symbolGoesInModuleStream(NewSym))
           File->ModuleDBI->addSymbol(NewSym);
         return Error::success();
       });
   cantFail(std::move(EC));
 }
 
-// Allocate memory for a .debug$S section and relocate it.
+// Allocate memory for a .debug$S / .debug$F section and relocate it.
 static ArrayRef<uint8_t> relocateDebugChunk(BumpPtrAllocator &Alloc,
-                                            SectionChunk *DebugChunk) {
-  uint8_t *Buffer = Alloc.Allocate<uint8_t>(DebugChunk->getSize());
-  assert(DebugChunk->OutputSectionOff == 0 &&
+                                            SectionChunk &DebugChunk) {
+  uint8_t *Buffer = Alloc.Allocate<uint8_t>(DebugChunk.getSize());
+  assert(DebugChunk.OutputSectionOff == 0 &&
          "debug sections should not be in output sections");
-  DebugChunk->writeTo(Buffer);
-  return consumeDebugMagic(makeArrayRef(Buffer, DebugChunk->getSize()),
-                           ".debug$S");
+  DebugChunk.writeTo(Buffer);
+  return makeArrayRef(Buffer, DebugChunk.getSize());
 }
 
 static pdb::SectionContrib createSectionContrib(const Chunk *C, uint32_t Modi) {
   OutputSection *OS = C->getOutputSection();
   pdb::SectionContrib SC;
   memset(&SC, 0, sizeof(SC));
   SC.ISect = OS->SectionIndex;
   SC.Off = C->getRVA() - OS->getRVA();
if (!ExpectedString) {
     warn("Invalid string table reference");
     consumeError(ExpectedString.takeError());
     return 0;
   }
 
   return PdbStrTable.insert(*ExpectedString);
 }
 
-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);
-
-  pdb::DbiStreamBuilder &DbiBuilder = Builder.getDbiBuilder();
-  File->ModuleDBI = &ExitOnErr(DbiBuilder.addModuleInfo(Name));
-  File->ModuleDBI->setObjFileName(Path);
-
-  auto Chunks = File->getChunks();
-  uint32_t Modi = File->ModuleDBI->getModuleIndex();
-  for (Chunk *C : Chunks) {
-    auto *SecChunk = dyn_cast<SectionChunk>(C);
-    if (!SecChunk || !SecChunk->Live)
-      continue;
-    pdb::SectionContrib SC = createSectionContrib(SecChunk, Modi);
-    File->ModuleDBI->setFirstSectionContrib(SC);
-    break;
-  }
-
-  // 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;
-  auto IndexMapResult = mergeDebugT(File, ObjectIndexMap);
-
-  // If the .debug$T sections fail to merge, assume there is no debug info.
-  if (!IndexMapResult) {
-    auto FileName = sys::path::filename(Path);
-    warn("Cannot use debug info for '" + FileName + "'\n" +
-         ">>> failed to load reference " +
-         StringRef(toString(IndexMapResult.takeError())));
-    return;
-  }
-
-  const CVIndexMap &IndexMap = *IndexMapResult;
-
-  ScopedTimer T(SymbolMergingTimer);
+void DebugSHandler::handleDebugS(lld::coff::SectionChunk &DebugS) {
+  DebugSubsectionArray Subsections;
 
-  // Now do all live .debug$S sections.
-  DebugStringTableSubsectionRef CVStrTab;
-  DebugChecksumsSubsectionRef Checksums;
-  std::vector<ulittle32_t *> StringTableReferences;
-  std::vector<DebugFrameDataSubsectionRef> FpoFrames;
-  for (SectionChunk *DebugChunk : File->getDebugChunks()) {
-    if (!DebugChunk->Live || DebugChunk->getSectionName() != ".debug$S")
-      continue;
+  ArrayRef<uint8_t> RelocatedDebugContents = consumeDebugMagic(
+      relocateDebugChunk(Linker.Alloc, DebugS), DebugS.getSectionName());
 
-    ArrayRef<uint8_t> RelocatedDebugContents =
-        relocateDebugChunk(Alloc, DebugChunk);
-    if (RelocatedDebugContents.empty())
-      continue;
-
-    DebugSubsectionArray Subsections;
-    BinaryStreamReader Reader(RelocatedDebugContents, support::little);
-    ExitOnErr(Reader.readArray(Subsections, RelocatedDebugContents.size()));
 
-    for (const DebugSubsectionRecord &SS : Subsections) {
-      switch (SS.kind()) {
-      case DebugSubsectionKind::StringTable: {
-        assert(!CVStrTab.valid() &&
-               "Encountered multiple string table subsections!");
-        ExitOnErr(CVStrTab.initialize(SS.getRecordData()));
-        break;
-      }
-      case DebugSubsectionKind::FileChecksums:
-        assert(!Checksums.valid() &&
-               "Encountered multiple checksum subsections!");
-        ExitOnErr(Checksums.initialize(SS.getRecordData()));
-        break;
-      case DebugSubsectionKind::Lines:
-        // We can add the relocated line table directly to the PDB without
-        // modification because the file checksum offsets will stay the same.
-        File->ModuleDBI->addDebugSubsection(SS);
-        break;
-      case DebugSubsectionKind::FrameData: {
-        // We need to re-write string table indices here, so save off all
-        // frame data subsections until we've processed the entire list of
-        // subsections so that we can be sure we have the string table.
-        DebugFrameDataSubsectionRef FDS;
-        ExitOnErr(FDS.initialize(SS.getRecordData()));
-        FpoFrames.push_back(std::move(FDS));
-        break;
-      }
-      case DebugSubsectionKind::Symbols:
-        if (Config->DebugGHashes) {
-          mergeSymbolRecords(Alloc, File, Builder.getGsiBuilder(), IndexMap,
-                             GlobalIDTable, StringTableReferences,
-                             SS.getRecordData());
-        } else {
-          mergeSymbolRecords(Alloc, File, Builder.getGsiBuilder(), IndexMap,
-                             IDTable, StringTableReferences,
-                             SS.getRecordData());
-        }
-        break;
-      default:
-        // FIXME: Process the rest of the subsections.
-        break;
-      }
-    }
-  }
+      File.ModuleDBI->addDebugSubsection(SS);
+      NewFpoFrames.push_back(std::move(FDS));
+        mergeSymbolRecords(Linker.Alloc, &File, Linker.Builder.getGsiBuilder(),
+                           IndexMap, Linker.GlobalIDTable,
+                           StringTableReferences, SS.getRecordData());
+        mergeSymbolRecords(Linker.Alloc, &File, Linker.Builder.getGsiBuilder(),
+                           IndexMap, Linker.IDTable, StringTableReferences,
 
+void DebugSHandler::finish() {
+  pdb::DbiStreamBuilder &DbiBuilder = Linker.Builder.getDbiBuilder();
+
   // We should have seen all debug subsections across the entire object file now
   // which means that if a StringTable subsection and Checksums subsection were
   // present, now is the time to handle them.
   if (!CVStrTab.valid()) {
     if (Checksums.valid())
       fatal(".debug$S sections with a checksums subsection must also contain a "
             "string table subsection");
 
     if (!StringTableReferences.empty())
       warn("No StringTable subsection was encountered, but there are string "
            "table references");
     return;
   }
 
   // Rewrite string table indices in the Fpo Data and symbol records to refer to
   // the global PDB string table instead of the object file string table.
-  for (DebugFrameDataSubsectionRef &FDS : FpoFrames) {
+  for (DebugFrameDataSubsectionRef &FDS : NewFpoFrames) {
     const uint32_t *Reloc = FDS.getRelocPtr();
     for (codeview::FrameData FD : FDS) {
       FD.RvaStart += *Reloc;
       FD.FrameFunc =
-          translateStringTableIndex(FD.FrameFunc, CVStrTab, PDBStrTab);
-      DbiBuilder.addFrameData(FD);
+          translateStringTableIndex(FD.FrameFunc, CVStrTab, Linker.PDBStrTab);
+      DbiBuilder.addNewFpoData(FD);
     }
   }
 
   for (ulittle32_t *Ref : StringTableReferences)
-    *Ref = translateStringTableIndex(*Ref, CVStrTab, PDBStrTab);
+    *Ref = translateStringTableIndex(*Ref, CVStrTab, Linker.PDBStrTab);
 
   // Make a new file checksum table that refers to offsets in the PDB-wide
   // string table. Generally the string table subsection appears after the
   // checksum table, so we have to do this after looping over all the
   // subsections.
-  auto NewChecksums = make_unique<DebugChecksumsSubsection>(PDBStrTab);
+  auto NewChecksums = make_unique<DebugChecksumsSubsection>(Linker.PDBStrTab);
   for (FileChecksumEntry &FC : Checksums) {
-    SmallString<128> FileName = ExitOnErr(CVStrTab.getString(FC.FileNameOffset));
-    if (!sys::path::is_absolute(FileName) &&
-        !Config->PDBSourcePath.empty()) {
+    SmallString<128> FileName =
+        ExitOnErr(CVStrTab.getString(FC.FileNameOffset));
+    if (!sys::path::is_absolute(FileName) && !Config->PDBSourcePath.empty()) {
       SmallString<128> AbsoluteFileName = Config->PDBSourcePath;
       sys::path::append(AbsoluteFileName, FileName);
       sys::path::native(AbsoluteFileName);
       sys::path::remove_dots(AbsoluteFileName, /*remove_dot_dots=*/true);
       FileName = std::move(AbsoluteFileName);
     }
-    ExitOnErr(Builder.getDbiBuilder().addModuleSourceFile(*File->ModuleDBI,
-                                                          FileName));
+    ExitOnErr(Linker.Builder.getDbiBuilder().addModuleSourceFile(
+        *File.ModuleDBI, FileName));
     NewChecksums->addChecksum(FileName, FC.Kind, FC.Checksum);
   }
-  File->ModuleDBI->addDebugSubsection(std::move(NewChecksums));
+  File.ModuleDBI->addDebugSubsection(std::move(NewChecksums));
+}
+
+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);
+
+  pdb::DbiStreamBuilder &DbiBuilder = Builder.getDbiBuilder();
+  File->ModuleDBI = &ExitOnErr(DbiBuilder.addModuleInfo(Name));
+  File->ModuleDBI->setObjFileName(Path);
+
+  auto Chunks = File->getChunks();
+  uint32_t Modi = File->ModuleDBI->getModuleIndex();
+  for (Chunk *C : Chunks) {
+    auto *SecChunk = dyn_cast<SectionChunk>(C);
+    if (!SecChunk || !SecChunk->Live)
+      continue;
+    pdb::SectionContrib SC = createSectionContrib(SecChunk, Modi);
+    File->ModuleDBI->setFirstSectionContrib(SC);
+    break;
+  }
+
+  // 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;
+  auto IndexMapResult = mergeDebugT(File, ObjectIndexMap);
+
+  // If the .debug$T sections fail to merge, assume there is no debug info.
+  if (!IndexMapResult) {
+    auto FileName = sys::path::filename(Path);
+    warn("Cannot use debug info for '" + FileName + "'\n" +
+         ">>> failed to load reference " +
+         StringRef(toString(IndexMapResult.takeError())));
+    return;
+  }
+
+  ScopedTimer T(SymbolMergingTimer);
+
+  DebugSHandler DSH(*this, *File, *IndexMapResult);
+  // Now do all live .debug$S and .debug$F sections.
+  for (SectionChunk *DebugChunk : File->getDebugChunks()) {
+    if (!DebugChunk->Live || DebugChunk->getSize() == 0)
+      continue;
+
+    if (DebugChunk->getSectionName() == ".debug$S") {
+      DSH.handleDebugS(*DebugChunk);
+      continue;
+    }
+
+    if (DebugChunk->getSectionName() == ".debug$F") {
+      ArrayRef<uint8_t> RelocatedDebugContents =
+          relocateDebugChunk(Alloc, *DebugChunk);
+
+      FixedStreamArray<object::FpoData> FpoRecords;
+      BinaryStreamReader Reader(RelocatedDebugContents, support::little);
+      uint32_t Count = RelocatedDebugContents.size() / sizeof(object::FpoData);
+      ExitOnErr(Reader.readArray(FpoRecords, Count));
+
+      // These are already relocated and don't refer to the string table, so we
+      // can just copy it.
+      for (const object::FpoData &FD : FpoRecords)
+        DbiBuilder.addOldFpoData(FD);
+      continue;
+    }
+  }
+
+  // Do any post-processing now that all .debug$S sections have been processed.
+  DSH.finish();
 }
 
 static PublicSym32 createPublic(Defined *Def) {
   PublicSym32 Pub(SymbolKind::S_PUB32);
   Pub.Name = Def->getName();
   if (auto *D = dyn_cast<DefinedCOFF>(Def)) {
     if (D->getCOFFSymbol().isFunctionDefinition())
       Pub.Flags = PublicSymFlags::Function;

test/COFF/pdb-debug-f.s

+# RUN: llvm-mc -triple=i386-pc-win32 -filetype=obj -o %t.obj %s
+# RUN: lld-link /subsystem:console /debug /nodefaultlib /entry:foo /out:%t.exe /pdb:%t.pdb %t.obj
+# RUN: llvm-pdbutil dump -fpo %t.pdb | FileCheck %s
+
+# CHECK:                         Old FPO Data
+# CHECK-NEXT: ============================================================
+# CHECK-NEXT:   RVA    | Code | Locals | Params | Prolog | Saved Regs | Use BP | Has SEH | Frame Type
+# CHECK-NEXT: 00001002 |    1 |      2 |      3 |      4 |          0 |  false |   false |       FPO
+
+.text
+_foo:
+ret
+
+.global _foo
+
+.section .debug$F,"dr"
+  .long _foo@IMGREL+2
+  .long 1 #  cbProc
+  .long 2 # cdwLocals;
+  .short 3 # cdwParams;
+  .short 4 # flags
+  # cbProlog : 8;
+  # cbRegs : 3;
+  # fHasSEH : 1;
+  # fUseBP : 1;
+  # reserved : 1;
+  # cbFrame : 2;
 No newline at end of file