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

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

We always need more tests. I've now added more tests for various boundary conditions.



================
Comment at: source/Plugins/SymbolFile/Breakpad/SymbolFileBreakpad.cpp:549
+  llvm::Optional<StackWinRecord> record = StackWinRecord::parse(*It);
+  assert(record.hasValue());
+
----------------
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.


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

https://reviews.llvm.org/D67067