Page MenuHomePhabricator

MinidumpYAML: Add support for the memory info list stream
ClosedPublic

Authored by labath on Oct 8 2019, 7:40 AM.

Details

Summary

The implementation is fairly straight-forward and uses the same patterns
as the existing streams. The yaml form does not attempt to preserve the
data in the "gaps" that can be created by setting a larger-than-required
header or entry size in the stream header, because the existing consumer
(lldb) does not make use of the information in the gap in any way, and
attempting to preserve that would make the implementation more
complicated.

Diff Detail

Event Timeline

labath created this revision.Oct 8 2019, 7:40 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 8 2019, 7:40 AM

Nit: Title says "thread" rather than "memory info"

labath retitled this revision from MinidumpYAML: Add support for the thread list stream to MinidumpYAML: Add support for the memory info list stream.Oct 8 2019, 8:00 AM

Nit: Title says "thread" rather than "memory info"

Woops, sorry about that. Should be fixed now.

FYI, I'm going to be away for 2 and a half weeks from the end of work today, so won't have time to look at these if I don't get to them later today. I have no issues with other people reviewing them. You might want to add @grimar and @MaskRay to the reviews as they've been doing a lot of work in obj2yaml/yaml2obj recently.

grimar added inline comments.Oct 9 2019, 7:47 AM
include/llvm/ObjectYAML/MinidumpYAML.h
111 ↗(On Diff #223862)

Maybe be more explicit here, i.e.

std::vector<minidump::MemoryInfo> &&Infos

?

lib/ObjectYAML/MinidumpEmitter.cpp
166 ↗(On Diff #223862)

Probably just

minidump::MemoryInfoListHeader Header = {
    (support::ulittle32_t)sizeof(minidump::MemoryInfoListHeader),
    (support::ulittle32_t)sizeof(minidump::MemoryInfo),
    (support::ulittle64_t)InfoList.Infos.size()};

?

Or perhaps it could have a constructor.

labath updated this revision to Diff 224065.Oct 9 2019, 8:35 AM
labath marked 2 inline comments as done.

Address review comments

lib/ObjectYAML/MinidumpEmitter.cpp
166 ↗(On Diff #223862)

The thought of a constructor has crossed my mind too. I've now added it.

grimar accepted this revision.Oct 9 2019, 9:58 AM

LGTM

This revision is now accepted and ready to land.Oct 9 2019, 9:58 AM
MaskRay accepted this revision.Oct 10 2019, 4:13 AM
MaskRay added inline comments.
include/llvm/ObjectYAML/MinidumpYAML.h
116 ↗(On Diff #224065)

Move default constructor above.

111 ↗(On Diff #223862)

Or let the constructor take iterator_range<MinidumpFile::MemoryInfoIterator> as the argument, and change the call site below.

labath marked 3 inline comments as done.Oct 10 2019, 6:03 AM
labath added inline comments.
include/llvm/ObjectYAML/MinidumpYAML.h
116 ↗(On Diff #224065)

Done, I've also moved the constructor of SystemInfoStream for consistency.

This revision was automatically updated to reflect the committed changes.
labath marked an inline comment as done.