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

[Adrian McCarthy via Phabricator via lldb-commits]

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

LGTM.  Please consider adding a comment to the assert that summarizes what you explained in the thread.



================
Comment at: source/Plugins/SymbolFile/Breakpad/SymbolFileBreakpad.cpp:549
+  llvm::Optional<StackWinRecord> record = StackWinRecord::parse(*It);
+  assert(record.hasValue());
+
----------------
labath wrote:
> amccarth wrote:
> > 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.
> This is an internal consistency check. An entry will be added to the `m_unwind_data->win` map only if it was already parsed successfully down in `ParseUnwindData`. This is parsing the same data once more, so it should always succeed.
> 
> Now the next question is, why parse the same data twice? :)
> The first parse is to build an index of the ranges covered by the breakpad file. In the second pass we actually parse the undwind data. Theoretically we could avoid the second parse if we stored more data in the first one. However, here I am operating under the assumption that most record will not be touched, so it's worth to save some space for *each* record for the price of having to parse twice *some* of them. This seems like a good tradeoff intuitively, but I am don't have hard data to back that up.
> 
> Also, the case was much stronger for STACK CFI records (which do the same thing), as there I only have to put the STACK CFI INIT records into the map (and each INIT record is followed by a potentially large number of non-INIT records). STACK WIN records don't have an INIT records, so I have to insert all of them anyway, which makes the savings smaller, but it still seems worth it.
Cool.  Could you just add a comment at the assertion that says a short version of that.  For example, "We've already parsed it once, so it shouldn't fail this time."


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

https://reviews.llvm.org/D67067