Add MinidumpFileWriter

Add MinidumpFileWriter and its test.

MinidumpFileWriter writes the top-level object to a minidump file. This
consists of a MINIDUMP_HEADER and a list of MINIDUMP_DIRECTORY entries
that point to streams, which are second-level objects in minidump files.
This change also adds the base class for stream writers,
MinidumpStreamWriter.

TEST=minidump_test MinidumpFileWriter*
R=rsesek@chromium.org

Committed: https://chromium.googlesource.com/crashpad/crashpad/+/859560f70dd9

[Robert Sesek, 2014-08-03 15:19:59]

Are there no tests for this piece yet because more pieces are necessary do test
meaningfully?

https://codereview.chromium.org/432863006/diff/1/minidump/minidump_file_write...
File minidump/minidump_file_writer.cc (right):

https://codereview.chromium.org/432863006/diff/1/minidump/minidump_file_write...
minidump/minidump_file_writer.cc:121: std::vector<MinidumpWritable*> children;
You're building a vector<MinidumpWritable*> yet the return type is of
vector<internal::MinidumpWitable*>. Any reason to not just use
std::vector<T>::vector(const std::vector<T>&) copy constructor?

https://codereview.chromium.org/432863006/diff/1/minidump/minidump_file_write...
minidump/minidump_file_writer.cc:153: iov.iov_len = sizeof(MINIDUMP_DIRECTORY);
If you moved this into the loop body, you could remove the empty check and do
the for loop unconditionally.

https://codereview.chromium.org/432863006/diff/1/minidump/minidump_file_writer.h
File minidump/minidump_file_writer.h (right):

https://codereview.chromium.org/432863006/diff/1/minidump/minidump_file_write...
minidump/minidump_file_writer.h:54: void
AddStream(internal::MinidumpStreamWriter* stream);
What are the ownership semantics here?

https://codereview.chromium.org/432863006/diff/1/minidump/minidump_file_write...
minidump/minidump_file_writer.h:54: void
AddStream(internal::MinidumpStreamWriter* stream);
It's a bit weird to have a public method in the non-internal:: namespace
accepting an internal:: param type.

https://codereview.chromium.org/432863006/diff/1/minidump/minidump_file_write...
minidump/minidump_file_writer.h:71: virtual std::vector<MinidumpWritable*>
Children() override;
Should this be a type of internal::MinidumpWritable?

https://codereview.chromium.org/432863006/diff/1/minidump/minidump_file_write...
minidump/minidump_file_writer.h:77: std::vector<internal::MinidumpStreamWriter*>
streams_;
Ownership?

https://codereview.chromium.org/432863006/diff/1/minidump/minidump_stream_wri...
File minidump/minidump_stream_writer.h (right):

https://codereview.chromium.org/432863006/diff/1/minidump/minidump_stream_wri...
minidump/minidump_stream_writer.h:53: virtual bool Freeze();
override?

[Mark Mentovai, 2014-08-03 20:14:10]

rsesek wrote:
> Are there no tests for this piece yet because more pieces are necessary do
> test meaningfully?

I thought so initially, but on the second thought, this can be tested pretty
well with a TestStream class, which will catch bad base class behaviors in a
dedicated test where they belong, instead of hoping that errors will cascade to
subclass tests.

https://codereview.chromium.org/432863006/diff/1/minidump/minidump_file_write...
File minidump/minidump_file_writer.cc (right):

https://codereview.chromium.org/432863006/diff/1/minidump/minidump_file_write...
minidump/minidump_file_writer.cc:121: std::vector<MinidumpWritable*> children;
rsesek wrote:
> You're building a vector<MinidumpWritable*> yet the return type is of
> vector<internal::MinidumpWitable*>. Any reason to not just use
> std::vector<T>::vector(const std::vector<T>&) copy constructor?

streams_ is std::vector<MinidumpStream*> and the return type is
std::vector<MinidumpWritable*>. MinidumpStream inherits from MinidumpWritable,
so a MinidumpStream* is also a MinidumpWritable*, but a vector of
MinidumpStream* isn’t compatible with a vector of MinidumpWritable*.

This is just the way the language works. It’s a limitation of std::vector, I
don’t think it’s worth a comment.

https://codereview.chromium.org/432863006/diff/1/minidump/minidump_file_writer.h
File minidump/minidump_file_writer.h (right):

https://codereview.chromium.org/432863006/diff/1/minidump/minidump_file_write...
minidump/minidump_file_writer.h:54: void
AddStream(internal::MinidumpStreamWriter* stream);
rsesek wrote:
> It's a bit weird to have a public method in the non-internal:: namespace
> accepting an internal:: param type.

The idea is that the internal namespace is shared by the Minidump*Writer family
of classes, so we’ll have MinidumpSystemInfoWriter and others that implement
internal::MinidumpStreamWriter. Users outside of this class family shouldn’t
touch anything in internal directly, but they’re allowed to make a
MinidumpSystemInfoWriter and pass it to this function.

Is this worth a comment? I don’t really know where to put it. On the
MinidumpStreamWriter class, maybe?

https://codereview.chromium.org/432863006/diff/1/minidump/minidump_file_write...
minidump/minidump_file_writer.h:71: virtual std::vector<MinidumpWritable*>
Children() override;
rsesek wrote:
> Should this be a type of internal::MinidumpWritable?

It is. Since MinidumpFileWriter inherits from internal::MinidumpWritable, it’s
able to use that type name without the internal:: qualification.

[Robert Sesek, 2014-08-03 21:20:55]

LGTM. Nice test.

https://codereview.chromium.org/432863006/diff/1/minidump/minidump_file_write...
File minidump/minidump_file_writer.cc (right):

https://codereview.chromium.org/432863006/diff/1/minidump/minidump_file_write...
minidump/minidump_file_writer.cc:121: std::vector<MinidumpWritable*> children;
On 2014/08/03 20:14:10, Mark Mentovai wrote:
> rsesek wrote:
> > You're building a vector<MinidumpWritable*> yet the return type is of
> > vector<internal::MinidumpWitable*>. Any reason to not just use
> > std::vector<T>::vector(const std::vector<T>&) copy constructor?
> 
> streams_ is std::vector<MinidumpStream*> and the return type is
> std::vector<MinidumpWritable*>. MinidumpStream inherits from MinidumpWritable,
> so a MinidumpStream* is also a MinidumpWritable*, but a vector of
> MinidumpStream* isn’t compatible with a vector of MinidumpWritable*.
> 
> This is just the way the language works. It’s a limitation of std::vector, I
> don’t think it’s worth a comment.

Ack. I misread streams_ as MinidumpWritable* instead of MinidumpStreamWriter.

https://codereview.chromium.org/432863006/diff/1/minidump/minidump_file_writer.h
File minidump/minidump_file_writer.h (right):

https://codereview.chromium.org/432863006/diff/1/minidump/minidump_file_write...
minidump/minidump_file_writer.h:54: void
AddStream(internal::MinidumpStreamWriter* stream);
On 2014/08/03 20:14:10, Mark Mentovai wrote:
> rsesek wrote:
> > It's a bit weird to have a public method in the non-internal:: namespace
> > accepting an internal:: param type.
> 
> The idea is that the internal namespace is shared by the Minidump*Writer
family
> of classes, so we’ll have MinidumpSystemInfoWriter and others that implement
> internal::MinidumpStreamWriter. Users outside of this class family shouldn’t
> touch anything in internal directly, but they’re allowed to make a
> MinidumpSystemInfoWriter and pass it to this function.
> 
> Is this worth a comment? I don’t really know where to put it. On the
> MinidumpStreamWriter class, maybe?

Got it, since MinidumpSystemInfoWriter won't be internal:: only its base class
will, that seems fine as-is.

https://codereview.chromium.org/432863006/diff/1/minidump/minidump_file_write...
minidump/minidump_file_writer.h:71: virtual std::vector<MinidumpWritable*>
Children() override;
On 2014/08/03 20:14:10, Mark Mentovai wrote:
> rsesek wrote:
> > Should this be a type of internal::MinidumpWritable?
> 
> It is. Since MinidumpFileWriter inherits from internal::MinidumpWritable, it’s
> able to use that type name without the internal:: qualification.

Right, it was just inconsistent with the .cc file (since you're required to
place the internal:: qualifier on the definition).

https://codereview.chromium.org/432863006/diff/20001/minidump/minidump_file_w...
File minidump/minidump_file_writer_test.cc (right):

https://codereview.chromium.org/432863006/diff/20001/minidump/minidump_file_w...
minidump/minidump_file_writer_test.cc:235:
ASSERT_DEATH(minidump_file.AddStream(&stream2), "already present");
I guess this is why exceptions are kind of nice, because then you don't need
death tests to validate important assertion failures…

[Mark Mentovai, 2014-08-03 22:45:55]

Thanks for all of the reviews!

https://codereview.chromium.org/432863006/diff/1/minidump/minidump_file_writer.h
File minidump/minidump_file_writer.h (right):

https://codereview.chromium.org/432863006/diff/1/minidump/minidump_file_write...
minidump/minidump_file_writer.h:71: virtual std::vector<MinidumpWritable*>
Children() override;
rsesek wrote:
> On 2014/08/03 20:14:10, Mark Mentovai wrote:
> > rsesek wrote:
> > > Should this be a type of internal::MinidumpWritable?
> > 
> > It is. Since MinidumpFileWriter inherits from internal::MinidumpWritable,
it’s
> > able to use that type name without the internal:: qualification.
> 
> Right, it was just inconsistent with the .cc file (since you're required to
> place the internal:: qualifier on the definition).

It’s not clear if you want me to change this, so I’m leaving it. It’s the same
situation as

class C {
 public:
  typedef int T;
  T F();
};

C::T F() {  // need to qualify with C here
  T t = …;  // but not here
  return t;
}

https://codereview.chromium.org/432863006/diff/20001/minidump/minidump_file_w...
File minidump/minidump_file_writer_test.cc (right):

https://codereview.chromium.org/432863006/diff/20001/minidump/minidump_file_w...
minidump/minidump_file_writer_test.cc:235:
ASSERT_DEATH(minidump_file.AddStream(&stream2), "already present");
rsesek wrote:
> I guess this is why exceptions are kind of nice, because then you don't need
> death tests to validate important assertion failures…

Exactly, but it doesn’t seem like a good idea to use assertions in Crashpad
since it’s part of the Chromium universe and uses Chromium’s exception-free base
interface.

[Mark Mentovai, 2014-08-03 22:47:39]

Committed patchset #2 manually as 859560f70dd9 (presubmit successful).