This is an archive of the discontinued LLVM Phabricator instance.

Read exception records from Windows mini dump
ClosedPublic

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

Details

Summary

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.

source/Plugins/Process/win-minidump/ProcessWinMiniDump.cpp
159–160

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

172

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

319–320

Same here

338

And here.

amccarth marked 2 inline comments as done.Aug 18 2015, 4:35 PM
amccarth added inline comments.
source/Plugins/Process/win-minidump/ProcessWinMiniDump.cpp
172

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.

319–320

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
source/Plugins/Process/win-minidump/ProcessWinMiniDump.cpp
172

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

http://llvm.org/docs/CodingStandards.html#use-early-exits-and-continue-to-simplify-code

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.