Page MenuHomePhabricator

Improve LLDB's handling of non-local minidumps
ClosedPublic

Authored by lemo on Apr 16 2018, 2:15 PM.

Details

Summary

Normally, LLDB is creating a high-fidelity representation of a live
process, including a list of modules and sections, with the
associated memory address ranges. In order to build the module and
section map LLDB tries to locate the local module image (object file)
and will parse it.

This does not work for postmortem debugging scenarios where the crash
dump (minidump in this case) was captured on a different machine.

Fortunately the minidump format encodes enough information about
each module's memory range to allow us to create placeholder modules.
This enables most LLDB functionality involving address-to-module
translations.

Also, we may want to completly disable the search for matching
local object files if we load minidumps unless we can prove that the
local image matches the one from the crash origin.
(not part of this change, see: llvm.org/pr35193)

Example: Identify the module from a stack frame PC:

Before:
thread #1, stop reason = Exception 0xc0000005 encountered at address 0x164d14

frame #0: 0x00164d14
frame #1: 0x00167c79
frame #2: 0x00167e6d
frame #3: 0x7510336a
frame #4: 0x77759882
frame #5: 0x77759855

After:
thread #1, stop reason = Exception 0xc0000005 encountered at address 0x164d14

frame #0: 0x00164d14 C:\Users\amccarth\Documents\Visual Studio 2013\Projects\fizzbuzz\Debug\fizzbuzz.exe
frame #1: 0x00167c79 C:\Users\amccarth\Documents\Visual Studio 2013\Projects\fizzbuzz\Debug\fizzbuzz.exe
frame #2: 0x00167e6d C:\Users\amccarth\Documents\Visual Studio 2013\Projects\fizzbuzz\Debug\fizzbuzz.exe
frame #3: 0x7510336a C:\Windows\SysWOW64\kernel32.dll
frame #4: 0x77759882 C:\Windows\SysWOW64\ntdll.dll
frame #5: 0x77759855 C:\Windows\SysWOW64\ntdll.dll

Example: target modules list

Before:
error: the target has no associated executable images

After:
[ 0] C:\Windows\System32\MSVCP120D.dll
[ 1] C:\Windows\SysWOW64\kernel32.dll
[ 2] C:\Users\amccarth\Documents\Visual Studio 2013\Projects\fizzbuzz\Debug\fizzbuzz.exe
[ 3] C:\Windows\System32\MSVCR120D.dll
[ 4] C:\Windows\SysWOW64\KERNELBASE.dll
[ 5] C:\Windows\SysWOW64\ntdll.dll

NOTE: the minidump format also includes the debug info GUID, so we can fill-in the module UUID from it, but this part was excluded from this change to keep the changes simple (the LLDB UUID is hardcoded to be either 16 or 20 bytes, while the CodeView GUIDs are normally 24 bytes)

Diff Detail

Repository
rL LLVM

Event Timeline

lemo created this revision.Apr 16 2018, 2:15 PM
lemo edited the summary of this revision. (Show Details)Apr 16 2018, 2:17 PM

Nice!

source/Plugins/Process/minidump/ProcessMinidump.cpp
53 ↗(On Diff #142694)

I wonder if this should just be part of the constructor. I don't see a scenario where you'd create a PlaceholderModule and not want to create exactly one fake image section. I know the style guide is against doing lots of work in a constructor, but that's mostly because it's hard to report failures, which you're not doing here anyway.

329 ↗(On Diff #142694)

nit: s/Fortunatelly/Fortunately/

339 ↗(On Diff #142694)

If CreateImageSection bit were done by the PlaceholderModule's constructor, then there'd be no need for the artificial my_module temporary, and nobody would ever make a PlaceholderModule without an image section.

lemo added inline comments.Apr 16 2018, 3:05 PM
source/Plugins/Process/minidump/ProcessMinidump.cpp
53 ↗(On Diff #142694)

Thanks for the suggestion. I agree, this would look a bit cleaner to fold everything in the constructor (except the extra arguments to the constructor, but it will probably still be a net win)

The reason for a separate method is the little "shared_from_this()". It can't be done from the constructor since the object is not yet managed by a shared_ptr, so we need to do it post-construction.

lemo updated this revision to Diff 142706.Apr 16 2018, 3:13 PM
lemo edited the summary of this revision. (Show Details)

Looks good. I questions if we want PlaceholderModule to be available for all symbolicators/core dump plugins. See inlined comments.

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

I would be worth putting this class maybe in the same folder as lldb_private::Module and possibly renaming it. I can see this kind of thing being useful for symbolication in general and it won't be limited to use in minidumps. It should have enough accessors that allows an external client to modify everything.

79 ↗(On Diff #142706)

Just use lldb_private::Module::m_sections_ap and put the sections in there? Any reason not to?

The part I am not sure about here is that you are creating a Module which has no associated object file, but it still has some sections. That's not how any of our current modules/object files work, and I worry that this may cause problems down the line (and plainly put, having sections without an object file feels weird). I am wondering whether it wouldn't be better to go all the way and create a "PlaceholderObjectFile" as well (or instead of PlaceholderModule).

I don't know what the right answer here is, but it is something to think about...

packages/Python/lldbsuite/test/functionalities/postmortem/minidump/TestMiniDump.py
53 ↗(On Diff #142706)

Could we strengthen these assertions a bit. Given that this is static data you are loading, I don't see why you couldn't hard-code the names of the modules you should expect.

source/Plugins/Process/minidump/ProcessMinidump.cpp
53 ↗(On Diff #142694)

This can be solved by hiding the constructor and having a static factory function which returns a shared_ptr.

47 ↗(On Diff #142706)

I concur. Besides postmortem, we can run into the situation where we cannot access the loaded modules for live debugging as well.

lemo updated this revision to Diff 142799.Apr 17 2018, 10:43 AM
lemo edited the summary of this revision. (Show Details)

Thanks for the feedback.

PS. The sample output in the description if from LLDB running on Linux, reading minidumps captured on Windows.

lemo marked 4 inline comments as done.Apr 17 2018, 10:55 AM
lemo added inline comments.
packages/Python/lldbsuite/test/functionalities/postmortem/minidump/TestMiniDump.py
53 ↗(On Diff #142706)

Done.

Just curious, do we have any support for golden output files? (it would be great for tests like this)

source/Plugins/Process/minidump/ProcessMinidump.cpp
53 ↗(On Diff #142694)

The factory is a great idea, done. (one downside with a private constructor is that we lose the benefits of std::make_shared though)

47 ↗(On Diff #142706)

Thanks, I agree. I think this needs a bit more baking first though, I say let's put it through some use first (and maybe identify a 2nd use case).
(in particular we'd also need a more generic interface and I'd like to avoid creating an overly complex solution which may not even fit future use cases)

79 ↗(On Diff #142706)

Thanks, done. (no good reason, I was just extra cautious not to introduce any unexpected side-effects)

aprantl removed a subscriber: aprantl.Apr 17 2018, 11:00 AM
labath added inline comments.Apr 17 2018, 11:40 AM
packages/Python/lldbsuite/test/functionalities/postmortem/minidump/TestMiniDump.py
53 ↗(On Diff #142706)

If you're feeling adventurous, you can try rewriting this as a lit test. You should be able to just do a lldb -c my-core -o "image list" and FileCheck the output.

lemo marked 2 inline comments as done.Apr 17 2018, 12:06 PM
lemo added inline comments.
packages/Python/lldbsuite/test/functionalities/postmortem/minidump/TestMiniDump.py
53 ↗(On Diff #142706)

Thanks for the tip, I'll keep this in mind for the future.

amccarth accepted this revision.Apr 17 2018, 4:48 PM

LGTM, but consider highlighting the side effect to target when the factory makes a Placeholder module.

In the future, we need to refactor the minidump tests to get rid of the redundancy.

source/Plugins/Process/minidump/ProcessMinidump.cpp
70 ↗(On Diff #142799)

I didn't notice before that target is a non-const ref. Maybe the comment should explain why target is modified (and/or maybe in PlaceholderModule::Create).

This revision is now accepted and ready to land.Apr 17 2018, 4:48 PM
lemo updated this revision to Diff 142960.Apr 18 2018, 10:26 AM
lemo marked an inline comment as done.Apr 18 2018, 10:31 AM

LGTM, but consider highlighting the side effect to target when the factory makes a Placeholder module.

Good observation: taking a step back, the factory introduces too much coupling, especially if we want to extend this placeholder module approach to other uses. Because of this, I reverted back to the standalone PlaceholderModule::CreateImageSection() approach. Thanks Adrian!

source/Plugins/Process/minidump/ProcessMinidump.cpp
70 ↗(On Diff #142799)

Updated the function comment. This is similar to how other places set the section load address (ex. ObjectFileELF::SetLoadAddress)

lemo marked an inline comment as done.Apr 18 2018, 1:10 PM
lemo added subscribers: amccarth, clayborg, labath, zturner.

Greg/Pavel, does the latest revision look good to you? Thanks!

amccarth accepted this revision.Apr 18 2018, 1:51 PM

I actually preferred the factory solution.

What I intended to express is that the modification of the target doesn't seems like it should be inside the PlaceholderModule class, so whether you use a factory or not doesn't really address that aspect of the coupling. In my head, modification of the target should be done by the client that instantiates the PlaceholderModule (whether it does that via constructor or factory).

But this isn't a new problem, so I'm happy to consider the coupling problem outside the scope of this patch.

This revision was automatically updated to reflect the committed changes.

It looks like nobody except me is worried about the module-without-an-object-file situation, so I guess we can try this out and see how it goes.

The test you've added here has been failing on windows though. I've tried to fix this in r330314, but it meant modifying the module.file.fullpath output expectations. I'm not sure where you're going with the minidump support, but if you are bothered by module.file.fullpath containing a forward slash, you may want to look into fixing the SBFileSpec behavior.

It looks like nobody except me is worried about the
module-without-an-object-file situation, so I guess we can try this out and
see how it goes.

Sorry Pavel, I meant to respond to this: most of the code seems to
explicitly handle this case (module-without-object-file), I just had to fix
a couple of cases. It's possible that more fixes will be required, but the
intention seems to be to accommodate for missing object files so at least
in this area I didn't have to break new ground.

I also considered creating placeholder object files, but it proved a bit
more intrusive since there are numerous places where it's assumed that
object files map to a real file which can be read and written to. Maybe at
some point we'll need to reconsider this (placeholder object files) but for
the initial iteration the placeholder modules seems to be sufficient. The
only downside I noticed is mostly cosmetic, for example things like "target
modules dump objfile" may print empty lines for the missing object files.

The test you've added here has been failing on windows though. I've tried

to fix this in r330314, but it meant modifying the module.file.fullpath
output expectations. I'm not sure where you're going with the minidump
support, but if you are bothered by module.file.fullpath containing a
forward slash, you may want to look into fixing the SBFileSpec behavior.

Thanks!