This is an archive of the discontinued LLVM Phabricator instance.

Create a Windows mini-dump target
ClosedPublic

Authored by amccarth on Jul 29 2015, 3:15 PM.

Details

Reviewers
zturner
Summary

This is the first (of many) steps to add support for Windows mini dumps as targets.

Like loading an ELF core file, this patch allows lldb to open a mini dump with a command like this:

(lldb) target create -c foo.dmp

All you can do now is see the thread list. I'm planning to add functionality in bite-sized chunks.

I'm still thinking about how to write tests for this functionality. I haven't found any ELF core tests to pattern after. If you have suggestions, let me know.

Diff Detail

Event Timeline

amccarth updated this revision to Diff 30957.Jul 29 2015, 3:15 PM
amccarth retitled this revision from to Create a Windows mini-dump target.
amccarth updated this object.
amccarth added a reviewer: zturner.
amccarth added a subscriber: lldb-commits.
zturner edited edge metadata.Jul 29 2015, 3:18 PM

Will review in more detail later, but to answer your question about test. A good first start would be to create a minidump of a test program which is stopped at the first line of main. Check in the minidump as well as the test program. Then just write a test that verifies the stack frame is correct.

zturner added inline comments.Aug 3 2015, 1:56 PM
cmake/LLDBDependencies.cmake
69

This needs to be removed. The reason you see ElfCore and other MacOSX ones is because they don't use any Posix-specific APIs, but instead just read/write the core file directly and parse it. This allows you to debug an ELF core on Windows, for example. We use Windows Minidump API, so we can't allow someone not on Windows to try and link it, which this will cause to happen. If someone ever decides to write a minidump plugin that works on non-Windows, we could add that plugin here, and leave the existing one under the windows-only check

source/API/SystemInitializerFull.cpp
41

All the changes to this file need to be inside of #if defined(_MSC_VER) checks

source/Plugins/Process/win-minidump/CMakeLists.txt
3

Some CMake files still have this line, but I think it's unnecessary. Feel free to remove

source/Plugins/Process/win-minidump/Makefile
1

I think you can delete this file, nobody on Windows is using the autoconf build, and I don't care to support it since the autoconf build is going away anyway.

source/Plugins/Process/win-minidump/ProcessWinMiniDump.cpp
237–238

We already have a DetermineArchitecture() function. Is there any reason we can't just return it right here? Does this function have slightly different semantics or something?

source/Plugins/Process/win-minidump/ProcessWinMiniDump.h
49

Need to reformat these declarations to put return value on separate line.

86

I'd rather remove the TODO and either put it in Data or leave it here. Either way, be decisive :)

amccarth marked 6 inline comments as done.Aug 3 2015, 3:42 PM

By the way, I also made the capitalization of WinMiniDump/WinMinidump consistent, going with the former because it's consistent with more Windows APIs (even though the latter is probably more "natural").

source/Plugins/Process/win-minidump/ProcessWinMiniDump.cpp
237–238

DetermineArchitecture can be called only after the core file has been opened. In practice, it seems the GetArchitecture is called at least once before that happens. In those cases, GetArchitecture returns a default that seems mostly right.

The ELF core handling works similarly, providing a default until the actual value is determined from the core file.

source/Plugins/Process/win-minidump/ProcessWinMiniDump.h
86

Sorry, I meant to take care of that before sending it out for review, but I searched for TODO(amccarth) instead of just TODO. :-(

Moved to the private ProcessWinMap::Data class.

amccarth updated this revision to Diff 31280.Aug 3 2015, 3:43 PM
amccarth edited edge metadata.
amccarth marked an inline comment as done.

Addresses earlier comments.

zturner accepted this revision.Aug 3 2015, 3:57 PM
zturner edited edge metadata.
This revision is now accepted and ready to land.Aug 3 2015, 3:57 PM
amccarth closed this revision.Aug 3 2015, 4:33 PM