This is an archive of the discontinued LLVM Phabricator instance.

Object/minidump: Add support for the MemoryInfoList stream
ClosedPublic

Authored by labath on Sep 30 2019, 5:39 AM.

Details

Summary

This patch adds the definitions of the constants and structures
necessary to interpret the MemoryInfoList minidump stream, as well as
the object::MinidumpFile interface to access the stream.

While the code is fairly simple, there is one important deviation from
the other minidump streams, which is worth calling out explicitly.
Unlike other "List" streams, the size of the records inside
MemoryInfoList stream is not known statically. Instead it is described
in the stream header. This makes it impossible to return
ArrayRef<MemoryInfo> from the accessor method, as it is done with other
streams. Instead, I create an iterator class, which can be parameterized
by the runtime size of the structure, and return
iterator_range<iterator> instead.

Diff Detail

Event Timeline

labath created this revision.Sep 30 2019, 5:39 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 30 2019, 5:39 AM

Can't comment too much on the file format details, but I've made some more general comments.

FYI, I'll be away from end of day Wednesday for 2 and a half weeks, so won't be able to further review after that point until I'm back.

include/llvm/BinaryFormat/Minidump.h
81 ↗(On Diff #222395)

I believe if you format this line as:

LLVM_MARK_AS_BITMASK_ENUM(/*LargestValue=*/0xffffffffu),

clang-format will leave it unedited. I believe it has special rules for /*<Name>=*/<value> to label parameters.

lib/Object/Minidump.cpp
58 ↗(On Diff #222395)

I probably should have picked up on this in previous reviews, but this is too much auto for my liking, as it's not obvious from the call site what getRawStream returns.

66 ↗(On Diff #222395)

Ditto.

unittests/Object/MinidumpTest.cpp
617–618 ↗(On Diff #222395)

I might make the data here be of size 15 to test the edge case.

It's probably also worth a test case where the header size as specified by SizeOfHeader fits in the data but is smaller than the expected value.

634 ↗(On Diff #222395)

I might go for a value of 49 to test the edge value here.

labath updated this revision to Diff 223616.Oct 7 2019, 8:41 AM
labath marked 6 inline comments as done.

Address review comments

labath added a comment.Oct 7 2019, 8:46 AM

Thanks for the review. I am fairly confident in the minidump details, as I based this code on the existing functional implementation in lldb, which I have also cross-referenced with the publicly available microsoft documentation. @amccarth, @clayborg: do you want to have a look at the minidump details?

lib/Object/Minidump.cpp
58 ↗(On Diff #222395)

Done. I've also changed the other calls to getRawStream.

jhenderson added inline comments.Oct 8 2019, 1:53 AM
lib/Object/Minidump.cpp
58 ↗(On Diff #222395)

Thanks!

unittests/Object/MinidumpTest.cpp
620 ↗(On Diff #223616)

Here and in the similar places, I'm not convinced that cantFail is appropriate (if the creation code is broken, this will assert and therefore possibly hide the actual testing failures that show where it went wrong more precisely). It should probably be a two phase thing:

Expected<std::unique_ptr<MinidumpFile>> Minidump = HeaderTooBig);
ASSERT_THAT_EXPECTED(Minidump, Succeeded());
EXPECTE_THAT_EXPECTED(Minidump->getMemoryInfoList(), Failed<BinaryError>());
624 ↗(On Diff #223616)

Nit: delete the ')'

labath updated this revision to Diff 223811.Oct 8 2019, 2:50 AM

Address review comments

labath added a comment.Oct 8 2019, 2:52 AM

(ignore latest diff. I just realized it's totally bogus)

labath updated this revision to Diff 223816.Oct 8 2019, 3:47 AM

use two phase checks

labath marked 2 inline comments as done.Oct 8 2019, 3:52 AM
labath added inline comments.
unittests/Object/MinidumpTest.cpp
620 ↗(On Diff #223616)

Done. The previous solution of just passing the creation error in the helper function was totally bogus, of course. Ideally, it would be possible to express this as a one-liner using gtest matchers (HasValue(Property(&getMemoryInfoList, Failed)), but unfortunately they are quite incompatible with the move-only Expected semantices.

jhenderson accepted this revision.Oct 8 2019, 3:59 AM

Thanks, LGTM (modulo the Minidump-specific details).

This revision is now accepted and ready to land.Oct 8 2019, 3:59 AM
This revision was automatically updated to reflect the committed changes.
labath marked an inline comment as done.