[PATCH] D88988: [llvm-symbolizer] Add inline stack traces for Windows.

[Reid Kleckner via Phabricator via llvm-commits]

rnk accepted this revision.
rnk added a comment.
This revision is now accepted and ready to land.

lgtm, but please address the commented out code before landing.

Sorry for the delay, I wrote all these comments, and then didn't hit send.



================
Comment at: llvm/include/llvm/DebugInfo/PDB/Native/NativeSession.h:133
+  IMap::Allocator IMapAllocator;
+  IMap AddrToModuleIndex;
 };
----------------
You know, we shouldn't actually need to create this intermediate data structure. The section contributions in the PDB are sorted by ascending RVA. It should be possible to binary search them directly with `std::lower_bound` without building a new interval map data structure.

But, I see you are just moving this code around, so this can be a follow-up optimization.


================
Comment at: llvm/include/llvm/DebugInfo/PDB/Native/SymbolCache.h:40
   /// cached.
-  std::vector<std::unique_ptr<NativeRawSymbol>> Cache;
+  mutable std::vector<std::unique_ptr<NativeRawSymbol>> Cache;
 
----------------
All the const/mutable changes make sense, the cache is "notionally" const. Looking things up fills the cache, but doesn't affect results.


================
Comment at: llvm/lib/DebugInfo/PDB/Native/NativeFunctionSymbol.cpp:113-114
+  uint32_t CodeOffset = VA - getVirtualAddress();
+  auto Start = Syms.at(RecordOffset);
+  auto End = Syms.at(Sym.End);
+  while (Start != End) {
----------------
Getting here requires doing `O(log(#inputsections))` work, which is pretty quick. This loop here is `O(#symbols in function)`. Maybe one day the lookup will need to be faster or be cached, but I don't see any obvious way to do that today.


================
Comment at: llvm/lib/DebugInfo/PDB/Native/SymbolCache.cpp:74
 SymbolCache::SymbolCache(NativeSession &Session, DbiStream *Dbi)
-    : Session(Session), Dbi(Dbi), AddrToModuleIndex(IMapAllocator) {
+    : Session(Session), Dbi(Dbi) { //, AddrToModuleIndex(IMapAllocator) {
   // Id 0 is reserved for the invalid symbol.
----------------
Please remove the commented out part.


================
Comment at: llvm/lib/DebugInfo/PDB/Native/SymbolCache.cpp:566
   // address range.
-  Optional<uint16_t> EndModi = getModuleIndexForAddr(VA + Length);
-  if (!EndModi)
----------------
akhuang wrote:
> fyi, I removed this bit of code since it was buggy. It basically searches for line numbers in the next compile unit if we haven't exceeded the length. I was just trying to match DIA's behavior here, but I don't think we ever use this
Makes sense.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88988