Page MenuHomePhabricator

Minidump plugin: Adding x86_32 register context converter
ClosedPublic

Authored by dvlahovski on Oct 20 2016, 10:11 AM.

Details

Summary

This, like the x86_64 case, reads the register values from the minidump
file, and emits a binary buffer that is ordered using the offsets from
the RegisterInfoInterface argument. That way we can reuse an existing
register context.
Added unit tests.

Diff Detail

Repository
rL LLVM

Event Timeline

dvlahovski retitled this revision from to Minidump plugin: Adding x86_32 register context converter.
dvlahovski updated this object.
dvlahovski added reviewers: labath, zturner.
dvlahovski added subscribers: lldb-commits, amccarth.
labath accepted this revision.Oct 24 2016, 7:38 AM
labath edited edge metadata.

looks good, minor comments below.

source/Plugins/Process/minidump/RegisterContextMinidump_x86_32.h
37 ↗(On Diff #75315)

You don't need the WithRegIface suffix. That's an ObjC style, which we don't use in LLVM

52 ↗(On Diff #75315)

typo

115 ↗(On Diff #75315)

This comment doesn't seem to apply to the macro below it.

unittests/Process/minidump/MinidumpParserTest.cpp
279 ↗(On Diff #75315)

If you want to split them off to a different file, do it now. If not, remove the todo. (I don't see a reason for the split btw)

This revision is now accepted and ready to land.Oct 24 2016, 7:38 AM
dvlahovski updated this revision to Diff 75578.Oct 24 2016, 7:46 AM
dvlahovski marked 3 inline comments as done.
dvlahovski edited edge metadata.

Fixes regarding Pavel's comments

dvlahovski added inline comments.Oct 24 2016, 7:47 AM
unittests/Process/minidump/MinidumpParserTest.cpp
279 ↗(On Diff #75315)

Yes, I removed the TODO in my next CL. I'll leave the tests here.

amccarth accepted this revision.Oct 28 2016, 10:51 AM
amccarth added a reviewer: amccarth.
amccarth added inline comments.
unittests/Process/minidump/MinidumpParserTest.cpp
340 ↗(On Diff #75578)

It seems a shame to remove the // clang-format off comment for this section. The aligned version is easier to read.

labath added inline comments.Oct 28 2016, 11:27 AM
unittests/Process/minidump/MinidumpParserTest.cpp
340 ↗(On Diff #75578)

I asked Dimitar to remove that. My feeling is that the directive should be reserved for just the extreme cases, where we would end up with a completely unreadable mess without it (like the current register contexts... ugh...). It's not a particularly strong feeling but e.g., there are zero occurrences of this in llvm.

This revision was automatically updated to reflect the committed changes.