Page MenuHomePhabricator

Minidump plugin: Adding ProcessMinidump, ThreadMinidump and register the plugin in SystemInitializerFull

Authored by dvlahovski on Oct 24 2016, 5:57 AM.



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 arches that this supports are x86_32 and x86_64.
This is because I have only written register contexts for those.
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


Event Timeline

dvlahovski updated this revision to Diff 75571.Oct 24 2016, 5:57 AM
dvlahovski retitled this revision from to Minidump plugin: Adding ProcessMinidump, ThreadMinidump and register the plugin in SystemInitializerFull.
dvlahovski updated this object.
dvlahovski added reviewers: labath, zturner.
dvlahovski added subscribers: amccarth, lldb-commits.
labath edited edge metadata.Oct 24 2016, 10:03 AM

first round of comments. I will give this another look tomorrow.

7 ↗(On Diff #75571)

Please format these consistently (llvm-style). clang-format will not work by default as we are still ignoring the test folder.

18 ↗(On Diff #75571)

Please put this inside the lldb_private::minidump namespace.

73 ↗(On Diff #75571)

return std::make_shared<ProcesMinidump>(...

274 ↗(On Diff #75571)


278 ↗(On Diff #75571)

There is a hidden assumption here that ReadModuleList() will be called before any other function that depends on the value of m_is_wow64. (I am not even sure if this is true in case of GetArchitecture().) I think we should make this more robust. Maybe you could initialize this (along with the filtered_modules list, I guess) in the constructor? It's not really doing extra work, as you're going to need to parse the module list at some very early point anyway.

291 ↗(On Diff #75571)

Did clang-format break this in such an ugly way? What will happen if you use raw strings instead: R"(D:\no\double\back\slash)" ?

dvlahovski marked 6 inline comments as done.
dvlahovski edited edge metadata.

Addressed Pavel's comments.

Please ignore the RegisterContextMinidump_x86_* changes. Can't seem to make arc diff understand what I want.

Just a bit more cleanup and this will be fine. Sorry about the back and forth, I was in a hurry yesterday.

Zachary, Adrian: any thoughts on your side?

7 ↗(On Diff #75571)

Still not correct (4 char indent, inconsistent braces). It might be easiest to just temporarily remove packages/Python/lldbsuite/.clang-format. We'll need to figure out a better solution for formatting new test code in the long run though.

1 ↗(On Diff #75617)

Please add a short blurb explaining how does this build work and why did we choose such a complicated way of generating the minidumps.

67 ↗(On Diff #75617)

Zach, Adrian: IIUC, the new plugin should generally have feature parity with the windows-only plugin. (Dimitar: could you say exactly what bits are missing?). You should be able to test out this plugin on windows minidumps by removing the windows check below.

After this goes in, we'll be looking to remove the windows-only plugin.

89 ↗(On Diff #75617)

Sorry about the back-and-forth, but I think we should put this back to LoadCore(). I did not realize that ReadModuleList was a private function and not a public interface of the class. :(

In this case we can assume that anyone will call LoadCore before expecting any other values to be valid, and we shouldn't be doing any parsing (especially not if it calls target->AddModule and such) as that is not the pattern anywhere else.

218 ↗(On Diff #75617)

return ArchSpec(triple);

dvlahovski updated this revision to Diff 75709.Oct 25 2016, 8:17 AM
dvlahovski marked 4 inline comments as done.

Formatting correctly the test files source code, and added explanation in their makefile
ReadModuleList() is called in DoLoadCore()

dvlahovski added inline comments.Oct 25 2016, 8:19 AM
67 ↗(On Diff #75617)

Yes, I think that now my plugin has full feature parity with the windows-only one.

dvlahovski updated this revision to Diff 76176.Oct 28 2016, 3:44 AM

Hopefully removing code that's not for this CL

amccarth accepted this revision.Oct 28 2016, 11:37 AM
amccarth added a reviewer: amccarth.

I like that this keeps the WoW64 detection and support. That's a very Windows-specific thing, and I was concerned that doing generic minidump parsing could lose this.

This looks really good. On Monday or Tuesday, I'll try patching it into a Windows build to see if it's complete enough to remove the Windows-specific implementation.

16 ↗(On Diff #76176)


17 ↗(On Diff #76176)


273 ↗(On Diff #76176)

This is probably fine, but it's a slightly less precise check than what we're doing in ProcessWinMiniDump.cpp. The endswith check would accept "bowwow64.dll". It's probably not important, since detecting WoW64 is by looking at module names is a hack anyway.

This revision is now accepted and ready to land.Oct 28 2016, 11:37 AM
This revision was automatically updated to reflect the committed changes.