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 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
