win: Gather memory information

win: Gather memory information

Not yet written as MINIDUMP_MEMORY_INFO_LIST to minidump.

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

[scottmg, 2015-09-25 21:36:51]

Patchset #1 (id:1) has been deleted

[Mark Mentovai, 2015-09-26 02:02:14]

LGTM (unless major changes are needed for 64-reads-32-on-64, then I’ll have
another look).

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

https://codereview.chromium.org/1369833002/diff/40001/util/win/process_info.c...
util/win/process_info.cc:276: uint64_t min_address =
const WinVMAddress?

https://codereview.chromium.org/1369833002/diff/40001/util/win/process_info.c...
util/win/process_info.cc:279:
reinterpret_cast<uint64_t>(system_info.lpMaximumApplicationAddress);
Is this correct for 64-reads-32-on-64?

https://codereview.chromium.org/1369833002/diff/40001/util/win/process_info.c...
util/win/process_info.cc:281: while (address <= max_address) {
for (WinVMAddress address = min_address; address <= max_address; address +=
memory_basic_information.RegionSize)

https://codereview.chromium.org/1369833002/diff/40001/util/win/process_info.c...
util/win/process_info.cc:296: address += memory_basic_information.RegionSize;
DCHECK_GT(RegionSize, 0);

so that a very busted memory map on a very sick system won’t cause this to spin
forever.

https://codereview.chromium.org/1369833002/diff/40001/util/win/process_info_t...
File util/win/process_info_test.cc (right):

https://codereview.chromium.org/1369833002/diff/40001/util/win/process_info_t...
util/win/process_info_test.cc:118: break;
Don’t break. Walk the whole list, and before you set found_region = true, do
EXPECT_FALSE(found_region). That assures that we didn’t goof and put a duplicate
entry into the list.

[scottmg, 2015-09-26 03:12:37]

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

https://codereview.chromium.org/1369833002/diff/40001/util/win/process_info.c...
util/win/process_info.cc:276: uint64_t min_address =
On 2015/09/26 02:02:14, Mark Mentovai - August is over wrote:
> const WinVMAddress?

Done.

https://codereview.chromium.org/1369833002/diff/40001/util/win/process_info.c...
util/win/process_info.cc:279:
reinterpret_cast<uint64_t>(system_info.lpMaximumApplicationAddress);
On 2015/09/26 02:02:14, Mark Mentovai - August is over wrote:
> Is this correct for 64-reads-32-on-64?

Hmm, good point, that's tricky. After playing around with it, the memory map
appears to always be returned for the bitness that you're asking from. So
removed some complexity and just use MEMORY_BASIC_INFORMATION directly when
reading without the templating. This seems directly contradictory to the
implication at the bottom of
https://msdn.microsoft.com/en-us/library/windows/desktop/aa366775(v=vs.85).aspx
but when I tried reading into a MEMORY_BASIC_INFORMATION32 for a 32-process from
an x64 handler it errors with an "incorrect size".

Added to the ProcessInfo.OtherProcess for this case.

https://codereview.chromium.org/1369833002/diff/40001/util/win/process_info.c...
util/win/process_info.cc:281: while (address <= max_address) {
On 2015/09/26 02:02:14, Mark Mentovai - August is over wrote:
> for (WinVMAddress address = min_address; address <= max_address; address +=
> memory_basic_information.RegionSize)

Done.

https://codereview.chromium.org/1369833002/diff/40001/util/win/process_info.c...
util/win/process_info.cc:296: address += memory_basic_information.RegionSize;
On 2015/09/26 02:02:14, Mark Mentovai - August is over wrote:
> DCHECK_GT(RegionSize, 0);
> 
> so that a very busted memory map on a very sick system won’t cause this to
spin
> forever.

Done. (if with LOG rather than DCHECK)

https://codereview.chromium.org/1369833002/diff/40001/util/win/process_info_t...
File util/win/process_info_test.cc (right):

https://codereview.chromium.org/1369833002/diff/40001/util/win/process_info_t...
util/win/process_info_test.cc:118: break;
On 2015/09/26 02:02:14, Mark Mentovai - August is over wrote:
> Don’t break. Walk the whole list, and before you set found_region = true, do
> EXPECT_FALSE(found_region). That assures that we didn’t goof and put a
duplicate
> entry into the list.

Done.

[Mark Mentovai, 2015-09-26 03:36:36]

LGTM, but the 64-reads-32 behavior still seems a little fishy to me if I’m
understanding correctly.

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

https://codereview.chromium.org/1369833002/diff/60001/util/win/process_info.c...
util/win/process_info.cc:268: size_t result = VirtualQueryEx(process,
So if you ask about a 32-bit process from a 64-bit process, you’ll get back a
big free region if you ask about what’s at, say, 0x100000000?

Do we really want that nonsense to show up in the minidump?

https://codereview.chromium.org/1369833002/diff/60001/util/win/process_info.c...
util/win/process_info.cc:307: region_size(mbi.RegionSize),
You need casts for the address fields but not the size ones?

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

https://codereview.chromium.org/1369833002/diff/60001/util/win/process_info.h...
util/win/process_info.h:55: MemoryInfo();
Is this form needed because of the vector?

[Mark Mentovai, 2015-09-26 03:55:32]

Actually, given what we discussed last week about how WoW64 works, the
64-reads-32 behavior kind of makes total sense. A WoW64 process still does have
a full 64-bit address space, but when it’s running from a 32-bit cs, it can’t
see the extended portion.

In that sense, VirtualQueryEx() is a little fishy in that it needs to lie by
truncating the address space in 32-reads-32-on-64.

So I guess everything’s OK here. It’s a little weird to see 64-bit addresses in
play for a 32-bit process, but given what’s actually going on under the hood
with how WoW64 is implemented, the process isn’t really purely 32 bits, and
these wide addresses aren’t exactly wrong.

[scottmg, 2015-09-26 04:08:53]

It's a bit weird, but it kind of makes sense I think. I'll investigate this more
once I add the plumbing through to the minidump anyway, so I think I'll land
like this for now.

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

https://codereview.chromium.org/1369833002/diff/60001/util/win/process_info.c...
util/win/process_info.cc:268: size_t result = VirtualQueryEx(process,
On 2015/09/26 03:36:36, Mark Mentovai - August is over wrote:
> So if you ask about a 32-bit process from a 64-bit process, you’ll get back a
> big free region if you ask about what’s at, say, 0x100000000?
> 
> Do we really want that nonsense to show up in the minidump?

Yeah, there's one very big region at:

		base_address	0x000000007fff0000	unsigned __int64
		region_size	0x00007ffe92850000	unsigned __int64

in running ProcessInfo.OtherProcessWOW64 on my machine. There's 96 regions. The
majority (88) are < 0x7fff`0000, and then there's a handful at the top of the
x64 address space. I guess those are the WOW64 code and its stack. I think as
far as making !vprot work in windbg it should be sufficient, but I won't know
for sure until they're actually plumbed all the way through to the minidump.

https://codereview.chromium.org/1369833002/diff/60001/util/win/process_info.c...
util/win/process_info.cc:307: region_size(mbi.RegionSize),
On 2015/09/26 03:36:36, Mark Mentovai - August is over wrote:
> You need casts for the address fields but not the size ones?

Yeah, the size is a SIZE_T, but the addresses are void*.

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

https://codereview.chromium.org/1369833002/diff/60001/util/win/process_info.h...
util/win/process_info.h:55: MemoryInfo();
On 2015/09/26 03:36:36, Mark Mentovai - August is over wrote:
> Is this form needed because of the vector?

No, just habit. Removed.

[scottmg, 2015-09-26 04:10:38]

On 2015/09/26 03:55:32, Mark Mentovai - August is over wrote:
> Actually, given what we discussed last week about how WoW64 works, the
> 64-reads-32 behavior kind of makes total sense. A WoW64 process still does
have
> a full 64-bit address space, but when it’s running from a 32-bit cs, it can’t
> see the extended portion.
> 
> In that sense, VirtualQueryEx() is a little fishy in that it needs to lie by
> truncating the address space in 32-reads-32-on-64.
> 
> So I guess everything’s OK here. It’s a little weird to see 64-bit addresses
in
> play for a 32-bit process, but given what’s actually going on under the hood
> with how WoW64 is implemented, the process isn’t really purely 32 bits, and
> these wide addresses aren’t exactly wrong.

Exactly. It's most confusing that the documentation is actively misleading. I've
only tested on Win10, I'm wondering if on other OSs it behaves differently
somehow, and perhaps it only gives you the 32-sized data when querying a 32 app.

[scottmg, 2015-09-26 04:11:09]

Committed patchset #4 (id:10003) manually as
56c8359b2765c27b2af9fcc528d841a1106c8edf (presubmit successful).