Add the Snapshot interface

Add the Snapshot interface.

These are all of the abstract base classes used in the Snapshot series,
discussed in the thread at
https://groups.google.com/a/chromium.org/d/topic/crashpad-dev/4pACgjhIz-I.

R=rsesek@chromium.org

Committed: https://chromium.googlesource.com/crashpad/crashpad/+/7b2e7efcc247583cd8044118c48480c5aa59c756

[Mark Mentovai, 2014-09-23 19:37:25]

mark@chromium.org changed reviewers:
+ rsesek@chromium.org

[Robert Sesek, 2014-09-25 15:44:09]

https://codereview.chromium.org/597673004/diff/1/snapshot/cpu_context.h
File snapshot/cpu_context.h (right):

https://codereview.chromium.org/597673004/diff/1/snapshot/cpu_context.h#newco...
snapshot/cpu_context.h:23: struct CPUContextX86 {
I understand why this is mostly duplicated from minidump/minidump_context.h, but
I'm not sure I like it :\.

https://codereview.chromium.org/597673004/diff/1/snapshot/exception_snapshot.h
File snapshot/exception_snapshot.h (right):

https://codereview.chromium.org/597673004/diff/1/snapshot/exception_snapshot....
snapshot/exception_snapshot.h:33: //! The caller does not take this object, it
is scoped to the lifetime of the
s/take/own

https://codereview.chromium.org/597673004/diff/1/snapshot/module_snapshot.h
File snapshot/module_snapshot.h (right):

https://codereview.chromium.org/597673004/diff/1/snapshot/module_snapshot.h#n...
snapshot/module_snapshot.h:90: virtual void FileVersion(uint16_t* version_0,
Could this take a uint16_t version[4] instead?

https://codereview.chromium.org/597673004/diff/1/snapshot/module_snapshot.h#n...
snapshot/module_snapshot.h:114: virtual void UUID(struct UUID* uuid) = 0;
crashpad::UUID for consistency with other places?

https://codereview.chromium.org/597673004/diff/1/snapshot/process_snapshot.h
File snapshot/process_snapshot.h (right):

https://codereview.chromium.org/597673004/diff/1/snapshot/process_snapshot.h#...
snapshot/process_snapshot.h:81: virtual SystemSnapshot* System() = 0;
Can this be const?

https://codereview.chromium.org/597673004/diff/1/snapshot/system_snapshot.h
File snapshot/system_snapshot.h (right):

https://codereview.chromium.org/597673004/diff/1/snapshot/system_snapshot.h#n...
snapshot/system_snapshot.h:31: enum CPUArchitecture {
This seems redundant with the CPUContext::ContextFlavor. Maybe something in
util/ that just defines these centrally?

https://codereview.chromium.org/597673004/diff/1/snapshot/thread_snapshot.h
File snapshot/thread_snapshot.h (right):

https://codereview.chromium.org/597673004/diff/1/snapshot/thread_snapshot.h#n...
snapshot/thread_snapshot.h:32: //! The caller does not take this object, it is
scoped to the lifetime of the
s/take/own

https://codereview.chromium.org/597673004/diff/1/snapshot/thread_snapshot.h#n...
snapshot/thread_snapshot.h:39: //! The caller does not take this object, it is
scoped to the lifetime of the
s/take/own

[Mark Mentovai, 2014-10-02 20:25:39]

Patchset #4 (id:50001) has been deleted

[Mark Mentovai, 2014-10-02 20:27:00]

Feedback addressed.

https://codereview.chromium.org/597673004/diff/1/snapshot/cpu_context.h
File snapshot/cpu_context.h (right):

https://codereview.chromium.org/597673004/diff/1/snapshot/cpu_context.h#newco...
snapshot/cpu_context.h:23: struct CPUContextX86 {
Robert Sesek wrote:
> I understand why this is mostly duplicated from minidump/minidump_context.h,
but
> I'm not sure I like it :\.

The minidump-defined structures are not the best. They’re not totally complete
in every way, and they duplicate data in some cases. I know, it’s sad, but
really, each format we ultimately end up needing to read or write is going to
have its own structures for CPU contexts, and we shouldn’t lock the generic
interface into a substandard one.

https://codereview.chromium.org/597673004/diff/1/snapshot/module_snapshot.h
File snapshot/module_snapshot.h (right):

https://codereview.chromium.org/597673004/diff/1/snapshot/module_snapshot.h#n...
snapshot/module_snapshot.h:90: virtual void FileVersion(uint16_t* version_0,
Robert Sesek wrote:
> Could this take a uint16_t version[4] instead?

It could, but that just gets downgraded to a uint16_t*, without any assurance
that the caller’s allocated the right storage size. Four separate arguments
sucks, but it lets the compiler do reasonable checking.

Other options are a struct parameter, or even a struct return value.

https://codereview.chromium.org/597673004/diff/1/snapshot/process_snapshot.h
File snapshot/process_snapshot.h (right):

https://codereview.chromium.org/597673004/diff/1/snapshot/process_snapshot.h#...
snapshot/process_snapshot.h:81: virtual SystemSnapshot* System() = 0;
Robert Sesek wrote:
> Can this be const?

Why stop there? Pretty much everything in here can.

I have mixed feelings about the interface mandating constness on downstream
implementations in this way. It might wind up requiring implementations to have
to say “mutable” on things in order to be lazy loaders, or it might inform
implementations that they shouldn’t be lazy loaders even when that might make
sense. But the one implementation that I already have works just fine with a
fully-const interface, so I’m happy to give this a try for now.

[Robert Sesek, 2014-10-02 20:55:53]

LGTM

[Mark Mentovai, 2014-10-02 21:09:41]

Committed patchset #3 (id:40001) manually as
7b2e7efcc247583cd8044118c48480c5aa59c756 (presubmit successful).