win: Handle binary with embedded CodeView debug record

win: Handle binary with embedded CodeView debug record

I considered writing the CodeView records to the minidump, but I didn't
find a ton of docs and debugging is only lightly supported (e.g.
http://www.debuginfo.com/articles/gendebuginfo.html#debuggersandformats
and it doesn't attempt to load at all on more recent Visual Studios).

As we won't be generating symbols in this format, and we don't expect to
have symbols for any weird modules that get injected into us in the
wild, it seems like we don't lose anything by just ignoring them.

R=mark@chromium.org
BUG=crashpad:47
Committed: https://chromium.googlesource.com/crashpad/crashpad/+/4860f64923df01fddc96803da3ffc14f47668d2a

[scottmg, 2015-10-30 19:05:08]

Description was changed from

==========
win: Handle binary with CodeView debug record

BUG=crashpad:47
==========

to

==========
win: Handle binary with embedded CodeView debug record

R=mark@chromium.org
BUG=crashpad:47
==========

[scottmg, 2015-10-30 19:05:09]

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

[Mark Mentovai, 2015-10-30 19:42:45]

https://codereview.chromium.org/1430773003/diff/40001/handler/win/crashy_test...
File handler/win/crashy_test_z7_loader.cc (right):

https://codereview.chromium.org/1430773003/diff/40001/handler/win/crashy_test...
handler/win/crashy_test_z7_loader.cc:21: #if !ARCH_CPU_32_BITS
Should be !defined(ARCH_CPU_32_BITS). Found the same thing going on in
process_info.cc.

Since you want 32-bit x86 specifically, you can also use the more-specific
!defined(ARCH_CPU_X86).

https://codereview.chromium.org/1430773003/diff/40001/handler/win/crashy_test...
handler/win/crashy_test_z7_loader.cc:31: return 1;
<stdlib.h> EXIT_FAILURE, EXIT_SUCCESS

https://codereview.chromium.org/1430773003/diff/40001/handler/win/crashy_test...
handler/win/crashy_test_z7_loader.cc:47: HMODULE z7_test =
LoadLibrary(L"../../handler/win/z7_test.dll");
If either of these fail, LOG(ERROR) and return EXIT_FAILURE. Wouldn’t want a
failure here to skew the test results because crash_me wound up as nullptr
because of a failure and crashed for an unrelated reason without the library
being loaded.

https://codereview.chromium.org/1430773003/diff/40001/handler/win/crashy_test...
handler/win/crashy_test_z7_loader.cc:47: HMODULE z7_test =
LoadLibrary(L"../../handler/win/z7_test.dll");
This will be treated as a path relative to cwd, which is no good. Since
z7_test.dll is checked-in test data, build a path relative to test/paths.h’s
test::TestDataRoot().

https://codereview.chromium.org/1430773003/diff/40001/handler/win/crashy_test...
handler/win/crashy_test_z7_loader.cc:60: 
Remove the blank line at EOF.

https://codereview.chromium.org/1430773003/diff/40001/handler/win/z7_test.cpp
File handler/win/z7_test.cpp (right):

https://codereview.chromium.org/1430773003/diff/40001/handler/win/z7_test.cpp...
handler/win/z7_test.cpp:14: //
This line shouldn’t have //s on it, it should be blank. Otherwise, the comment
that follows, which is actually interesting, looks like license garbage.

https://codereview.chromium.org/1430773003/diff/40001/handler/win/z7_test.cpp...
handler/win/z7_test.cpp:15: // Build in VC6 or older command prompt with:
VC++6

https://codereview.chromium.org/1430773003/diff/40001/snapshot/win/module_sna...
File snapshot/win/module_snapshot_win.cc (right):

https://codereview.chromium.org/1430773003/diff/40001/snapshot/win/module_sna...
snapshot/win/module_snapshot_win.cc:64: pdb_name_ = base::UTF16ToUTF8(name_);
Add a short comment saying that this isn’t how the MS tools deal with this
situation.

I think what you’ve done here is totally reasonable.

https://codereview.chromium.org/1430773003/diff/40001/snapshot/win/pe_image_r...
File snapshot/win/pe_image_reader.cc (right):

https://codereview.chromium.org/1430773003/diff/40001/snapshot/win/pe_image_r...
snapshot/win/pe_image_reader.cc:187: debug_directory.SizeOfData,
Reindent.

https://codereview.chromium.org/1430773003/diff/40001/snapshot/win/pe_image_r...
snapshot/win/pe_image_reader.cc:212: //
https://code.google.com/p/crashpad/issues/detail?id=47.
Use the new format: https://crashpad.chromium.org/bug/47. I changed all of the
existing bug links to this format yesterday.

[scottmg, 2015-10-30 23:52:08]

Thanks

https://codereview.chromium.org/1430773003/diff/40001/handler/win/crashy_test...
File handler/win/crashy_test_z7_loader.cc (right):

https://codereview.chromium.org/1430773003/diff/40001/handler/win/crashy_test...
handler/win/crashy_test_z7_loader.cc:21: #if !ARCH_CPU_32_BITS
On 2015/10/30 19:42:44, Mark Mentovai wrote:
> Should be !defined(ARCH_CPU_32_BITS). Found the same thing going on in
> process_info.cc.
> 
> Since you want 32-bit x86 specifically, you can also use the more-specific
> !defined(ARCH_CPU_X86).

Done.

https://codereview.chromium.org/1430773003/diff/40001/handler/win/crashy_test...
handler/win/crashy_test_z7_loader.cc:31: return 1;
On 2015/10/30 19:42:44, Mark Mentovai wrote:
> <stdlib.h> EXIT_FAILURE, EXIT_SUCCESS

Done.

https://codereview.chromium.org/1430773003/diff/40001/handler/win/crashy_test...
handler/win/crashy_test_z7_loader.cc:47: HMODULE z7_test =
LoadLibrary(L"../../handler/win/z7_test.dll");
On 2015/10/30 19:42:44, Mark Mentovai wrote:
> If either of these fail, LOG(ERROR) and return EXIT_FAILURE. Wouldn’t want a
> failure here to skew the test results because crash_me wound up as nullptr
> because of a failure and crashed for an unrelated reason without the library
> being loaded.

Done.

https://codereview.chromium.org/1430773003/diff/40001/handler/win/crashy_test...
handler/win/crashy_test_z7_loader.cc:47: HMODULE z7_test =
LoadLibrary(L"../../handler/win/z7_test.dll");
On 2015/10/30 19:42:44, Mark Mentovai wrote:
> This will be treated as a path relative to cwd, which is no good. Since
> z7_test.dll is checked-in test data, build a path relative to test/paths.h’s
> test::TestDataRoot().

Windows is very promiscuous and eventually tries relative the current module
too! But being explicit about that is probably better.

https://codereview.chromium.org/1430773003/diff/40001/handler/win/crashy_test...
handler/win/crashy_test_z7_loader.cc:60: 
On 2015/10/30 19:42:44, Mark Mentovai wrote:
> Remove the blank line at EOF.

Done.

https://codereview.chromium.org/1430773003/diff/40001/handler/win/z7_test.cpp
File handler/win/z7_test.cpp (right):

https://codereview.chromium.org/1430773003/diff/40001/handler/win/z7_test.cpp...
handler/win/z7_test.cpp:14: //
On 2015/10/30 19:42:44, Mark Mentovai wrote:
> This line shouldn’t have //s on it, it should be blank. Otherwise, the comment
> that follows, which is actually interesting, looks like license garbage.

Done.

https://codereview.chromium.org/1430773003/diff/40001/handler/win/z7_test.cpp...
handler/win/z7_test.cpp:15: // Build in VC6 or older command prompt with:
On 2015/10/30 19:42:44, Mark Mentovai wrote:
> VC++6

Done.

https://codereview.chromium.org/1430773003/diff/40001/snapshot/win/module_sna...
File snapshot/win/module_snapshot_win.cc (right):

https://codereview.chromium.org/1430773003/diff/40001/snapshot/win/module_sna...
snapshot/win/module_snapshot_win.cc:64: pdb_name_ = base::UTF16ToUTF8(name_);
On 2015/10/30 19:42:44, Mark Mentovai wrote:
> Add a short comment saying that this isn’t how the MS tools deal with this
> situation.
> 
> I think what you’ve done here is totally reasonable.

Done.

https://codereview.chromium.org/1430773003/diff/40001/snapshot/win/pe_image_r...
File snapshot/win/pe_image_reader.cc (right):

https://codereview.chromium.org/1430773003/diff/40001/snapshot/win/pe_image_r...
snapshot/win/pe_image_reader.cc:187: debug_directory.SizeOfData,
On 2015/10/30 19:42:44, Mark Mentovai wrote:
> Reindent.

Done.

https://codereview.chromium.org/1430773003/diff/40001/snapshot/win/pe_image_r...
snapshot/win/pe_image_reader.cc:212: //
https://code.google.com/p/crashpad/issues/detail?id=47.
On 2015/10/30 19:42:44, Mark Mentovai wrote:
> Use the new format: https://crashpad.chromium.org/bug/47. I changed all of the
> existing bug links to this format yesterday.

Done.

[Mark Mentovai, 2015-10-31 01:29:34]

LGTM!

https://codereview.chromium.org/1430773003/diff/40001/handler/win/crashy_test...
File handler/win/crashy_test_z7_loader.cc (right):

https://codereview.chromium.org/1430773003/diff/40001/handler/win/crashy_test...
handler/win/crashy_test_z7_loader.cc:47: HMODULE z7_test =
LoadLibrary(L"../../handler/win/z7_test.dll");
scottmg wrote:
> On 2015/10/30 19:42:44, Mark Mentovai wrote:
> > This will be treated as a path relative to cwd, which is no good. Since
> > z7_test.dll is checked-in test data, build a path relative to test/paths.h’s
> > test::TestDataRoot().
> 
> Windows is very promiscuous

Ha!

https://codereview.chromium.org/1430773003/diff/80001/handler/win/crashy_test...
File handler/win/crashy_test_z7_loader.cc (right):

https://codereview.chromium.org/1430773003/diff/80001/handler/win/crashy_test...
handler/win/crashy_test_z7_loader.cc:24: #if !defined(ARCH_CPU_X86)
#include "build/build_config.h"

https://codereview.chromium.org/1430773003/diff/80001/handler/win/z7_test.cpp
File handler/win/z7_test.cpp (right):

https://codereview.chromium.org/1430773003/diff/80001/handler/win/z7_test.cpp...
handler/win/z7_test.cpp:24: __declspec(dllexport) void CrashMe() {
Maybe “extern "C"” so this wouldn’t have mangled with all of that @YAXXZ.

[scottmg, 2015-10-31 18:39:47]

https://codereview.chromium.org/1430773003/diff/80001/handler/win/crashy_test...
File handler/win/crashy_test_z7_loader.cc (right):

https://codereview.chromium.org/1430773003/diff/80001/handler/win/crashy_test...
handler/win/crashy_test_z7_loader.cc:24: #if !defined(ARCH_CPU_X86)
On 2015/10/31 01:29:33, Mark Mentovai wrote:
> #include "build/build_config.h"

Done.

https://codereview.chromium.org/1430773003/diff/80001/handler/win/z7_test.cpp
File handler/win/z7_test.cpp (right):

https://codereview.chromium.org/1430773003/diff/80001/handler/win/z7_test.cpp...
handler/win/z7_test.cpp:24: __declspec(dllexport) void CrashMe() {
On 2015/10/31 01:29:33, Mark Mentovai wrote:
> Maybe “extern "C"” so this wouldn’t have mangled with all of that @YAXXZ.

Done.

[scottmg, 2015-10-31 18:45:45]

Committed patchset #7 (id:120001) manually as
4860f64923df01fddc96803da3ffc14f47668d2a (presubmit successful).