Add MinidumpThreadWriter, MinidumpThreadListWriter, and their test

Add MinidumpThreadWriter, MinidumpThreadListWriter, and their test.

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

Committed: https://chromium.googlesource.com/crashpad/crashpad/+/b8684a8a3c85a672521d77800805fc9c547ee971

[Mark Mentovai, 2014-10-07 21:38:40]

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

[Mark Mentovai, 2014-10-07 21:38:40]

This depends on https://codereview.chromium.org/639513002/.

[Robert Sesek, 2014-10-08 21:49:31]

https://codereview.chromium.org/637503006/diff/20001/minidump/minidump_thread...
File minidump/minidump_thread_writer.h (right):

https://codereview.chromium.org/637503006/diff/20001/minidump/minidump_thread...
minidump/minidump_thread_writer.h:48: //! This method is expected to be called
by a MinidumpModuleListWriter in
MinidumpThreadListWriter?

https://codereview.chromium.org/637503006/diff/20001/minidump/minidump_thread...
minidump/minidump_thread_writer.h:136: //! considered children of their
MINIDUMP_THREAD. It is also possible for
It's possible but when is it the right thing to do? If you don't set this, the
thread's MINIDUMP_MEMORY will still be written since the MemoryDescriptor is
registered, right? If you set this, will it be written twice?

https://codereview.chromium.org/637503006/diff/20001/minidump/minidump_thread...
File minidump/minidump_thread_writer_test.cc (right):

https://codereview.chromium.org/637503006/diff/20001/minidump/minidump_thread...
minidump/minidump_thread_writer_test.cc:91: if (Test::HasFatalFailure()) {
You use testing::Test above but not here.

https://codereview.chromium.org/637503006/diff/20001/minidump/minidump_thread...
minidump/minidump_thread_writer_test.cc:290: void ExpectMemoryDescriptors(const
MINIDUMP_MEMORY_DESCRIPTOR* expected,
This may be useful in a more general location.

https://codereview.chromium.org/637503006/diff/20001/minidump/minidump_thread...
minidump/minidump_thread_writer_test.cc:409: SCOPED_TRACE("thread 0");
This is "thread 0" in the trace but "1" for all the variables.

https://codereview.chromium.org/637503006/diff/20001/minidump/minidump_thread...
minidump/minidump_thread_writer_test.cc:437: if (Test::HasFatalFailure()) {
This could almost become a macro.

[Mark Mentovai, 2014-10-08 22:41:04]

https://codereview.chromium.org/637503006/diff/20001/minidump/minidump_thread...
File minidump/minidump_thread_writer.h (right):

https://codereview.chromium.org/637503006/diff/20001/minidump/minidump_thread...
minidump/minidump_thread_writer.h:136: //! considered children of their
MINIDUMP_THREAD. It is also possible for
Robert Sesek wrote:
> It's possible but when is it the right thing to do?

Allays(1). This is what makes it possible to access arbitrary memory in minidump
files, rather than having to interpret each stack’s memory individually. It’s
what allows a user of a minidump reader API to say “give me 4 bytes from
0x12345678” without having to care whether 0x12345678 is in some thread’s stack
or somewhere else.

> If you don't set this, the
> thread's MINIDUMP_MEMORY will still be written since the MemoryDescriptor is
> registered, right? If you set this, will it be written twice?

Only the memory descriptor will be written in the MINIDUMP_MEMORY_LIST stream.
Both descriptors will point to the same data in the file, which will be owned by
the MinidumpThreadWriter, not the MinidumpMemoryListWriter. This is assured
because the stack memory’s MinidumpMemoryWriter is only a child (returned by
Children()) of MinidumpThreadWriter. It gets added to the
MinidumpMemoryListWriter by its AddExtraMemory() call, which does not make it a
child of the memory list. (AddMemory() is the one that makes it a child).

https://codereview.chromium.org/637503006/diff/20001/minidump/minidump_thread...
File minidump/minidump_thread_writer_test.cc (right):

https://codereview.chromium.org/637503006/diff/20001/minidump/minidump_thread...
minidump/minidump_thread_writer_test.cc:91: if (Test::HasFatalFailure()) {
Robert Sesek wrote:
> You use testing::Test above but not here.

You don’t need testing:: inside a TEST() or TEST_F(), but you do need it
outside. This is consistent with the other tests.

[Mark Mentovai, 2014-10-09 18:58:28]

Updated per feedback.

The review comments here also inspired
https://codereview.chromium.org/643683003/ and
https://codereview.chromium.org/640383002/.

[Robert Sesek, 2014-10-09 19:27:39]

LGTM

[Mark Mentovai, 2014-10-09 19:31:34]

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