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

[Zequan Wu via Phabricator via llvm-commits]

zequanwu added inline comments.


================
Comment at: llvm/lib/DebugInfo/CodeView/CVSymbolVisitor.cpp:108
+        }
+      }
+    }
----------------
rnk wrote:
> 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.
An opening scope is only added in to `ParentSymbols` when the scope range [begin, end] encompass the desired offset.
In the case of `(()(*))`, the first inner scope`()` will not be added into `ParentSymbols`. 

Added a test case for `(()(*))`. 


================
Comment at: llvm/lib/DebugInfo/CodeView/CVSymbolVisitor.cpp:149
+      uint32_t ParentEndOffset =
+          getScopeEndOffset(*Symbols.at(ParentSymbols[StartIndex]));
+      CVSymbol ParentEnd = *Symbols.at(ParentEndOffset);
----------------
rnk wrote:
> 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.
Yes, all offsets and parent/end are 0. So, the filter options are only used in `DumpOutputStyle::dumpModuleSymsForPdb`, and silently ignored when dumping obj file.


================
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))
----------------
rnk wrote:
> 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.
Add `VarStreamArray::isOffsetValid` to check if an offset is valid, and use it to check offset before dereference.


================
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`
----------------
rnk wrote:
> Please include the C++ source used to produce the input YAML file to help illustrate the expected structure.
I deleted the cpp file, so I rewrote new one.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124317