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

Thu Oct 8 16:20:52 PDT 2020

amccarth added a comment.

This looks pretty good.

- Are the sources for the binary test inputs around somewhere so they could be recreated if necessary someday?
- The test turns a small stack into file+line numbers.  Is that sufficient or should there also be one to get function name+offsets?
- Most of the formatting nits listed by lint are legit and should be fixed before committing.

I'll be curious to hear what you figure out about the ASAN tests.

[It's so weird how methods likes getSymbolById return `std::unique_ptr`s.  That's not a bug in this patch, just an unintuitive interface design that we probably inherited from the original DIA wrappers.  Every time I see it, though, I end up chasing down calls to make sure it's correct.]

================
Comment at: llvm/include/llvm/DebugInfo/PDB/Native/NativeSession.h:133
+  IMap AddrToModuleIndex;
+  void parseSectionContribs();
 };
----------------
I don't see anything explicit in LLVM's style guidelines, but I think it's common to segregate the data members and methods whenever possible.  So perhaps the `parseSectionContribes()` should be moved up.

================
Comment at: llvm/lib/DebugInfo/PDB/Native/NativeFunctionSymbol.cpp:59
+
+static bool inlineSiteContainsAddress(InlineSiteSym &IS,
+                                      uint32_t OffsetInFunc) {
----------------
akhuang wrote:
> I think this sometimes returns true incorrectly, seems like it happens when there are multiple ChangeCodeLength annotations. Maybe I'm interpreting those incorrectly? 
I don't know much about these annotations.  Can these be nested?  For example if `foo` runs from f0 to f1 and the annotations also describe another subrange inside of f0..f1, then I would think `Found` would have to be a counter rather than a bool.  If it's 0, you're not inside any range.  If it's 2, you're inside two nested ranges.

If they don't nest, then I don't see a problem.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88988