Keeping track of modules without symbols during crash report processing

The purpose of this change is to enable easy and fast troubleshooting of issues
related to missing symbols.

Although this change may look big, most of the stuff is related to unittests, so
it is pretty straightforward.

Essentially, this introduces a new member in the ProcessState object that is
getting populated with the modules that don't have symbols while the minidump is
being processed (during stack walk).  Note that this list will only contain
modules for which the symbols were actually needed (module address is on the
stack).

This will allow the tools built on top of the Breakpad processor (e.g.
Breakpad's minidump_stackwalk, Google's crash processor) to show the missing
symbols in the resulting crash reports.

Here is an example output from minidump_stackwalk:

==========================
...
Loaded modules:
0x7fa2915ef000 - 0x7fa2917f2fff libnss_cache-2.15.so
0x7fa2917f3000 - 0x7fa2919fffff libnss_files-2.15.so
0x7fa291fb8000 - 0x7fa29236cfff libc-2.15.so
...

Modules without symbols:       <<=============  new section
0x7fa291fb8000 - 0x7fa29236cfff libc-2.15.so
0x7fa292876000 - 0x7fa292a8efff libpthread-2.15.so
...
==========================


Thanks,
-Ivan

[Ivan Penkov]

Please, review!

Thanks,
-Ivan

[Mark Mentovai]

http://breakpad.appspot.com/534002/diff/1/processor/minidump_stackwalk.cc
File processor/minidump_stackwalk.cc (right):

http://breakpad.appspot.com/534002/diff/1/processor/minidump_stackwalk.cc#new...
processor/minidump_stackwalk.cc:472:
PrintModulesWithoutSymbols(process_state.modules_without_symbols());
Would this whole thing be easier if we didn’t break these into two categories
but printed them out together, coalesced, with the symbol-less modules marked
somehow?

I personally think that’d be better.

http://breakpad.appspot.com/534002/diff/1/processor/testdata/minidump2.stackw...
File processor/testdata/minidump2.stackwalk.out (right):

http://breakpad.appspot.com/534002/diff/1/processor/testdata/minidump2.stackw...
processor/testdata/minidump2.stackwalk.out:28: 0x59a60000 - 0x59b00fff
dbghelp.dll                          5.1.2600.2180             
dbghelp.pdb|39559573E21B46F28E286923BE9E6A761
This is an interesting choice of format.

[Ivan Penkov]

Mark's suggestions are addressed.  Please, take another look.

Thanks,
-Ivan

http://breakpad.appspot.com/534002/diff/1/processor/minidump_stackwalk.cc
File processor/minidump_stackwalk.cc (right):

http://breakpad.appspot.com/534002/diff/1/processor/minidump_stackwalk.cc#new...
processor/minidump_stackwalk.cc:472:
PrintModulesWithoutSymbols(process_state.modules_without_symbols());
On 2013/03/04 21:12:12, Mark Mentovai wrote:
> Would this whole thing be easier if we didn’t break these into two categories
> but printed them out together, coalesced, with the symbol-less modules marked
> somehow?
> 
> I personally think that’d be better.

Sure, this was just an option I picked.  We can do it either way.  Please, take
another look.  Now symbol-less modules are marked with (WARNING: no symbols,
debug_file, debug_id).  Here is an example.

0x6c5d0000 - 0x6c876fff UIPFull.dll                          2.3.2.244          
       (WARNING: No symbols, UIPFull.pdb, 5D85B380695441E5AA4251F14A681E651)

http://breakpad.appspot.com/534002/diff/1/processor/testdata/minidump2.stackw...
File processor/testdata/minidump2.stackwalk.out (right):

http://breakpad.appspot.com/534002/diff/1/processor/testdata/minidump2.stackw...
processor/testdata/minidump2.stackwalk.out:28: 0x59a60000 - 0x59b00fff
dbghelp.dll                          5.1.2600.2180             
dbghelp.pdb|39559573E21B46F28E286923BE9E6A761
On 2013/03/04 21:12:12, Mark Mentovai wrote:
> This is an interesting choice of format.

This is a convention we use in Google, but I agree that it may not be that
obvious for others.  I updated the output to only show the debug file and the
debug identifier for symbol-less modules and the format is:

 (WARNING: No symbols, debug_file, debug_id)

I hope this makes more sense.  I'm open to suggestions.

[Mark Mentovai]

http://breakpad.appspot.com/534002/diff/5001/processor/minidump_stackwalk.cc
File processor/minidump_stackwalk.cc (right):

http://breakpad.appspot.com/534002/diff/5001/processor/minidump_stackwalk.cc#...
processor/minidump_stackwalk.cc:345: return false;
I’d personally think that this is “true” in the sense that symbols are missing
for a module that can’t even be identified.

http://breakpad.appspot.com/534002/diff/5001/processor/minidump_stackwalk.cc#...
processor/minidump_stackwalk.cc:376: printf("0x%08" PRIx64 " - 0x%08" PRIx64 "
%-36s %-17s%-8s  %s\n",
Changing the output format here from %s to fields of a certain width seems a
little gratuitous and not at all related to this change. I don’t think it’s a
good idea. It’s making lines really long and is causing them to wrap on
80-column displays. It certainly does’t belong as part of this change.

[Ivan Penkov]

Reverted the changes to the table output.

http://breakpad.appspot.com/534002/diff/5001/processor/minidump_stackwalk.cc
File processor/minidump_stackwalk.cc (right):

http://breakpad.appspot.com/534002/diff/5001/processor/minidump_stackwalk.cc#...
processor/minidump_stackwalk.cc:345: return false;
On 2013/03/04 23:34:57, Mark Mentovai wrote:
> I’d personally think that this is “true” in the sense that symbols are missing
> for a module that can’t even be identified.

Yes, I understand.  I think that the names (or terms) used here don't self
describe their exact meaning.

The use of the terms "symbol-less" modules or modules "without symbols" in this
context is slightly stricter than what is implied by the names.

In this context, a "symbol-less" module is a module for which the symbols were
required during the stack-walk, load was attempted, and the symbols were
confirmed to be missing.

This stricter definition is better than the generic one for the following two
reasons:

 1. The smaller set of modules is better for troubleshooting.  These are the
kind of modules for which the symbols were required, and it may be worth to
investigate and supply the missing symbols.

 2. We simply don't have information for all modules.  We don't attempt to load
the symbols for all modules and we can't claim completeness.

In this context, skipping the unidentifiable modules (e.g. fonts, other data,
deleted modules, ..) would make more sense.  Even if we list them, there is
nothing the user can do because the symbols for these modules cannot be
identified by definition (or they simply don't exist).

http://breakpad.appspot.com/534002/diff/5001/processor/minidump_stackwalk.cc#...
processor/minidump_stackwalk.cc:376: printf("0x%08" PRIx64 " - 0x%08" PRIx64 "
%-36s %-17s%-8s  %s\n",
On 2013/03/04 23:34:57, Mark Mentovai wrote:
> Changing the output format here from %s to fields of a certain width seems a
> little gratuitous and not at all related to this change. I don’t think it’s a
> good idea. It’s making lines really long and is causing them to wrap on
> 80-column displays. It certainly does’t belong as part of this change.

OK.  Makes sense.  I reverted this part of my changes.

[Jess]

I don't have much to add beyond the current discussion.  Just a few comment
misspellings.

http://breakpad.appspot.com/534002/diff/11001/processor/minidump_stackwalk.cc
File processor/minidump_stackwalk.cc (right):

http://breakpad.appspot.com/534002/diff/11001/processor/minidump_stackwalk.cc...
processor/minidump_stackwalk.cc:364: // confrimed to be missing their symbols
during the stack walk.
confrimed -> confirmed

http://breakpad.appspot.com/534002/diff/11001/processor/minidump_stackwalk.cc...
processor/minidump_stackwalk.cc:386: // confrimed to be missing their symbols
during the stack walk.
Ditto

[Ivan Penkov]

Typos fixed.

http://breakpad.appspot.com/534002/diff/11001/processor/minidump_stackwalk.cc
File processor/minidump_stackwalk.cc (right):

http://breakpad.appspot.com/534002/diff/11001/processor/minidump_stackwalk.cc...
processor/minidump_stackwalk.cc:364: // confrimed to be missing their symbols
during the stack walk.
On 2013/03/05 01:39:43, Jess wrote:
> confrimed -> confirmed 

Done.

http://breakpad.appspot.com/534002/diff/11001/processor/minidump_stackwalk.cc...
processor/minidump_stackwalk.cc:386: // confrimed to be missing their symbols
during the stack walk.
On 2013/03/05 01:39:43, Jess wrote:
> Ditto

Done.

[Mark Mentovai]

http://breakpad.appspot.com/534002/diff/5001/processor/minidump_stackwalk.cc
File processor/minidump_stackwalk.cc (right):

http://breakpad.appspot.com/534002/diff/5001/processor/minidump_stackwalk.cc#...
processor/minidump_stackwalk.cc:345: return false;
You only mark a module as having missing symbols if a stackwalk winds through
it. Normally, a stackwalk wouldn’t wind through an unidentifiable module such as
a font. If it does, then it’s correct to identify that module as having missing
symbols, no matter what the reason for the symbols being missing was.

On 2013/03/05 00:18:17, Ivan Penkov wrote:
> On 2013/03/04 23:34:57, Mark Mentovai wrote:
> > I’d personally think that this is “true” in the sense that symbols are
missing
> > for a module that can’t even be identified.
> 
> Yes, I understand.  I think that the names (or terms) used here don't self
> describe their exact meaning.
> 
> The use of the terms "symbol-less" modules or modules "without symbols" in
this
> context is slightly stricter than what is implied by the names.
> 
> In this context, a "symbol-less" module is a module for which the symbols were
> required during the stack-walk, load was attempted, and the symbols were
> confirmed to be missing.
> 
> This stricter definition is better than the generic one for the following two
> reasons:
> 
>  1. The smaller set of modules is better for troubleshooting.  These are the
> kind of modules for which the symbols were required, and it may be worth to
> investigate and supply the missing symbols.
> 
>  2. We simply don't have information for all modules.  We don't attempt to
load
> the symbols for all modules and we can't claim completeness.
> 
> In this context, skipping the unidentifiable modules (e.g. fonts, other data,
> deleted modules, ..) would make more sense.  Even if we list them, there is
> nothing the user can do because the symbols for these modules cannot be
> identified by definition (or they simply don't exist).

[Ivan Penkov]

I'm renaming the function to ContainsModule and I'm removing the check for the
empty debug ID.

Thanks,
-Ivan

http://breakpad.appspot.com/534002/diff/5001/processor/minidump_stackwalk.cc
File processor/minidump_stackwalk.cc (right):

http://breakpad.appspot.com/534002/diff/5001/processor/minidump_stackwalk.cc#...
processor/minidump_stackwalk.cc:345: return false;
On 2013/03/05 16:37:45, Mark Mentovai wrote:
> You only mark a module as having missing symbols if a stackwalk winds through
> it. Normally, a stackwalk wouldn’t wind through an unidentifiable module such
as
> a font. If it does, then it’s correct to identify that module as having
missing
> symbols, no matter what the reason for the symbols being missing was.
> 
> On 2013/03/05 00:18:17, Ivan Penkov wrote:
> > On 2013/03/04 23:34:57, Mark Mentovai wrote:
> > > I’d personally think that this is “true” in the sense that symbols are
> missing
> > > for a module that can’t even be identified.
> > 
> > Yes, I understand.  I think that the names (or terms) used here don't self
> > describe their exact meaning.
> > 
> > The use of the terms "symbol-less" modules or modules "without symbols" in
> this
> > context is slightly stricter than what is implied by the names.
> > 
> > In this context, a "symbol-less" module is a module for which the symbols
were
> > required during the stack-walk, load was attempted, and the symbols were
> > confirmed to be missing.
> > 
> > This stricter definition is better than the generic one for the following
two
> > reasons:
> > 
> >  1. The smaller set of modules is better for troubleshooting.  These are the
> > kind of modules for which the symbols were required, and it may be worth to
> > investigate and supply the missing symbols.
> > 
> >  2. We simply don't have information for all modules.  We don't attempt to
> load
> > the symbols for all modules and we can't claim completeness.
> > 
> > In this context, skipping the unidentifiable modules (e.g. fonts, other
data,
> > deleted modules, ..) would make more sense.  Even if we list them, there is
> > nothing the user can do because the symbols for these modules cannot be
> > identified by definition (or they simply don't exist).
> 

Yes, I also thought about it yesterday.  You are correct.  If the stack walk
cannot find symbols for a module then we should print the warning regardless of
whether the module has a proper debug ID or not (the instruction can be in a
font or in a deleted module and we should warn that we were unable to load the
symbols for it).

Also, I think that I'm overloading this function with too much semantics.  All
that it is supposed to do is check whether a given module is in a list of
modules.

I'm renaming the function to ContainsModule and I'm removing the check for the
empty debug ID..

[Mark Mentovai]

LGTM with these changes.

http://breakpad.appspot.com/534002/diff/18001/processor/minidump_stackwalk.cc
File processor/minidump_stackwalk.cc (right):

http://breakpad.appspot.com/534002/diff/18001/processor/minidump_stackwalk.cc...
processor/minidump_stackwalk.cc:341: assert(module);
Nit: I’d put these in the same order that they appear in the argument list.

http://breakpad.appspot.com/534002/diff/18001/processor/minidump_stackwalk.cc...
processor/minidump_stackwalk.cc:362: missing_symbols = "(WARNING: No symbols, "
+
I would put the leading spaces into this string, and take them out of the
unconditional format string on line 367, so that the format string on line 367
would end with "%s%s%s\n" and each of the last two supplementary strings are
responsible for including their own spacing if present.

[Ivan Penkov]

Done.

http://breakpad.appspot.com/534002/diff/18001/processor/minidump_stackwalk.cc
File processor/minidump_stackwalk.cc (right):

http://breakpad.appspot.com/534002/diff/18001/processor/minidump_stackwalk.cc...
processor/minidump_stackwalk.cc:341: assert(module);
On 2013/03/05 20:19:58, Mark Mentovai wrote:
> Nit: I’d put these in the same order that they appear in the argument list.

Done.

http://breakpad.appspot.com/534002/diff/18001/processor/minidump_stackwalk.cc...
processor/minidump_stackwalk.cc:362: missing_symbols = "(WARNING: No symbols, "
+
On 2013/03/05 20:19:58, Mark Mentovai wrote:
> I would put the leading spaces into this string, and take them out of the
> unconditional format string on line 367, so that the format string on line 367
> would end with "%s%s%s\n" and each of the last two supplementary strings are
> responsible for including their own spacing if present.

Done.

[Mark Mentovai]

LGTM