Add MinidumpSystemInfoWriter and its test

Add MinidumpSystemInfoWriter and its test.

TEST=minidump_test MinidumpSystemInfoWriter
R=rsesek@chromium.org

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

[Will Harris, 2014-08-05 19:55:39]

On 2014/08/04 20:21:59, Mark Mentovai wrote:

+siggi who has done some windows minidump work before.

[Robert Sesek, 2014-08-06 17:35:47]

https://codereview.chromium.org/435243002/diff/1/minidump/minidump_system_inf...
File minidump/minidump_system_info_writer.cc (right):

https://codereview.chromium.org/435243002/diff/1/minidump/minidump_system_inf...
minidump/minidump_system_info_writer.cc:45:
DCHECK(system_info_.ProcessorArchitecture == kMinidumpCPUArchitectureX86 ||
Not kMinidumpCPUArchitectureAMD64 here or the other x86-related methods? This
seems a little contradictory with
https://chromium.googlesource.com/crashpad/crashpad/+/master/compat/non_win/d....

Maybe clarify this at SetCPUOtherFeatures(), since that's what the test has
AMD64 using.

https://codereview.chromium.org/435243002/diff/1/minidump/minidump_system_inf...
minidump/minidump_system_info_writer.cc:139: std::vector<MinidumpWritable*>
children(1, csd_version_.get());
Could you use a C++11 initializer list here?

http://en.cppreference.com/w/cpp/utility/initializer_list

https://codereview.chromium.org/435243002/diff/1/minidump/minidump_system_inf...
File minidump/minidump_system_info_writer.h (right):

https://codereview.chromium.org/435243002/diff/1/minidump/minidump_system_inf...
minidump/minidump_system_info_writer.h:29: #include
"minidump/minidump_string_writer.h"
You could forward-declare this instead.

https://codereview.chromium.org/435243002/diff/1/minidump/minidump_system_inf...
minidump/minidump_system_info_writer.h:35: //! \brief The writer for a
MINIDUMP_SYSTEM_INFO stream in a minidump file.
\ref MINIDUMP_SYSTEM_INFO ?

https://codereview.chromium.org/435243002/diff/1/minidump/minidump_system_inf...
minidump/minidump_system_info_writer.h:41: //! \brief Sets
MINIDUMP_SYSTEM_INFO::ProcessorArchitecture.
Doxygen auto-hyperlinks with the :: right?

https://codereview.chromium.org/435243002/diff/1/minidump/minidump_system_inf...
minidump/minidump_system_info_writer.h:91: //! \brief Sets \ref
CPU_INFORMATION::VendorId
Why do you \ref here and on line 108, but not above when referencing the
MINIDUMP_SYSTME_INFO fields?

[Mark Mentovai, 2014-08-06 18:14:03]

Thanks, Robert!

https://codereview.chromium.org/435243002/diff/1/minidump/minidump_system_inf...
File minidump/minidump_system_info_writer.cc (right):

https://codereview.chromium.org/435243002/diff/1/minidump/minidump_system_inf...
minidump/minidump_system_info_writer.cc:45:
DCHECK(system_info_.ProcessorArchitecture == kMinidumpCPUArchitectureX86 ||
rsesek wrote:
> Not kMinidumpCPUArchitectureAMD64 here or the other x86-related methods? This
> seems a little contradictory with
>
https://chromium.googlesource.com/crashpad/crashpad/+/master/compat/non_win/d....

This is correct. The CPU architecture reported in a minidump is the architecture
of the process, not of the CPU itself, so a 32-bit process on x86_64 will have
kMinidumpCPUArchitectureX86, not kMinidumpCPUArchitectureAMD64. This is
explained at
https://chromium.googlesource.com/crashpad/crashpad/+/master/compat/non_win/d....

I think the docs at SetCPUOtherFeatures() are correct too, but I admit that the
situation is a little confusing. If there’s something you think I could do to
clarify it, I’ll do it.

https://codereview.chromium.org/435243002/diff/1/minidump/minidump_system_inf...
minidump/minidump_system_info_writer.cc:139: std::vector<MinidumpWritable*>
children(1, csd_version_.get());
rsesek wrote:
> Could you use a C++11 initializer list here?
> 
> http://en.cppreference.com/w/cpp/utility/initializer_list

We only have C++11 language support, not library support, thanks to having to
support Mac OS X 10.6. We don’t have <initializer_list>.

I’d love to do this and more (like using emplace, unique_ptr, etc.) if we could
use proper library support.

https://codereview.chromium.org/435243002/diff/1/minidump/minidump_system_inf...
File minidump/minidump_system_info_writer.h (right):

https://codereview.chromium.org/435243002/diff/1/minidump/minidump_system_inf...
minidump/minidump_system_info_writer.h:29: #include
"minidump/minidump_string_writer.h"
rsesek wrote:
> You could forward-declare this instead.

I haven’t used any forward declarations except where needed to break up circular
relationships. I’d prefer not to forward-declare.

https://codereview.chromium.org/435243002/diff/1/minidump/minidump_system_inf...
minidump/minidump_system_info_writer.h:35: //! \brief The writer for a
MINIDUMP_SYSTEM_INFO stream in a minidump file.
rsesek wrote:
> \ref MINIDUMP_SYSTEM_INFO ?

Doxygen turns this into a link without a \ref.
http://www.doxygen.org/manual/autolink.html#linkclass.

I put the generated docs at
http://doxygen.crashpad.googlecode.com/git/index.html, although those correspond
to the current master HEAD, not this change.

https://codereview.chromium.org/435243002/diff/1/minidump/minidump_system_inf...
minidump/minidump_system_info_writer.h:41: //! \brief Sets
MINIDUMP_SYSTEM_INFO::ProcessorArchitecture.
rsesek wrote:
> Doxygen auto-hyperlinks with the :: right?

Yup, that’s what this is all about.

https://codereview.chromium.org/435243002/diff/1/minidump/minidump_system_inf...
minidump/minidump_system_info_writer.h:91: //! \brief Sets \ref
CPU_INFORMATION::VendorId
rsesek wrote:
> Why do you \ref here and on line 108, but not above when referencing the
> MINIDUMP_SYSTME_INFO fields?

Doxygen has a bug where it doesn’t see through a type defined in a struct very
well, so it thinks that VendorId needs to be called CPU_INFORMATION::VendorId,
not CPU_INFORMATION::X86CpuInfo::VendorId. The latter is correct.

Because MINIDUMP_SYSTEM_INFO is what this class is concerned with writing, I
wrote the documentation in terms of that struct, which has a CPU_INFORMATION Cpu
field, so the qualified name in the generated documentation is
MINIDUMP_SYSTEM_INFO::Cpu::X86CpuInfo::VendorId.

[Mark Mentovai, 2014-08-06 20:52:19]

I thought about forward declarations a little bit more and decided to go for it.

[Robert Sesek, 2014-08-07 16:48:14]

LGTM

https://codereview.chromium.org/435243002/diff/1/minidump/minidump_system_inf...
File minidump/minidump_system_info_writer.cc (right):

https://codereview.chromium.org/435243002/diff/1/minidump/minidump_system_inf...
minidump/minidump_system_info_writer.cc:45:
DCHECK(system_info_.ProcessorArchitecture == kMinidumpCPUArchitectureX86 ||
On 2014/08/06 18:14:02, Mark Mentovai wrote:
> rsesek wrote:
> > Not kMinidumpCPUArchitectureAMD64 here or the other x86-related methods?
This
> > seems a little contradictory with
> >
>
https://chromium.googlesource.com/crashpad/crashpad/+/master/compat/non_win/d....
> 
> This is correct. The CPU architecture reported in a minidump is the
architecture
> of the process, not of the CPU itself, so a 32-bit process on x86_64 will have
> kMinidumpCPUArchitectureX86, not kMinidumpCPUArchitectureAMD64. This is
> explained at
>
https://chromium.googlesource.com/crashpad/crashpad/+/master/compat/non_win/d....
> 
> I think the docs at SetCPUOtherFeatures() are correct too, but I admit that
the
> situation is a little confusing. If there’s something you think I could do to
> clarify it, I’ll do it.

The only thing I can think of would maybe explicitly call out
kMinidumpCPUArchitectureAMD64 at SetCPUOtherFeatures(), but that may be a little
out of place. I agree all the docs are technically correct, though.

[Mark Mentovai, 2014-08-07 18:49:46]

I didn’t want to just mention it for the CPUOther method, so I also made mention
to not use the CPUX86 methods for AMD64.

[Mark Mentovai, 2014-08-07 18:58:57]

Committed patchset #5 manually as aa3adf2c5b11 (presubmit successful).