[Lldb-commits] [PATCH] D67067: Breakpad: Basic support for STACK WIN unwinding

[Adrian McCarthy via Phabricator via lldb-commits]

amccarth added a comment.

This is looking pretty good.

I'm wondering whether it needs some "negative" tests.



================
Comment at: source/Plugins/SymbolFile/Breakpad/SymbolFileBreakpad.cpp:549
+  llvm::Optional<StackWinRecord> record = StackWinRecord::parse(*It);
+  assert(record.hasValue());
+
----------------
Should we log and bail out rather than just assert?  A corrupt symbol file shouldn't kill the debugger, right?

Also, it's Optional rather than Expected, so it seems even more plausible to hit this.


================
Comment at: source/Plugins/SymbolFile/Breakpad/SymbolFileBreakpad.cpp:586
+  // We assume the first value will be the CFA. It is usually called T0, but
+  // clang will use T1, if it needs to realing the stack.
+  if (!postfix::ResolveSymbols(it->second, symbol_resolver)) {
----------------
Typo: realign


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

https://reviews.llvm.org/D67067