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

[Pavel Labath via Phabricator via lldb-commits]

labath marked an inline comment as done.
labath added inline comments.


================
Comment at: source/Plugins/SymbolFile/Breakpad/SymbolFileBreakpad.cpp:549
+  llvm::Optional<StackWinRecord> record = StackWinRecord::parse(*It);
+  assert(record.hasValue());
+
----------------
amccarth wrote:
> 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."
Will do. I'll also add the comment to the STACK CFI version of this function.


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

https://reviews.llvm.org/D67067