Page MenuHomePhabricator

MinidumpYAML: Add support for ModuleList stream
ClosedPublic

Authored by labath on Apr 8 2019, 6:15 AM.

Details

Summary

This patch adds support for yaml (de)serialization of the minidump
ModuleList stream. It's a fairly straight forward-application of the
existing patterns to the ModuleList structures defined in previous
patches.

One thing, which may be interesting to call out explicitly is the
addition of "new" allocation functions to the helper BlobAllocator
class. The reason for this was, that there was an emerging pattern of a
need to allocate space for entities, which do not have a suitable
lifetime for use with the existing allocation functions. A typical
example of that was the "size" of various lists, which is only available
as a temporary returned by the .size() method of some container. For
these cases, one can use the new set of allocation functions, which
will take a temporary object, and store it in an allocator-managed
buffer until it is written to disk.

Diff Detail

Repository
rL LLVM

Event Timeline

labath created this revision.Apr 8 2019, 6:15 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 8 2019, 6:15 AM

LGTM. Probably should get one more ok

lib/ObjectYAML/MinidumpYAML.cpp
91 ↗(On Diff #194130)

Might be nice to unique the strings in here?

labath marked an inline comment as done.Apr 9 2019, 12:56 AM
labath added inline comments.
lib/ObjectYAML/MinidumpYAML.cpp
91 ↗(On Diff #194130)

Yeah, that would be possible, but it does not seem worth the trouble at the moment. However, I do have some ideas about reusing this code for saving minidump snapshots from within lldb (right now that is only supported on windows, by delegating to some windows APIs). At that point it may become more important to generate more compact minidump representations.

clayborg added inline comments.Apr 9 2019, 7:08 AM
lib/ObjectYAML/MinidumpYAML.cpp
91 ↗(On Diff #194130)

Yeah, after seeing some less than optimal Breakpad generated minidump files, I wanted to see string sharing and general data blob sharing for any LocationDescription entries that just specify a size and RVA offset. Breakpad likes to generate UUIDs for each module in the module list, including all zeros in unique blobs for each UUID. That got me thinking about sharing data blobs

amccarth accepted this revision.Apr 17 2019, 3:23 PM

Nice!

include/llvm/ObjectYAML/MinidumpYAML.h
57 ↗(On Diff #194130)

nit: s/it's/its/

This revision is now accepted and ready to land.Apr 17 2019, 3:23 PM
jhenderson accepted this revision.Apr 18 2019, 3:46 AM
jhenderson added inline comments.
lib/ObjectYAML/MinidumpYAML.cpp
21 ↗(On Diff #194130)

plan -> plain (?)

97 ↗(On Diff #194130)

Double "the"

This revision was automatically updated to reflect the committed changes.
labath marked 4 inline comments as done.
labath added inline comments.Apr 18 2019, 7:59 AM
lib/ObjectYAML/MinidumpYAML.cpp
21 ↗(On Diff #194130)

yep. thanks.