This is an archive of the discontinued LLVM Phabricator instance.

Minidump plugin: redesign the x86_64 register context
ClosedPublic

Authored by dvlahovski on Oct 17 2016, 7:40 AM.

Details

Summary

I misunderstood the format of the register context layout.
I thought it was a dynamically changing structure, and that it's size
depended on context_flags.
It turned out that it always has the same fixed layout and size,
and the context_flags says which fields of the
struct have valid values.
This required a minor redesign of the register context class.

The layout inconsistency, however, was not a "problem" before (e.g. the plugin was working)
because there also was a bug with checking context_flags - the code was
parsing the entire struct regardless of context_flags.
This bug is also fixed in this commit.

Event Timeline

dvlahovski updated this revision to Diff 74845.Oct 17 2016, 7:40 AM
dvlahovski retitled this revision from to Minidump plugin: redesign the x86_64 register context.
dvlahovski updated this object.
dvlahovski added reviewers: labath, zturner.
dvlahovski added subscribers: amccarth, lldb-commits.
dvlahovski updated this revision to Diff 74852.Oct 17 2016, 8:22 AM

Adding sanity check for the context's size

labath edited edge metadata.Oct 17 2016, 8:46 AM
source/Plugins/Process/minidump/RegisterContextMinidump_x86_64.cpp
60

This looks like error handling. Should we return an empty buffer in that case (so that the caller can see that we actually failed in parsing this)?

source/Plugins/Process/minidump/RegisterContextMinidump_x86_64.h
141

uint128_struct xmm[16]; ?

I'm not that sure about that struct and it's name though, I'll need to think about that.

dvlahovski updated this revision to Diff 74856.Oct 17 2016, 8:59 AM
dvlahovski edited edge metadata.

Return a nullptr in case of an error

dvlahovski marked an inline comment as done.Oct 17 2016, 9:00 AM
dvlahovski updated this revision to Diff 74859.Oct 17 2016, 9:04 AM

Make a single array of sse registers instead of separated fields

dvlahovski updated this revision to Diff 74987.Oct 18 2016, 6:07 AM

Merge if's checking the same context flag

Just a couple of cleanups and then I think we're ready.

source/Plugins/Process/minidump/RegisterContextMinidump_x86_64.cpp
77

These would be cleaner, if we made writeRegister call getDestRegister internally.
Then these two lines would become:

writeRegister(&context->cs, result_base, reg_info[lldb_cs_x86_64]);

Note that you don't have to pass in lldb_cs_x86_64 explicitly, as that information can be recovered via reg_info->kinds[eRegisterKindLLDB].

source/Plugins/Process/minidump/RegisterContextMinidump_x86_64.h
141

I don't really have a good idea about struct issue. Let's at least call it Uint128, so that it's consistent with the rest of the struct's in this class, and then call it a day.

dvlahovski updated this revision to Diff 75154.Oct 19 2016, 8:12 AM
dvlahovski marked an inline comment as done.

Simplified writeRegister function

s/uint128_struct/Uint128/

dvlahovski marked an inline comment as done.Oct 19 2016, 8:13 AM
labath accepted this revision.Oct 19 2016, 8:21 AM
labath edited edge metadata.

Looks much better, thanks. :)

This revision is now accepted and ready to land.Oct 19 2016, 8:21 AM
This revision was automatically updated to reflect the committed changes.