win: Make reading NT_IMAGE_HEADERS work cross-bitness

win: Make reading NT_IMAGE_HEADERS work cross-bitness

Factor out some test launching code used in cross-bitness tests.

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

[Mark Mentovai, 2015-09-19 03:02:47]

https://codereview.chromium.org/1352323002/diff/90014/snapshot/win/crashpad_s...
File snapshot/win/crashpad_snapshot_test_image_reader_module.cc (right):

https://codereview.chromium.org/1352323002/diff/90014/snapshot/win/crashpad_s...
snapshot/win/crashpad_snapshot_test_image_reader_module.cc:17: #include
"client/crashpad_info.h"
Do you need this?

https://codereview.chromium.org/1352323002/diff/90014/snapshot/win/module_sna...
File snapshot/win/module_snapshot_win.h (right):

https://codereview.chromium.org/1352323002/diff/90014/snapshot/win/module_sna...
snapshot/win/module_snapshot_win.h:67: return *pe_image_reader_.get();
Just “return *pe_image_reader_” should be enough, there’s an &-returning
operator*.

https://codereview.chromium.org/1352323002/diff/90014/snapshot/win/process_sn...
File snapshot/win/process_snapshot_win_test.cc (right):

https://codereview.chromium.org/1352323002/diff/90014/snapshot/win/process_sn...
snapshot/win/process_snapshot_win_test.cc:37: UUID
done_uuid(reinterpret_cast<const uint8_t*>(&system_uuid.Data1));
Is &system_uuid enough? But even more, isn’t this what
UUID::InitializeFromSystemUUID() is for? If you want to construct it from a
system UUID instead of making a zero UUID and then rewriting it, we can add a
new constructor overload for that.

https://codereview.chromium.org/1352323002/diff/90014/snapshot/win/process_sn...
snapshot/win/process_snapshot_win_test.cc:39: CreateEvent(nullptr, true, false,
done_uuid.ToString16().c_str()));
<windows.h>

https://codereview.chromium.org/1352323002/diff/90014/snapshot/win/process_sn...
snapshot/win/process_snapshot_win_test.cc:43: std::wstring child_test_executable
=
<string>

https://codereview.chromium.org/1352323002/diff/90014/snapshot/win/process_sn...
snapshot/win/process_snapshot_win_test.cc:54: ASSERT_EQ(c, ' ');
Reverse the arguments.

https://codereview.chromium.org/1352323002/diff/90014/snapshot/win/process_sn...
snapshot/win/process_snapshot_win_test.cc:72: ->pe_image_reader()
All right if this is what clang-format decided should be done here, but it’s not
the most readable.

https://codereview.chromium.org/1352323002/diff/90014/test/win/child_launcher.cc
File test/win/child_launcher.cc (right):

https://codereview.chromium.org/1352323002/diff/90014/test/win/child_launcher...
test/win/child_launcher.cc:43: stdout_read_handle_.reset(stdout_read);
ASSERT_FALSE(stdout_read_handle_) first to ensure no abuse. You can do this up
at the top of this method, too. Same for the other two handles.

https://codereview.chromium.org/1352323002/diff/90014/test/win/child_launcher...
test/win/child_launcher.cc:55: std::wstring command_line = executable_ + L" " +
command_line_;
Some sort of escaping (CRT-compatible, I guess) should be applied to
executable_. We may already have a helper for that somewhere?

https://codereview.chromium.org/1352323002/diff/90014/test/win/child_launcher.h
File test/win/child_launcher.h (right):

https://codereview.chromium.org/1352323002/diff/90014/test/win/child_launcher...
test/win/child_launcher.h:45: //! \brief The main thread's `HANDLE`.
child process’ main thread’s `HANDLE`, right?

[scottmg, 2015-09-19 04:24:42]

Thanks

https://codereview.chromium.org/1352323002/diff/90014/snapshot/win/crashpad_s...
File snapshot/win/crashpad_snapshot_test_image_reader_module.cc (right):

https://codereview.chromium.org/1352323002/diff/90014/snapshot/win/crashpad_s...
snapshot/win/crashpad_snapshot_test_image_reader_module.cc:17: #include
"client/crashpad_info.h"
On 2015/09/19 03:02:47, Mark Mentovai - August is over wrote:
> Do you need this?

Oops, I had other plans originally. Done.

https://codereview.chromium.org/1352323002/diff/90014/snapshot/win/module_sna...
File snapshot/win/module_snapshot_win.h (right):

https://codereview.chromium.org/1352323002/diff/90014/snapshot/win/module_sna...
snapshot/win/module_snapshot_win.h:67: return *pe_image_reader_.get();
On 2015/09/19 03:02:47, Mark Mentovai - August is over wrote:
> Just “return *pe_image_reader_” should be enough, there’s an &-returning
> operator*.

Done.

https://codereview.chromium.org/1352323002/diff/90014/snapshot/win/process_sn...
File snapshot/win/process_snapshot_win_test.cc (right):

https://codereview.chromium.org/1352323002/diff/90014/snapshot/win/process_sn...
snapshot/win/process_snapshot_win_test.cc:37: UUID
done_uuid(reinterpret_cast<const uint8_t*>(&system_uuid.Data1));
On 2015/09/19 03:02:47, Mark Mentovai - August is over wrote:
> Is &system_uuid enough? But even more, isn’t this what
> UUID::InitializeFromSystemUUID() is for? If you want to construct it from a
> system UUID instead of making a zero UUID and then rewriting it, we can add a
> new constructor overload for that.

Even better, we have a constructor that does this already. (Also fixed where I
copied it from).

https://codereview.chromium.org/1352323002/diff/90014/snapshot/win/process_sn...
snapshot/win/process_snapshot_win_test.cc:39: CreateEvent(nullptr, true, false,
done_uuid.ToString16().c_str()));
On 2015/09/19 03:02:47, Mark Mentovai - August is over wrote:
> <windows.h>

It's in process_snapshot_win.h, or does that exemption only apply for
process_snapshot_win.cc?

https://codereview.chromium.org/1352323002/diff/90014/snapshot/win/process_sn...
snapshot/win/process_snapshot_win_test.cc:43: std::wstring child_test_executable
=
On 2015/09/19 03:02:47, Mark Mentovai - August is over wrote:
> <string>

Same.

https://codereview.chromium.org/1352323002/diff/90014/snapshot/win/process_sn...
snapshot/win/process_snapshot_win_test.cc:54: ASSERT_EQ(c, ' ');
On 2015/09/19 03:02:47, Mark Mentovai - August is over wrote:
> Reverse the arguments.

Done.

https://codereview.chromium.org/1352323002/diff/90014/snapshot/win/process_sn...
snapshot/win/process_snapshot_win_test.cc:72: ->pe_image_reader()
On 2015/09/19 03:02:47, Mark Mentovai - August is over wrote:
> All right if this is what clang-format decided should be done here, but it’s
not
> the most readable.

Broke it up a bit.

https://codereview.chromium.org/1352323002/diff/90014/test/win/child_launcher.cc
File test/win/child_launcher.cc (right):

https://codereview.chromium.org/1352323002/diff/90014/test/win/child_launcher...
test/win/child_launcher.cc:43: stdout_read_handle_.reset(stdout_read);
On 2015/09/19 03:02:47, Mark Mentovai - August is over wrote:
> ASSERT_FALSE(stdout_read_handle_) first to ensure no abuse. You can do this up
> at the top of this method, too. Same for the other two handles.

Done.

https://codereview.chromium.org/1352323002/diff/90014/test/win/child_launcher...
test/win/child_launcher.cc:55: std::wstring command_line = executable_ + L" " +
command_line_;
On 2015/09/19 03:02:47, Mark Mentovai - August is over wrote:
> Some sort of escaping (CRT-compatible, I guess) should be applied to
> executable_. We may already have a helper for that somewhere?

Done.

https://codereview.chromium.org/1352323002/diff/90014/test/win/child_launcher.h
File test/win/child_launcher.h (right):

https://codereview.chromium.org/1352323002/diff/90014/test/win/child_launcher...
test/win/child_launcher.h:45: //! \brief The main thread's `HANDLE`.
On 2015/09/19 03:02:47, Mark Mentovai - August is over wrote:
> child process’ main thread’s `HANDLE`, right?

Yes, done.

[Mark Mentovai, 2015-09-19 19:20:27]

LGTM

https://codereview.chromium.org/1352323002/diff/90014/snapshot/win/process_sn...
File snapshot/win/process_snapshot_win_test.cc (right):

https://codereview.chromium.org/1352323002/diff/90014/snapshot/win/process_sn...
snapshot/win/process_snapshot_win_test.cc:39: CreateEvent(nullptr, true, false,
done_uuid.ToString16().c_str()));
scottmg wrote:
> On 2015/09/19 03:02:47, Mark Mentovai - August is over wrote:
> > <windows.h>
> 
> It's in process_snapshot_win.h, or does that exemption only apply for
> process_snapshot_win.cc? 

It totally applies to tests too. I must have been rushing, sorry.

https://codereview.chromium.org/1352323002/diff/110001/snapshot/win/crashpad_...
File snapshot/win/crashpad_snapshot_test_crashing_child.cc (right):

https://codereview.chromium.org/1352323002/diff/110001/snapshot/win/crashpad_...
snapshot/win/crashpad_snapshot_test_crashing_child.cc:34: CHECK_NE(out,
INVALID_HANDLE_VALUE);
P on this CHECK?

https://codereview.chromium.org/1352323002/diff/110001/snapshot/win/crashpad_...
File snapshot/win/crashpad_snapshot_test_image_reader.cc (right):

https://codereview.chromium.org/1352323002/diff/110001/snapshot/win/crashpad_...
snapshot/win/crashpad_snapshot_test_image_reader.cc:26:
LoadLibrary(L"crashpad_snapshot_test_image_reader_module.dll");
Maybe this should be PCHECKed.

https://codereview.chromium.org/1352323002/diff/110001/snapshot/win/crashpad_...
snapshot/win/crashpad_snapshot_test_image_reader.cc:29: CHECK_NE(out,
INVALID_HANDLE_VALUE);
P on this CHECK too?

[scottmg, 2015-09-20 18:16:26]

https://codereview.chromium.org/1352323002/diff/110001/snapshot/win/crashpad_...
File snapshot/win/crashpad_snapshot_test_crashing_child.cc (right):

https://codereview.chromium.org/1352323002/diff/110001/snapshot/win/crashpad_...
snapshot/win/crashpad_snapshot_test_crashing_child.cc:34: CHECK_NE(out,
INVALID_HANDLE_VALUE);
On 2015/09/19 19:20:27, Mark Mentovai - August is over wrote:
> P on this CHECK?

Done.

https://codereview.chromium.org/1352323002/diff/110001/snapshot/win/crashpad_...
File snapshot/win/crashpad_snapshot_test_image_reader.cc (right):

https://codereview.chromium.org/1352323002/diff/110001/snapshot/win/crashpad_...
snapshot/win/crashpad_snapshot_test_image_reader.cc:26:
LoadLibrary(L"crashpad_snapshot_test_image_reader_module.dll");
On 2015/09/19 19:20:27, Mark Mentovai - August is over wrote:
> Maybe this should be PCHECKed.

Done.

https://codereview.chromium.org/1352323002/diff/110001/snapshot/win/crashpad_...
snapshot/win/crashpad_snapshot_test_image_reader.cc:29: CHECK_NE(out,
INVALID_HANDLE_VALUE);
On 2015/09/19 19:20:27, Mark Mentovai - August is over wrote:
> P on this CHECK too?

Done.

[scottmg, 2015-09-20 18:16:37]

Committed patchset #8 (id:120015) manually as
4a34a3dd896675cdc101bf21679e8ed38de63472 (presubmit successful).