win: support x64 reading x86 (wow64)

win: support x64 reading x86 (wow64)

Removes the bitness-specific targets in favour of pulling binaries from
the other build directory. This is to avoid the added complexity of
duplicating all the targets for the x86 in x64 build.

Overall, mostly templatizing more functions to support the
wow64-flavoured structures. The only additional functionality required
is reading the x86 TEB that's chained from the x64 TEB when running
as WOW64.

The crashing child test was switched to a manual CreateProcess because
it needs to launch a binary other than itself.


R=mark@chromium.org
BUG=crashpad:50
Committed: https://chromium.googlesource.com/crashpad/crashpad/+/bf556829d90992dfb777d8294f5c19e56527eaa2

[Mark Mentovai, 2015-09-18 15:44:07]

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

https://codereview.chromium.org/1349313003/diff/120001/snapshot/win/cpu_conte...
snapshot/win/cpu_context_win.cc:26: template <int Control,
These flags have the same value regardless of whether you’re using the WOW64_*
names in 64-bit or the native names in 32-bit. Maybe we should dump the extra
parameters and have our own constants. Or if we can see the WOW64_* macros even
in 32-bit mode, maybe just use those.

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

https://codereview.chromium.org/1349313003/diff/120001/snapshot/win/exception...
snapshot/win/exception_snapshot_win.cc:61: using ExceptionPointers64 =
Goes in ARCH_CPU_64_BITS?

https://codereview.chromium.org/1349313003/diff/120001/snapshot/win/exception...
snapshot/win/exception_snapshot_win.cc:91: ExceptionPointers32
exception_pointers;
It’s only a small amount of duplication, but it’d be nice to share a little
more.

#if defined(ARCH_CPU_32_BITS)
  const bool is_64_bit = false;
  using NativeContext32 = CONTEXT;
#elif defined(ARCH_CPU_64_BITS)
  const bool is_64_bit = process_reader->Is64Bit();
  using NativeContext32 = WOW64_CONTEXT;
  if (is_64_bit) {
    ExceptionPointers64 …
  }
#endif
  if (!is_64_bit) {
    ExceptionPointers32 …
    CommonX86Initialize<NativeContext32>(…)
  }

but maybe we can get away with using WOW64_CONTEXT even for the native 32-bit
case?

https://codereview.chromium.org/1349313003/diff/120001/snapshot/win/exception...
File snapshot/win/exception_snapshot_win_test.cc (right):

https://codereview.chromium.org/1349313003/diff/120001/snapshot/win/exception...
snapshot/win/exception_snapshot_win_test.cc:60: void set_break_near(uint64_t
break_near) { break_near_ = break_near; }
WinVMAddress for the argument and the member.

https://codereview.chromium.org/1349313003/diff/120001/snapshot/win/exception...
snapshot/win/exception_snapshot_win_test.cc:196: uint64_t break_near_address;
And this.

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

https://codereview.chromium.org/1349313003/diff/120001/snapshot/win/process_r...
snapshot/win/process_reader_win.cc:130: DCHECK(!is_wow64);
Why not?

(After reading more of this file) Oh, I see, you force is_wow64 to false for
ARCH_CPU_32_BIT.

That’s kind of confusing…I see what you’re doing, but maybe the variable needs
to be renamed. Someone looking at this function in isolation isn’t going to get
it.

https://codereview.chromium.org/1349313003/diff/120001/snapshot/win/process_r...
snapshot/win/process_reader_win.cc:146: memset(&thread->context_wow64, 0,
sizeof(thread->context_wow64));
Maybe give the union a name so that you can memset the whole thing to 0 outside
of any conditional.

https://codereview.chromium.org/1349313003/diff/120001/snapshot/win/process_r...
snapshot/win/process_reader_win.cc:148: if
(!Wow64GetThreadContext(thread_handle, &thread->context_wow64)) {
Isn’t this Vista and later?

https://codereview.chromium.org/1349313003/diff/120001/snapshot/win/process_r...
snapshot/win/process_reader_win.cc:160: #if defined(ARCH_CPU_64_BITS)
Maybe do the const bool thing I suggested before to avoid the preprocessor
funnies and this extra #ifdef just for a }.

https://codereview.chromium.org/1349313003/diff/120001/snapshot/win/process_r...
snapshot/win/process_reader_win.cc:178: context_wow64(),
If the union is named, you should just initialize the whole thing once.

Maybe a better structure is

  union {
    CONTEXT native;
#if defined(ARCH_CPU_64_BITS)
    WOW64_CONTEXT wow64;
#endif
  } context;

https://codereview.chromium.org/1349313003/diff/120001/snapshot/win/process_r...
snapshot/win/process_reader_win.cc:261: #if ARCH_CPU_64_BITS
defined()

https://codereview.chromium.org/1349313003/diff/120001/snapshot/win/process_r...
File snapshot/win/process_reader_win.h (right):

https://codereview.chromium.org/1349313003/diff/120001/snapshot/win/process_r...
snapshot/win/process_reader_win.h:53: CONTEXT context_native;
Fix indentation.

https://codereview.chromium.org/1349313003/diff/120001/util/win/process_struc...
File util/win/process_structs.h (right):

https://codereview.chromium.org/1349313003/diff/120001/util/win/process_struc...
util/win/process_structs.h:288: // See
https://msdn.microsoft.com/en-us/library/dn424783.aspx.
But the rest of NT_TIB isn’t right for WoW64 either, is it? Maybe this:

  union {
    struct {
      ExceptionList;
      StackBase;
    };
    Wow64Teb;
  };

[scottmg, 2015-09-18 19:44:35]

Patchset #8 (id:140001) has been deleted

[scottmg, 2015-09-18 19:45:26]

Thanks

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

https://codereview.chromium.org/1349313003/diff/120001/snapshot/win/cpu_conte...
snapshot/win/cpu_context_win.cc:26: template <int Control,
On 2015/09/18 15:44:06, Mark Mentovai - August is over wrote:
> These flags have the same value regardless of whether you’re using the WOW64_*
> names in 64-bit or the native names in 32-bit. Maybe we should dump the extra
> parameters and have our own constants. Or if we can see the WOW64_* macros
even
> in 32-bit mode, maybe just use those.

Ah, I noticed they were identical, but not that the WOW64_* versions were
available when compiling as x86. Done.

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

https://codereview.chromium.org/1349313003/diff/120001/snapshot/win/exception...
snapshot/win/exception_snapshot_win.cc:61: using ExceptionPointers64 =
On 2015/09/18 15:44:06, Mark Mentovai - August is over wrote:
> Goes in ARCH_CPU_64_BITS?

I moved them to process_structs because they're basically just "missing" as
compared with the availability of EXCEPTION_RECORD32/64, etc.

https://codereview.chromium.org/1349313003/diff/120001/snapshot/win/exception...
snapshot/win/exception_snapshot_win.cc:91: ExceptionPointers32
exception_pointers;
On 2015/09/18 15:44:06, Mark Mentovai - August is over wrote:
> It’s only a small amount of duplication, but it’d be nice to share a little
> more.
> 
> #if defined(ARCH_CPU_32_BITS)
>   const bool is_64_bit = false;
>   using NativeContext32 = CONTEXT;
> #elif defined(ARCH_CPU_64_BITS)
>   const bool is_64_bit = process_reader->Is64Bit();
>   using NativeContext32 = WOW64_CONTEXT;
>   if (is_64_bit) {
>     ExceptionPointers64 …
>   }
> #endif
>   if (!is_64_bit) {
>     ExceptionPointers32 …
>     CommonX86Initialize<NativeContext32>(…)
>   }
> 
> but maybe we can get away with using WOW64_CONTEXT even for the native 32-bit
> case?

Done, and that made it clearer how to collapse the two helper functions too.

I didn't use WOW64_CONTEXT on x86, they're the same of course, but it just feels
wrong and only saves a couple using lines.

https://codereview.chromium.org/1349313003/diff/120001/snapshot/win/exception...
File snapshot/win/exception_snapshot_win_test.cc (right):

https://codereview.chromium.org/1349313003/diff/120001/snapshot/win/exception...
snapshot/win/exception_snapshot_win_test.cc:60: void set_break_near(uint64_t
break_near) { break_near_ = break_near; }
On 2015/09/18 15:44:06, Mark Mentovai - August is over wrote:
> WinVMAddress for the argument and the member.

Done.

https://codereview.chromium.org/1349313003/diff/120001/snapshot/win/exception...
snapshot/win/exception_snapshot_win_test.cc:196: uint64_t break_near_address;
On 2015/09/18 15:44:06, Mark Mentovai - August is over wrote:
> And this.

Done.

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

https://codereview.chromium.org/1349313003/diff/120001/snapshot/win/process_r...
snapshot/win/process_reader_win.cc:130: DCHECK(!is_wow64);
On 2015/09/18 15:44:07, Mark Mentovai - August is over wrote:
> Why not?
> 
> (After reading more of this file) Oh, I see, you force is_wow64 to false for
> ARCH_CPU_32_BIT.
> 
> That’s kind of confusing…I see what you’re doing, but maybe the variable needs
> to be renamed. Someone looking at this function in isolation isn’t going to
get
> it.

I'm not quite sure what you mean, is that a better name?

https://codereview.chromium.org/1349313003/diff/120001/snapshot/win/process_r...
snapshot/win/process_reader_win.cc:146: memset(&thread->context_wow64, 0,
sizeof(thread->context_wow64));
On 2015/09/18 15:44:07, Mark Mentovai - August is over wrote:
> Maybe give the union a name so that you can memset the whole thing to 0
outside
> of any conditional.

Done.

https://codereview.chromium.org/1349313003/diff/120001/snapshot/win/process_r...
snapshot/win/process_reader_win.cc:148: if
(!Wow64GetThreadContext(thread_handle, &thread->context_wow64)) {
On 2015/09/18 15:44:06, Mark Mentovai - August is over wrote:
> Isn’t this Vista and later?

Yes, but it's in ARCH_CPU_64_BITS, and we've never pretended to try to support
WinXP x64.

https://codereview.chromium.org/1349313003/diff/120001/snapshot/win/process_r...
snapshot/win/process_reader_win.cc:160: #if defined(ARCH_CPU_64_BITS)
On 2015/09/18 15:44:06, Mark Mentovai - August is over wrote:
> Maybe do the const bool thing I suggested before to avoid the preprocessor
> funnies and this extra #ifdef just for a }.

Is that what you were thinking?

https://codereview.chromium.org/1349313003/diff/120001/snapshot/win/process_r...
snapshot/win/process_reader_win.cc:178: context_wow64(),
On 2015/09/18 15:44:07, Mark Mentovai - August is over wrote:
> If the union is named, you should just initialize the whole thing once.
> 
> Maybe a better structure is
> 
>   union {
>     CONTEXT native;
> #if defined(ARCH_CPU_64_BITS)
>     WOW64_CONTEXT wow64;
> #endif
>   } context;

Done.

https://codereview.chromium.org/1349313003/diff/120001/snapshot/win/process_r...
snapshot/win/process_reader_win.cc:261: #if ARCH_CPU_64_BITS
On 2015/09/18 15:44:06, Mark Mentovai - August is over wrote:
> defined()

Done.

https://codereview.chromium.org/1349313003/diff/120001/snapshot/win/process_r...
File snapshot/win/process_reader_win.h (right):

https://codereview.chromium.org/1349313003/diff/120001/snapshot/win/process_r...
snapshot/win/process_reader_win.h:53: CONTEXT context_native;
On 2015/09/18 15:44:07, Mark Mentovai - August is over wrote:
> Fix indentation.

Done.

https://codereview.chromium.org/1349313003/diff/120001/util/win/process_struc...
File util/win/process_structs.h (right):

https://codereview.chromium.org/1349313003/diff/120001/util/win/process_struc...
util/win/process_structs.h:288: // See
https://msdn.microsoft.com/en-us/library/dn424783.aspx.
On 2015/09/18 15:44:07, Mark Mentovai - August is over wrote:
> But the rest of NT_TIB isn’t right for WoW64 either, is it? Maybe this:
> 
>   union {
>     struct {
>       ExceptionList;
>       StackBase;
>     };
>     Wow64Teb;
>   };

I'm not sure if it is. I think it probably has the associated 64 bit data? But
we don't care about that, so seems reasonable.

[Mark Mentovai, 2015-09-18 22:10:50]

LGTM

https://codereview.chromium.org/1349313003/diff/120001/util/win/process_struc...
File util/win/process_structs.h (right):

https://codereview.chromium.org/1349313003/diff/120001/util/win/process_struc...
util/win/process_structs.h:288: // See
https://msdn.microsoft.com/en-us/library/dn424783.aspx.
scottmg wrote:
> On 2015/09/18 15:44:07, Mark Mentovai - August is over wrote:
> > But the rest of NT_TIB isn’t right for WoW64 either, is it? Maybe this:
> > 
> >   union {
> >     struct {
> >       ExceptionList;
> >       StackBase;
> >     };
> >     Wow64Teb;
> >   };
> 
> I'm not sure if it is. I think it probably has the associated 64 bit data? But
> we don't care about that, so seems reasonable.

Well, if each thread has its own distinct 64-bit stack, then what you had is
maybe more correct. Your call. Like you said, we don’t actually use anything
else in the WoW64 case, so there’s no practical difference.

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

https://codereview.chromium.org/1349313003/diff/160001/snapshot/win/cpu_conte...
snapshot/win/cpu_context_win.cc:30: // We assume in this function that the
WOW64_CONTEXT_* and x64 CONTEXT_*
x86, not x64

https://codereview.chromium.org/1349313003/diff/160001/snapshot/win/cpu_conte...
snapshot/win/cpu_context_win.cc:85: LOG_IF(ERROR, !(context.ContextFlags &
WOW64_CONTEXT_i386))
You can move this LOG into the common function too, but I’d go with “non-x86”
string in there. That makes sense for both native x86 and WoW64.

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

https://codereview.chromium.org/1349313003/diff/160001/snapshot/win/process_r...
snapshot/win/process_reader_win.cc:152: PLOG(ERROR) << "Wow64GetThreadContext
failed";
“failed” not necessary with PLOG, the error string will be appended and that’ll
make failure obvious. Same on line 160.

https://codereview.chromium.org/1349313003/diff/160001/snapshot/win/process_r...
File snapshot/win/process_reader_win.h (right):

https://codereview.chromium.org/1349313003/diff/160001/snapshot/win/process_r...
snapshot/win/process_reader_win.h:117: void ReadThreadData(bool
is_64_reading_32);
Yeah, this is better. My problem with is_wow64 was that it’d be false with a
32-bit handler on a 64-bit OS, but it’s not correct to say that the 32-bit
crashy app is not WoW64.

[scottmg, 2015-09-18 22:25:00]

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

https://codereview.chromium.org/1349313003/diff/160001/snapshot/win/cpu_conte...
snapshot/win/cpu_context_win.cc:30: // We assume in this function that the
WOW64_CONTEXT_* and x64 CONTEXT_*
On 2015/09/18 22:10:50, Mark Mentovai - August is over wrote:
> x86, not x64

Oops, done.

https://codereview.chromium.org/1349313003/diff/160001/snapshot/win/cpu_conte...
snapshot/win/cpu_context_win.cc:85: LOG_IF(ERROR, !(context.ContextFlags &
WOW64_CONTEXT_i386))
On 2015/09/18 22:10:50, Mark Mentovai - August is over wrote:
> You can move this LOG into the common function too, but I’d go with “non-x86”
> string in there. That makes sense for both native x86 and WoW64.

Done.

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

https://codereview.chromium.org/1349313003/diff/160001/snapshot/win/process_r...
snapshot/win/process_reader_win.cc:152: PLOG(ERROR) << "Wow64GetThreadContext
failed";
On 2015/09/18 22:10:50, Mark Mentovai - August is over wrote:
> “failed” not necessary with PLOG, the error string will be appended and
that’ll
> make failure obvious. Same on line 160.

Done (and SuspendThread and ResumeThread).

https://codereview.chromium.org/1349313003/diff/160001/snapshot/win/process_r...
File snapshot/win/process_reader_win.h (right):

https://codereview.chromium.org/1349313003/diff/160001/snapshot/win/process_r...
snapshot/win/process_reader_win.h:117: void ReadThreadData(bool
is_64_reading_32);
On 2015/09/18 22:10:50, Mark Mentovai - August is over wrote:
> Yeah, this is better. My problem with is_wow64 was that it’d be false with a
> 32-bit handler on a 64-bit OS, but it’s not correct to say that the 32-bit
> crashy app is not WoW64.

Gotcha.

[scottmg, 2015-09-18 22:29:31]

(Oops, and .gyp fix for Mac.)

The x64 bots will go red after landing this until I update their config to run
the build the extra tree. Working on that now.

[Mark Mentovai, 2015-09-18 22:30:41]

LGTM

[scottmg, 2015-09-18 23:05:27]

On 2015/09/18 22:29:31, scottmg wrote:
> (Oops, and .gyp fix for Mac.)
> 
> The x64 bots will go red after landing this until I update their config to run
> the build the extra tree. Working on that now.

Bot update at https://codereview.chromium.org/1354973003/.

[scottmg, 2015-09-18 23:06:10]

Committed patchset #10 (id:200001) manually as
bf556829d90992dfb777d8294f5c19e56527eaa2 (presubmit successful).

[scottmg, 2015-09-19 00:05:29]

On 2015/09/18 23:05:27, scottmg wrote:
> On 2015/09/18 22:29:31, scottmg wrote:
> > (Oops, and .gyp fix for Mac.)
> > 
> > The x64 bots will go red after landing this until I update their config to
run
> > the build the extra tree. Working on that now.
> 
> Bot update at https://codereview.chromium.org/1354973003/.

Took me a few minutes to realize what's going on (the bots didn't go red).

Because of the changes to gclient runhooks (Debug is now always x86 and
Debug_x64 is x64) it means that both crashpad_win_dbg and crashpad_win_x86_dbg
are both testing an x86 build running on an x64 OS. Once the bot config changes
land, the names will be improved, and we'll have x64 and x64-reading-wow64 test
coverage on the _x64 bot.