Use MEMORY_BASIC_INFORMATION64 rather than a custom MemoryInfo

Use MEMORY_BASIC_INFORMATION64 rather than a custom MemoryInfo

We already use all the shared constants for page protection and type,
so rather than making various incompatible structures, just use
the MEMORY_BASIC_INFORMATION64 one directly, so that it can be directly
used.

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

[scottmg, 2015-10-01 23:07:11]

Simple move out of win/ in service of
https://codereview.chromium.org/1377133006/.

I'm not sure this is the best setup.

It's pretty Windows-specific obviously, but it's also a bit tortured to make a
very-similar-but-with-different-constants-and-names to then turn around and only
use it on Windows, and have to map both to the new structure and from the new
structure, only to turn around again and turn it back into a
MINIDUMP_MEMORY_INFO, which is identical as well modulo pointer lengths.

I guess we'll need to put MINIDUMP_MEMORY_INFO and the various constants into
compat/non_win/dbghelp.h anyway, so maybe a simpler design would be to use that
directly on all platforms in both util/win/process_info.* and snapshot/.

Any preference?

[scottmg, 2015-10-02 00:01:00]

Patchset #7 (id:120001) has been deleted

[scottmg, 2015-10-02 00:01:53]

Patchset #5 (id:80001) has been deleted

[scottmg, 2015-10-02 00:02:01]

Patchset #5 (id:80001) has been deleted

[scottmg, 2015-10-02 00:02:06]

Patchset #4 (id:60001) has been deleted

[scottmg, 2015-10-02 00:02:10]

Patchset #3 (id:40001) has been deleted

[scottmg, 2015-10-02 00:02:18]

Patchset #2 (id:20001) has been deleted

[scottmg, 2015-10-02 00:02:23]

Patchset #1 (id:1) has been deleted

[scottmg, 2015-10-02 00:04:18]

On 2015/10/01 23:07:11, scottmg wrote:
> Simple move out of win/ in service of
> https://codereview.chromium.org/1377133006/.
> 
> I'm not sure this is the best setup.
> 
> It's pretty Windows-specific obviously, but it's also a bit tortured to make a
> very-similar-but-with-different-constants-and-names to then turn around and
only
> use it on Windows, and have to map both to the new structure and from the new
> structure, only to turn around again and turn it back into a
> MINIDUMP_MEMORY_INFO, which is identical as well modulo pointer lengths.
> 
> I guess we'll need to put MINIDUMP_MEMORY_INFO and the various constants into
> compat/non_win/dbghelp.h anyway, so maybe a simpler design would be to use
that
> directly on all platforms in both util/win/process_info.* and snapshot/.
> 
> Any preference?

Yeah, I think I like this better (just using MINIDUMP_MEMORY_INFO directly).

(ps#1 is the original that I asked for comment on with title "Move MemoryInfo to
non-win to make available to future MemoryMapSnapshot", but I think ps#2
matching the current title is better.)

[Mark Mentovai, 2015-10-05 15:46:06]

Before we start heading down any of these paths, I want to have a better idea of
how we’re going to deal with this on other platforms. Once we’ve got the
minidump and snapshot support for it, I want to wire it up to work on OS X too.

It seems that the memory map has more differences than many of the other types
that minidumps carry, which is kind of weird because everyone’s basically using
the same architecture for this.

Here’s the set of information that we can get on Mac (note that base address and
size aren’t provided in the struct but are obviously properties of map entries):

struct vm_region_submap_info {
        vm_prot_t               protection;     /* present access protection */
        vm_prot_t               max_protection; /* max avail through vm_prot */
        vm_inherit_t            inheritance;/* behavior of map/obj on fork */
        uint32_t                offset;         /* offset into object/map */
        unsigned int            user_tag;       /* user tag on map entry */
        unsigned int            pages_resident; /* only valid for objects */
        unsigned int            pages_shared_now_private; /* only for objects */
        unsigned int            pages_swapped_out; /* only for objects */
        unsigned int            pages_dirtied;   /* only for objects */
        unsigned int            ref_count;       /* obj/map mappers, etc */
        unsigned short          shadow_depth;   /* only for obj */
        unsigned char           external_pager;  /* only for obj */
        unsigned char           share_mode;     /* see enumeration */
        boolean_t               is_submap;      /* submap vs obj */
        vm_behavior_t           behavior;       /* access behavior hint */
        vm32_object_id_t        object_id;      /* obj/map name, not a handle */
        unsigned short          user_wired_count; 
};

Now, not all of that is useful in a minidump, but I’d say that in addition to
address and size (obviously), protection and max_protect are both useful,
user_tag is pretty useful, and share_mode is kind of useful. The other
attributes (page counts) are probably less interesting, although it’d be nice to
carry the filename associated with a file-backed mapping somewhere (it’s
actually not represented in this struct).

Linux is similar-ish (minus user_tag and max_protect), based on what’s made
available at /proc/<pid>/smaps, but that’s also different per-architecture.

So at this point, I’m trying to decide if we want to try to overload
MINIDUMP_MEMORY_INFO on other platforms, or have a different struct that we use
in a MemoryInfo stream on other platforms (with the structure size the same,
this is basically the same as overloading), or if we want to have a different
thing hanging off of CrashpadInfo altogether for other platforms.

Other things that I think will inform this decision are whether Windows wants to
represent more about a memory region than MINIDUMP_MEMORY_INFO allows. The path
of a file backing a mapping seems like it might be interesting there too, for
example.

Anyway, I wanted to kind of have a direction in mind for cross-OS support before
digging into the code. It sounds like you’ve already given this some thought, so
if you’ve got ideas on how this is going to work for non-Windows, let’s
hear ’em!

[scottmg, 2015-10-05 17:48:52]

On 2015/10/05 15:46:06, Mark Mentovai wrote:
> Before we start heading down any of these paths, I want to have a better idea
of
> how we’re going to deal with this on other platforms. Once we’ve got the
> minidump and snapshot support for it, I want to wire it up to work on OS X
too.
> 
> It seems that the memory map has more differences than many of the other types
> that minidumps carry, which is kind of weird because everyone’s basically
using
> the same architecture for this.
> 
> Here’s the set of information that we can get on Mac (note that base address
and
> size aren’t provided in the struct but are obviously properties of map
entries):
> 
> struct vm_region_submap_info {
>         vm_prot_t               protection;     /* present access protection
*/
>         vm_prot_t               max_protection; /* max avail through vm_prot
*/
>         vm_inherit_t            inheritance;/* behavior of map/obj on fork */
>         uint32_t                offset;         /* offset into object/map */
>         unsigned int            user_tag;       /* user tag on map entry */
>         unsigned int            pages_resident; /* only valid for objects */
>         unsigned int            pages_shared_now_private; /* only for objects
*/
>         unsigned int            pages_swapped_out; /* only for objects */
>         unsigned int            pages_dirtied;   /* only for objects */
>         unsigned int            ref_count;       /* obj/map mappers, etc */
>         unsigned short          shadow_depth;   /* only for obj */
>         unsigned char           external_pager;  /* only for obj */
>         unsigned char           share_mode;     /* see enumeration */
>         boolean_t               is_submap;      /* submap vs obj */
>         vm_behavior_t           behavior;       /* access behavior hint */
>         vm32_object_id_t        object_id;      /* obj/map name, not a handle
*/
>         unsigned short          user_wired_count; 
> };
> 
> Now, not all of that is useful in a minidump, but I’d say that in addition to
> address and size (obviously), protection and max_protect are both useful,
> user_tag is pretty useful, and share_mode is kind of useful. The other
> attributes (page counts) are probably less interesting, although it’d be nice
to
> carry the filename associated with a file-backed mapping somewhere (it’s
> actually not represented in this struct).
> 
> Linux is similar-ish (minus user_tag and max_protect), based on what’s made
> available at /proc/<pid>/smaps, but that’s also different per-architecture.
> 
> So at this point, I’m trying to decide if we want to try to overload
> MINIDUMP_MEMORY_INFO on other platforms, or have a different struct that we
use
> in a MemoryInfo stream on other platforms (with the structure size the same,
> this is basically the same as overloading), or if we want to have a different
> thing hanging off of CrashpadInfo altogether for other platforms.
> 
> Other things that I think will inform this decision are whether Windows wants
to
> represent more about a memory region than MINIDUMP_MEMORY_INFO allows. The
path
> of a file backing a mapping seems like it might be interesting there too, for
> example.
> 
> Anyway, I wanted to kind of have a direction in mind for cross-OS support
before
> digging into the code. It sounds like you’ve already given this some thought,
so
> if you’ve got ideas on how this is going to work for non-Windows, let’s
> hear&nbsp;’em!

I hadn't really thought about other platforms too much. I'm not sure how things
are consumed on non-Windows I guess. At the moment, if I can't easily view it in
windbg it might as well not exist, so here and in the follow-ups I was just
focusing on getting !vprot to work.

I guess for other platforms there's not much benefit to constraining ourselves
to overloading, since there's no real upside to following the standard minidump
format conventions. If we have a pipeline set up where we can add extra data,
modify the processor, include it in the crash UI, view it in the debugger, etc.
then yeah, things like the name of the backing file would be useful to have.

[[ !mapped_file looks like it doesn't work in minidumps, so I'm not sure if
there's a standard place to store the mapping information. !address is what I'd
use on a running process and it figures it out
(https://gist.github.com/sgraham/875d2bfd5901da0b54dc but you have to scroll to
the right), but it doesn't for a current breakpad dump. I don't know if that's
because we're not storing something in the dump, or whether it just doesn't work
though. ]]

[Mark Mentovai, 2015-10-06 20:57:14]

scottmg wrote:
> I hadn't really thought about other platforms too much. I'm not sure how
> things are consumed on non-Windows I guess. At the moment, if I can't easily
> view it in windbg it might as well not exist, so here and in the follow-ups I
> was just focusing on getting !vprot to work.

The idea is that the future Crashpad-based processor will expose this via its
ProcessSnapshotMinidump/MemoryMapRangeSnapshotMinidump, where we can make it
available on, for example, the crash server.

> I guess for other platforms there's not much benefit to constraining ourselves
> to overloading, since there's no real upside to following the standard
> minidump format conventions.

Well, we’ve realized some benefits by sticking to the minidump format for
threads, modules, etc. The benefits may be smaller for the memory map, but even
so, it’ll all need to show up in the same snapshot on the other end, so maybe it
does make sense to try to share a little bit.

That said, I have no intention of overloading the MINIDUMP_HANDLE_DATA_STREAM to
carry file descriptor information for POSIX platforms, or Mach port information
for OS X. We’re going to hang that stuff off of the MinidumpCrashpadInfo stream.
So there’s about to be precedent for not overloading, too.

https://codereview.chromium.org/1375313005/diff/140001/util/win/process_info.h
File util/win/process_info.h (right):

https://codereview.chromium.org/1375313005/diff/140001/util/win/process_info....
util/win/process_info.h:98: const std::vector<MINIDUMP_MEMORY_INFO>&
MemoryInfo() const;
OK, this was what I was missing when reading
https://codereview.chromium.org/1377133006/. I see now.

Should this be MEMORY_BASIC_INFORMATION here? (I think you said that the 64 was
broken, but since we can’t easily equip for 32-reads-64, that should be fine.)
All the way in util, we’re more working with live processes than minidumps. The
structs are functionally equivalent, and it’d probably let you get rid of some
conversiony stuff here at the expense of having to write it somewhere else, but
I think that having the conversion thing happen elsewhere closer to
minidump-land is probably more correct.

[scottmg, 2015-10-06 22:43:26]

OK, I'm not sure about other platforms yet, and just looking at the snapshot
now, but I guess at least this part for Windows-only is an improvement.

https://codereview.chromium.org/1375313005/diff/140001/util/win/process_info.h
File util/win/process_info.h (right):

https://codereview.chromium.org/1375313005/diff/140001/util/win/process_info....
util/win/process_info.h:98: const std::vector<MINIDUMP_MEMORY_INFO>&
MemoryInfo() const;
On 2015/10/06 20:57:14, Mark Mentovai wrote:
> OK, this was what I was missing when reading
> https://codereview.chromium.org/1377133006/. I see now.
> 
> Should this be MEMORY_BASIC_INFORMATION here? (I think you said that the 64
was
> broken, but since we can’t easily equip for 32-reads-64, that should be fine.)
> All the way in util, we’re more working with live processes than minidumps.
The
> structs are functionally equivalent, and it’d probably let you get rid of some
> conversiony stuff here at the expense of having to write it somewhere else,
but
> I think that having the conversion thing happen elsewhere closer to
> minidump-land is probably more correct.

Makes sense. I went with MEMORY_BASIC_INFORMATION64 since that one's ULONG64s
for addresses instead of void*, which makes more sense when we're discussing the
memory of a foreign process's address space.

[Mark Mentovai, 2015-10-07 01:17:24]

LGTM

https://codereview.chromium.org/1375313005/diff/160001/util/win/process_info.cc
File util/win/process_info.cc (right):

https://codereview.chromium.org/1375313005/diff/160001/util/win/process_info....
util/win/process_info.cc:449: if (range.OverlapsRange(Range(mi.BaseAddress,
mi.RegionSize)))
Since these types are now not WinVMAddress in name, but they should be the same
in underlying fundamental type, can you add static_asserts to test that they’re
really the same?

https://codereview.chromium.org/1375313005/diff/160001/util/win/process_info_...
File util/win/process_info_test.cc (right):

https://codereview.chromium.org/1375313005/diff/160001/util/win/process_info_...
util/win/process_info_test.cc:113:
GetTimestampForLoadedLibrary(GetModuleHandle(nullptr)));
Oh, awesome!

The arguments should be flipped here: (expected, actual). It’s so
counter-intuitive to people who speak English when they’re not speaking C++.
Same on lines 104 and 106.

[scottmg, 2015-10-07 18:16:50]

Patchset #4 (id:180001) has been deleted

[scottmg, 2015-10-07 18:19:17]

Thanks

https://codereview.chromium.org/1375313005/diff/160001/util/win/process_info.cc
File util/win/process_info.cc (right):

https://codereview.chromium.org/1375313005/diff/160001/util/win/process_info....
util/win/process_info.cc:449: if (range.OverlapsRange(Range(mi.BaseAddress,
mi.RegionSize)))
On 2015/10/07 01:17:24, Mark Mentovai wrote:
> Since these types are now not WinVMAddress in name, but they should be the
same
> in underlying fundamental type, can you add static_asserts to test that
they’re
> really the same?

Done.

https://codereview.chromium.org/1375313005/diff/160001/util/win/process_info_...
File util/win/process_info_test.cc (right):

https://codereview.chromium.org/1375313005/diff/160001/util/win/process_info_...
util/win/process_info_test.cc:113:
GetTimestampForLoadedLibrary(GetModuleHandle(nullptr)));
On 2015/10/07 01:17:24, Mark Mentovai wrote:
> Oh, awesome!
> 
> The arguments should be flipped here: (expected, actual). It’s so
> counter-intuitive to people who speak English when they’re not speaking C++.
> Same on lines 104 and 106.

Some day I will write a clang machine translation to correct that error, and the
world.

[Mark Mentovai, 2015-10-07 18:50:52]

LGTM

https://codereview.chromium.org/1375313005/diff/160001/util/win/process_info_...
File util/win/process_info_test.cc (right):

https://codereview.chromium.org/1375313005/diff/160001/util/win/process_info_...
util/win/process_info_test.cc:113:
GetTimestampForLoadedLibrary(GetModuleHandle(nullptr)));
scottmg wrote:
> On 2015/10/07 01:17:24, Mark Mentovai wrote:
> > Oh, awesome!
> > 
> > The arguments should be flipped here: (expected, actual). It’s so
> > counter-intuitive to people who speak English when they’re not speaking C++.
> > Same on lines 104 and 106.
> 
> Some day I will write a clang machine translation to correct that error, and
the
> world.

Or we could just fix gtest, since that’s what’s being stupid.

https://codereview.chromium.org/1375313005/diff/200001/util/win/process_info.cc
File util/win/process_info.cc (right):

https://codereview.chromium.org/1375313005/diff/200001/util/win/process_info....
util/win/process_info.cc:21: #include <type_traits>
OK if this is guaranteed to be available in all of the MSVC versions we care
about.

[scottmg, 2015-10-07 19:22:31]

https://codereview.chromium.org/1375313005/diff/160001/util/win/process_info_...
File util/win/process_info_test.cc (right):

https://codereview.chromium.org/1375313005/diff/160001/util/win/process_info_...
util/win/process_info_test.cc:113:
GetTimestampForLoadedLibrary(GetModuleHandle(nullptr)));
On 2015/10/07 18:50:52, Mark Mentovai wrote:
> scottmg wrote:
> > On 2015/10/07 01:17:24, Mark Mentovai wrote:
> > > Oh, awesome!
> > > 
> > > The arguments should be flipped here: (expected, actual). It’s so
> > > counter-intuitive to people who speak English when they’re not speaking
C++.
> > > Same on lines 104 and 106.
> > 
> > Some day I will write a clang machine translation to correct that error, and
> the
> > world.
> 
> Or we could just fix gtest, since that’s what’s being stupid.

Right, but then I have to fix all the broken code that's written them (expected,
actual). :)

(Oh, or did you just mean making it [val1, val2] and printing the value of both?
Yeah that would probably be more tractable.)

https://codereview.chromium.org/1375313005/diff/200001/util/win/process_info.cc
File util/win/process_info.cc (right):

https://codereview.chromium.org/1375313005/diff/200001/util/win/process_info....
util/win/process_info.cc:21: #include <type_traits>
On 2015/10/07 18:50:52, Mark Mentovai wrote:
> OK if this is guaranteed to be available in all of the MSVC versions we care
> about.

Hmm, it is, but strictly speaking it's also a C++11 library feature I guess so
shouldn't be used yet (even though it's obviously header-only and compile
time-only). Turns out we have a version in base/template_util.h so switched to
that.

[scottmg, 2015-10-07 19:23:17]

Committed patchset #5 (id:220001) manually as
d3bdb23ffe0f930b82452c74c8d129786c044f6c (presubmit successful).