(In "it outputs the following two addresses", "it" == "dump_syms".)

- gyp in breakpad isn't new enough to use VS2015
- gyp in breakpad with ninja fails due to missing some gtest file
- gyp in breakpad with vs doesn't generate dump_syms target (?...)
- ah, there's a manual vcproj in the dump_syms dir!

When I build with that, I can reproduce the same output (i.e. with a local debug build and a recent/local version of a DIA dll). The two output values are coming from

  void MapAddressRange(const ImageMap& image_map,
                       const AddressRange& original_range,
                       AddressRangeVector* mapped_ranges)

but I'll need to do more debugging to try to figure out the intricacies of that problem.

I'm not doing so well at understanding the OMAP data, but from slugging around in the Win7 x86 and x64 ntdll.dlls for far too long, I am at least now convinced that TppWaiterpThread really is where the exception is happening (i.e. we don't have to all give up and become baristas). The function's splattered all over in about 100 basic blocks and interweaved with other functions that seemed plausible (e.g. LdrpInitializeTls), so it's a bit tedious to trace through.

So, back to dump_syms, one (maybe?) interesting thing is that the RVA (0x35dc0) for TppWaiterpThread falls into an OMAP range 

+		[14759]	{rva_transformed=0x00035d92 rva_original=0x000254ae }	google_breakpad::`anonymous-namespace'::OmapTranToOrig
+		[14760]	{rva_transformed=0x00035db7 rva_original=0x00000000 }	google_breakpad::`anonymous-namespace'::OmapTranToOrig
+		[14761]	{rva_transformed=0x00035dd0 rva_original=0x0002c6c0 }	google_breakpad::`anonymous-namespace'::OmapTranToOrig

with an rva_original == 0. It's not the only one, but it's relatively uncommon. I'm not clear on if that is important or not to this problem.

Hmm, I had been looking at an x64 dump. Looking at an x86 one now (crash 7e90419080000000) dump_syms outputs only *one* entry for TppWaiterpThread, which corresponds to the entry point of the function (RVA 2FC03 == 77EEFC03 at default base)

But there's clearly other functions between that location and the far away other part of the function where the exception is happening. So now I'm wondering if there's more data in the PDB that we're not even trying to process via DIA?

…or even if it’s available via DIA. I’ve got this background fear that one day we won’t be able to get what we need through that interface. The PDB internals can change a lot more quickly than DIA seems to be able to.

Is the correct version of msdia*.dll registered? In the past, that’s magically solved problems.

I was wondering that too. When I run in the debugger I see

'dump_syms.exe' (Win32): Loaded 'C:\Program Files (x86)\Microsoft Visual Studio 14.0\Common7\Packages\Debugger\msdia140.dll'.

so I think I'm using something current-ish. Playing around with Dia2Dump to see if there's anything we might be missing.

> …or even if it’s available via DIA.

Good news: Here's the dump for the RVA entry point of TppWaiterpThread of 0x2fc03:

c:\stuff\x86>"C:\Program Files (x86)\Microsoft Visual Studio 14.0\DIA SDK\Samples\DIA2Dump\x64\Debug\Dia2Dump.exe" -sym 0x2fC03 ntdll.dll
Displacement = 0x0
                        SymIndex:        0x1
                          SymTag:        0xA
                            Name:        _TppWaiterpThread@4
                   LexicalParent:        000001F80E5755B0
                    LocationType:        0x1
                  AddressSection:        0x1
                   AddressOffset:        0xA5AD9
          RelativeVirtualAddress:        0x2FC03
                  VirtualAddress:        0x2FC03
                          Length:        0x72E
                 LexicalParentId:        0x2
                            Code:        true
                        Function:        true
                         Managed:        false
                            MSIL:        false
                 UndecoratedName:        _TppWaiterpThread@4
                  export is DATA:        false
PublicSymbol: [0002FC03][0001:000A5AD9] _TppWaiterpThread@4(_TppWaiterpThread@4)

Parent
Executable (Global): ntdll



And then at the RVA of where the exception happened:

c:\stuff\x86>"C:\Program Files (x86)\Microsoft Visual Studio 14.0\DIA SDK\Samples\DIA2Dump\x64\Debug\Dia2Dump.exe" -sym 0x9b8ce ntdll.dll
Displacement = 0x623
                        SymIndex:        0x1
                          SymTag:        0xA
                            Name:        _TppWaiterpThread@4
                   LexicalParent:        0000019F816359B0
                    LocationType:        0x1
                  AddressSection:        0x1
                   AddressOffset:        0xA5AD9
          RelativeVirtualAddress:        0x2FC03
                  VirtualAddress:        0x2FC03
                          Length:        0x72E
                 LexicalParentId:        0x2
                            Code:        true
                        Function:        true
                         Managed:        false
                            MSIL:        false
                 UndecoratedName:        _TppWaiterpThread@4
                  export is DATA:        false
PublicSymbol: [0002FC03][0001:000A5AD9] _TppWaiterpThread@4(_TppWaiterpThread@4)

Parent
Executable (Global): ntdll


So the data seems to be available to a more cunning user of DIA. The "Displacement" here also matches what windbg displays at the .ecxr location:

  03e6f830 7619ef1c ntdll!TppWaiterpThread+0x623

So that makes me believe we're in luck and this is just a plain old bug, though I'm not sure where in dump_syms at all yet.

It's a little unsatisfying in that it just calls IDiaSession::findSymbolByRVAEx() which returns that result. So I guess it's still possible that it's using data that isn't otherwise exposed in raw form. (?)

Or maybe we do something in dump_syms to avoid generating output with overlapping ranges, and a couple of functions both claim the range we’re interested in, so all but one lose.

Here's what I've figured out so far:

The parsing of OMAP data seems fine in dump_syms. I traced through the target location processing. The symbol data for 0x9b8ce has a mapping to 0xa70fc (original) just past 0x2fc03 which is mapped to 0xa6ad9 (which is what TppWaiterpThread maps to for entry point).

However, there's nothing at the top level that ever asks for that mapping. In the Win7 symbols, there's only "public symbols" for each function (nothing of type "function"). Additionally, when iterating all compilands (obj files) and asking for all basic blocks, there's no "functions" in that list. That seems to be where we attempt to handle the PGO case of functions split up over large ranges.

OTOH, in the Win10 symbols, there's the public symbol, but also functions. TppWaiterpThread no longer exists in win10, but e.g. there's _RtlMultiAppendUnicodeStringBuffer@12 (symbol), a global RtlMultiAppendUnicodeStringBuffer (function), and in the buffer.obj compiland another a block that's explicitly attached to RtlMultiAppendUnicodeStringBuffer, which would cover the additional block of the function at a far away address. The win7 symbols have none of that.

[ Interestingly I found this old crash which lists TppWaiterpThread as its crash location and on Win7. Maybe it predates Breakpad? Or maybe the Windows 7 symbols used to use function types but were "broken" in later revisions. https://bugs.chromium.org/p/chromium/issues/detail?id=10703 ]

(+wfh out of general interest or maybe knowing things about this.)

I guess instead of walking the compilands, we would have to do something like walking the whole address space, finding original locations for each RVA in the image, finding the function that it'd be in, then clumping those together and fabricating fake function blocks for them.

In the symbols for win7, there's an RVA hole between 7c880 and 9e29b, i.e.:

PUBLIC 7c880 8 MD4Transform
PUBLIC 9e29b 8 LdrpRelocateStartContext

which is why crash 7e90419080000000 gets assigned to MD4Transform, so we need to "fill in" that gap.

I'm not quite sure how that code would look yet, or whether its worth the effort or not.

If all we have are SymTagPublicSymbol, then we don’t really know the length of the code to synthesize a function. Unless we could infer that from the OMAP?

> I guess instead of walking the compilands,

We don’t walk the compilands to grab these now, we just walk the symbols. The compiland walking thing is just to grab source file names and the line numbers hanging off of them.

From https://developer.microsoft.com/en-us/windows/hardware/download-symbols:

"""Feedback- We are interested in your feedback about symbols. Please mail suggestions or bug reports to windbgfb@microsoft.com. Technical support is not available from this address, but your feedback will help us to plan future changes for symbols and will make them more useful to you in the future."""

Usually I use this email alias to complain when Microsoft's symbol server is not working but this also sounds a valid use. Maybe send drop them a line with your discoveries and see if 

Cut off my own comment...

and see if they have any suggestions.

> > I guess instead of walking the compilands,
> 
> We don’t walk the compilands to grab these now, we just walk the symbols. The
> compiland walking thing is just to grab source file names and the line numbers
> hanging off of them.

Are you sure? I'm somewhat confident that the PrintFunction() following

https://chromium.googlesource.com/breakpad/breakpad/+/master/src/common/windows/pdb_source_line_writer.cc#476

is why this works on the Win10 symbols.

I don't see how else we would emit multiple locations for a function other than for the entry point. Here https://chromium.googlesource.com/breakpad/breakpad/+/master/src/common/windows/pdb_source_line_writer.cc#853 we look up the ranges the correspond to the RVA for a function, but that would only handle the first block of a function. But it doesn't seem like it'd handle separate non-contiguous addresses that the function jumps to later.

Hmm. This is what I was thinking about: https://chromium.googlesource.com/breakpad/breakpad/+/master/src/common/windows/pdb_source_line_writer.cc#394

Nothing to handle discontiguity in there either, though.

Right, there's no "functions" for Win7, only "public symbols" which are much more anemic in terms of child nodes.

I guess bugdroid doesn't do breakpad, but I landed https://chromium.googlesource.com/breakpad/breakpad/+/19af23e3c033fb79e6157d7c113d50b6473e1243 for this.

Ivan, could import past this point into the processor when you have some time?

That's great Scott. It's amazing that this issue has existed for long without being diagnosed. The fix should avoid a lot of confusion.

Thanks for fixing this Scott.  We would need to import this into Google3 and then rebuild the symbol converter.

In the shower this morning I realized this is client-only (in dump_syms) so no google3 merge required.

It'd be good to get into 57 though, so I'll get it merged back there.

Usually, symbols for Windows DLLs are not generated as part of regular builds, so someone either needs to manually run dump_symps and upload the Windows system symbols or we can rely on the Windows Symbol Converter.  The Windows Symbol Converter checks regularly (every 20 minutes or so) for missing Windows symbols, fetches the symbols from Microsoft public symbols servers, converts them to Breakpad and uploads to Crash Collector.  In this case we already have bad symbols so the Windows Symbol Converter won't re-process the bad symbols without manual intervention.  After it is updated, we'll need to delete the old Windows system symbols so that it can see them as missing and upload better symbols.

Oh, right! I was thinking we uploaded everything from client builds, but of course not Windows symbols, only Chrome symbols.

Are you able to take on the work you described in #c24? If not, can we sync up offline on that, as I'm not sure about how to accomplish a number of steps you mention.

We haven't started working on the steps described in #c24 yet.  We should be able to start working on them sometime next week.  The breakpad import and the Symbol Converter rebuild and release will take some time (it lives in 3 VMs hosted in CGE).  I'll try to find someone to start working on it next week.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/12a3f61803fefb0aeb06f3f4093ea8b06a29fbb3

commit 12a3f61803fefb0aeb06f3f4093ea8b06a29fbb3
Author: scottmg <scottmg@chromium.org>
Date: Fri Feb 24 23:04:27 2017

Roll breakpad 37070c5..eaedc45

eaedc45 Improve stack sanitization unittests.
0ca6751 Handle ntdll only emitting PUBLIC at func entry
c7e826f Make stack sanitization elide pointers to non-executable mappings.
918f3dc Use the correct PC when determining whether to skip storing a stack.
f9d3ab8 minidump: mark Read as override in derived classes
3ff7ca4 Fix compile errors arising from compiling breakpad with clang.
3996c1b libdisasm: add upstream/license details
603f8b6 There is no need to use the main queue just for perform selector.
e6123b1 Appveyor CI for Windows MSVS build
376784d processor: drop set-but-unused variable
122d65d macho_reader_unittest: use EXPECT_FALSE
093fab2 windows: fix build on pre-Win10 systems
620e0fb windows: update gtest/gmock paths
fc92bb3 fix write() unused-result warning
d5f233d Fixed leak of unloaded module lists.

symupload.exe was built by generating a vsproj using gyp, and building a
Release build in VS 2015.

R=thestig@chromium.org
BUG= 678874 

Review-Url: https://codereview.chromium.org/2719553002
Cr-Commit-Position: refs/heads/master@{#452979}

[modify] https://crrev.com/12a3f61803fefb0aeb06f3f4093ea8b06a29fbb3/DEPS
[modify] https://crrev.com/12a3f61803fefb0aeb06f3f4093ea8b06a29fbb3/breakpad/symupload.exe

This bug requires manual review: DEPS changes referenced in bugdroid comments.
Please contact the milestone owner if you have questions.
Owners: amineer@(clank), cmasso@(bling), ketakid@(cros), govind@(desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

I was mistaken, it's not useful to merge this after all, removing labels.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/f1b19ba2112904f307bd9a7d59f1e6bace633e89

commit f1b19ba2112904f307bd9a7d59f1e6bace633e89
Author: scottmg <scottmg@chromium.org>
Date: Sun Feb 26 07:02:57 2017

Revert of Roll breakpad 37070c5..eaedc45 (patchset #2 id:20001 of https://codereview.chromium.org/2719553002/ )

Reason for revert:
Likely causing crbug.com/696257.

Original issue's description:
> Roll breakpad 37070c5..eaedc45
>
> eaedc45 Improve stack sanitization unittests.
> 0ca6751 Handle ntdll only emitting PUBLIC at func entry
> c7e826f Make stack sanitization elide pointers to non-executable mappings.
> 918f3dc Use the correct PC when determining whether to skip storing a stack.
> f9d3ab8 minidump: mark Read as override in derived classes
> 3ff7ca4 Fix compile errors arising from compiling breakpad with clang.
> 3996c1b libdisasm: add upstream/license details
> 603f8b6 There is no need to use the main queue just for perform selector.
> e6123b1 Appveyor CI for Windows MSVS build
> 376784d processor: drop set-but-unused variable
> 122d65d macho_reader_unittest: use EXPECT_FALSE
> 093fab2 windows: fix build on pre-Win10 systems
> 620e0fb windows: update gtest/gmock paths
> fc92bb3 fix write() unused-result warning
> d5f233d Fixed leak of unloaded module lists.
>
> symupload.exe was built by generating a vsproj using gyp, and building a
> Release build in VS 2015.
>
> R=thestig@chromium.org
> BUG= 678874 
>
> Review-Url: https://codereview.chromium.org/2719553002
> Cr-Commit-Position: refs/heads/master@{#452979}
> Committed: https://chromium.googlesource.com/chromium/src/+/12a3f61803fefb0aeb06f3f4093ea8b06a29fbb3

TBR=thestig@chromium.org
# Not skipping CQ checks because original CL landed more than 1 days ago.
BUG= 678874 

Review-Url: https://codereview.chromium.org/2719703004
Cr-Commit-Position: refs/heads/master@{#453115}

[modify] https://crrev.com/f1b19ba2112904f307bd9a7d59f1e6bace633e89/DEPS
[modify] https://crrev.com/f1b19ba2112904f307bd9a7d59f1e6bace633e89/breakpad/symupload.exe

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/5f3ea1407ad98cdb9d8418a52d3dbeff7ec5033e

commit 5f3ea1407ad98cdb9d8418a52d3dbeff7ec5033e
Author: scottmg <scottmg@chromium.org>
Date: Mon Feb 27 20:46:55 2017

Roll breakpad 37070c5..eaedc45

eaedc45 Improve stack sanitization unittests.
0ca6751 Handle ntdll only emitting PUBLIC at func entry
c7e826f Make stack sanitization elide pointers to non-executable mappings.
918f3dc Use the correct PC when determining whether to skip storing a stack.
f9d3ab8 minidump: mark Read as override in derived classes
3ff7ca4 Fix compile errors arising from compiling breakpad with clang.
3996c1b libdisasm: add upstream/license details
603f8b6 There is no need to use the main queue just for perform selector.
e6123b1 Appveyor CI for Windows MSVS build
376784d processor: drop set-but-unused variable
122d65d macho_reader_unittest: use EXPECT_FALSE
093fab2 windows: fix build on pre-Win10 systems
620e0fb windows: update gtest/gmock paths
fc92bb3 fix write() unused-result warning
d5f233d Fixed leak of unloaded module lists.

Previous reverted attempt was
https://codereview.chromium.org/2719553002. This rolls in the same code.
However, this time symupload was instead built with VS2013. I confirmed
the link line included:

/LIBPATH:"C:\Program Files (x86)\Microsoft Visual Studio 12.0\/DIA SDK/lib"

and:

c:\src\cr\src\breakpad>dumpbin /headers symupload.exe | grep linker
           12.00 linker version

This is expected to help because the suspicion is that only msdia120.dll
is registered on official bots, not msdia140.dll.

R=mark@chromium.org
BUG= 678874 ,696257, 696671 

Review-Url: https://codereview.chromium.org/2718173002
Cr-Commit-Position: refs/heads/master@{#453325}

[modify] https://crrev.com/5f3ea1407ad98cdb9d8418a52d3dbeff7ec5033e/DEPS
[modify] https://crrev.com/5f3ea1407ad98cdb9d8418a52d3dbeff7ec5033e/breakpad/symupload.exe

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/8af45a18bab6cf4f1dc16a92a229dfa2b7e81dd1

commit 8af45a18bab6cf4f1dc16a92a229dfa2b7e81dd1
Author: scottmg <scottmg@chromium.org>
Date: Tue Feb 28 18:33:17 2017

Revert of Roll breakpad 37070c5..eaedc45 (patchset #1 id:1 of https://codereview.chromium.org/2718173002/ )

Reason for revert:
Failing for chrome_child.dll on x86 builds only. crbug.com/696911.

Original issue's description:
> Roll breakpad 37070c5..eaedc45
>
> eaedc45 Improve stack sanitization unittests.
> 0ca6751 Handle ntdll only emitting PUBLIC at func entry
> c7e826f Make stack sanitization elide pointers to non-executable mappings.
> 918f3dc Use the correct PC when determining whether to skip storing a stack.
> f9d3ab8 minidump: mark Read as override in derived classes
> 3ff7ca4 Fix compile errors arising from compiling breakpad with clang.
> 3996c1b libdisasm: add upstream/license details
> 603f8b6 There is no need to use the main queue just for perform selector.
> e6123b1 Appveyor CI for Windows MSVS build
> 376784d processor: drop set-but-unused variable
> 122d65d macho_reader_unittest: use EXPECT_FALSE
> 093fab2 windows: fix build on pre-Win10 systems
> 620e0fb windows: update gtest/gmock paths
> fc92bb3 fix write() unused-result warning
> d5f233d Fixed leak of unloaded module lists.
>
> Previous reverted attempt was
> https://codereview.chromium.org/2719553002. This rolls in the same code.
> However, this time symupload was instead built with VS2013. I confirmed
> the link line included:
>
> /LIBPATH:"C:\Program Files (x86)\Microsoft Visual Studio 12.0\/DIA SDK/lib"
>
> and:
>
> c:\src\cr\src\breakpad>dumpbin /headers symupload.exe | grep linker
>            12.00 linker version
>
> This is expected to help because the suspicion is that only msdia120.dll
> is registered on official bots, not msdia140.dll.
>
> R=mark@chromium.org
> BUG= 678874 ,696257, 696671 
>
> Review-Url: https://codereview.chromium.org/2718173002
> Cr-Commit-Position: refs/heads/master@{#453325}
> Committed: https://chromium.googlesource.com/chromium/src/+/5f3ea1407ad98cdb9d8418a52d3dbeff7ec5033e

TBR=mark@chromium.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG= 678874 ,696257, 696671 

Review-Url: https://codereview.chromium.org/2726633002
Cr-Commit-Position: refs/heads/master@{#453647}

[modify] https://crrev.com/8af45a18bab6cf4f1dc16a92a229dfa2b7e81dd1/DEPS
[modify] https://crrev.com/8af45a18bab6cf4f1dc16a92a229dfa2b7e81dd1/breakpad/symupload.exe

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/7f67e4310802a5f05d702276d5111c9b99391921

commit 7f67e4310802a5f05d702276d5111c9b99391921
Author: scottmg <scottmg@chromium.org>
Date: Tue Feb 28 21:14:08 2017

Roll breakpad 37070c5..10162f6

10162f6 win: Set LargeAddressAware on symupload
eaedc45 Improve stack sanitization unittests.
0ca6751 Handle ntdll only emitting PUBLIC at func entry
c7e826f Make stack sanitization elide pointers to non-executable mappings.
918f3dc Use the correct PC when determining whether to skip storing a stack.
f9d3ab8 minidump: mark Read as override in derived classes
3ff7ca4 Fix compile errors arising from compiling breakpad with clang.
3996c1b libdisasm: add upstream/license details
603f8b6 There is no need to use the main queue just for perform selector.
e6123b1 Appveyor CI for Windows MSVS build
376784d processor: drop set-but-unused variable
122d65d macho_reader_unittest: use EXPECT_FALSE
093fab2 windows: fix build on pre-Win10 systems
620e0fb windows: update gtest/gmock paths
fc92bb3 fix write() unused-result warning
d5f233d Fixed leak of unloaded module lists.

Previous attempts https://codereview.chromium.org/2719553002 and
https://codereview.chromium.org/2718173002. This time, we have both a
MSVS 2013 build, and the LargeAddressAware flag set on the .exe.

[roll-breakpad-3]c:\src\cr\src>dumpbin /headers breakpad\symupload.exe | grep linker
           12.00 linker version

[roll-breakpad-3]c:\src\cr\src>dumpbin /headers breakpad\symupload.exe | grep large
                   Application can handle large (>2GB) addresses

R=mark@chromium.org
BUG= 678874 ,696257, 696671 ,696911

Review-Url: https://codereview.chromium.org/2720873004
Cr-Commit-Position: refs/heads/master@{#453702}

[modify] https://crrev.com/7f67e4310802a5f05d702276d5111c9b99391921/DEPS
[modify] https://crrev.com/7f67e4310802a5f05d702276d5111c9b99391921/breakpad/symupload.exe

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/73dd82d9969add23c5070c12cdbeba8e9e3f13ff

commit 73dd82d9969add23c5070c12cdbeba8e9e3f13ff
Author: scottmg <scottmg@chromium.org>
Date: Wed Mar 01 23:19:06 2017

Try x64 binary for symupload

symupload is still failing for chrome_child.dll on the 32 bit official
bots. One last try of making an x64 binary instead, and then I'm going
to give up.

TBR=mark@chromium.org
BUG= 678874 , 696257,  696671 , 696911,697638

Review-Url: https://codereview.chromium.org/2723233003
Cr-Commit-Position: refs/heads/master@{#454089}

[modify] https://crrev.com/73dd82d9969add23c5070c12cdbeba8e9e3f13ff/breakpad/symupload.exe

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/bf0b0926a4ad3ad196540c7a9c31c465591fc645

commit bf0b0926a4ad3ad196540c7a9c31c465591fc645
Author: Scott Graham <scottmg@chromium.org>
Date: Thu Mar 02 02:24:12 2017

Try x64 binary for symupload

symupload is still failing for chrome_child.dll on the 32 bit official
bots. One last try of making an x64 binary instead, and then I'm going
to give up.

TBR=mark@chromium.org
BUG= 678874 , 696257,  696671 , 696911,697638

Review-Url: https://codereview.chromium.org/2723233003
Cr-Commit-Position: refs/heads/master@{#454089}
(cherry picked from commit 73dd82d9969add23c5070c12cdbeba8e9e3f13ff)

Review-Url: https://codereview.chromium.org/2728903002 .
Cr-Commit-Position: refs/branch-heads/3026@{#7}
Cr-Branched-From: fe586ab75aca1b8ab839db23bceac5f621389fed-refs/heads/master@{#453454}

[modify] https://crrev.com/bf0b0926a4ad3ad196540c7a9c31c465591fc645/breakpad/symupload.exe