Page MenuHomePhabricator

Adding a new Minidump post-mortem debugging plugin
ClosedPublic

Authored by dvlahovski on Oct 3 2016, 9:44 AM.

Details

Summary

This plugin resembles the already existing Windows-only Minidump plugin.
The WinMinidumpPlugin uses the Windows API for parsing Minidumps
while this plugin is cross-platform because it includes a Minidump
parser (which is already commited)

It is able to produce a backtrace, to read the general puprose regiters,
inspect local variables, show image list, do memory reads, etc.

For now the only arch that this supports is x86 64 bit
This is because I have only written a register context for that arch.
Others will come in next CLs.

I copied the WinMinidump tests and adapted them a little bit for them to
work with the new plugin (and they pass)
I will add more tests, aiming for better code coverage.

There is still functionality to be added, see TODOs in code.

Diff Detail

Repository
rL LLVM

Event Timeline

dvlahovski updated this revision to Diff 73296.Oct 3 2016, 9:44 AM
dvlahovski retitled this revision from to Adding a new Minidump post-mortem debugging plugin.
dvlahovski updated this object.
dvlahovski added reviewers: labath, zturner.
dvlahovski added subscribers: lldb-commits, amccarth.

So, there will be changes to the way I handle the case where there is no ExceptionStream - I will probably create a StopReason = None. But I have to investigate if in that case lldb still hangs because it's trying to resume a process (that does not exists). I will ask Jim Ingham about some insight about StopReasons

labath requested changes to this revision.Oct 3 2016, 11:07 AM
labath edited edge metadata.

I have a bunch of small comments. I'll have another look through this once they are done.

The high-level change we need is to avoid choosing the plugin to use at compile-time (your plugin should be host independent, so-lets use it everywhere). Since you still don't have full feature parity with the windows plugin, add a runtime check, which makes sure that your plugin does not kick in for windows minidump files. Once you have feature parity, we can remove that check and ask Zachary and Adrian to validate everything still works as they expect.

packages/Python/lldbsuite/test/functionalities/postmortem/minidump-new/TestMiniDumpNew.py
19 ↗(On Diff #73296)

You can replace these with NO_DEBUG_INFO_TESTCASE = True at class level.

66 ↗(On Diff #73296)

I don't think these are necessary.

source/Plugins/Process/minidump/MinidumpParser.cpp
228 ↗(On Diff #73296)

Return llvm::Optional<range_out> ?

source/Plugins/Process/minidump/MinidumpParser.h
36 ↗(On Diff #73296)

Make these two an ArrayRef ?

source/Plugins/Process/minidump/ProcessMinidump.cpp
47 ↗(On Diff #73296)

Please rewrite this to use the early-return style:

if (!crash_file)
  return nullptr;

etc.

55 ↗(On Diff #73296)

the check for non-nullness of data_sp is superfluous (or too late).

67 ↗(On Diff #73296)

It already is return true.

Is that correct?

121 ↗(On Diff #73296)

I think it is. You can remove the TODO.

source/Plugins/Process/minidump/ThreadMinidump.cpp
37 ↗(On Diff #73296)

If you make the constructor take a ProcessMinidump & you can avoid this cast. (although, if we change the code below, you may not even need it).

39 ↗(On Diff #73296)

This should be implemented inside the minidump parser. There's no reason for doing memory slicing at this level.

48 ↗(On Diff #73296)

If you don't know whether this is necessary, prefer simplicity and don't do it

66 ↗(On Diff #73296)

if (!m_reg_context_sp)

also use nullptr instead of NULL.

source/Plugins/Process/minidump/ThreadMinidump.h
1 ↗(On Diff #73296)

typo (-h)

unittests/Process/minidump/MinidumpParserTest.cpp
142 ↗(On Diff #73296)

Please do this now.

This revision now requires changes to proceed.Oct 3 2016, 11:07 AM

I was hoping that, with your new mini dump parser, you'd be able to eliminate the need for the Windows-specific minidump process plugin.

When I wrote the Windows mini dump plugin, I tried to isolate the Windows API-specific bits using the pimpl idiom. Now that you've written a mini dump parser, we shouldn't need the Windows API calls, and nearly all the rest of the code should be shareable between Windows and Linux. Is there a plan to eliminate this redundancy and merge this new mini dump process plugin with the Windows-specific one?

dvlahovski added inline comments.Oct 3 2016, 11:34 AM
source/Plugins/Process/minidump/ProcessMinidump.cpp
67 ↗(On Diff #73296)

What I meant here is: is there functionality that needs to be added here.
In the WinMinidump plugin, this method is only returning true (as mine) and has a TODO saying that it should have actual logic in it

I was hoping that, with your new mini dump parser, you'd be able to eliminate the need for the Windows-specific minidump process plugin.

When I wrote the Windows mini dump plugin, I tried to isolate the Windows API-specific bits using the pimpl idiom. Now that you've written a mini dump parser, we shouldn't need the Windows API calls, and nearly all the rest of the code should be shareable between Windows and Linux. Is there a plan to eliminate this redundancy and merge this new mini dump process plugin with the Windows-specific one?

Yes, the plan is that my plugin will replace the Windows one. (and it has almost the same functionality)

What I have been doing so far is actually copying all of the methods from the WinMinidump plugin to mine and changing them to use the new Minidump parser.

Probably I could have fitted into you pimpl implementation pattern, but I chose just to start a 'new' plugin and copy everything from the WinMinidump, and then change it accordingly.

dvlahovski added inline comments.Oct 3 2016, 2:10 PM
packages/Python/lldbsuite/test/functionalities/postmortem/minidump-new/TestMiniDumpNew.py
19 ↗(On Diff #73296)

But, should test_deeper_stack_in_mini_dump and test_local_variables_in_mini_dump not have this decorator ?

labath added inline comments.Oct 3 2016, 5:59 PM
packages/Python/lldbsuite/test/functionalities/postmortem/minidump-new/TestMiniDumpNew.py
19 ↗(On Diff #73296)

They are not building any code, so the would behave the same way anyway. You would be just running the test 2--3 times for nothing.

dvlahovski marked 5 inline comments as done.Oct 4 2016, 2:31 AM
dvlahovski added inline comments.
packages/Python/lldbsuite/test/functionalities/postmortem/minidump-new/TestMiniDumpNew.py
19 ↗(On Diff #73296)

Aah I understand now. Ok thanks :)

dvlahovski updated this revision to Diff 73504.Oct 4 2016, 10:14 AM
dvlahovski edited edge metadata.
dvlahovski marked 7 inline comments as done.

Updated the CL with regard to Pavel's comments

Just a couple more details and I think we're ready.

source/Plugins/Process/minidump/MinidumpParser.cpp
105 ↗(On Diff #73504)

I think you have made it over-encapsulated now. :)
Either a Parser function which takes a thread or a Thread function which takes a parser as argument should be enough. Is there a reason you need both?

234 ↗(On Diff #73504)

Not necessary.

255 ↗(On Diff #73504)

return Range(...) is much cleaner and shorter.

Add an appropriate constructor if it is needed to make this work.

source/Plugins/Process/minidump/MinidumpTypes.cpp
94 ↗(On Diff #73504)

This feels very unsafe. Is there anything guaranteeing that thread_context.rva does not point beyond the end of the file?

source/Plugins/Process/minidump/ProcessMinidump.cpp
69 ↗(On Diff #73504)

This could also be an early-return.

dvlahovski updated this revision to Diff 73516.Oct 4 2016, 11:35 AM
dvlahovski marked 5 inline comments as done.
dvlahovski edited edge metadata.

Second iteration over CL - regarded Pavel's comments and encapsulated m_data_sp more in MinidumpParser

zturner added inline comments.Oct 4 2016, 11:45 AM
source/Plugins/Process/minidump/MinidumpParser.cpp
81–82 ↗(On Diff #73516)

Change this line to return GetData().slice(iter->second.rva, iter->second.data_size);

106–107 ↗(On Diff #73516)

Same as above, use slice()

255 ↗(On Diff #73516)

slice()

source/Plugins/Process/minidump/MinidumpParser.h
35–36 ↗(On Diff #73516)

If the comment is long enough to wrap, maybe better to just put it on the line before. Looks awkward this way.

39–40 ↗(On Diff #73516)

You don't need the underscores here. It might look awkward, but the usual LLVM pattern is to just call the constructor parameters and member variables the same name.

source/Plugins/Process/minidump/MinidumpTypes.cpp
188–190 ↗(On Diff #73516)

you can write return llvm::makeArrayRef(reinterpret_cast<const MinidumpMemoryDescriptor*>(data.data()), *mem_ranges_count)); to avoid specifying the type name twice. It's a little shorter (admittedly not much though).

dvlahovski updated this revision to Diff 73526.Oct 4 2016, 12:06 PM
dvlahovski marked 6 inline comments as done.

Changes after Zachary's comments

dvlahovski marked an inline comment as done.Oct 4 2016, 12:08 PM
dvlahovski added inline comments.
source/Plugins/Process/minidump/MinidumpParser.cpp
81–82 ↗(On Diff #73516)

Nice! :)

source/Plugins/Process/minidump/MinidumpParser.h
35–36 ↗(On Diff #73516)

This comment is for the ArrayrRef. Added a clarification

source/Plugins/Process/minidump/MinidumpTypes.cpp
188–190 ↗(On Diff #73516)

I think it looks better with the makeArrayrRef :)

Thanks for fixing all the comments. Unfortunately, on my last pass, I found one more case of unverified input (I think).

source/Plugins/Process/minidump/MinidumpParser.cpp
252 ↗(On Diff #73526)

Is anything verifying that loc_desc points into the file ?

source/Plugins/Process/minidump/ProcessMinidump.cpp
187 ↗(On Diff #73526)

typo: "in our"

dvlahovski updated this revision to Diff 73541.Oct 4 2016, 1:00 PM
dvlahovski marked 2 inline comments as done.

Added a sanity check for loc_descr

labath accepted this revision.Oct 4 2016, 1:57 PM
labath edited edge metadata.

lgtm

This revision is now accepted and ready to land.Oct 4 2016, 1:57 PM
zturner accepted this revision.Oct 4 2016, 1:59 PM
zturner edited edge metadata.
This revision was automatically updated to reflect the committed changes.