[PATCH] D124317: [llvm-pdbutil] Add options to only dump symbol record at specified offset and its parents or children with spcified depth.

[Reid Kleckner via Phabricator via llvm-commits]

rnk added inline comments.


================
Comment at: llvm/include/llvm/DebugInfo/CodeView/CVSymbolVisitor.h:37
   Error visitSymbolStream(const CVSymbolArray &Symbols, uint32_t InitialOffset);
+  Error visitSymbolStreamAtOffset(const CVSymbolArray &Symbols);
 
----------------
Rather than having these fields, I think it might make more sense to have a new method, `visitSymbolStreamFiltered(Syms, FilterOptions)`, which implements this new behavior. It would be slightly more functional and less stateful. WDYT?

I'm imagining a simple struct combining all the options, in case we want to add more later:
```
struct FilterOptions {
  Optional<uint32_t> SymbolOffset;
  Optional<uint32_t> ParentRecursiveDepth;
  Optional<uint32_t> ChildRecursiveDepth;
};
```


================
Comment at: llvm/lib/DebugInfo/CodeView/CVSymbolVisitor.cpp:108
+        }
+      }
+    }
----------------
I think you need to "pop off" a parent record every time you see a closing scope. Consider this sequence of records, where `(` is open, `)` is close, `*` is the desired record: `(()(*))`

In your existing test case, this would correspond with the output of a second inline call site that isn't part of the first.


================
Comment at: llvm/lib/DebugInfo/CodeView/CVSymbolVisitor.cpp:149
+      uint32_t ParentEndOffset =
+          getScopeEndOffset(*Symbols.at(ParentSymbols[StartIndex]));
+      CVSymbol ParentEnd = *Symbols.at(ParentEndOffset);
----------------
I don't think these scope end offsets are available in the typical object file. Try running llvm-pdbutil dump with these filtration options on a single object file instead of a PDB, I think you'll find that the end offset is zero.


================
Comment at: llvm/lib/DebugInfo/CodeView/CVSymbolVisitor.cpp:150
+          getScopeEndOffset(*Symbols.at(ParentSymbols[StartIndex]));
+      CVSymbol ParentEnd = *Symbols.at(ParentEndOffset);
+      if (auto EC = visitSymbolRecord(ParentEnd, ParentEndOffset))
----------------
This would crash if the PDB file is corrupt, please make it handle this case gracefully. I think in dumping code, we should be as defensive as possible because dumpers are often used on new or non-conforming inputs that trigger linker bugs.


================
Comment at: llvm/lib/DebugInfo/CodeView/CVSymbolVisitor.cpp:156
+      --StartIndex;
+    }
+  }
----------------
To try to simplify things and avoid OOB error checking, I would try to restructure this into a single loop. Always maintain a stack of open parent record offsets. Pop them off when you reach closing scopes. When you reach the record you are looking for, print the open scopes.


================
Comment at: llvm/test/tools/llvm-pdbutil/symbol-offset.test:36
+; RUN: | FileCheck --check-prefix=SHOW-CHILDREN-DEPTH2 %s
+
+OFFSET:      80 | S_LOCAL [size = 16] `argc`
----------------
Please include the C++ source used to produce the input YAML file to help illustrate the expected structure.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124317