Mostly-boilerplate to add MemoryMapSnapshot

Mostly-boilerplate to add MemoryMapSnapshot

Follows https://codereview.chromium.org/1375313005.

Adds MINIDUMP_MEMORY_INFO for non-win in dbghelp.h.

R=mark@chromium.org
BUG=crashpad:20, crashpad:46
Committed: https://chromium.googlesource.com/crashpad/crashpad/+/937d3d710c9e062127c5556243155eec25c6c7cb

[Mark Mentovai, 2015-10-06 20:44:50]

Profuse apologies for the delay. I started reading this on Friday night, and
then the weekend happened.

I’m not sure about having MINIDUMP_MEMORY_INFO be part of the interface. Even if
we’re not using MINIDUMP_MEMORY_INFO to encode the memory map on non-Windows
platforms, I think that the snapshot interface should do its best to keep things
uniform.

Obviously I don’t expect you to hook that stuff up on the other platforms
(unless you want to!) but I think the plumbing should at least be right, and
starting out with MINIDUMP_MEMORY_INFO exposed in the snapshot interface will
just make it harder to replumb Windows when we want to add the memory map
snapshot to another platform.

It’s probably fine if the thing that the snapshot interface exposes
(MemoryMapRegionSnapshot?) just gives out MINIDUMP_MEMORY_INFO objects for now.
Then at least the plumbing would be all there.

https://codereview.chromium.org/1377133006/diff/120001/compat/non_win/dbghelp.h
File compat/non_win/dbghelp.h (right):

https://codereview.chromium.org/1377133006/diff/120001/compat/non_win/dbghelp...
compat/non_win/dbghelp.h:141: struct __attribute__((packed, aligned(4)))
MINIDUMP_MEMORY_INFO {
Everything in here is ordered the same as in dbghelp.h (or now
minidumpapiset.h), so MINIDUMP_MEMORY_INFO should follow MINIDUMP_MISC_INFO_4.

https://codereview.chromium.org/1377133006/diff/120001/compat/non_win/dbghelp...
compat/non_win/dbghelp.h:167: //!     one of the values listed for the
AllocationProtect member.
#AllocationProtect for proper linking in the generated documentation.

https://codereview.chromium.org/1377133006/diff/120001/compat/non_win/dbghelp...
compat/non_win/dbghelp.h:875: typedef MINIDUMP_MISC_INFO_4 MINIDUMP_MISC_INFO_N;
Hey, whaddya know, Windows 10 adds MINIDUMP_MISC_INFO_5!

    XSTATE_CONFIG_FEATURE_MSC_INFO XStateData;    
    ULONG32 ProcessCookie;

The XStateData isn’t interesting until we have AVX support (and even then,
probably not until we have AVX-512). That’s
https://code.google.com/p/crashpad/issues/detail?id=58.

I don’t know what ProcessCookie is. My two guesses are the stack canary value
and the EncodePointer() key.

No need to do anything here yet, I just thought you’d be interested if you
haven’t seen it yet.

https://codereview.chromium.org/1377133006/diff/120001/snapshot/memory_map_sn...
File snapshot/memory_map_snapshot.h (right):

https://codereview.chromium.org/1377133006/diff/120001/snapshot/memory_map_sn...
snapshot/memory_map_snapshot.h:33: virtual const
std::vector<MINIDUMP_MEMORY_INFO>& Regions() const = 0;
Is there a need for any other member functions in this interface? If not, we may
as well do away with MemoryMapSnapshot, and just add std::vector<Type>
ProcessSnapshot::MemoryMapRegions(), the same way that it deals with vectors of
modules and threads.

I can’t really think of anything that might need to be a property of the memory
map that’s not a region, and any hypothetical thing that could go here would
probably also also make sense as an attribute of ProcessSnapshot.

https://codereview.chromium.org/1377133006/diff/120001/snapshot/win/memory_ma...
File snapshot/win/memory_map_snapshot_win.cc (right):

https://codereview.chromium.org/1377133006/diff/120001/snapshot/win/memory_ma...
snapshot/win/memory_map_snapshot_win.cc:29: regions_ =
process_reader->GetProcessInfo().MemoryInfo();
The types are getting confused here. (Or is this fixed up in one of the other
CLs?)

[scottmg, 2015-10-07 21:45:09]

Hopefully this is what you meant by having the region give out
MINIDUMP_MEMORY_INFO, but not exposing those directly as part of the snapshot
interface.

It should be relatively straightforward to break up the method on
MemoryMapRegionSnapshot::AsMinidumpMemoryInfo() to have it return all the
required values via individual methods, but it seems just as easy to do that
when we do other platforms once we know what they need, rather than upfront.

https://codereview.chromium.org/1377133006/diff/120001/compat/non_win/dbghelp.h
File compat/non_win/dbghelp.h (right):

https://codereview.chromium.org/1377133006/diff/120001/compat/non_win/dbghelp...
compat/non_win/dbghelp.h:141: struct __attribute__((packed, aligned(4)))
MINIDUMP_MEMORY_INFO {
On 2015/10/06 20:44:49, Mark Mentovai wrote:
> Everything in here is ordered the same as in dbghelp.h (or now
> minidumpapiset.h), so MINIDUMP_MEMORY_INFO should follow MINIDUMP_MISC_INFO_4.

Done.

https://codereview.chromium.org/1377133006/diff/120001/compat/non_win/dbghelp...
compat/non_win/dbghelp.h:167: //!     one of the values listed for the
AllocationProtect member.
On 2015/10/06 20:44:49, Mark Mentovai wrote:
> #AllocationProtect for proper linking in the generated documentation.

Done.

https://codereview.chromium.org/1377133006/diff/120001/compat/non_win/dbghelp...
compat/non_win/dbghelp.h:875: typedef MINIDUMP_MISC_INFO_4 MINIDUMP_MISC_INFO_N;
On 2015/10/06 20:44:49, Mark Mentovai wrote:
> Hey, whaddya know, Windows 10 adds MINIDUMP_MISC_INFO_5!
> 
>     XSTATE_CONFIG_FEATURE_MSC_INFO XStateData;    
>     ULONG32 ProcessCookie;
> 
> The XStateData isn’t interesting until we have AVX support (and even then,
> probably not until we have AVX-512). That’s
> https://code.google.com/p/crashpad/issues/detail?id=58.
> 
> I don’t know what ProcessCookie is. My two guesses are the stack canary value
> and the EncodePointer() key.
> 
> No need to do anything here yet, I just thought you’d be interested if you
> haven’t seen it yet.

Ah, I hadn't. It'd be nice if they documented something past _2. :/

https://codereview.chromium.org/1377133006/diff/120001/snapshot/memory_map_sn...
File snapshot/memory_map_snapshot.h (right):

https://codereview.chromium.org/1377133006/diff/120001/snapshot/memory_map_sn...
snapshot/memory_map_snapshot.h:33: virtual const
std::vector<MINIDUMP_MEMORY_INFO>& Regions() const = 0;
On 2015/10/06 20:44:49, Mark Mentovai wrote:
> Is there a need for any other member functions in this interface? If not, we
may
> as well do away with MemoryMapSnapshot, and just add std::vector<Type>
> ProcessSnapshot::MemoryMapRegions(), the same way that it deals with vectors
of
> modules and threads.
> 
> I can’t really think of anything that might need to be a property of the
memory
> map that’s not a region, and any hypothetical thing that could go here would
> probably also also make sense as an attribute of ProcessSnapshot.

I thought there might be when I started, but I can't think of anything either.
Done.

https://codereview.chromium.org/1377133006/diff/120001/snapshot/win/memory_ma...
File snapshot/win/memory_map_snapshot_win.cc (right):

https://codereview.chromium.org/1377133006/diff/120001/snapshot/win/memory_ma...
snapshot/win/memory_map_snapshot_win.cc:29: regions_ =
process_reader->GetProcessInfo().MemoryInfo();
On 2015/10/06 20:44:49, Mark Mentovai wrote:
> The types are getting confused here. (Or is this fixed up in one of the other
> CLs?)

This is gone now.

[Mark Mentovai, 2015-10-12 13:47:53]

LGTM

https://codereview.chromium.org/1377133006/diff/220001/compat/non_win/dbghelp.h
File compat/non_win/dbghelp.h (right):

https://codereview.chromium.org/1377133006/diff/220001/compat/non_win/dbghelp...
compat/non_win/dbghelp.h:855: uint32_t __alignment1;
No comment necessary for __alignment fields. There’s precedent elsewhere in this
file.

https://codereview.chromium.org/1377133006/diff/220001/compat/non_win/dbghelp...
compat/non_win/dbghelp.h:862: //! `MEM_COMMIT`, `MEM_FREE`, or `MEM_RESERVE`.
Indent.

https://codereview.chromium.org/1377133006/diff/220001/snapshot/win/memory_ma...
File snapshot/win/memory_map_region_snapshot_win.cc (right):

https://codereview.chromium.org/1377133006/diff/220001/snapshot/win/memory_ma...
snapshot/win/memory_map_region_snapshot_win.cc:22: memset(&memory_info_, 0,
sizeof(memory_info_));
: memory_info_() instead of an explicit memset() to initialize it?

[Mark Mentovai, 2015-10-12 14:35:54]

https://codereview.chromium.org/1377133006/diff/220001/snapshot/memory_map_re...
File snapshot/memory_map_region_snapshot.h (right):

https://codereview.chromium.org/1377133006/diff/220001/snapshot/memory_map_re...
snapshot/memory_map_region_snapshot.h:30: virtual MINIDUMP_MEMORY_INFO
AsMinidumpMemoryInfo() const = 0;
This may need to to change to something that can express “I can’t give you that
format” in the future. For now, this is fine, but make this return a const
MINIDUMP_MEMORY_INFO&.

[scottmg, 2015-10-13 19:33:31]

https://codereview.chromium.org/1377133006/diff/220001/compat/non_win/dbghelp.h
File compat/non_win/dbghelp.h (right):

https://codereview.chromium.org/1377133006/diff/220001/compat/non_win/dbghelp...
compat/non_win/dbghelp.h:855: uint32_t __alignment1;
On 2015/10/12 13:47:52, Mark Mentovai wrote:
> No comment necessary for __alignment fields. There’s precedent elsewhere in
this
> file.

Done.

https://codereview.chromium.org/1377133006/diff/220001/compat/non_win/dbghelp...
compat/non_win/dbghelp.h:862: //! `MEM_COMMIT`, `MEM_FREE`, or `MEM_RESERVE`.
On 2015/10/12 13:47:52, Mark Mentovai wrote:
> Indent.

Done.

https://codereview.chromium.org/1377133006/diff/220001/snapshot/memory_map_re...
File snapshot/memory_map_region_snapshot.h (right):

https://codereview.chromium.org/1377133006/diff/220001/snapshot/memory_map_re...
snapshot/memory_map_region_snapshot.h:30: virtual MINIDUMP_MEMORY_INFO
AsMinidumpMemoryInfo() const = 0;
On 2015/10/12 14:35:54, Mark Mentovai wrote:
> This may need to to change to something that can express “I can’t give you
that
> format” in the future. For now, this is fine, but make this return a const
> MINIDUMP_MEMORY_INFO&.

Done.

https://codereview.chromium.org/1377133006/diff/220001/snapshot/win/memory_ma...
File snapshot/win/memory_map_region_snapshot_win.cc (right):

https://codereview.chromium.org/1377133006/diff/220001/snapshot/win/memory_ma...
snapshot/win/memory_map_region_snapshot_win.cc:22: memset(&memory_info_, 0,
sizeof(memory_info_));
On 2015/10/12 13:47:52, Mark Mentovai wrote:
> : memory_info_() instead of an explicit memset() to initialize it?

Oops, I always feel like that's going to do nothing for POD structs. Done.

[scottmg, 2015-10-13 19:37:50]

Committed patchset #13 (id:240001) manually as
937d3d710c9e062127c5556243155eec25c6c7cb (presubmit successful).