[PATCH] D87736: [PDB] Split TypeServerSource and extend type index map lifetime

[Reid "Away June-Sep" Kleckner via Phabricator via llvm-commits]

rnk added inline comments.


================
Comment at: lld/COFF/PDB.cpp:191
 public:
-  DebugSHandler(PDBLinker &linker, ObjFile &file, const CVIndexMap *indexMap)
-      : linker(linker), file(file), indexMap(indexMap) {}
+  DebugSHandler(PDBLinker &linker, ObjFile &file, TpiSource *source)
+      : linker(linker), file(file), source(source) {}
----------------
aganea wrote:
> Can we only have `TpiSource *` here and infer `file`?
Almost, but there are ObjFiles which lack a corresponding TpiSource, and we must add their symbols. So TpiSource is optional (may be null) and ObjFile is required (reference). I checked, and this case is tested in tree.


================
Comment at: lld/COFF/PDB.cpp:276
+    ArrayRef<TypeIndex> typeOrItemMap;
+    if (source)
+      typeOrItemMap = isItemIndex ? source->ipiMap : source->tpiMap;
----------------
aganea wrote:
> Don't we always expect a `source` at this point?
I wish. Perhaps we should fix this by making a source for all objs, even if they lack types to avoid the null check.


================
Comment at: lld/COFF/PDB.cpp:755
     TypeIndex &inlinee = *const_cast<TypeIndex *>(&line.Header->Inlinee);
-    ArrayRef<TypeIndex> typeOrItemMap =
-        indexMap->isTypeServerMap ? indexMap->ipiMap : indexMap->tpiMap;
-    if (!remapTypeIndex(inlinee, typeOrItemMap)) {
+    if (!remapTypeIndex(inlinee, source->ipiMap)) {
       log("bad inlinee line record in " + file.getName() +
----------------
aganea wrote:
> `source` isn't checked here. Perhaps `assert(source)` in the `DebugSHandler` constructor?
Huh, I thought I added a check above and had it warn and return. Must have gotten lost.


================
Comment at: lld/COFF/PDB.cpp:921
 
-  addDebugSymbols(source->file, indexMap);
+  // If the source has an object file, add its symbol records.
+  if (source->file)
----------------
aganea wrote:
> ...otherwise if you don't want the `if`, it is worth adding a line to explain in which situation the source doesn't have a file.
I can make the comment more explicit: PDB sources don't have symbols and don't have an object file, so only add symbols if the source is for an object file.


================
Comment at: lld/COFF/PDB.cpp:923
+  if (source->file)
+    addDebugSymbols(source->file, source);
 }
----------------
aganea wrote:
> No need to pass down both `file` and `source`.
I think we can eliminate that if we make empty TpiSources for objects with symbols but no types, I'll try that.


================
Comment at: lld/COFF/PDB.cpp:931
-
-  if (source->kind == TpiSource::PDB)
-    return; // No symbols in TypeServer PDBs
----------------
aganea wrote:
> I liked it better with this `if` as it is self-explantory. Is there another situation where we should return here? The `TypeServerIpiSource` is never returned, right?
Actually, we do come here for IPI sources. We iterate over all TpiSource objects and call addDebug on them, so if we keep this if, it will need two conditions. IMO the new `if` handles it more neatly: only object files have symbols, so if this is a source for an object file, add symbols.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87736