Page MenuHomePhabricator

minidump: Create memory regions from the sections of loaded modules
ClosedPublic

Authored by labath on Oct 17 2019, 5:21 AM.

Details

Summary

Not all minidumps contain information about memory permissions. However,
it is still important to know which regions of memory contain
potentially executable code. This is particularly important for
unwinding on win32, as the default unwind method there relies on
scanning the stack for things which "look like" code pointers.

This patch enables ProcessMinidump to reconstruct the likely permissions
of memory regions using the sections of loaded object files. It only
does this if we don't have a better source (memory info list stream, or
linux /proc/maps) for this information, and only if the information in
the object files does not conflict with the information in the minidump.

Theoretically that last bit could be improved, since the permissions
obtained from the MemoryList streams is also only a very rough guess,
but it did not seem worthwhile to complicate the implementation because
of that because there will generally be no overlap in practice as the
MemoryList will contain the stack contents and not any module data.

The patch adds a test checking that the module section permissions are
entered into the memory region list, and also a test which demonstrate
that now the unwinder is able to correctly find return addresses even in
minidumps without memory info list streams.

There's one TODO left in this patch, which is that the "memory region"
output does not give any indication about the "don't know" values of
memory region permissions (it just prints them as if they permission bit
was set). I address this in a follow up.

Diff Detail

Event Timeline

labath created this revision.Oct 17 2019, 5:21 AM
clayborg added inline comments.Oct 17 2019, 8:48 AM
source/Plugins/Process/minidump/ProcessMinidump.cpp
348–349 ↗(On Diff #225409)

Might be nice to just assign memory regions and then ask the minidump parser what its source of memory regions was? I am thinking of:

m_memory_regions = m_minidump_parser->BuildMemoryRegions();
switch (m_minidump_parser->GetMemoryRegionsSource()) {
  case MemoryInfoList:
  case LinuxProcMaps:
    break;
  case MemoryList:
    // Memory list is not always a exhaustive list of memory regions in a process...
    // Insert code below the "if (is_complete)" in switch?

Actually thinking of this a bit more, we should really just do this work inside "m_minidump_parser->BuildMemoryRegions();". The minidump itself can then make the call and we don't need to return the "is_complete". Otherwise if anyone else uses the info from "m_minidump_parser->BuildMemoryRegions();" they would have to do the same thing as this code which would be bad.

labath marked an inline comment as done.Oct 18 2019, 4:30 AM
labath added inline comments.
source/Plugins/Process/minidump/ProcessMinidump.cpp
348–349 ↗(On Diff #225409)

I don't think that doing this work in the MinidumpParser is right, as the main reason for it's existence was to isolate minidump parsing from the stuff happening elsewhere in lldb (to enable unit testing and potentially be able to reuse it elsewhere). Now that a lot of the parsing has moved to llvm the second reason probably doesn't apply, but the unit testing separation is still nice to have. Even if we ignore that (and say have unit tests pass in an empty ModuleList), it still doesn't seem fully right because it's ProcessMinidump who creates the Module objects (MinidumpParser only works with minidump "modules"), so it seems natural to me that it should be the one who handles their regionizing too.

clayborg accepted this revision.Oct 18 2019, 9:56 AM
This revision is now accepted and ready to land.Oct 18 2019, 9:56 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptOct 31 2019, 3:35 AM