Add explicit OMAP support to dump_syms.

Add explicit OMAP support to dump_syms.

This CL adds new utilities to common/windows for handling OMAP information in
PDB files. It then augments PdbSourceLineWriter with explicit OMAP knowledge so
that symbolization will proceed more cleanly for images whose PDB files contain
OMAP information. This makes breakpad handle OMAPped symbol files as cleanly as
WinDbg.

Committed: https://code.google.com/p/google-breakpad/source/detail?r=1167

[chrisha]

Sorry for the giant code dump. If people want I can cut this up into a series of
smaller CLs, but the code in omap* is still going to be in a single CL.

PTAL.

siggi: You've got knowledge of how OMAP works and how WinDbg resolves symbols,
so I'd appreciate a review.
mmentovai: You've done previous work in pdb_source_line_writer. Feel free to
point me at another reviewer if you think that's appropriate.

https://breakpad.appspot.com/570002/diff/2001/tools/windows/dump_syms/dump_sy...
File tools/windows/dump_syms/dump_syms.vcproj (right):

https://breakpad.appspot.com/570002/diff/2001/tools/windows/dump_syms/dump_sy...
tools/windows/dump_syms/dump_syms.vcproj:1: <?xml version="1.0"
encoding="UTF-8"?>
Is it worth maybe turfing this file in favor of a GYP-based build?

[chrisha]

(Now with actual specified reviewers!)

Sorry for the giant code dump. If people want I can cut this up into a series of
smaller CLs, but the code in omap* is still going to be in a single CL.

PTAL.

siggi: You've got knowledge of how OMAP works and how WinDbg resolves symbols,
so I'd appreciate a review.
mmentovai: You've done previous work in pdb_source_line_writer. Feel free to
point me at another reviewer if you think that's appropriate.

[Siggi]

looks pretty good, though I didn't read much past the point where I got confused
on how duplicate ranges are handled.

https://breakpad.appspot.com/570002/diff/2001/common/windows/omap.cc
File common/windows/omap.cc (right):

https://breakpad.appspot.com/570002/diff/2001/common/windows/omap.cc#newcode34
common/windows/omap.cc:34: // address spaces. It consists of two stream, each of
them a vector 2-tuples.
great comment!

https://breakpad.appspot.com/570002/diff/2001/common/windows/omap.cc#newcode55
common/windows/omap.cc:55: //                [0x00001005, 0xFFFFFFFF],
Is this what we do under the --debug-friendly flag?
I'd have thought we'd map 0x1005->0x1005 in addition to 0x100a->0x1005?

https://breakpad.appspot.com/570002/diff/2001/common/windows/omap.cc#newcode118
common/windows/omap.cc:118: };
Maybe COMPILE_ASSERT, or otherwise assert that this has the same sizeof() as
Omap?
In V8 code, these asserts are routinely at the site of every afflicted
reinterpret_cast<> which makes a lot of sense...

https://breakpad.appspot.com/570002/diff/2001/common/windows/omap.cc#newcode188
common/windows/omap.cc:188: 
nit: is there reason to check that the OMAP table meets our expectations on
ordering here?

https://breakpad.appspot.com/570002/diff/2001/common/windows/omap.cc#newcode265
common/windows/omap.cc:265: reinterpret_cast<const
OmapToTable&>(omap_data.omap_to);
yups, test the relative sizeofs here, somehow?

https://breakpad.appspot.com/570002/diff/2001/common/windows/omap.cc#newcode285
common/windows/omap.cc:285: // as regions of the transformed address space that
does not map back to
In debug-friendly mode we have multiple ranges that have the same origin - see
[https://code.google.com/p/sawbuck/source/browse/trunk/syzygy/instrument/transforms/asan_transform.cc#594],
where we refer each injected instruction back to the instrumented instruction's
source range.
It'd be great to amend this comment with a discussion of how this is handled -
I'm having trouble following the logic as-is.

https://breakpad.appspot.com/570002/diff/2001/common/windows/omap.h
File common/windows/omap.h (right):

https://breakpad.appspot.com/570002/diff/2001/common/windows/omap.h#newcode53
common/windows/omap.h:53: //     |omap_data| is empty then this will also be
empty.
nit: indent - or is this in breakpad style?

[Mark Mentovai]

This seems fine to me. I appreciated your description of OMAP in omap.cc.

[chrisha]

Alright, a bunch of cleanup according to Siggi's comments and more unittests.

I'll make another CL to update sym_upload and its build system, and finally
another CL to check in new binaries. Sound good?

PTAnotherL

https://breakpad.appspot.com/570002/diff/2001/common/windows/omap.cc
File common/windows/omap.cc (right):

https://breakpad.appspot.com/570002/diff/2001/common/windows/omap.cc#newcode55
common/windows/omap.cc:55: //                [0x00001005, 0xFFFFFFFF],
On 2013/04/24 14:09:56, Siggi wrote:
> Is this what we do under the --debug-friendly flag?
> I'd have thought we'd map 0x1005->0x1005 in addition to 0x100a->0x1005?

Added a discussion for when we inject new source info (both cases can happen,
and both are handled).

https://breakpad.appspot.com/570002/diff/2001/common/windows/omap.cc#newcode118
common/windows/omap.cc:118: };
On 2013/04/24 14:09:56, Siggi wrote:
> Maybe COMPILE_ASSERT, or otherwise assert that this has the same sizeof() as
> Omap?
> In V8 code, these asserts are routinely at the site of every afflicted
> reinterpret_cast<> which makes a lot of sense...

Done.

https://breakpad.appspot.com/570002/diff/2001/common/windows/omap.cc#newcode188
common/windows/omap.cc:188: 
On 2013/04/24 14:09:56, Siggi wrote:
> nit: is there reason to check that the OMAP table meets our expectations on
> ordering here?

Done.

https://breakpad.appspot.com/570002/diff/2001/common/windows/omap.cc#newcode265
common/windows/omap.cc:265: reinterpret_cast<const
OmapToTable&>(omap_data.omap_to);
On 2013/04/24 14:09:56, Siggi wrote:
> yups, test the relative sizeofs here, somehow?

Done.

https://breakpad.appspot.com/570002/diff/2001/common/windows/omap.cc#newcode285
common/windows/omap.cc:285: // as regions of the transformed address space that
does not map back to
On 2013/04/24 14:09:56, Siggi wrote:
> In debug-friendly mode we have multiple ranges that have the same origin - see
>
[https://code.google.com/p/sawbuck/source/browse/trunk/syzygy/instrument/transforms/asan_transform.cc#594],
> where we refer each injected instruction back to the instrumented
instruction's
> source range.
> It'd be great to amend this comment with a discussion of how this is handled -
> I'm having trouble following the logic as-is.

I've expanded the discussion here and elsewhere. Handling multiple ranges with
the same origin actually happens when we do the interval query and merging of
intervals in the query phase. Nothing really happens here in the map building
phase.

https://breakpad.appspot.com/570002/diff/2001/common/windows/omap.h
File common/windows/omap.h (right):

https://breakpad.appspot.com/570002/diff/2001/common/windows/omap.h#newcode53
common/windows/omap.h:53: //     |omap_data| is empty then this will also be
empty.
On 2013/04/24 14:09:56, Siggi wrote:
> nit: indent - or is this in breakpad style?

This is a continuation of the previous line, not actually a new parameter.

https://breakpad.appspot.com/570002/diff/9001/common/windows/omap.cc
File common/windows/omap.cc (right):

https://breakpad.appspot.com/570002/diff/9001/common/windows/omap.cc#newcode275
common/windows/omap.cc:275: //   Range 2:     -------
Siggi: This should give some more insight into how 'repeated source ranges' are
handled. This is the pertinent function.

https://breakpad.appspot.com/570002/diff/9001/tools/windows/dump_syms/testdat...
File tools/windows/dump_syms/testdata/dump_syms_regtest.cc (right):

https://breakpad.appspot.com/570002/diff/9001/tools/windows/dump_syms/testdat...
tools/windows/dump_syms/testdata/dump_syms_regtest.cc:30: // cl /Zi
dump_syms_regtest.cc /link /PROFILE
This is needed to create a build that can then be used with Syzygy, to create
PDBs with OMAP information.

[Siggi]

lgtm with a couple of nits.

https://breakpad.appspot.com/570002/diff/9001/common/windows/common_windows.gyp
File common/windows/common_windows.gyp (right):

https://breakpad.appspot.com/570002/diff/9001/common/windows/common_windows.g...
common/windows/common_windows.gyp:40: '$(VSInstallDir)\DIA SDK\include',
This will build against MSVS' version of DIA - is that kosher here?
I believe the final binaries will still require the 8.0 DIA DLL if they're to
run on the existing bots?

https://breakpad.appspot.com/570002/diff/9001/common/windows/omap_internal.h
File common/windows/omap_internal.h (right):

https://breakpad.appspot.com/570002/diff/9001/common/windows/omap_internal.h#...
common/windows/omap_internal.h:77: // overlapping. Otherwise, they compare as
equal than.
nit: compare as equal than->compare equal?

https://breakpad.appspot.com/570002/diff/9001/common/windows/omap_unittest.cc
File common/windows/omap_unittest.cc (right):

https://breakpad.appspot.com/570002/diff/9001/common/windows/omap_unittest.cc...
common/windows/omap_unittest.cc:41: return ::memcmp(&mr1, &mr2, sizeof(mr1)) ==
0;
this only works if there's no padding in the mapped ranges, or if they're
explicitly initialized with memset(). As-is this looks correct, but brittle to
maintenance and extension.

https://breakpad.appspot.com/570002/diff/9001/common/windows/omap_unittest.cc...
common/windows/omap_unittest.cc:43: bool operator==(const EndpointIndex& ei1,
const EndpointIndex& ei2) {
ditto.

https://breakpad.appspot.com/570002/diff/9001/common/windows/omap_unittest.cc...
common/windows/omap_unittest.cc:159: }
how about testing that an empty map results in the identity mapping - as that's
necessary for backwards compatibility with non-Omapped images?

https://breakpad.appspot.com/570002/diff/9001/common/windows/omap_unittest.cc...
common/windows/omap_unittest.cc:315: }
very thorough testing, nice

[chrisha]

Addressed siggi's nits. Waiting for a final approval for Mark.

https://breakpad.appspot.com/570002/diff/9001/common/windows/common_windows.gyp
File common/windows/common_windows.gyp (right):

https://breakpad.appspot.com/570002/diff/9001/common/windows/common_windows.g...
common/windows/common_windows.gyp:40: '$(VSInstallDir)\DIA SDK\include',
On 2013/05/01 15:58:09, Siggi wrote:
> This will build against MSVS' version of DIA - is that kosher here?
> I believe the final binaries will still require the 8.0 DIA DLL if they're to
> run on the existing bots?

I copied the build configuration from the .vcproj currently included in the
repository, so I assume this will work fine. I'll double check before committing
new binaries.

https://breakpad.appspot.com/570002/diff/9001/common/windows/omap_internal.h
File common/windows/omap_internal.h (right):

https://breakpad.appspot.com/570002/diff/9001/common/windows/omap_internal.h#...
common/windows/omap_internal.h:77: // overlapping. Otherwise, they compare as
equal than.
On 2013/05/01 15:58:09, Siggi wrote:
> nit: compare as equal than->compare equal?

Done.

https://breakpad.appspot.com/570002/diff/9001/common/windows/omap_unittest.cc
File common/windows/omap_unittest.cc (right):

https://breakpad.appspot.com/570002/diff/9001/common/windows/omap_unittest.cc...
common/windows/omap_unittest.cc:41: return ::memcmp(&mr1, &mr2, sizeof(mr1)) ==
0;
On 2013/05/01 15:58:09, Siggi wrote:
> this only works if there's no padding in the mapped ranges, or if they're
> explicitly initialized with memset(). As-is this looks correct, but brittle to
> maintenance and extension.

Done.

https://breakpad.appspot.com/570002/diff/9001/common/windows/omap_unittest.cc...
common/windows/omap_unittest.cc:43: bool operator==(const EndpointIndex& ei1,
const EndpointIndex& ei2) {
On 2013/05/01 15:58:09, Siggi wrote:
> ditto.

Done.

https://breakpad.appspot.com/570002/diff/9001/common/windows/omap_unittest.cc...
common/windows/omap_unittest.cc:159: }
On 2013/05/01 15:58:09, Siggi wrote:
> how about testing that an empty map results in the identity mapping - as
that's
> necessary for backwards compatibility with non-Omapped images?

Done.

[Mark Mentovai]

LGTM

[chrisha]

Thanks, committing.