Google-breakpad processor doesn't recognize and use context records generated by ::RtlCaptureContext

The Google-breakpad processor rejects (ignores) context records that lack CPU
type information in their context_flags fields.  Such context records can be
valid (e.g. contexts captured by ::RtlCaptureContext).

The proposed fix is to load the CPU type information from the system info stream
(which all minidumps should have) when the CPU type information is missing from
the context record.  This will allow processing of context records produced by
::RtlCaptureContext.

Two unittest were added:
 - A test that covers the scenario where CPU flags are missing from
context_flags but the CPU type can be loaded from the system info stream:
Dump.OneExceptionX86NoCPUFlag.
 - A test that covers the scenario where CPU flags are missing from
context_flags and there is no system info stream in the dump:
Dump.OneExceptionX86NoCPUFlagNoSystemInfo.

A few places in the code where the lack of CPU type information in the
context_flags was considered to be invalid had to be updated (including a
unittest).

While at it, I also enabled warning -Wreorder to be treated as error in
Makefile.am and I fixed the single occurrence of this issue in the constructor
of MinidumpContext.  It was failing to compile in our Google project and it is a
nice and small (one line) fix.

This issue is already tracked here:
http://code.google.com/p/google-breakpad/issues/detail?id=493&can=1&q=excepti...

Currently, this issue affects about 25% of all Chrome crash reports on Windows -
as measured on a random sample consisting of 2 million crash reports.

Thanks,
-Ivan

[Ivan Penkov]

.

[Jess]

LGTM.

cc-ing google breakpad dev group to bring everyone into the loop on the review.

http://breakpad.appspot.com/500002/diff/5001/src/processor/minidump.cc
File src/processor/minidump.cc (left):

http://breakpad.appspot.com/500002/diff/5001/src/processor/minidump.cc#oldcod...
src/processor/minidump.cc:323: //TODO: fall through to switch below?
Have you resolved this old TODO comment?

[Ivan Penkov]

.

http://breakpad.appspot.com/500002/diff/5001/src/processor/minidump.cc
File src/processor/minidump.cc (left):

http://breakpad.appspot.com/500002/diff/5001/src/processor/minidump.cc#oldcod...
src/processor/minidump.cc:323: //TODO: fall through to switch below?
On 2012/11/28 21:53:58, Jess wrote:
> Have you resolved this old TODO comment?

No, this is still unresolved.  This is a very good point though.  The TODO
comment suggests a nice to have refactoring.  It may as well become must have in
the future.  Currently, AMD64 context records are being detected by their size,
and if we ever have another architecture with the exact same context record size
then we'll be in trouble and we'll definitely need to implement the suggested
fall though mechanism.  My only problem with doing it now (in this change) is
that it will result in a large and messy refactoring that better be kept
separate.

[Mark Mentovai]

http://breakpad.appspot.com/500002/diff/5001/src/processor/minidump.cc
File src/processor/minidump.cc (right):

http://breakpad.appspot.com/500002/diff/5001/src/processor/minidump.cc#newcod...
src/processor/minidump.cc:322: if (0 == cpu_type) {
Unusual, write cpu_type == 0. Lines 434 and 3844 too. You wrote things in the
customary way on 4180 and 4202.

http://breakpad.appspot.com/500002/diff/5001/src/processor/minidump.cc#newcod...
src/processor/minidump.cc:4181: BPLOG_IF(ERROR, cookie != stream_)
You’re just checking stream_ but not the position in stream_? The header comment
indicated that you’d be checking the position. I don’t see the point of the
cookie if you’re not doing that.

http://breakpad.appspot.com/500002/diff/5001/src/processor/synth_minidump.cc
File src/processor/synth_minidump.cc (right):

http://breakpad.appspot.com/500002/diff/5001/src/processor/synth_minidump.cc#...
src/processor/synth_minidump.cc:131: assert((0 == (context.context_flags &
MD_CONTEXT_CPU_MASK)) ||
== 0, not 0 ==.

[Ivan Penkov]

I replaced 0 == with == 0.  Mark, I think I should get rid of the 'cookie'
stuff.  Can you take another look and confirm.

Thanks,
-Ivan

http://breakpad.appspot.com/500002/diff/5001/src/processor/minidump.cc
File src/processor/minidump.cc (right):

http://breakpad.appspot.com/500002/diff/5001/src/processor/minidump.cc#newcod...
src/processor/minidump.cc:322: if (0 == cpu_type) {
On 2012/12/07 22:39:26, Mark Mentovai wrote:
> Unusual, write cpu_type == 0. Lines 434 and 3844 too. You wrote things in the
> customary way on 4180 and 4202.

Sorry.  This is an old habit of mine to prevent mistakes resulting from
mistyping '=' instead of '=='.  I fixed it.  I believe gcc is now smart enough
to warn you if this ever occurs so this shouldn't be an issue anymore.

http://breakpad.appspot.com/500002/diff/5001/src/processor/minidump.cc#newcod...
src/processor/minidump.cc:4181: BPLOG_IF(ERROR, cookie != stream_)
On 2012/12/07 22:39:26, Mark Mentovai wrote:
> You’re just checking stream_ but not the position in stream_? The header
comment
> indicated that you’d be checking the position. I don’t see the point of the
> cookie if you’re not doing that.

The position in stream_ is expected to change.

The point of the 'cookie' is to ensure the underlying stream_ doesn't change
between the calls to Tell and SeekSet (in cases where you want to temporarily
change the current stream position, and then restore it).  Now that you are
raising this issue I examined the code and it appears that stream_ can only
change in Open and then it gets freed in the destructor so it seems impossible
to change between these two calls.  Let me know if you agree, but I think I can
get rid of the cookie altogether.

Thanks,
-Ivan

http://breakpad.appspot.com/500002/diff/5001/src/processor/synth_minidump.cc
File src/processor/synth_minidump.cc (right):

http://breakpad.appspot.com/500002/diff/5001/src/processor/synth_minidump.cc#...
src/processor/synth_minidump.cc:131: assert((0 == (context.context_flags &
MD_CONTEXT_CPU_MASK)) ||
On 2012/12/07 22:39:26, Mark Mentovai wrote:
> == 0, not 0 ==.

Done.

[Mark Mentovai]

Yup, I'd get rid of the cookie stuff.

gcc and clang definitely detect accidental assignments of constants to
variables in conditional expressions, so that old defensive practice of
putting the constant first should yield to writing in more natural
"language" by putting the constant last.
On Dec 7, 2012 6:57 PM, <ivan.penkov@gmail.com> wrote:

> I replaced 0 == with == 0.  Mark, I think I should get rid of the
> 'cookie' stuff.  Can you take another look and confirm.
>
> Thanks,
> -Ivan
>
>
>
http://breakpad.appspot.com/**500002/diff/5001/src/**processor/minidump.cc<ht...>
> File src/processor/minidump.cc (right):
>
> http://breakpad.appspot.com/**500002/diff/5001/src/**
>
processor/minidump.cc#**newcode322<http://breakpad.appspot.com/500002/diff/5001/src/processor/minidump.cc#newcode322>
> src/processor/minidump.cc:322: if (0 == cpu_type) {
> On 2012/12/07 22:39:26, Mark Mentovai wrote:
>
>> Unusual, write cpu_type == 0. Lines 434 and 3844 too. You wrote things
>>
> in the
>
>> customary way on 4180 and 4202.
>>
>
> Sorry.  This is an old habit of mine to prevent mistakes resulting from
> mistyping '=' instead of '=='.  I fixed it.  I believe gcc is now smart
> enough to warn you if this ever occurs so this shouldn't be an issue
> anymore.
>
> http://breakpad.appspot.com/**500002/diff/5001/src/**
>
processor/minidump.cc#**newcode4181<http://breakpad.appspot.com/500002/diff/5001/src/processor/minidump.cc#newcode4181>
> src/processor/minidump.cc:**4181: BPLOG_IF(ERROR, cookie != stream_)
> On 2012/12/07 22:39:26, Mark Mentovai wrote:
>
>> You’re just checking stream_ but not the position in stream_? The
>>
> header comment
>
>> indicated that you’d be checking the position. I don’t see the point
>>
> of the
>
>> cookie if you’re not doing that.
>>
>
> The position in stream_ is expected to change.
>
> The point of the 'cookie' is to ensure the underlying stream_ doesn't
> change between the calls to Tell and SeekSet (in cases where you want to
> temporarily change the current stream position, and then restore it).
> Now that you are raising this issue I examined the code and it appears
> that stream_ can only change in Open and then it gets freed in the
> destructor so it seems impossible to change between these two calls.
> Let me know if you agree, but I think I can get rid of the cookie
> altogether.
>
> Thanks,
> -Ivan
>
> http://breakpad.appspot.com/**500002/diff/5001/src/**
>
processor/synth_minidump.cc<http://breakpad.appspot.com/500002/diff/5001/src/processor/synth_minidump.cc>
> File src/processor/synth_minidump.**cc (right):
>
> http://breakpad.appspot.com/**500002/diff/5001/src/**
>
processor/synth_minidump.cc#**newcode131<http://breakpad.appspot.com/500002/diff/5001/src/processor/synth_minidump.cc#newcode131>
> src/processor/synth_minidump.**cc:131: assert((0 == (context.context_flags
> & MD_CONTEXT_CPU_MASK)) ||
> On 2012/12/07 22:39:26, Mark Mentovai wrote:
>
>> == 0, not 0 ==.
>>
>
> Done.
>
> http://breakpad.appspot.com/**500002/<http://breakpad.appspot.com/500002/>
>

[Ivan Penkov]

Done.  The cookie stuff is now gone.

Thanks,
-Ivan

[Mark Mentovai]

LGTM

http://breakpad.appspot.com/500002/diff/17001/src/processor/minidump.cc
File src/processor/minidump.cc (right):

http://breakpad.appspot.com/500002/diff/17001/src/processor/minidump.cc#newco...
src/processor/minidump.cc:3842: if (-1 == saved_position) {
Fix.

[Ivan Penkov]

On 2012/12/08 03:08:17, Mark Mentovai wrote:
> LGTM
> 
> http://breakpad.appspot.com/500002/diff/17001/src/processor/minidump.cc
> File src/processor/minidump.cc (right):
> 
>
http://breakpad.appspot.com/500002/diff/17001/src/processor/minidump.cc#newco...
> src/processor/minidump.cc:3842: if (-1 == saved_position) {
> Fix.

I missed that, sorry.  It is fixed now.

Thanks,
-Ivan