This is an archive of the discontinued LLVM Phabricator instance.

minidump: Refactor memory region computation code
ClosedPublic

Authored by labath on Oct 16 2019, 6:49 AM.

Details

Summary

The goal of this refactor is to enable ProcessMinidump to take into
account the loaded modules and their sections when computing the
permissions of various ranges of memory, as discussed in D66638.

This patch moves some of the responsibility for computing the ranges
from MinidumpParser into ProcessMinidump. MinidumpParser still does the
parsing, but ProcessMinidump becomes responsible for answering the
actual queries about memory ranges. This will enable it (in a follow-up
patch) to augment the information obtained from the parser with data
obtained from actual object files.

The changes in the actual code are fairly straight-forward and just
involve moving code around. MinidumpParser::GetMemoryRegions is renamed
to BuildMemoryRegions to emphasize that it does no caching. The only new
thing is the additional bool flag returned from this function. This
indicates whether the returned regions describe all memory mapped into
the target process. Data obtained from /proc/maps and the MemoryInfoList
stream is considered to be exhaustive. Data obtained from Memory(64)List
is not. This will be used to determine whether we need to augment the
data or not.

This reshuffle means that it is no longer possible/easy to test some of
this code via unit tests, as constructing a ProcessMinidump instance is
hard. Instead, I update the unit tests to only test the parsing of the
actual data, and test the answering of queries through a lit test using
the "memory region" command. The patch also includes some tweaks to the
MemoryRegion class to make the unit tests easier to write.

Event Timeline

labath created this revision.Oct 16 2019, 6:49 AM
labath planned changes to this revision.Oct 16 2019, 7:36 AM

Actually, it looks like the "follow-up" patch will be more complicated than I had hoped for, and so I am starting to wonder whether we shouldn't just have the unwinder check for section permissions himself (which would be a one-liner). I'll need to investigate more, so you may want to hold off reviewing this.

labath requested review of this revision.Oct 17 2019, 5:05 AM

With some additional COFF tweaks (D69100, D69102), the next patch doesn't turn out that bad (though I'm still happy to accept "unwinder should just check section permissions instead of us translating them to memory regions" as an answer).

This revision was not accepted when it landed; it landed in state Needs Review.Oct 25 2019, 3:34 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptOct 25 2019, 3:34 PM