Change the way function and public symbols are obtained to use the findChildren DIA function.

Change the way function and public symbols are obtained to use the findChildren
DIA function.

This has a substantial performance improvement over using the getSymbolsByAddr
iterator, especially on certain 64bit DLLS.  e.g. Time to process
chrome_child.dll drops from 51 minutes to 21 secs.

Note: new test data looks different because the ordering of lines is no longer
by memory address.  This does not affect processing.  The test data has been
manually compared to old data and matches (except additional PUBLIC symbols). 
Also, INFO lines are omitted because the source executable files are not checked
in, so they are unavailable.

R=ivanpe@chromium.org, mark@chromium.org

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

[Will Harris]

I'm still updating the dump_syms_unittest data (as we now get a bit more PUBLIC
symbol data than before), but in the meantime I'd appreciate some views on this
CL.

[Mark Mentovai]

Seems OK. Wonder why the prebaked DIA routines to do this are so much slower. I
hope we’re not missing anything…

How is the output different? Is it strictly more PUBLICs than before, with
nothing removed? Can you discern anything about the PUBLICs that we’re adding
that caused them not to show up before?

https://breakpad.appspot.com/1574002/diff/1/src/common/windows/pdb_source_lin...
File src/common/windows/pdb_source_line_writer.cc (right):

https://breakpad.appspot.com/1574002/diff/1/src/common/windows/pdb_source_lin...
src/common/windows/pdb_source_line_writer.cc:359: CComPtr<IDiaSymbol> symbol =
NULL;
Can you declare this in a narrower scope? (You’d wind up declaring a different
“symbol” several times in this function, but it keeps distinct uses separate, so
that’s arguably a benefit.)

https://breakpad.appspot.com/1574002/diff/1/src/common/windows/pdb_source_lin...
src/common/windows/pdb_source_line_writer.cc:369: };
No semicolon. Same on line 391.

https://breakpad.appspot.com/1574002/diff/1/src/common/windows/pdb_source_lin...
src/common/windows/pdb_source_line_writer.cc:372: }
Need an else clause? Same on line 394.

https://breakpad.appspot.com/1574002/diff/1/src/common/windows/pdb_source_lin...
src/common/windows/pdb_source_line_writer.cc:384: rvas.insert(rva);
Is it faster to PrintFunction here if rva wasn’t already in the set, eliminating
the loop through the set below? If you do that, is the first symbol you get here
for a given RVA the same one that you would have gotten as the one from
findSymbolByRVA?

[Will Harris]

Thanks for the quick review.

I haven't worked out why the prebaked DIA routines are so slow, but they are
only slow right at the end of navigating the enumerator (i.e. last item). 
Looking at the stack, it looks like they are trying to find more data that
doesn't exist in the binary - it looks like it might be a bug in DIA.

The PUBLICs were not showing before because they were also declared as private
functions (FUNC) and DIA was only returning one function (the private one) when
querying the memory address - see comment in deleted code "For a given function,
DIA seems to give either a symbol SymTagFunction or SymTagPublicSymbol, but not
both.".

As I'm now navigating both trees, we get both.  I considered depuping the public
symbols against the private symbols to get identical output, but after looking
at the processor code, the additional PUBLIC symbols make no difference since
private FUNC data also takes precedence.

[Will Harris]

https://breakpad.appspot.com/1574002/diff/1/src/common/windows/pdb_source_lin...
File src/common/windows/pdb_source_line_writer.cc (right):

https://breakpad.appspot.com/1574002/diff/1/src/common/windows/pdb_source_lin...
src/common/windows/pdb_source_line_writer.cc:359: CComPtr<IDiaSymbol> symbol =
NULL;
That's my old school C coming out - trying to conserve stack space by reusing
variables.  I can certainly narrow down the scope if that's preferred.

https://breakpad.appspot.com/1574002/diff/1/src/common/windows/pdb_source_lin...
src/common/windows/pdb_source_line_writer.cc:372: }
no else clause neede here or there, because if the binary has no public symbols
then it's not necessarily an error.

https://breakpad.appspot.com/1574002/diff/1/src/common/windows/pdb_source_lin...
src/common/windows/pdb_source_line_writer.cc:384: rvas.insert(rva);
That would produce duplicate symbol lines, and I wanted to keep the output the
same as before.

[bsmedberg]

I am worried about this change. For Firefox PGO builds we know that there are
commonly two or more symbol ranges for a single public function because the hot
and cold code is compiled into separate ranges. Will this code that enumerates
by public symbol report on both of these code segments?

Related, see https://bugzilla.mozilla.org/show_bug.cgi?id=889822 where the code
range sizes reported by DIA are incorrect and were causing breakpad to throw
away symbols.

[Will Harris]

I should add I've done end-to-end testing on chrome://crash generating .sym
files manually with this new code and running minidump_stackwalk

There's two changes that this CL does to the output:

Firstly, the additional PUBLIC symbols.  As I've stated already, the private
(FUNC) data always takes precidence over the PUBLIC data - see
BasicSourceLineResolver::Module::LookupAddress in basic_source_line_resolver.cc
so I do not think this is an issue.

Secondly, I've also done manual diffs on the output before/after this change. 
DIA is non-deterministic in what symbol name it gives you when you ask for the
symbol at a particular address where there are multiple symbols (i.e. in WinDBG
or MSVC when you hit breakpoint you get that dialog with multiple options). 
When navigating using getSymbolsByAddr and then symbolByAddr (old code) it can
return one symbol, and when using findSymbolByRVA (new code) it can return
another string.

Example - chrome_child.dll has (at least) two symbols at address 40540.

new code returns:

FUNC 40540 26 0 content::ContentPluginClient::`scalar deleting
destructor'(unsigned int)

old code returns:

FUNC 40540 26 0 chrome::ChromeContentPluginClient::`scalar deleting
destructor'(unsigned int)

I would argue that the old code was returning an equally arbitrary symbol name
for a given address.  The proper fix for this is to return all symbol names in
the .sym files, and let the backend processing handle this correctly when
decoding the stack data.  I am in touch with ivanpe@ about doing exactly this.

Until then, I do not think this change has a negative impact on the quality of
the symbol files produced.  However, if you feel strongly that the output must
exactly match the old output (even though the old output was essentially
arbitrary in which name it picked for an address with multiple names), I can
certainly remove the additional PUBLIC symbols that clash with private ones, and
work on getting a matching output from findSymbolByRVA.

Arguably 889822 is fixed by this since we are now able to output overlapping
symbol names from dump_syms - we just don't have the backend processing to
support this yet.  That can be addressed in a future CL.

[Mark Mentovai]

OK, I was wondering if the only difference was about the now-deleted “FUNC or
PUBLIC but not both” comment or if there was something else. It’s not necessary
to de-dupe PUBLICs against FUNCs. Windows is the only platform where dump_syms
ever did that, our other platforms output PUBLICs from the symbol table and
FUNCs from the debug info, and the processor deals with it by preferring a FUNC
if it’s present for an address.

https://breakpad.appspot.com/1574002/diff/1/src/common/windows/pdb_source_lin...
File src/common/windows/pdb_source_line_writer.cc (right):

https://breakpad.appspot.com/1574002/diff/1/src/common/windows/pdb_source_lin...
src/common/windows/pdb_source_line_writer.cc:359: CComPtr<IDiaSymbol> symbol =
NULL;
wfh wrote:
> That's my old school C coming out - trying to conserve stack space by reusing
> variables.  I can certainly narrow down the scope if that's preferred.

Yeah, it’s preferred. When properly-scoped, the compiler can reuse stack space
formerly used by things that have gone out of scope.

https://breakpad.appspot.com/1574002/diff/1/src/common/windows/pdb_source_lin...
src/common/windows/pdb_source_line_writer.cc:384: rvas.insert(rva);
wfh wrote:
> That would produce duplicate symbol lines, and I wanted to keep the output the
> same as before.

How would it produce a duplicate? You’d still be using a set, you’d just be
checking for existence in the set before calling PrintFunction, instead of
collecting everything in the set and calling PrintFunction in a distinct loop
later.

In other words:

  if (rvas.count(rva) == 0) {
    rvas.insert(rva);
    if (!PrintFunction(symbol, symbol))
      return false;
  }

[Will Harris]

https://breakpad.appspot.com/1574002/diff/1/src/common/windows/pdb_source_lin...
File src/common/windows/pdb_source_line_writer.cc (right):

https://breakpad.appspot.com/1574002/diff/1/src/common/windows/pdb_source_lin...
src/common/windows/pdb_source_line_writer.cc:384: rvas.insert(rva);
Right, sorry I misunderstood what you were saying.  I can certainly do that.

In answer to your previous question - the first symbol obtained here is
occasionally different from the one from findSymbolByRVA - see comment #6

[Will Harris]

PTAL.

I should add that after diagnosing where the bug was occurring in the DIA
library (it was in the symbols->Next call on the final element in the
enumeration loop), an alternative approach I considered here was to enumerate
all the symbols using the more reliable API, determine the maximum RVA address
(i.e. the last symbol we should encounter using the old iterator code) and then
simply break out of the loop before we hit the DIA bug.

Even though this would result in identical output, and 'avoid' triggering the
DIA bug, I considered this a less elegant approach, and it doesn't leave the
door open for producing multiple symbol names when they exist for an address,
once the backend support is there for this, as well as being slower than the
findChildren approach.  However, the benefit of this alternative approach would
be that the output would be identical as before the change.

https://breakpad.appspot.com/1574002/diff/1/src/common/windows/pdb_source_lin...
File src/common/windows/pdb_source_line_writer.cc (right):

https://breakpad.appspot.com/1574002/diff/1/src/common/windows/pdb_source_lin...
src/common/windows/pdb_source_line_writer.cc:369: };
On 2014/04/16 16:56:28, Mark Mentovai wrote:
> No semicolon. Same on line 391.

Done.

[Mark Mentovai]

LGTM, but please watch very closely for fallout in crash reports once this
lands.

https://breakpad.appspot.com/1574002/diff/20002/src/common/windows/pdb_source...
File src/common/windows/pdb_source_line_writer.cc (right):

https://breakpad.appspot.com/1574002/diff/20002/src/common/windows/pdb_source...
src/common/windows/pdb_source_line_writer.cc:384: // we place the rva onto a set
here to uniquify them.
This sentence makes just as much sense without the word “we” (but with a comma
in its place).

https://breakpad.appspot.com/1574002/diff/20002/src/common/windows/pdb_source...
src/common/windows/pdb_source_line_writer.cc:385: if (rvas.count(rva) == 0) {
Cool.

This may be slightly faster if you used std::set::lower_bound to get an
iterator, and then used that iterator as a hint to the hint-taking overload of
std::set::insert. But the code is also a little more complex, and the gain may
not be worthwhile. Your call.

https://breakpad.appspot.com/1574002/diff/20002/src/common/windows/pdb_source...
src/common/windows/pdb_source_line_writer.cc:453: global.Release();
OK, not really necessary, but fine. I’ll allow it on the consistency principle:
blocks and parent a few lines above didn’t need to be Release()d either, but
they are.

[bsmedberg]

I am worried about this change. For Firefox PGO builds we know that there are
commonly two or more symbol ranges for a single public function because the hot
and cold code is compiled into separate ranges. Will this code that enumerates
by public symbol report on both of these code segments?

Related, see https://bugzilla.mozilla.org/show_bug.cgi?id=889822 where the code
range sizes reported by DIA are incorrect and were causing breakpad to throw
away symbols.

[bsmedberg]

oops, please ignore my duplicate comment

[Mark Mentovai]

Will, have you also taken any steps to make Microsoft aware of the performance
problem with IDiaEnumSymbolsByAddr?

[Will Harris]

MS are already aware of the bug and say "Unfortunately there is no work around
to this problem, unless you are willing to query the symbols any other way than
strictly by address."

http://connect.microsoft.com/VisualStudio/feedback/details/722366

[ivanpe]

https://breakpad.appspot.com/1574002/diff/80002/src/common/windows/pdb_source...
File src/common/windows/pdb_source_line_writer.cc (right):

https://breakpad.appspot.com/1574002/diff/80002/src/common/windows/pdb_source...
src/common/windows/pdb_source_line_writer.cc:384: // rva onto a set here to
uniquify them.
Will, how do you guarantee that the symbol returned for a given rva will be the
same as before your change?  I'm not sure how important that is, but I just
wanted to make sure it is discussed here.

[Will Harris]

> Will, how do you guarantee that the symbol returned for a given rva will be
the
> same as before your change?  I'm not sure how important that is, but I just
> wanted to make sure it is discussed here.

Hi Ivan - as discussed in #6 - we can't guarantee this behavior with the new
code, because the DIA library returns a different symbol name for the same
address depending on which API is used to access it.  I think we are okay with
this minor change to some of the symbol names, given the name we are already
using is equally arbitrary.

I will make sure that I alert you and your team when I push the new binary to
the build servers, and we can carefully monitor that there is no fallout from
this change.

[ivanpe]

On 2014/04/16 21:11:32, wfh wrote:
> > Will, how do you guarantee that the symbol returned for a given rva will be
> the
> > same as before your change?  I'm not sure how important that is, but I just
> > wanted to make sure it is discussed here.
> 
> Hi Ivan - as discussed in #6 - we can't guarantee this behavior with the new
> code, because the DIA library returns a different symbol name for the same
> address depending on which API is used to access it.  I think we are okay with
> this minor change to some of the symbol names, given the name we are already
> using is equally arbitrary.
> 
> I will make sure that I alert you and your team when I push the new binary to
> the build servers, and we can carefully monitor that there is no fallout from
> this change.

LGTM.  On my first pass I only looked at comments associated with line numbers
(I was looking at the diffs), so I missed your #6.

[Will Harris]

Committed patchset #4 manually as r1316 (presubmit successful).