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