Make swapimports.exe work with 64-bit binaries by adding 64-bit

Make swapimports.exe work with 64-bit binaries by adding 64-bit
PE image support to PEFile.

This will allow other tools that perform operations solely on image
headers to work with 64 bit binaries.

Run with --x64 to operate on a 64 bit binary.

Also suppress noisy logging during normal running. (Add a --verbose
flag to re-enable noisy logging).

Also, suppress SamplerAppTest.SampleSelfPidWhitelist on Win8+.

Also, update stack capture code used in ASAN tests to not explode on
the NULL stack frames that ::CaptureStackBackTrace returns on Win8
(a documented behaviour).

BUG=https://code.google.com/p/sawbuck/issues/detail?id=85
TEST=pe_unittests.exe
R=chrisha@chromium.org

Committed: https://code.google.com/p/sawbuck/source/detail?r=2010

[chrisha, 2014-01-27 15:00:18]

Nice! Thanks for taking this on!

Basic approach looks sound to me, but I've got a handful of nits.

https://codereview.appspot.com/55700044/diff/60001/syzygy/pe/pe.gyp
File syzygy/pe/pe.gyp (right):

https://codereview.appspot.com/55700044/diff/60001/syzygy/pe/pe.gyp#newcode329
syzygy/pe/pe.gyp:329: 'includes': ['../build/masm.gypi'],
Don't think you need this... it handles any .asm files in the source listing.

https://codereview.appspot.com/55700044/diff/60001/syzygy/pe/pe_file.h
File syzygy/pe/pe_file.h (right):

https://codereview.appspot.com/55700044/diff/60001/syzygy/pe/pe_file.h#newcod...
syzygy/pe/pe_file.h:152: DCHECK(signature != NULL);
For new code we use:

DCHECK_NE(static_cast<Signature*>(NULL), signature);

I personally hate this, but call it Siggi legacy :)

(Not that you wrote this code... but you know, while you're in there already...)

https://codereview.appspot.com/55700044/diff/60001/syzygy/pe/pe_file.h#newcod...
syzygy/pe/pe_file.h:846: 
For large templated classes (like this has now become), we tend to leave the
declarations in the .h file, and make a companion _impl.h file which contains
the function definitions. We include the _impl.h at the end of the plain old .h.

See pe_coff_file.h and pe_coff_file_impl.h for an example.

https://codereview.appspot.com/55700044/diff/60001/syzygy/pe/pe_file_unittest.cc
File syzygy/pe/pe_file_unittest.cc (right):

https://codereview.appspot.com/55700044/diff/60001/syzygy/pe/pe_file_unittest...
syzygy/pe/pe_file_unittest.cc:378: // Validation the read imports section.
Validate*

https://codereview.appspot.com/55700044/diff/60001/syzygy/pe/pe_file_unittest...
syzygy/pe/pe_file_unittest.cc:385: if (0 == base::strcasecmp("kernel32.dll",
dll.name.c_str()) ||
We generally stick to (a == 0) rather than (0 == a), despite there being strong
arguments to do otherwise :)

https://codereview.appspot.com/55700044/diff/60001/syzygy/pe/test_dll_x64.cc
File syzygy/pe/test_dll_x64.cc (right):

https://codereview.appspot.com/55700044/diff/60001/syzygy/pe/test_dll_x64.cc#...
syzygy/pe/test_dll_x64.cc:15: #include <windows.h>  // NOLINT
No need for the NOLINT. We only use it when because of some bass-ackwards
windows logic we're forced to include it not in alphabetical order.

https://codereview.appspot.com/55700044/diff/60001/syzygy/pe/unittest_util.h
File syzygy/pe/unittest_util.h (right):

https://codereview.appspot.com/55700044/diff/60001/syzygy/pe/unittest_util.h#...
syzygy/pe/unittest_util.h:34: // Name of the test DLLs and PDsB. These exist in
the output directory, and
PDBs*

https://codereview.appspot.com/55700044/diff/60001/syzygy/swapimport/swapimpo...
File syzygy/swapimport/swapimport_app.cc (right):

https://codereview.appspot.com/55700044/diff/60001/syzygy/swapimport/swapimpo...
syzygy/swapimport/swapimport_app.cc:37: "    --x64                 Decompose a
64 bit binary rather than a\n"
64-bit

https://codereview.appspot.com/55700044/diff/60001/syzygy/swapimport/swapimpo...
syzygy/swapimport/swapimport_app.cc:234: if (x64_)
For compound if-else statements we always use curly braces.

[Robert, 2014-01-27 18:14:26]

Thanks! PTAL

https://codereview.appspot.com/55700044/diff/60001/syzygy/pe/pe.gyp
File syzygy/pe/pe.gyp (right):

https://codereview.appspot.com/55700044/diff/60001/syzygy/pe/pe.gyp#newcode329
syzygy/pe/pe.gyp:329: 'includes': ['../build/masm.gypi'],
On 2014/01/27 15:00:18, chrisha wrote:
> Don't think you need this... it handles any .asm files in the source listing.

Done.

https://codereview.appspot.com/55700044/diff/60001/syzygy/pe/pe_file.h
File syzygy/pe/pe_file.h (right):

https://codereview.appspot.com/55700044/diff/60001/syzygy/pe/pe_file.h#newcod...
syzygy/pe/pe_file.h:152: DCHECK(signature != NULL);
On 2014/01/27 15:00:18, chrisha wrote:
> For new code we use:
> 
> DCHECK_NE(static_cast<Signature*>(NULL), signature);
> 
> I personally hate this, but call it Siggi legacy :)
> 
> (Not that you wrote this code... but you know, while you're in there
already...)

Sacrifices code readability to get a not-really-more-useful runtime check IMO.

Done anyway :-)

https://codereview.appspot.com/55700044/diff/60001/syzygy/pe/pe_file.h#newcod...
syzygy/pe/pe_file.h:846: 
On 2014/01/27 15:00:18, chrisha wrote:
> For large templated classes (like this has now become), we tend to leave the
> declarations in the .h file, and make a companion _impl.h file which contains
> the function definitions. We include the _impl.h at the end of the plain old
.h.
> 
> See pe_coff_file.h and pe_coff_file_impl.h for an example.

Done.

https://codereview.appspot.com/55700044/diff/60001/syzygy/pe/pe_file_unittest.cc
File syzygy/pe/pe_file_unittest.cc (right):

https://codereview.appspot.com/55700044/diff/60001/syzygy/pe/pe_file_unittest...
syzygy/pe/pe_file_unittest.cc:378: // Validation the read imports section.
On 2014/01/27 15:00:18, chrisha wrote:
> Validate*

Done.

https://codereview.appspot.com/55700044/diff/60001/syzygy/pe/pe_file_unittest...
syzygy/pe/pe_file_unittest.cc:385: if (0 == base::strcasecmp("kernel32.dll",
dll.name.c_str()) ||
On 2014/01/27 15:00:18, chrisha wrote:
> We generally stick to (a == 0) rather than (0 == a), despite there being
strong
> arguments to do otherwise :)

Done.

https://codereview.appspot.com/55700044/diff/60001/syzygy/pe/test_dll_x64.cc
File syzygy/pe/test_dll_x64.cc (right):

https://codereview.appspot.com/55700044/diff/60001/syzygy/pe/test_dll_x64.cc#...
syzygy/pe/test_dll_x64.cc:15: #include <windows.h>  // NOLINT
On 2014/01/27 15:00:18, chrisha wrote:
> No need for the NOLINT. We only use it when because of some bass-ackwards
> windows logic we're forced to include it not in alphabetical order.

Done.

https://codereview.appspot.com/55700044/diff/60001/syzygy/pe/unittest_util.h
File syzygy/pe/unittest_util.h (right):

https://codereview.appspot.com/55700044/diff/60001/syzygy/pe/unittest_util.h#...
syzygy/pe/unittest_util.h:34: // Name of the test DLLs and PDsB. These exist in
the output directory, and
On 2014/01/27 15:00:18, chrisha wrote:
> PDBs*

Done.

https://codereview.appspot.com/55700044/diff/60001/syzygy/swapimport/swapimpo...
File syzygy/swapimport/swapimport_app.cc (right):

https://codereview.appspot.com/55700044/diff/60001/syzygy/swapimport/swapimpo...
syzygy/swapimport/swapimport_app.cc:37: "    --x64                 Decompose a
64 bit binary rather than a\n"
On 2014/01/27 15:00:18, chrisha wrote:
> 64-bit

Done.

https://codereview.appspot.com/55700044/diff/60001/syzygy/swapimport/swapimpo...
syzygy/swapimport/swapimport_app.cc:234: if (x64_)
On 2014/01/27 15:00:18, chrisha wrote:
> For compound if-else statements we always use curly braces.

Done.

[chrisha, 2014-01-27 19:32:03]

Awesome! One last nit and lgtm!

https://codereview.appspot.com/55700044/diff/80001/syzygy/pe/pe_file_impl.h
File syzygy/pe/pe_file_impl.h (right):

https://codereview.appspot.com/55700044/diff/80001/syzygy/pe/pe_file_impl.h#n...
syzygy/pe/pe_file_impl.h:546: in_archive->Load(&module_checksum);
Just noticed the weird indent... should be +4 and not +8 (here and above).

[Robert, 2014-01-27 20:26:15]

Thanks!

https://codereview.appspot.com/55700044/diff/80001/syzygy/pe/pe_file_impl.h
File syzygy/pe/pe_file_impl.h (right):

https://codereview.appspot.com/55700044/diff/80001/syzygy/pe/pe_file_impl.h#n...
syzygy/pe/pe_file_impl.h:546: in_archive->Load(&module_checksum);
On 2014/01/27 19:32:03, chrisha wrote:
> Just noticed the weird indent... should be +4 and not +8 (here and above).

Done.

[chrisha, 2014-01-27 20:32:04]

Is it too late to ask for a swapimport_app_unittest to be added? Something like
the 'RunSucceeds', but working on test_dll_x64 and in 64-bit mode?

Maybe also a test in 32-bit mode that tries to work on a 64-bit binary and
fails, and vice-versa?

[Robert, 2014-01-28 00:12:35]

On 2014/01/27 20:32:04, chrisha wrote:
> Is it too late to ask for a swapimport_app_unittest to be added? Something
like
> the 'RunSucceeds', but working on test_dll_x64 and in 64-bit mode?
> 
> Maybe also a test in 32-bit mode that tries to work on a 64-bit binary and
> fails, and vice-versa?

Above tests added. PTAL!

[chrisha, 2014-01-28 16:10:33]

Awesome! lgtm for real this time

[Robert, 2014-01-28 19:50:46]

Committed patchset #9 manually as r2010 (presubmit successful).