Create StackwalkerAddressList.

Create StackwalkerAddressList.

This creates a pseudo stack-walker which does nothing except symbolize an
already walked array of addresses. Will be used for adding 'additional stack
trace' support to MinidumpProcessor.

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

[chrisha]

PTAL

[Mark Mentovai]

https://breakpad.appspot.com/620002/diff/7/src/processor/stackwalker_null.cc
File src/processor/stackwalker_null.cc (right):

https://breakpad.appspot.com/620002/diff/7/src/processor/stackwalker_null.cc#...
src/processor/stackwalker_null.cc:36: #include <cassert>
It is normal to #include the C header, not the C++ wrapper: <assert.h>.

https://breakpad.appspot.com/620002/diff/7/src/processor/stackwalker_null.cc#...
src/processor/stackwalker_null.cc:49: StackwalkerNull::StackwalkerNull(const
uint64_t* frames,
Null isn’t a great name for this. That makes it sound like it doesn’t do
anything at all, especially if you’re familiar with stackwalkers being
CPU-specific. “StackwalkerX86” would know about the frame layout on x86,
“StackwalkerNull” sounds like a no-op implementation for CPUs that the Breakpad
processor doesn’t understand.

Since it works on a list of addresses that have already been “walked,” how about
StackwalkerAddressList? Since it’s not really “walking,” you could even get away
with PseudoStackwalkerAddressList…

https://breakpad.appspot.com/620002/diff/7/src/processor/stackwalker_null.cc#...
src/processor/stackwalker_null.cc:82: // given to us.
Avoid “us,” “we.” This would be fine if it ended as “explicitly given” or
“explicitly provided.”

Same on line 77.

https://breakpad.appspot.com/620002/diff/7/src/processor/stackwalker_null.h
File src/processor/stackwalker_null.h (right):

https://breakpad.appspot.com/620002/diff/7/src/processor/stackwalker_null.h#n...
src/processor/stackwalker_null.h:37: #ifndef PROCESSOR_STACKWALKER_NULL_H__
You’re supposed to use only one trailing underscore per the style guide. Two
trailing underscores are reserved for the system.

https://breakpad.appspot.com/620002/diff/7/src/processor/stackwalker_null.h#n...
src/processor/stackwalker_null.h:66: };
DISALLOW_COPY_AND_ASSIGN?

[Ivan Penkov]

I agree with Mark's assessment.  The main issue for me is also the name of the
class.  Otherwise, the code and the unittests look good.

[chrisha]

Addressed Mark's comments. PTAnotherL.

(Sorry for the crappy diff, but the rename made upload.py happy.)

https://breakpad.appspot.com/620002/diff/7/src/processor/stackwalker_null.cc
File src/processor/stackwalker_null.cc (right):

https://breakpad.appspot.com/620002/diff/7/src/processor/stackwalker_null.cc#...
src/processor/stackwalker_null.cc:36: #include <cassert>
On 2013/08/22 00:09:53, Mark Mentovai wrote:
> It is normal to #include the C header, not the C++ wrapper: <assert.h>.

Done.

https://breakpad.appspot.com/620002/diff/7/src/processor/stackwalker_null.cc#...
src/processor/stackwalker_null.cc:49: StackwalkerNull::StackwalkerNull(const
uint64_t* frames,
On 2013/08/22 00:09:53, Mark Mentovai wrote:
> Null isn’t a great name for this. That makes it sound like it doesn’t do
> anything at all, especially if you’re familiar with stackwalkers being
> CPU-specific. “StackwalkerX86” would know about the frame layout on x86,
> “StackwalkerNull”&nbsp;sounds like a no-op implementation for CPUs that the
Breakpad
> processor doesn’t understand.
> 
> Since it works on a list of addresses that have already been “walked,” how
about
> StackwalkerAddressList? Since it’s not really “walking,” you could even get
away
> with PseudoStackwalkerAddressList…

Changed this to StackwalkerAddressList (no leading pseudo, to reflect the naming
convention of other stack walkers).

https://breakpad.appspot.com/620002/diff/7/src/processor/stackwalker_null.cc#...
src/processor/stackwalker_null.cc:82: // given to us.
On 2013/08/22 00:09:53, Mark Mentovai wrote:
> Avoid “us,”&nbsp;“we.” This would be fine if it ended as “explicitly given” or
> “explicitly provided.”
> 
> Same on line 77.

Done.

https://breakpad.appspot.com/620002/diff/7/src/processor/stackwalker_null.h
File src/processor/stackwalker_null.h (right):

https://breakpad.appspot.com/620002/diff/7/src/processor/stackwalker_null.h#n...
src/processor/stackwalker_null.h:37: #ifndef PROCESSOR_STACKWALKER_NULL_H__
On 2013/08/22 00:09:53, Mark Mentovai wrote:
> You’re supposed to use only one trailing underscore per the style guide. Two
> trailing underscores are reserved for the system.

Every other .h file in the project uses 2 trailing underscores, and I was simply
following that convention.

https://breakpad.appspot.com/620002/diff/7/src/processor/stackwalker_null.h#n...
src/processor/stackwalker_null.h:66: };
On 2013/08/22 00:09:53, Mark Mentovai wrote:
> DISALLOW_COPY_AND_ASSIGN?

Aha... I hadn't seen that anywhere else in Breakpad, and didn't think we had it.
Turns out it lives in common/basictypes.h.

Done.

[Mark Mentovai]

LG otherwise

https://breakpad.appspot.com/620002/diff/11001/src/processor/stackwalker_addr...
File src/processor/stackwalker_address_list.cc (right):

https://breakpad.appspot.com/620002/diff/11001/src/processor/stackwalker_addr...
src/processor/stackwalker_address_list.cc:37: #include <assert.h>
C system headers before C++ system headers. We also put a blank line between the
C and C++ sections.

https://breakpad.appspot.com/620002/diff/11001/src/processor/stackwalker_addr...
src/processor/stackwalker_address_list.cc:64: frame->instruction = frames_[0];
If frame_count_ is 0, return NULL instead of doing this potentially-dangerous
thing.

https://breakpad.appspot.com/620002/diff/11001/src/processor/stackwalker_addr...
src/processor/stackwalker_address_list.cc:68: 
Are you leaving one blank line between things or two? You seem to have done two
elsewhere in this file, but this time, you did one.

https://breakpad.appspot.com/620002/diff/11001/src/processor/stackwalker_addr...
File src/processor/stackwalker_address_list.h (right):

https://breakpad.appspot.com/620002/diff/11001/src/processor/stackwalker_addr...
src/processor/stackwalker_address_list.h:38: #ifndef
PROCESSOR_STACKWALKER_ADDRESS_LIST_H__
I thought I mentioned this in the last pass:

One trailing underscore, not two.

https://breakpad.appspot.com/620002/diff/11001/src/processor/stackwalker_addr...
src/processor/stackwalker_address_list.h:38: #ifndef
PROCESSOR_STACKWALKER_ADDRESS_LIST_H__
The convention in Breakpad is wrong for new code. New files use a single
underscore, but there hasn’t been a massive fixing of old files.

Background history: the Google style guide used to require two underscores,
until it was pointed out that those names were reserved, so it was quickly
changed to require one underscore. Most of Breakpad was written before the rule
changed.

New code should undoubtedly adhere to the new rule.

[chrisha]

Okay, hopefully all better now :) Last look?

https://breakpad.appspot.com/620002/diff/11001/src/processor/stackwalker_addr...
File src/processor/stackwalker_address_list.cc (right):

https://breakpad.appspot.com/620002/diff/11001/src/processor/stackwalker_addr...
src/processor/stackwalker_address_list.cc:37: #include <assert.h>
On 2013/08/22 15:44:29, Mark Mentovai wrote:
> C system headers before C++ system headers. We also put a blank line between
the
> C and C++ sections.

Done.

https://breakpad.appspot.com/620002/diff/11001/src/processor/stackwalker_addr...
src/processor/stackwalker_address_list.cc:64: frame->instruction = frames_[0];
On 2013/08/22 15:44:29, Mark Mentovai wrote:
> If frame_count_ is 0, return NULL instead of doing this potentially-dangerous
> thing.

Good catch! Done.

https://breakpad.appspot.com/620002/diff/11001/src/processor/stackwalker_addr...
src/processor/stackwalker_address_list.cc:68: 
On 2013/08/22 15:44:29, Mark Mentovai wrote:
> Are you leaving one blank line between things or two? You seem to have done
two
> elsewhere in this file, but this time, you did one.

Went to leaving one blank for consistency.

https://breakpad.appspot.com/620002/diff/11001/src/processor/stackwalker_addr...
File src/processor/stackwalker_address_list.h (right):

https://breakpad.appspot.com/620002/diff/11001/src/processor/stackwalker_addr...
src/processor/stackwalker_address_list.h:38: #ifndef
PROCESSOR_STACKWALKER_ADDRESS_LIST_H__
On 2013/08/22 15:44:29, Mark Mentovai wrote:
> The convention in Breakpad is wrong for new code. New files use a single
> underscore, but there hasn’t been a massive fixing of old files.
> 
> Background history: the Google style guide used to require two underscores,
> until it was pointed out that those names were reserved, so it was quickly
> changed to require one underscore. Most of Breakpad was written before the
rule
> changed.
> 
> New code should undoubtedly adhere to the new rule.

Done.

[Mark Mentovai]

LGTM. You might want to wait to see if Ivan has any final comments also.

[Ivan Penkov]

LGTM

[chrisha]

Thanks, committed.

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

[Ted Mielczarek]

This looks really nice, I could definitely use something like this. (We're
storing stacks at garbage collection time on nightly builds in our minidumps.) I
just have one post-facto review comment.

https://breakpad.appspot.com/620002/diff/16001/src/processor/stackwalker_addr...
File src/processor/stackwalker_address_list.cc (right):

https://breakpad.appspot.com/620002/diff/16001/src/processor/stackwalker_addr...
src/processor/stackwalker_address_list.cc:85: // explicitly provided.
I think I would have preferred for you to invent a new frame trust value here.
They're not really context frames, they were walked elsewhere. (I guess ideally
you'd allow whatever is walking the stack to give you frame trust values as
well, but that might be overkill.)

[chrisha]

On 2013/08/23 14:51:48, Ted Mielczarek wrote:
> This looks really nice, I could definitely use something like this. (We're
> storing stacks at garbage collection time on nightly builds in our minidumps.)
I
> just have one post-facto review comment.
> 
>
https://breakpad.appspot.com/620002/diff/16001/src/processor/stackwalker_addr...
> File src/processor/stackwalker_address_list.cc (right):
> 
>
https://breakpad.appspot.com/620002/diff/16001/src/processor/stackwalker_addr...
> src/processor/stackwalker_address_list.cc:85: // explicitly provided.
> I think I would have preferred for you to invent a new frame trust value here.
> They're not really context frames, they were walked elsewhere. (I guess
ideally
> you'd allow whatever is walking the stack to give you frame trust values as
> well, but that might be overkill.)

That's not a bad idea. Suggestions for a name? Mark, input?

[Mark Mentovai]

“Prewalked?”

In principle, it’s an attribute that doesn’t exclude the current set of trust
values. A stack frame can have been recovered by something other than the
Breakpad processor by the normal FP-based chain walk and thus qualify as both
FRAME_TRUST_FP and FRAME_TRUST_PREWALKED. But that’s probably overcomplicating
matters, and I doubt that anything else that’s providing prewalked stacks to
Breakpad will be providing Breakpad-style trust values along with its scan
results. We can turn “prewalked” into a distinct attribute at some point in the
future if it turns out that we need it. For now, it’s probably fine to use a
“prewalked” value for all frames and not care about how the prewalk arrived at
its values.

[Mark Mentovai]

P.S. I like Ted’s suggestion too.

[chrisha]

On 2013/08/23 16:10:01, Mark Mentovai wrote:
> P.S. I like Ted’s suggestion too.

See https://breakpad.appspot.com/621002/.