win x86: Grab bag of restructuring to get tests working on x86-on-x86

win x86: Grab bag of restructuring to get tests working on x86-on-x86

A few function implementations that were missing, various switches
for functions/functionality that didn't exist on XP, and far too long
figuring out what exactly was wrong with SYSTEM_PROCESS_INFORMATION
on x86 (the "alignment_for_x86" fields).

R=mark@chromium.org
BUG=crashpad:1, crashpad:50, chromium:531663
Committed: https://chromium.googlesource.com/crashpad/crashpad/+/8ce88d89536615bcc332859fcd7ba00e43cd80db

[scottmg, 2015-09-15 21:08:00]

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

[scottmg, 2015-09-15 21:13:35]

(There's not much tying this CL together except the end goal. If you think it'd
help review, I can try to split it into a few pieces.)

A few notes inline.

https://codereview.chromium.org/1336823002/diff/180001/snapshot/win/pe_image_...
File snapshot/win/pe_image_reader_test.cc (right):

https://codereview.chromium.org/1336823002/diff/180001/snapshot/win/pe_image_...
snapshot/win/pe_image_reader_test.cc:29: BOOL
CrashpadGetModuleInformation(HANDLE process,
This craziness is because XP -> 7+ did some weird things moving this function
around:
https://msdn.microsoft.com/en-us/library/windows/desktop/ms683201(v=vs.85).aspx

https://codereview.chromium.org/1336823002/diff/180001/snapshot/win/process_r...
File snapshot/win/process_reader_win.cc (right):

https://codereview.chromium.org/1336823002/diff/180001/snapshot/win/process_r...
snapshot/win/process_reader_win.cc:160: template <class Traits>
On XP, the "extended" version of this structure doesn't get a filled out
TebBase. So, switch to a more "normal" approach of just getting the thread
handle from the process enumeration, then using NtQueryInformationThread to get
the stack range.

https://codereview.chromium.org/1336823002/diff/180001/snapshot/win/process_r...
File snapshot/win/process_reader_win_test.cc (right):

https://codereview.chromium.org/1336823002/diff/180001/snapshot/win/process_r...
snapshot/win/process_reader_win_test.cc:109: EXPECT_EQ(GetCurrentThreadId(),
threads[0].id);
No GetThreadId() on XP.

https://codereview.chromium.org/1336823002/diff/180001/snapshot/win/process_r...
snapshot/win/process_reader_win_test.cc:176: // Create three dummy threads so we
can confirm we read successfully read
The size of the thread data structure was wrong on x86 and so only the first
thread was read successfully. So this is adding multiple threads to catch that
case.

https://codereview.chromium.org/1336823002/diff/180001/test/win/win_child_pro...
File test/win/win_child_process_test.cc (right):

https://codereview.chromium.org/1336823002/diff/180001/test/win/win_child_pro...
test/win/win_child_process_test.cc:53: return testing::Test::HasFailure() ?
EXIT_FAILURE : EXIT_SUCCESS;
Was ignoring the EXPECT failures in the child process otherwise.

[Mark Mentovai, 2015-09-16 02:57:20]

The grab bag approach is fine.

https://codereview.chromium.org/1336823002/diff/180001/snapshot/win/cpu_conte...
File snapshot/win/cpu_context_win.cc (right):

https://codereview.chromium.org/1336823002/diff/180001/snapshot/win/cpu_conte...
snapshot/win/cpu_context_win.cc:1: // Copyright 2015 The Crashpad Authors. All
rights reserved.
In the change description, BUG=chromium:531663 so that it makes more sense when
it shows up in Crashpad’s history.

https://codereview.chromium.org/1336823002/diff/180001/snapshot/win/cpu_conte...
snapshot/win/cpu_context_win.cc:72: out->eax = context.Eax;
It occurs to me that we should consult context.ContextFlags, both here and in
the X64 function, to make sure we’re only setting things to valid data. If bit
we need in ContextFlags is clear, we should probably write zeroes to |out| for
the fields that it controls.

We may actually want to add validity fields to CPUContextX86 and
CPUContextX86_64 too, otherwise, a minidump reader would think that all of those
zeroes are actual register data.

https://codereview.chromium.org/1336823002/diff/180001/snapshot/win/cpu_conte...
snapshot/win/cpu_context_win.cc:94: //
http://en.wikipedia.org/wiki/X86_debug_register.
For here and the x86 funciton: use https in links when it works, as it does
here. I would have referenced the Intel SDM for this as it’s more authoritative
than Wikipedia, but we have that reference in minidump_context.h already so I
guess this is all right.

https://codereview.chromium.org/1336823002/diff/180001/snapshot/win/cpu_conte...
snapshot/win/cpu_context_win.cc:102: memcpy(&out->fxsave,
&context.ExtendedRegisters, sizeof(out->fxsave));
ExtendedRegisters won’t be valid on a CPU that doesn’t support fxsave (meaning
those that don’t support SSE), but there will still be valid x87 state in
FloatSave. In that case, ContextFlags won’t have CONTEXT_EXTENDED_REGISTERS but
will have CONTEXT_FLOATING_POINT. When this happens, we should fill out as much
of out->fxsave as we can given the x87 data, and zero out the rest. Most of this
is straightforward, but we’ll need to add an FsaveToFxsaveTagWord() function.
That part is a little involved and it’s OK to do in its own change if you’d
like. Fortunately, I already wrote the inverse function, which is much more
complex. Going from the full fsave to the abridged fxsave tag word is easier
because you don’t need to interpret the x87 st registers at all, but you will
need to map between physical and logical slots based on fsw.

https://codereview.chromium.org/1336823002/diff/180001/snapshot/win/exception...
File snapshot/win/exception_snapshot_win.cc (right):

https://codereview.chromium.org/1336823002/diff/180001/snapshot/win/exception...
snapshot/win/exception_snapshot_win.cc:110: EXCEPTION_RECORD32 first_record;
This is so similar to what’s above that it’s kind of unfortunate to have so much
duplication. The TODO for the WoW64 case above will probably be a third
nearly-identical branch. Is there a decent way to share? Templatized helper
function with traits?

https://codereview.chromium.org/1336823002/diff/180001/snapshot/win/exception...
snapshot/win/exception_snapshot_win.cc:130: // We assume 64-on-64 here in that
we're relying on the CONTEXT definition
Well, the comment shouldn’t have been copied and pasted as-is.

https://codereview.chromium.org/1336823002/diff/180001/snapshot/win/pe_image_...
File snapshot/win/pe_image_reader_test.cc (right):

https://codereview.chromium.org/1336823002/diff/180001/snapshot/win/pe_image_...
snapshot/win/pe_image_reader_test.cc:29: BOOL
CrashpadGetModuleInformation(HANDLE process,
scottmg wrote:
> This craziness is because XP -> 7+ did some weird things moving this function
> around:
>
https://msdn.microsoft.com/en-us/library/windows/desktop/ms683201(v=vs.85).aspx

What you wrote looks right, but that document appears to say the opposite.

https://codereview.chromium.org/1336823002/diff/180001/snapshot/win/pe_image_...
snapshot/win/pe_image_reader_test.cc:33: static
decltype(::GetModuleInformation)* get_module_information =
Usually no :: in Crashpad, here and on the next line.

https://codereview.chromium.org/1336823002/diff/180001/snapshot/win/process_r...
File snapshot/win/process_reader_win.cc (right):

https://codereview.chromium.org/1336823002/diff/180001/snapshot/win/process_r...
snapshot/win/process_reader_win.cc:61: const int kSystemProcessInformation = 5;
Before this, kSystemExtendedProcessInformation had documentary value because it
wasn’t present in any system header. Now, it seems that you can use
SystemProcessInformation from <winternl.h>.

https://codereview.chromium.org/1336823002/diff/180001/snapshot/win/process_r...
snapshot/win/process_reader_win.cc:63:
static_cast<SYSTEM_INFORMATION_CLASS>(kSystemProcessInformation),
And then you won’t need a cast anymore!

https://codereview.chromium.org/1336823002/diff/180001/snapshot/win/process_r...
snapshot/win/process_reader_win.cc:161: void ReadThreadData(ProcessReaderWin*
process_reader) {
This belongs the unnamed namespace.

https://codereview.chromium.org/1336823002/diff/180001/snapshot/win/process_r...
snapshot/win/process_reader_win.cc:180:
FillThreadContextAndSuspendCount<Traits>(
(This and the next comment describe things found in the original code too.)

Maybe this should be bool-returning so that you can continue if it fails, like
with other failures in this loop.

https://codereview.chromium.org/1336823002/diff/180001/snapshot/win/process_r...
snapshot/win/process_reader_win.cc:209: // Note, "backwards" because of
direction of stack growth.
Might be good to actually check that limit <= base then, since you’re sticking
the result of subtraction into an unsigned variable.

https://codereview.chromium.org/1336823002/diff/180001/snapshot/win/process_r...
File snapshot/win/process_reader_win_test.cc (right):

https://codereview.chromium.org/1336823002/diff/180001/snapshot/win/process_r...
snapshot/win/process_reader_win_test.cc:109: EXPECT_EQ(GetCurrentThreadId(),
threads[0].id);
scottmg wrote:
> No GetThreadId() on XP.

This is better anyway.

https://codereview.chromium.org/1336823002/diff/180001/snapshot/win/process_r...
snapshot/win/process_reader_win_test.cc:154: ASSERT_GE(threads.size(),
static_cast<unsigned int>(kCreatedThreads + 1));
enum : unsigned int should eliminate the need for this cast.

https://codereview.chromium.org/1336823002/diff/180001/snapshot/win/process_r...
snapshot/win/process_reader_win_test.cc:192: for (int i = 0; i <
kCreatedThreads; ++i)
This time, arraysize(threads).

https://codereview.chromium.org/1336823002/diff/180001/util/util_test.gyp
File util/util_test.gyp (right):

https://codereview.chromium.org/1336823002/diff/180001/util/util_test.gyp#new...
util/util_test.gyp:129: 'MinimumRequiredVersion': '5.02',  # Server 2003.
Is this a good thing to set on other targets? Do we need to set something on the
32-bit target below?

https://codereview.chromium.org/1336823002/diff/180001/util/win/process_info.cc
File util/win/process_info.cc (left):

https://codereview.chromium.org/1336823002/diff/180001/util/win/process_info....
util/win/process_info.cc:1: // Copyright 2015 The Crashpad Authors. All rights
reserved.
Pretty sure we still own this file.

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

https://codereview.chromium.org/1336823002/diff/180001/util/win/process_info....
util/win/process_info.cc:140: *peb_address =
process_basic_information.PebBaseAddress;
if (!is_wow64) {
  // this assignment
} else {
  // call NtQueryInformationProcess() again
}

https://codereview.chromium.org/1336823002/diff/180001/util/win/process_info....
util/win/process_info.cc:144: ProcessWow64Information,
It’s a little weird that the thing named this way and documented as returning
nonzero for a WoW64 process actually gives you back the 32-bit PEB here. But
this exploration into WoW64 internals has taught me that the whole system is a
little weird.

https://codereview.chromium.org/1336823002/diff/180001/util/win/process_info....
util/win/process_info.cc:166: 
Unnecessary new blank line.

https://codereview.chromium.org/1336823002/diff/180001/util/win/process_info....
util/win/process_info.cc:169: LOG(ERROR) << "peb address" << peb_address_vmaddr
<< " out of range";
Need a space after the word “address”.

peb_address_vmaddr will probably be formatted as a decimal number, you may want
to use base::StringPrintf() here.

https://codereview.chromium.org/1336823002/diff/180001/util/win/process_info....
util/win/process_info.cc:273: bool self_is_wow64 =
IsProcessWow64(GetCurrentProcess());
Don’t declare it ’til you need it. Since you only use it once, you can just
write it inline in the compound expression below, to avoid having to make these
calls unless they’re required.

https://codereview.chromium.org/1336823002/diff/180001/util/win/process_info....
util/win/process_info.cc:296: (is_64_bit_ || (is_wow64_ && !self_is_wow64))
This condition makes my head hurt. It seems like it should be simpler.

[scottmg, 2015-09-16 18:02:10]

Patchset #11 (id:200001) has been deleted

[scottmg, 2015-09-16 18:05:07]

Thanks!

https://codereview.chromium.org/1336823002/diff/180001/snapshot/win/cpu_conte...
File snapshot/win/cpu_context_win.cc (right):

https://codereview.chromium.org/1336823002/diff/180001/snapshot/win/cpu_conte...
snapshot/win/cpu_context_win.cc:1: // Copyright 2015 The Crashpad Authors. All
rights reserved.
On 2015/09/16 02:57:19, Mark Mentovai - August is over wrote:
> In the change description, BUG=chromium:531663 so that it makes more sense
when
> it shows up in Crashpad’s history.

Done.

https://codereview.chromium.org/1336823002/diff/180001/snapshot/win/cpu_conte...
snapshot/win/cpu_context_win.cc:72: out->eax = context.Eax;
On 2015/09/16 02:57:19, Mark Mentovai - August is over wrote:
> It occurs to me that we should consult context.ContextFlags, both here and in
> the X64 function, to make sure we’re only setting things to valid data. If bit
> we need in ContextFlags is clear, we should probably write zeroes to |out| for
> the fields that it controls.

Done.

> 
> We may actually want to add validity fields to CPUContextX86 and
> CPUContextX86_64 too, otherwise, a minidump reader would think that all of
those
> zeroes are actual register data.

Maybe a follow up for this to make sure we have something makes sense for both
OS X and Windows.

https://codereview.chromium.org/1336823002/diff/180001/snapshot/win/cpu_conte...
snapshot/win/cpu_context_win.cc:94: //
http://en.wikipedia.org/wiki/X86_debug_register.
On 2015/09/16 02:57:19, Mark Mentovai - August is over wrote:
> For here and the x86 funciton: use https in links when it works, as it does
> here. I would have referenced the Intel SDM for this as it’s more
authoritative
> than Wikipedia, but we have that reference in minidump_context.h already so I
> guess this is all right.

Done.

https://codereview.chromium.org/1336823002/diff/180001/snapshot/win/cpu_conte...
snapshot/win/cpu_context_win.cc:102: memcpy(&out->fxsave,
&context.ExtendedRegisters, sizeof(out->fxsave));
On 2015/09/16 02:57:19, Mark Mentovai - August is over wrote:
> ExtendedRegisters won’t be valid on a CPU that doesn’t support fxsave (meaning
> those that don’t support SSE), but there will still be valid x87 state in
> FloatSave. In that case, ContextFlags won’t have CONTEXT_EXTENDED_REGISTERS
but
> will have CONTEXT_FLOATING_POINT. When this happens, we should fill out as
much
> of out->fxsave as we can given the x87 data, and zero out the rest. Most of
this
> is straightforward, but we’ll need to add an FsaveToFxsaveTagWord() function.
> That part is a little involved and it’s OK to do in its own change if you’d
> like. Fortunately, I already wrote the inverse function, which is much more
> complex. Going from the full fsave to the abridged fxsave tag word is easier
> because you don’t need to interpret the x87 st registers at all, but you will
> need to map between physical and logical slots based on fsw.

Ah yes, good point. I CHECKd in that case for now, and I'll take a look
separately.

https://codereview.chromium.org/1336823002/diff/180001/snapshot/win/exception...
File snapshot/win/exception_snapshot_win.cc (right):

https://codereview.chromium.org/1336823002/diff/180001/snapshot/win/exception...
snapshot/win/exception_snapshot_win.cc:110: EXCEPTION_RECORD32 first_record;
On 2015/09/16 02:57:19, Mark Mentovai - August is over wrote:
> This is so similar to what’s above that it’s kind of unfortunate to have so
much
> duplication. The TODO for the WoW64 case above will probably be a third
> nearly-identical branch. Is there a decent way to share? Templatized helper
> function with traits?

Done.

https://codereview.chromium.org/1336823002/diff/180001/snapshot/win/exception...
snapshot/win/exception_snapshot_win.cc:130: // We assume 64-on-64 here in that
we're relying on the CONTEXT definition
On 2015/09/16 02:57:19, Mark Mentovai - August is over wrote:
> Well, the comment shouldn’t have been copied and pasted as-is.

Oops, I noticed that too, but forgot to upload the last snapshot. Anyway,
templated now.

https://codereview.chromium.org/1336823002/diff/180001/snapshot/win/pe_image_...
File snapshot/win/pe_image_reader_test.cc (right):

https://codereview.chromium.org/1336823002/diff/180001/snapshot/win/pe_image_...
snapshot/win/pe_image_reader_test.cc:29: BOOL
CrashpadGetModuleInformation(HANDLE process,
On 2015/09/16 02:57:19, Mark Mentovai - August is over wrote:
> scottmg wrote:
> > This craziness is because XP -> 7+ did some weird things moving this
function
> > around:
> >
>
https://msdn.microsoft.com/en-us/library/windows/desktop/ms683201(v=vs.85).aspx
> 
> What you wrote looks right, but that document appears to say the opposite.

As far as I can tell the K32... goop applies only to static linking, so only the
last sentence of the Remarks applies here -- that is, load psapi.dll and use the
function as usual.

/exports of xp, win10-x64 and win10-x86 here:
https://gist.github.com/sgraham/442af65edb21ecf24089

https://codereview.chromium.org/1336823002/diff/180001/snapshot/win/pe_image_...
snapshot/win/pe_image_reader_test.cc:33: static
decltype(::GetModuleInformation)* get_module_information =
On 2015/09/16 02:57:19, Mark Mentovai - August is over wrote:
> Usually no :: in Crashpad, here and on the next line.

Oops, I was playing with trying to call this function just
"GetModuleInformation" but couldn't get it to work. Done.

https://codereview.chromium.org/1336823002/diff/180001/snapshot/win/process_r...
File snapshot/win/process_reader_win.cc (right):

https://codereview.chromium.org/1336823002/diff/180001/snapshot/win/process_r...
snapshot/win/process_reader_win.cc:61: const int kSystemProcessInformation = 5;
On 2015/09/16 02:57:19, Mark Mentovai - August is over wrote:
> Before this, kSystemExtendedProcessInformation had documentary value because
it
> wasn’t present in any system header. Now, it seems that you can use
> SystemProcessInformation from <winternl.h>.

Aha, good catch, done.

https://codereview.chromium.org/1336823002/diff/180001/snapshot/win/process_r...
snapshot/win/process_reader_win.cc:63:
static_cast<SYSTEM_INFORMATION_CLASS>(kSystemProcessInformation),
On 2015/09/16 02:57:19, Mark Mentovai - August is over wrote:
> And then you won’t need a cast anymore!

Done.

https://codereview.chromium.org/1336823002/diff/180001/snapshot/win/process_r...
snapshot/win/process_reader_win.cc:161: void ReadThreadData(ProcessReaderWin*
process_reader) {
On 2015/09/16 02:57:19, Mark Mentovai - August is over wrote:
> This belongs the unnamed namespace.

It wasn't because it needed a friend declaration in ProcessReaderWin, but it
should have just been a member function anyway, so changed to that.

https://codereview.chromium.org/1336823002/diff/180001/snapshot/win/process_r...
snapshot/win/process_reader_win.cc:180:
FillThreadContextAndSuspendCount<Traits>(
On 2015/09/16 02:57:19, Mark Mentovai - August is over wrote:
> (This and the next comment describe things found in the original code too.)
> 
> Maybe this should be bool-returning so that you can continue if it fails, like
> with other failures in this loop.

Done.

https://codereview.chromium.org/1336823002/diff/180001/snapshot/win/process_r...
snapshot/win/process_reader_win.cc:209: // Note, "backwards" because of
direction of stack growth.
On 2015/09/16 02:57:19, Mark Mentovai - August is over wrote:
> Might be good to actually check that limit <= base then, since you’re sticking
> the result of subtraction into an unsigned variable.

Done.

https://codereview.chromium.org/1336823002/diff/180001/snapshot/win/process_r...
File snapshot/win/process_reader_win_test.cc (right):

https://codereview.chromium.org/1336823002/diff/180001/snapshot/win/process_r...
snapshot/win/process_reader_win_test.cc:154: ASSERT_GE(threads.size(),
static_cast<unsigned int>(kCreatedThreads + 1));
On 2015/09/16 02:57:19, Mark Mentovai - August is over wrote:
> enum : unsigned int should eliminate the need for this cast.

Done.

https://codereview.chromium.org/1336823002/diff/180001/snapshot/win/process_r...
snapshot/win/process_reader_win_test.cc:192: for (int i = 0; i <
kCreatedThreads; ++i)
On 2015/09/16 02:57:19, Mark Mentovai - August is over wrote:
> This time, arraysize(threads).

Done.

https://codereview.chromium.org/1336823002/diff/180001/util/util_test.gyp
File util/util_test.gyp (right):

https://codereview.chromium.org/1336823002/diff/180001/util/util_test.gyp#new...
util/util_test.gyp:129: 'MinimumRequiredVersion': '5.02',  # Server 2003.
On 2015/09/16 02:57:20, Mark Mentovai - August is over wrote:
> Is this a good thing to set on other targets? Do we need to set something on
the
> 32-bit target below?

It's a bit tricky, but build/common.gypi sets most cases correctly, that is:
5.01 for XP on x86, and 5.02 for Server 2003 on x64, based on target_arch.

For this one (x64-during-x86-build) it would be set to 5.01, but that's invalid
for x64 links, so we need to override to 5.02.

Below, when it's x86-during-x86 it'll be set to 5.01 as desired based on
target_arch.

But when it's x86-during-x64-build, it'll be set to 5.02 so that binary won't
run on XP -- but that's OK because neither will the rest of the x64 build that's
targeted a minimum of Server 2003.

https://codereview.chromium.org/1336823002/diff/180001/util/win/process_info.cc
File util/win/process_info.cc (left):

https://codereview.chromium.org/1336823002/diff/180001/util/win/process_info....
util/win/process_info.cc:1: // Copyright 2015 The Crashpad Authors. All rights
reserved.
On 2015/09/16 02:57:20, Mark Mentovai - August is over wrote:
> Pretty sure we still own this file.

Done.

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

https://codereview.chromium.org/1336823002/diff/180001/util/win/process_info....
util/win/process_info.cc:140: *peb_address =
process_basic_information.PebBaseAddress;
On 2015/09/16 02:57:20, Mark Mentovai - August is over wrote:
> if (!is_wow64) {
>   // this assignment
> } else {
>   // call NtQueryInformationProcess() again
> }

Done.

https://codereview.chromium.org/1336823002/diff/180001/util/win/process_info....
util/win/process_info.cc:144: ProcessWow64Information,
On 2015/09/16 02:57:20, Mark Mentovai - August is over wrote:
> It’s a little weird that the thing named this way and documented as returning
> nonzero for a WoW64 process actually gives you back the 32-bit PEB here. But
> this exploration into WoW64 internals has taught me that the whole system is a
> little weird.

Yeah, the limited bit that MSDN documented is confusing. We could probably find
another way to get the WOW64 PEB, but this seems like the closest to
almost-documented.

https://codereview.chromium.org/1336823002/diff/180001/util/win/process_info....
util/win/process_info.cc:166: 
On 2015/09/16 02:57:20, Mark Mentovai - August is over wrote:
> Unnecessary new blank line.

Done.

https://codereview.chromium.org/1336823002/diff/180001/util/win/process_info....
util/win/process_info.cc:169: LOG(ERROR) << "peb address" << peb_address_vmaddr
<< " out of range";
On 2015/09/16 02:57:20, Mark Mentovai - August is over wrote:
> Need a space after the word “address”.
> 
> peb_address_vmaddr will probably be formatted as a decimal number, you may
want
> to use base::StringPrintf() here.

Done.

https://codereview.chromium.org/1336823002/diff/180001/util/win/process_info....
util/win/process_info.cc:273: bool self_is_wow64 =
IsProcessWow64(GetCurrentProcess());
On 2015/09/16 02:57:20, Mark Mentovai - August is over wrote:
> Don’t declare it ’til you need it. Since you only use it once, you can just
> write it inline in the compound expression below, to avoid having to make
these
> calls unless they’re required.

Gone.

https://codereview.chromium.org/1336823002/diff/180001/util/win/process_info....
util/win/process_info.cc:296: (is_64_bit_ || (is_wow64_ && !self_is_wow64))
On 2015/09/16 02:57:20, Mark Mentovai - August is over wrote:
> This condition makes my head hurt. It seems like it should be simpler.

Me too! I think what it really needed to be was based on the host arch.

[Mark Mentovai, 2015-09-16 18:56:58]

LGTM

https://codereview.chromium.org/1336823002/diff/220001/snapshot/win/cpu_conte...
File snapshot/win/cpu_context_win.cc (right):

https://codereview.chromium.org/1336823002/diff/220001/snapshot/win/cpu_conte...
snapshot/win/cpu_context_win.cc:32: 
Maybe also LOG_IF(…, !(context.ContextFlags & CONTEXT_AMD64)), and similar in
the x86 function below.

https://codereview.chromium.org/1336823002/diff/220001/snapshot/win/process_r...
File snapshot/win/process_reader_win.cc (right):

https://codereview.chromium.org/1336823002/diff/220001/snapshot/win/process_r...
snapshot/win/process_reader_win.cc:317: thread.stack_region_address =
tib.StackLimit;
You can do this outside of the conditional.

https://codereview.chromium.org/1336823002/diff/220001/snapshot/win/process_r...
snapshot/win/process_reader_win.cc:321: thread.stack_region_size = tib.StackBase
- tib.StackLimit;
Is this the current size of the stack, the maximum size it’s ever reached, or
the maximum possible? (I know it’s not the first one.)

The Mac ProcessReader::CalculateStackRegion() takes care to not go too far below
esp/rsp, because that would just capture irrelevant garbage which would bloat
minidumps.

No need to deal with that in this change, but it may be suitable for a follow-up
if we find that our minidump files are bigger than we think they should be.

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

https://codereview.chromium.org/1336823002/diff/220001/util/win/process_info....
util/win/process_info.cc:297: #if ARCH_CPU_64_BITS
Much better. #include "build/build_config.h".

[scottmg, 2015-09-16 19:36:27]

Patchset #12 (id:240001) has been deleted

[scottmg, 2015-09-16 19:38:21]

https://codereview.chromium.org/1336823002/diff/220001/snapshot/win/cpu_conte...
File snapshot/win/cpu_context_win.cc (right):

https://codereview.chromium.org/1336823002/diff/220001/snapshot/win/cpu_conte...
snapshot/win/cpu_context_win.cc:32: 
On 2015/09/16 18:56:58, Mark Mentovai - August is over wrote:
> Maybe also LOG_IF(…, !(context.ContextFlags & CONTEXT_AMD64)), and similar in
> the x86 function below.

Done.

(I was confused at first why these don't trigger in the test, but
CONTEXT_INTEGER, etc. all have it or'd in. So it won't work for WOW64 once I get
there.)

https://codereview.chromium.org/1336823002/diff/220001/snapshot/win/process_r...
File snapshot/win/process_reader_win.cc (right):

https://codereview.chromium.org/1336823002/diff/220001/snapshot/win/process_r...
snapshot/win/process_reader_win.cc:317: thread.stack_region_address =
tib.StackLimit;
On 2015/09/16 18:56:58, Mark Mentovai - August is over wrote:
> You can do this outside of the conditional.

Done.

https://codereview.chromium.org/1336823002/diff/220001/snapshot/win/process_r...
snapshot/win/process_reader_win.cc:321: thread.stack_region_size = tib.StackBase
- tib.StackLimit;
On 2015/09/16 18:56:58, Mark Mentovai - August is over wrote:
> Is this the current size of the stack, the maximum size it’s ever reached, or
> the maximum possible? (I know it’s not the first one.)

Right, I think it's the lowest committed page for the stack, so "a bit more than
the maximum size".
 
> The Mac ProcessReader::CalculateStackRegion() takes care to not go too far
below
> esp/rsp, because that would just capture irrelevant garbage which would bloat
> minidumps.

Ooh, that's some tricky stuff. I guess I'd rather err towards capturing (a bit?)
more for now. It's certainly possible it could have grown very large and crashed
later when it was very small, but I don't know how common that would be in
practice.
 
> No need to deal with that in this change, but it may be suitable for a
follow-up
> if we find that our minidump files are bigger than we think they should be.

Filed https://code.google.com/p/crashpad/issues/detail?id=54 for that.

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

https://codereview.chromium.org/1336823002/diff/220001/util/win/process_info....
util/win/process_info.cc:297: #if ARCH_CPU_64_BITS
On 2015/09/16 18:56:58, Mark Mentovai - August is over wrote:
> Much better. #include "build/build_config.h".

Done.

[scottmg, 2015-09-16 19:42:25]

Committed patchset #12 (id:260001) manually as
8ce88d89536615bcc332859fcd7ba00e43cd80db (presubmit successful).

[Mark Mentovai, 2015-09-16 19:44:15]

https://codereview.chromium.org/1336823002/diff/220001/snapshot/win/process_r...
File snapshot/win/process_reader_win.cc (right):

https://codereview.chromium.org/1336823002/diff/220001/snapshot/win/process_r...
snapshot/win/process_reader_win.cc:321: thread.stack_region_size = tib.StackBase
- tib.StackLimit;
scottmg wrote:
> Ooh, that's some tricky stuff. I guess I'd rather err towards capturing (a
bit?)
> more for now. It's certainly possible it could have grown very large and
crashed
> later when it was very small, but I don't know how common that would be in
> practice.

I don’t think it’ll be as tricky on Windows. A lot of the function on Mac is
geared towards working out what the limits of the stack region are. The TIB
gives that to you on Windows, all you’ll need to do is cut into it a little bit
based on the stack pointer.

You’ll still need to deal with red zones (if the Windows ABI has them), but you
won’t need to mess with the memory map like on the Mac.

[scottmg, 2015-09-16 19:50:54]

Oops, apparently crashing on x86 release, looking...

[Mark Mentovai, 2015-09-16 20:52:59]

https://codereview.chromium.org/1336823002/diff/260001/snapshot/win/cpu_conte...
File snapshot/win/cpu_context_win.cc (right):

https://codereview.chromium.org/1336823002/diff/260001/snapshot/win/cpu_conte...
snapshot/win/cpu_context_win.cc:64: // SegDs ignored.
Here’s a question that’s got nothing to do with this code, it’s more about the
CONTEXT structures and WoW64.

Based on some of the things we talked about last week and this week, my
understanding of WoW64 is that the 64-bit kernel roughly doesn’t care about
whether a thread is in 32-bit or 64-bit mode, which is controlled by its choice
of cs. In order for this to work, the context that gets saved and restored needs
to always be the full 64-bit register file, because regardless of whether a
thread has the 32-bit or 64-bit cs in effect, it may have switched in the past
and may switch again in the future.

Now the problem is that the 64-bit CONTEXT structure doesn’t have room for ds,
es, and ss. So what happens if a thread in 32-bit mode modifies those segment
registers and then flips to a 64-bit cs? When it gets preempted, will the
changes it made to the segment registers be lost?

There’s WOW64_CONTEXT which is identical to the 32-bit CONTEXT, but that doesn’t
have the full 64-bit register extensions, so if that’s used to save the state of
a thread found in 32-bit mode, and the thread later flips back to 64, all of the
high bits will be lost along with the full contents of the extra registers.

I figure there are a few possibilities:
 - This actually doesn’t work at all, and if you mess with cs, you’d better not
change ds, es, or ss. (This may be the policy even if you’re not changing cs.)
 - Some structure other than CONTEXT is used internally for preemption.
 - CONTEXT and WOW64_CONTEXT are used depending on whether the process is
supposed to be WoW64, so if you flip to the other-bitted cs and get preempted,
your register file will be in an inconsistent state and you’ll almost certainly
crash as soon as you get a chance to run again. (This can’t be right, because
under WoW64, the Windows libraries themselves flip to 64-bit mode.)
 - Some other part of my mental model of how this works isn’t right.

Do you know anything else about this? I’m trying to square these CONTEXT
structures with mixed 32/64-bit code, which must work somehow.

[scottmg, 2015-09-16 20:59:58]

On 2015/09/16 20:52:59, Mark Mentovai - August is over wrote:
>
https://codereview.chromium.org/1336823002/diff/260001/snapshot/win/cpu_conte...
> File snapshot/win/cpu_context_win.cc (right):
> 
>
https://codereview.chromium.org/1336823002/diff/260001/snapshot/win/cpu_conte...
> snapshot/win/cpu_context_win.cc:64: // SegDs ignored.
> Here’s a question that’s got nothing to do with this code, it’s more about the
> CONTEXT structures and WoW64.
> 
> Based on some of the things we talked about last week and this week, my
> understanding of WoW64 is that the 64-bit kernel roughly doesn’t care about
> whether a thread is in 32-bit or 64-bit mode, which is controlled by its
choice
> of cs. In order for this to work, the context that gets saved and restored
needs
> to always be the full 64-bit register file, because regardless of whether a
> thread has the 32-bit or 64-bit cs in effect, it may have switched in the past
> and may switch again in the future.
> 
> Now the problem is that the 64-bit CONTEXT structure doesn’t have room for ds,
> es, and ss. So what happens if a thread in 32-bit mode modifies those segment
> registers and then flips to a 64-bit cs? When it gets preempted, will the
> changes it made to the segment registers be lost?
> 
> There’s WOW64_CONTEXT which is identical to the 32-bit CONTEXT, but that
doesn’t
> have the full 64-bit register extensions, so if that’s used to save the state
of
> a thread found in 32-bit mode, and the thread later flips back to 64, all of
the
> high bits will be lost along with the full contents of the extra registers.
> 
> I figure there are a few possibilities:
>  - This actually doesn’t work at all, and if you mess with cs, you’d better
not
> change ds, es, or ss. (This may be the policy even if you’re not changing cs.)
>  - Some structure other than CONTEXT is used internally for preemption.
>  - CONTEXT and WOW64_CONTEXT are used depending on whether the process is
> supposed to be WoW64, so if you flip to the other-bitted cs and get preempted,
> your register file will be in an inconsistent state and you’ll almost
certainly
> crash as soon as you get a chance to run again. (This can’t be right, because
> under WoW64, the Windows libraries themselves flip to 64-bit mode.)
>  - Some other part of my mental model of how this works isn’t right.
> 
> Do you know anything else about this? I’m trying to square these CONTEXT
> structures with mixed 32/64-bit code, which must work somehow.

I ... don't know anything about that. :)

The are SegSs, SegDs, and SegEs in the x64 CONTEXT though.

[scottmg, 2015-09-16 21:00:35]

On 2015/09/16 19:50:54, scottmg wrote:
> Oops, apparently crashing on x86 release, looking...

It's crashing in RtlCaptureContext for reasons unclear. I will figure out why,
but maybe it's time to drop in our replacement anyway.

[Mark Mentovai, 2015-09-16 21:23:11]

scottmg wrote:
> I ... don't know anything about that. :)
> 
> The are SegSs, SegDs, and SegEs in the x64 CONTEXT though.

Oh, there they are. Well then that’s how this works.

So it’s the Crashpad struct that’s missing these. OK, we can fix that.
https://code.google.com/p/crashpad/issues/detail?id=55

[scottmg, 2015-09-16 21:30:09]

On 2015/09/16 21:00:35, scottmg wrote:
> On 2015/09/16 19:50:54, scottmg wrote:
> > Oops, apparently crashing on x86 release, looking...
> 
> It's crashing in RtlCaptureContext for reasons unclear. I will figure out why,
> but maybe it's time to drop in our replacement anyway.

(https://codereview.chromium.org/1348893003 resolves this)

[Mark Mentovai, 2015-09-18 03:30:51]

https://codereview.chromium.org/1336823002/diff/260001/snapshot/win/process_r...
File snapshot/win/process_reader_win.cc (right):

https://codereview.chromium.org/1336823002/diff/260001/snapshot/win/process_r...
snapshot/win/process_reader_win.cc:146: if (!GetThreadContext(thread_handle,
&thread->context)) {
I was looking at the documentation for GetThreadContext() for another reason and
noticed a note in the “community additions” section pointing to
http://zachsaw.blogspot.com.au/2010/11/wow64-bug-getthreadcontext-may-return.....

Seems pretty gnarly, but if you work your way to some of the later comments,
maybe it’s not hopeless.

Something to keep in mind…

[scottmg, 2015-09-18 03:50:09]

https://codereview.chromium.org/1336823002/diff/260001/snapshot/win/process_r...
File snapshot/win/process_reader_win.cc (right):

https://codereview.chromium.org/1336823002/diff/260001/snapshot/win/process_r...
snapshot/win/process_reader_win.cc:146: if (!GetThreadContext(thread_handle,
&thread->context)) {
On 2015/09/18 03:30:51, Mark Mentovai - August is over wrote:
> I was looking at the documentation for GetThreadContext() for another reason
and
> noticed a note in the “community additions” section pointing to
>
http://zachsaw.blogspot.com.au/2010/11/wow64-bug-getthreadcontext-may-return.....
> 
> Seems pretty gnarly, but if you work your way to some of the later comments,
> maybe it’s not hopeless.
> 
> Something to keep in mind…

2/3rds of the way through the comment sludge... :)

(Looks like we ought to have gotten alexeypa@ to write this while he was here
too!)

[scottmg, 2015-09-18 04:01:30]

On 2015/09/18 03:50:09, scottmg wrote:
>
https://codereview.chromium.org/1336823002/diff/260001/snapshot/win/process_r...
> File snapshot/win/process_reader_win.cc (right):
> 
>
https://codereview.chromium.org/1336823002/diff/260001/snapshot/win/process_r...
> snapshot/win/process_reader_win.cc:146: if (!GetThreadContext(thread_handle,
> &thread->context)) {
> On 2015/09/18 03:30:51, Mark Mentovai - August is over wrote:
> > I was looking at the documentation for GetThreadContext() for another reason
> and
> > noticed a note in the “community additions” section pointing to
> >
>
http://zachsaw.blogspot.com.au/2010/11/wow64-bug-getthreadcontext-may-return.....
> > 
> > Seems pretty gnarly, but if you work your way to some of the later comments,
> > maybe it’s not hopeless.
> > 
> > Something to keep in mind…
> 
> 2/3rds of the way through the comment sludge... :)
> 
> (Looks like we ought to have gotten alexeypa@ to write this while he was here
> too!)

So, based on the fine comments from on "Anonymous" (sigh) we should be passing
in CONTEXT_EXCEPTION_REQUEST when running as x86 on x64, and then... panicing
(?) if we don't get 0's for CONTEXT_EXCEPTION_ACTIVE and CONTEXT_SERVICE_ACTIVE,
is that your interpretation?

I wonder if Wow64GetThreadContext suffers from this problem -- we could also
disallow by policy running an x86 crash handler on x64 OS.

[Mark Mentovai, 2015-09-18 15:09:49]

scottmg wrote:
> > 2/3rds of the way through the comment sludge... :)

:(

> > (Looks like we ought to have gotten alexeypa@ to write this while he was
here
> > too!)

:)

> So, based on the fine comments from on "Anonymous" (sigh) we should be passing
> in CONTEXT_EXCEPTION_REQUEST when running as x86 on x64, and then... panicing
> (?) if we don't get 0's for CONTEXT_EXCEPTION_ACTIVE and
CONTEXT_SERVICE_ACTIVE,
> is that your interpretation?

Yeah, it was, but now I don’t think that makes sense. At least if
CONTEXT_SERVICE_ACTIVE is set, the thread successfully made it all the way
through the 64-bit transition code in user mode into the kernel, so the WoW64
context should already have been saved, since that’s done in user mode. (I
think.) If CONTEXT_EXCEPTION_ACTIVE is set, control passed straight into kernel
mode without a chance to update the WoW64 context in user mode, but then, there
wasn’t ever any user-mode transition code running so the 64-bit register file
truncated to 32 bits ought to represent the true state of the user program
(assuming it’s got the 32-bit cs).

Basically, I can see how these CONTEXT_EXCEPTION_REPORTING bits are useful for
deciding if it’s safe to call SetThreadContext(), but we don’t care about that.
I don’t really know if they’re going to help us here.

And yeah, even if we could detect that we were getting garbage, I don’t know if
there’s anything we could do other than cry. It’s not like we’re going to try
resuming the thread for a moment to see if we can get it back into 32-bit code
in user mode.

> I wonder if Wow64GetThreadContext suffers from this problem -- we could also
> disallow by policy running an x86 crash handler on x64 OS.

I don’t think the bitness of the handler is what’s relevant, it’s the bitness of
the crashy program.

As I understand it, Wow64GetThreadContext() is just the 32-bit
GetThreadContext() that you get in a program running in WoW64, but exposed to
64-bit code too. We’d need to use Wow64GetThreadContext() for the 64-reads-32
case. Or not, because it was introduced in Vista, but that’s where something
like http://www.nynaeve.net/Code/GetThreadWow64Context.cpp comes in, although I
think I’d do it a little differently when we can get CONTEXT_EXCEPTION_REPORTING
given the above.

Maybe the best thing to do is to just ignore it for now. If there’s really a
window where a thread can flip to the 64-bit cs without first updating its saved
WoW64 context, then the whole thing probably broken beyond our ability to
reasonably deal with. And if the WoW64 MiniDumpWriteDump() is susceptible to the
problem too, then, well…ExternalDependency?

[scottmg, 2015-09-22 18:14:56]

https://codereview.chromium.org/1336823002/diff/180001/snapshot/win/cpu_conte...
File snapshot/win/cpu_context_win.cc (right):

https://codereview.chromium.org/1336823002/diff/180001/snapshot/win/cpu_conte...
snapshot/win/cpu_context_win.cc:102: memcpy(&out->fxsave,
&context.ExtendedRegisters, sizeof(out->fxsave));
On 2015/09/16 02:57:19, Mark Mentovai - August is over wrote:
> ExtendedRegisters won’t be valid on a CPU that doesn’t support fxsave (meaning
> those that don’t support SSE), but there will still be valid x87 state in
> FloatSave. In that case, ContextFlags won’t have CONTEXT_EXTENDED_REGISTERS
but
> will have CONTEXT_FLOATING_POINT. When this happens, we should fill out as
much
> of out->fxsave as we can given the x87 data, and zero out the rest. Most of
this
> is straightforward, but we’ll need to add an FsaveToFxsaveTagWord() function.
> That part is a little involved and it’s OK to do in its own change if you’d
> like. Fortunately, I already wrote the inverse function, which is much more
> complex. Going from the full fsave to the abridged fxsave tag word is easier
> because you don’t need to interpret the x87 st registers at all, but you will
> need to map between physical and logical slots based on fsw.

I just digested this a little better... fxsave became available with SSE (1),
right? I don't think there's much point in supporting pre-SSE hardware given
that VS2013 assumes SSE2, and Chrome has required it for quite a while now:
https://code.google.com/p/chromium/issues/detail?id=349320.

[Mark Mentovai, 2015-09-22 18:23:15]

https://codereview.chromium.org/1336823002/diff/180001/snapshot/win/cpu_conte...
File snapshot/win/cpu_context_win.cc (right):

https://codereview.chromium.org/1336823002/diff/180001/snapshot/win/cpu_conte...
snapshot/win/cpu_context_win.cc:102: memcpy(&out->fxsave,
&context.ExtendedRegisters, sizeof(out->fxsave));
On 2015/09/22 18:14:56, scottmg wrote:
> On 2015/09/16 02:57:19, Mark Mentovai - August is over wrote:
> > ExtendedRegisters won’t be valid on a CPU that doesn’t support fxsave
(meaning
> > those that don’t support SSE), but there will still be valid x87 state in
> > FloatSave. In that case, ContextFlags won’t have CONTEXT_EXTENDED_REGISTERS
> but
> > will have CONTEXT_FLOATING_POINT. When this happens, we should fill out as
> much
> > of out->fxsave as we can given the x87 data, and zero out the rest. Most of
> this
> > is straightforward, but we’ll need to add an FsaveToFxsaveTagWord()
function.
> > That part is a little involved and it’s OK to do in its own change if you’d
> > like. Fortunately, I already wrote the inverse function, which is much more
> > complex. Going from the full fsave to the abridged fxsave tag word is easier
> > because you don’t need to interpret the x87 st registers at all, but you
will
> > need to map between physical and logical slots based on fsw.
> 
> I just digested this a little better... fxsave became available with SSE (1),
> right?

Yup, that’s right.

> I don't think there's much point in supporting pre-SSE hardware given
> that VS2013 assumes SSE2, and Chrome has required it for quite a while now:
> https://code.google.com/p/chromium/issues/detail?id=349320.

Oh, I remember that bug now. I forgot that Chrome required SSE on Windows. I was
remembering some old code in Chrome that had to test for SSE or SSE2 on Windows
and Linux, but was allowed to assume it was available on Mac.