Page MenuHomePhabricator

Read exception records from Windows mini dump

Authored by amccarth on Aug 18 2015, 4:07 PM.



Now when you do a thread list, you'll see the exception information in the stop info for the thread that stopped.

I'd like to move the ExceptionRecord.h file to a common place as a separate patch.

Diff Detail


Event Timeline

amccarth updated this revision to Diff 32470.Aug 18 2015, 4:07 PM
amccarth retitled this revision from to Read exception records from Windows mini dump.
amccarth updated this object.
amccarth added a reviewer: zturner.
amccarth added a subscriber: lldb-commits.
zturner edited edge metadata.Aug 18 2015, 4:17 PM

I would really like to start seeing tests go in at the same time as the patches. Can any of this functionality be tested yet? i.e. are there commands that will exercise this code? Also still haven't seen tests go in for the basic minidump functionality, like a test that just creates and loads a minidump without any errors. Do you think it's better to do that now or as a followup patch? Either way, I don't think we should wait too much longer before building up some tests.

160 ↗(On Diff #32470)

Shouldn't this be std::numeric_limits<uint32_t>::max() and the loop index variable be unsigned as well?

173 ↗(On Diff #32470)

Can you invert this conditional and early-out to keep the indentation level reduced?

320 ↗(On Diff #32470)

Same here

339 ↗(On Diff #32470)

And here.

amccarth marked 2 inline comments as done.Aug 18 2015, 4:35 PM
amccarth added inline comments.
173 ↗(On Diff #32470)

Done, but >grumble<. It seems likely this code will evolve into a cascaded if-else-if (as per the ProcessWindows::RefreshStateAfterStop), in which case the early out would have to be reverted.

Checking for prerequisites before doing something is natural, flexible, and avoids code duplication. Early outs are like Yoda comparisons--harder to read for dubious advantage.

320 ↗(On Diff #32470)

That would violate the DRY principle, since I'd have to repeat the construction of an invalid or unknown ArchSpec.

amccarth updated this revision to Diff 32475.Aug 18 2015, 4:36 PM
amccarth edited edge metadata.
amccarth marked an inline comment as done.

Addresses some of Zach's comments.

zturner added inline comments.Aug 18 2015, 4:43 PM
172 ↗(On Diff #32475)

I know :-/ Not saying I agree or disagree, it's just the LLVM style.

amccarth updated this revision to Diff 32476.Aug 18 2015, 4:43 PM

Forgot to flip logic for the early outs.

This revision was automatically updated to reflect the committed changes.