Page MenuHomePhabricator

Minidump: Add support for the MemoryList stream
ClosedPublic

Authored by labath on May 14 2019, 2:41 AM.

Details

Summary

the stream format is exactly the same as for ThreadList and ModuleList
streams, only the entry types are slightly different, so the changes in
this patch are just straight-forward applications of established
patterns.

Diff Detail

Repository
rL LLVM

Event Timeline

labath created this revision.May 14 2019, 2:41 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 14 2019, 2:41 AM
jhenderson added inline comments.May 14 2019, 8:29 AM
include/llvm/Object/Minidump.h
85–86 ↗(On Diff #199384)

These two lines talk about threads. Is that a copy/paste error?

include/llvm/ObjectYAML/MinidumpYAML.h
92 ↗(On Diff #199384)

Would it make more sense to call this ParsedMermoryList, to match the StreamType?

test/tools/obj2yaml/basic-minidump.yaml
54–57 ↗(On Diff #199384)

I'd probably find this neater if the Indentation of values for each entry were more consistent, but I'm not too fussed.

Also, in the ThreadList above, the Content is not quoted, but here it is. Please standardise it on one or the other.

unittests/Object/MinidumpTest.cpp
483 ↗(On Diff #199384)

thread? Copy/paste error?

labath updated this revision to Diff 199570.May 15 2019, 2:50 AM
labath marked 6 inline comments as done.

Address review comments.

include/llvm/Object/Minidump.h
85–86 ↗(On Diff #199384)

Yep, sorry. This is the downside of "straight-forward applications of established
patterns" :/

include/llvm/ObjectYAML/MinidumpYAML.h
92 ↗(On Diff #199384)

Not really, because this represents only one entry in the MemoryList stream, and not list as a whole. However, we could call it ParsedMemoryDescriptor, as that's the type of the list entries.

test/tools/obj2yaml/basic-minidump.yaml
54–57 ↗(On Diff #199384)

Done. The different quoting of is actually a relict of how obj2yaml prints BinaryRef values (they omit quotes if the data happens to be contain hex (A-F) characters). Do you think it would be worth making this output more consistent too?

jhenderson accepted this revision.May 15 2019, 3:15 AM

LGTM, though I haven't bothered to inspect the detail of the minidump format. I'll leave it up to you whether you think it's worth getting a separate pair of eyes.

test/tools/obj2yaml/basic-minidump.yaml
54–57 ↗(On Diff #199384)

Probably, but that sounds like an unrelated changes.

This revision is now accepted and ready to land.May 15 2019, 3:15 AM

Thanks. This is very straight-forward, so I don't think another set of eyes is needed (though I expect Greg is keeping at least one eye on this).

This revision was automatically updated to reflect the committed changes.