Refactor the logic of resolving source line info into helper class

The logic of how symbol supplier interacts with source line resolver to fill
source line info and get WindowsFrameInfo & CFIFrameInfo, is currently embedded
in StackWalker::Walk().

Therefore it is difficult to inject custom logic into the process of
resolving(symbolizing) stack frame, without the refactoring.

In this CL, I moved the logic & code from StackWalker into a separate helper
class named SourceLineResolverHelper.  This enables us to extend or override the
logic by subclassing the helper class, for example, adding a cache layer, or
symbolizing remotely via network, etc.

This CL shouldn't affect any existing user of breakpad processor.

[Ivan Penkov]

.

http://breakpad.appspot.com/459002/diff/1/google_breakpad/processor/minidump_...
File google_breakpad/processor/minidump_processor.h (right):

http://breakpad.appspot.com/459002/diff/1/google_breakpad/processor/minidump_...
google_breakpad/processor/minidump_processor.h:165: SymbolSupplier *supplier_;
Do we still need supplier_ and resolver_?  Why not just have a resolver_helper_
here?  I can be initialized by the old constructor which accepts supplier and
resolver.  It seems dangerous to leave these two here.  Are they set by the new
constructor which accepts resolver_helper_?

http://breakpad.appspot.com/459002/diff/1/google_breakpad/processor/stack_fra...
File google_breakpad/processor/stack_frame.h (right):

http://breakpad.appspot.com/459002/diff/1/google_breakpad/processor/stack_fra...
google_breakpad/processor/stack_frame.h:73: frame->function_base =
this->function_base;
Did you miss to copy 'instruction'?  Why not virtual?

http://breakpad.appspot.com/459002/diff/1/processor/minidump_processor.cc
File processor/minidump_processor.cc (right):

http://breakpad.appspot.com/459002/diff/1/processor/minidump_processor.cc#new...
processor/minidump_processor.cc:61: : supplier_(NULL), resolver_(NULL),
resolver_helper_(resolver_helper),
This seems dangerous.  What if someone adds code to access supplier_ or
resolver_ (assuming the old constructor sets them).  I think you should either
initialize them here or get rid of these members.

http://breakpad.appspot.com/459002/diff/1/processor/minidump_processor.cc#new...
processor/minidump_processor.cc:137: // Initialize a local resolver helper if
it's not provided via constructor.
This is ugly.  See my previous comments on that matter.

[SiyangXie]

http://breakpad.appspot.com/459002/diff/1/google_breakpad/processor/minidump_...
File google_breakpad/processor/minidump_processor.h (right):

http://breakpad.appspot.com/459002/diff/1/google_breakpad/processor/minidump_...
google_breakpad/processor/minidump_processor.h:165: SymbolSupplier *supplier_;
I've removed them.  I added a boolean to indicate whether the helper is locally
owned (when resolver_helper is not explicitly pass through constructor) or not.

On 2012/09/28 18:57:59, Ivan Penkov wrote:
> Do we still need supplier_ and resolver_?  Why not just have a
resolver_helper_
> here?  I can be initialized by the old constructor which accepts supplier and
> resolver.  It seems dangerous to leave these two here.  Are they set by the
new
> constructor which accepts resolver_helper_?

http://breakpad.appspot.com/459002/diff/1/google_breakpad/processor/stack_fra...
File google_breakpad/processor/stack_frame.h (right):

http://breakpad.appspot.com/459002/diff/1/google_breakpad/processor/stack_fra...
google_breakpad/processor/stack_frame.h:73: frame->function_base =
this->function_base;
On 2012/09/28 18:57:59, Ivan Penkov wrote:
> Did you miss to copy 'instruction'?  Why not virtual?

Oh actually I'm going to remove these two new methods (which were only needed in
my earlier prototype code), forgot to clean them up.

http://breakpad.appspot.com/459002/diff/1/processor/minidump_processor.cc
File processor/minidump_processor.cc (right):

http://breakpad.appspot.com/459002/diff/1/processor/minidump_processor.cc#new...
processor/minidump_processor.cc:61: : supplier_(NULL), resolver_(NULL),
resolver_helper_(resolver_helper),
On 2012/09/28 18:57:59, Ivan Penkov wrote:
> This seems dangerous.  What if someone adds code to access supplier_ or
> resolver_ (assuming the old constructor sets them).  I think you should either
> initialize them here or get rid of these members.

Done.

http://breakpad.appspot.com/459002/diff/1/processor/minidump_processor.cc#new...
processor/minidump_processor.cc:137: // Initialize a local resolver helper if
it's not provided via constructor.
On 2012/09/28 18:57:59, Ivan Penkov wrote:
> This is ugly.  See my previous comments on that matter.

Done.
I added a Reset() interface in SourceLineResolverHelper, and invoked Reset()
before walking the stacks in a minidump. In this way, original behavior is kept,
while allowing subclass of helper to override Reset() with different behavior if
necessary.

[Ivan Penkov]

On 2012/09/28 22:14:13, SiyangXie wrote:
>
http://breakpad.appspot.com/459002/diff/1/google_breakpad/processor/minidump_...
> File google_breakpad/processor/minidump_processor.h (right):
> 
>
http://breakpad.appspot.com/459002/diff/1/google_breakpad/processor/minidump_...
> google_breakpad/processor/minidump_processor.h:165: SymbolSupplier *supplier_;
> I've removed them.  I added a boolean to indicate whether the helper is
locally
> owned (when resolver_helper is not explicitly pass through constructor) or
not.
> 
> On 2012/09/28 18:57:59, Ivan Penkov wrote:
> > Do we still need supplier_ and resolver_?  Why not just have a
> resolver_helper_
> > here?  I can be initialized by the old constructor which accepts supplier
and
> > resolver.  It seems dangerous to leave these two here.  Are they set by the
> new
> > constructor which accepts resolver_helper_?
> 
>
http://breakpad.appspot.com/459002/diff/1/google_breakpad/processor/stack_fra...
> File google_breakpad/processor/stack_frame.h (right):
> 
>
http://breakpad.appspot.com/459002/diff/1/google_breakpad/processor/stack_fra...
> google_breakpad/processor/stack_frame.h:73: frame->function_base =
> this->function_base;
> On 2012/09/28 18:57:59, Ivan Penkov wrote:
> > Did you miss to copy 'instruction'?  Why not virtual?
> 
> Oh actually I'm going to remove these two new methods (which were only needed
in
> my earlier prototype code), forgot to clean them up.
> 
> http://breakpad.appspot.com/459002/diff/1/processor/minidump_processor.cc
> File processor/minidump_processor.cc (right):
> 
>
http://breakpad.appspot.com/459002/diff/1/processor/minidump_processor.cc#new...
> processor/minidump_processor.cc:61: : supplier_(NULL), resolver_(NULL),
> resolver_helper_(resolver_helper),
> On 2012/09/28 18:57:59, Ivan Penkov wrote:
> > This seems dangerous.  What if someone adds code to access supplier_ or
> > resolver_ (assuming the old constructor sets them).  I think you should
either
> > initialize them here or get rid of these members.
> 
> Done.
> 
>
http://breakpad.appspot.com/459002/diff/1/processor/minidump_processor.cc#new...
> processor/minidump_processor.cc:137: // Initialize a local resolver helper if
> it's not provided via constructor.
> On 2012/09/28 18:57:59, Ivan Penkov wrote:
> > This is ugly.  See my previous comments on that matter.
> 
> Done.
> I added a Reset() interface in SourceLineResolverHelper, and invoked Reset()
> before walking the stacks in a minidump. In this way, original behavior is
kept,
> while allowing subclass of helper to override Reset() with different behavior
if
> necessary.

LGTM

[Ted Mielczarek]

I didn't finish reviewing this, but I figured I'd get my comments up so you
could make sure you addressed them.

http://breakpad.appspot.com/459002/diff/1/google_breakpad/processor/source_li...
File google_breakpad/processor/source_line_resolver_helper.h (right):

http://breakpad.appspot.com/459002/diff/1/google_breakpad/processor/source_li...
google_breakpad/processor/source_line_resolver_helper.h:52: class
SourceLineResolverHelper {
SourceLineResolverHelper is kind of a weird name.

http://breakpad.appspot.com/459002/diff/1/google_breakpad/processor/stackwalk...
File google_breakpad/processor/stackwalker.h (right):

http://breakpad.appspot.com/459002/diff/1/google_breakpad/processor/stackwalk...
google_breakpad/processor/stackwalker.h:79: SourceLineResolverHelper
*resolver_helper);
Can you slide all the *s over to sit next to the type names while you're
touching this anyway?

http://breakpad.appspot.com/459002/diff/1/processor/minidump_processor.cc
File processor/minidump_processor.cc (right):

http://breakpad.appspot.com/459002/diff/1/processor/minidump_processor.cc#new...
processor/minidump_processor.cc:61: : supplier_(NULL), resolver_(NULL),
resolver_helper_(resolver_helper),
I agree with this, but the class ought to handle the NULL supplier_ and
resolver_ case as long as we allow passing pointers into the constructor. I
actually fixed a few crashes in StackWalker implementations when you passed a
NULL resolver or supplier a while ago. I added unittests for the stack walkers,
but apparently not for MinidumpProcessor.

http://breakpad.appspot.com/459002/diff/1/processor/source_line_resolver_help...
File processor/source_line_resolver_helper.cc (right):

http://breakpad.appspot.com/459002/diff/1/processor/source_line_resolver_help...
processor/source_line_resolver_helper.cc:30: // Implementation of
SourceLineResolver.
This comment is wrong.

[Ted Mielczarek]

Here's a more thorough review. Please take a look at my previous partial review
as well. I'd also be interested to know if Mark has any thoughts on the API
design/naming here.

Overall I like the idea of this patch, we have plans to redesign our symbol
backend and this will make it a lot simpler.

http://breakpad.appspot.com/459002/diff/11002/src/google_breakpad/processor/m...
File src/google_breakpad/processor/minidump_processor.h (right):

http://breakpad.appspot.com/459002/diff/11002/src/google_breakpad/processor/m...
src/google_breakpad/processor/minidump_processor.h:104: bool
enable_exploitability);
I'd be in favor of just removing this constructor and fixing the callers. From
what I can tell most users of Breakpad are just using minidump_stackwalk
directly anyway, so if you were to fix all the in-tree code it probably wouldn't
hurt anyone.

(That could be a follow-up patch, though, since this patch is already fairly
large.)

http://breakpad.appspot.com/459002/diff/11002/src/google_breakpad/processor/s...
File src/google_breakpad/processor/source_line_resolver_helper.h (right):

http://breakpad.appspot.com/459002/diff/11002/src/google_breakpad/processor/s...
src/google_breakpad/processor/source_line_resolver_helper.h:39: #include
<string>
You should be #including "common/using_std_string.h" and using "string" instead
of "std::string".

http://breakpad.appspot.com/459002/diff/11002/src/google_breakpad/processor/s...
src/google_breakpad/processor/source_line_resolver_helper.h:52: class
SourceLineResolverHelper {
I'll repeat my comment about naming. What do you think about
"StackFrameSymbolizer"? That describes its functionality pretty well. "Helper"
is not very descriptive.

http://breakpad.appspot.com/459002/diff/11002/src/google_breakpad/processor/s...
src/google_breakpad/processor/source_line_resolver_helper.h:66: };
It seems unfortunate to essentially duplicate this enum from SymbolSupplier.

Also, NO_MODULE doesn't seem like a symbol status, really.

http://breakpad.appspot.com/459002/diff/11002/src/google_breakpad/processor/s...
src/google_breakpad/processor/source_line_resolver_helper.h:68:
SourceLineResolverInterface *resolver);
Would it be worthwhile to add a subclass of this like
BaseSourceLineResolverHelper that instantiates its own SimpleSymbolSupplier and
BasicSourceLineResolver?

http://breakpad.appspot.com/459002/diff/11002/src/google_breakpad/processor/s...
src/google_breakpad/processor/source_line_resolver_helper.h:87: virtual void
Reset() { no_symbol_modules_.clear(); }
This doesn't seem super-useful in this base class. I assume you have a more
compelling use case for your subclass?

I experimented with something very similar to this a while ago, where I had a
long-lived BasicSourceLineResolver, and I think I understand what issues this
solves (keeping too many symbols in memory).

http://breakpad.appspot.com/459002/diff/11002/src/google_breakpad/processor/s...
src/google_breakpad/processor/source_line_resolver_helper.h:97: // one minidump.
I know this is probably copy/pasted from somewhere else, but please fix the
comment to be third-person (no "we") since you're adding this as new code.

http://breakpad.appspot.com/459002/diff/11002/src/processor/source_line_resol...
File src/processor/source_line_resolver_helper.cc (right):

http://breakpad.appspot.com/459002/diff/11002/src/processor/source_line_resol...
src/processor/source_line_resolver_helper.cc:50: SourceLineResolverInterface
*resolver) : supplier_(supplier),
Put the * next to the type name in new code.

http://breakpad.appspot.com/459002/diff/11002/src/processor/source_line_resol...
src/processor/source_line_resolver_helper.cc:57: SymbolStatus *symbol_status) {
It seems a little strange to have this method return void but have an outparam
for status. Why not return SymbolStatus instead?

Related, "SymbolStatus" seems misleading here, since you're reporting the
combination of status from the SymbolSupplier and the SourceLineResolver.

http://breakpad.appspot.com/459002/diff/11002/src/processor/source_line_resol...
src/processor/source_line_resolver_helper.cc:98: *symbol_status = load_success ?
LOADED : LOAD_ERROR;
Interestingly, we probably have a long-standing bug here where if you have a
symbol file but it fails to load the stackwalker will repeatedly attempt to load
it for every frame in that module. We should probably cache "tried to load
symbols but failed" on a per-module basis.

http://breakpad.appspot.com/459002/diff/11002/src/processor/stackwalker.cc
File src/processor/stackwalker.cc (right):

http://breakpad.appspot.com/459002/diff/11002/src/processor/stackwalker.cc#ne...
src/processor/stackwalker.cc:90: frame.get(), &symbol_status);
This is going to crash if resolver_helper_ is NULL.

http://breakpad.appspot.com/459002/diff/11002/src/processor/stackwalker.cc#ne...
src/processor/stackwalker.cc:93: return false;
I do like how this simplifies the code here!

http://breakpad.appspot.com/459002/diff/11002/src/processor/stackwalker.cc#ne...
src/processor/stackwalker.cc:181: if (!resolver_helper_->resolver() ||
!resolver_helper_->supplier()) {
I think you could refactor this by adding a bool method to ResolverHelper that
just returns "supplier_ && resolver_". I'm not sure what to call it that would
make sense in subclasses, maybe bool has_implementation?

Also if you handle NULL resolver_helper_ above you should handle that here as
well.

http://breakpad.appspot.com/459002/diff/11002/src/processor/stackwalker.cc#ne...
src/processor/stackwalker.cc:188: // we don't have symbols, but we're inside a
loaded module
Fix the "we" in the comments here while you're at it. (Sorry, I wrote that
comment.)

http://breakpad.appspot.com/459002/diff/11002/src/processor/stackwalker_amd64...
File src/processor/stackwalker_amd64_unittest.cc (right):

http://breakpad.appspot.com/459002/diff/11002/src/processor/stackwalker_amd64...
src/processor/stackwalker_amd64_unittest.cc:144: &resolver_helper);
These tests aren't quite as valid as before. They do test that passing NULL
supplier/resolver arguments to SourceLineResolverHelper doesn't crash, but
you're not testing that passing a NULL resolver_helper doesn't crash (which I'm
pretty sure it would).

http://breakpad.appspot.com/459002/diff/11002/src/processor/stackwalker_arm_u...
File src/processor/stackwalker_arm_unittest.cc (right):

http://breakpad.appspot.com/459002/diff/11002/src/processor/stackwalker_arm_u...
src/processor/stackwalker_arm_unittest.cc:138: SourceLineResolverHelper
resolver_helper(NULL, NULL);
Likewise here.

http://breakpad.appspot.com/459002/diff/11002/src/processor/stackwalker_x86_u...
File src/processor/stackwalker_x86_unittest.cc (right):

http://breakpad.appspot.com/459002/diff/11002/src/processor/stackwalker_x86_u...
src/processor/stackwalker_x86_unittest.cc:152: SourceLineResolverHelper
resolver_helper(NULL, NULL);
And here.

[SiyangXie]

http://breakpad.appspot.com/459002/diff/1/google_breakpad/processor/source_li...
File google_breakpad/processor/source_line_resolver_helper.h (right):

http://breakpad.appspot.com/459002/diff/1/google_breakpad/processor/source_li...
google_breakpad/processor/source_line_resolver_helper.h:52: class
SourceLineResolverHelper {
On 2012/09/30 17:11:16, Ted Mielczarek wrote:
> SourceLineResolverHelper is kind of a weird name.

OK, I'm going to rename it to StackFrameSymbolizer which does sound much better
:-)

http://breakpad.appspot.com/459002/diff/1/google_breakpad/processor/stackwalk...
File google_breakpad/processor/stackwalker.h (right):

http://breakpad.appspot.com/459002/diff/1/google_breakpad/processor/stackwalk...
google_breakpad/processor/stackwalker.h:79: SourceLineResolverHelper
*resolver_helper);
On 2012/09/30 17:11:16, Ted Mielczarek wrote:
> Can you slide all the *s over to sit next to the type names while you're
> touching this anyway?

Done.

http://breakpad.appspot.com/459002/diff/1/processor/source_line_resolver_help...
File processor/source_line_resolver_helper.cc (right):

http://breakpad.appspot.com/459002/diff/1/processor/source_line_resolver_help...
processor/source_line_resolver_helper.cc:30: // Implementation of
SourceLineResolver.
On 2012/09/30 17:11:16, Ted Mielczarek wrote:
> This comment is wrong.

Done.

http://breakpad.appspot.com/459002/diff/11002/src/google_breakpad/processor/m...
File src/google_breakpad/processor/minidump_processor.h (right):

http://breakpad.appspot.com/459002/diff/11002/src/google_breakpad/processor/m...
src/google_breakpad/processor/minidump_processor.h:104: bool
enable_exploitability);
On 2012/10/01 16:01:07, Ted Mielczarek wrote:
> I'd be in favor of just removing this constructor and fixing the callers. From
> what I can tell most users of Breakpad are just using minidump_stackwalk
> directly anyway, so if you were to fix all the in-tree code it probably
wouldn't
> hurt anyone.
> 
> (That could be a follow-up patch, though, since this patch is already fairly
> large.)

OK, I'm going to send a follow-up patch. I agree that as long as all existing
users are aware of the change and approve it, then it should be fine to just
removing these constructors.

http://breakpad.appspot.com/459002/diff/11002/src/google_breakpad/processor/s...
File src/google_breakpad/processor/source_line_resolver_helper.h (right):

http://breakpad.appspot.com/459002/diff/11002/src/google_breakpad/processor/s...
src/google_breakpad/processor/source_line_resolver_helper.h:39: #include
<string>
On 2012/10/01 16:01:07, Ted Mielczarek wrote:
> You should be #including "common/using_std_string.h" and using "string"
instead
> of "std::string".

Done.

http://breakpad.appspot.com/459002/diff/11002/src/google_breakpad/processor/s...
src/google_breakpad/processor/source_line_resolver_helper.h:52: class
SourceLineResolverHelper {
On 2012/10/01 16:01:07, Ted Mielczarek wrote:
> I'll repeat my comment about naming. What do you think about
> "StackFrameSymbolizer"? That describes its functionality pretty well. "Helper"
> is not very descriptive.

StackFrameSymbolizer sounds much better!

http://breakpad.appspot.com/459002/diff/11002/src/google_breakpad/processor/s...
src/google_breakpad/processor/source_line_resolver_helper.h:66: };
On 2012/10/01 16:01:07, Ted Mielczarek wrote:
> It seems unfortunate to essentially duplicate this enum from SymbolSupplier.
> 
> Also, NO_MODULE doesn't seem like a symbol status, really.

I realize it's not necessary to duplicate the enum.  I should simplify the enum
to have less granularity, since this helper class already hides details of how
each SymbolStatus is handled.

From the viewpoint of the StackWalker, it only needs to know the result is one
of:
LOADED(Success?), NOT_LOADED(no module, no symbol, load error), INTERRUPT,
basically 1 case of success + 2 cases of failure

http://breakpad.appspot.com/459002/diff/11002/src/google_breakpad/processor/s...
src/google_breakpad/processor/source_line_resolver_helper.h:66: };
On 2012/10/01 16:01:07, Ted Mielczarek wrote:
> It seems unfortunate to essentially duplicate this enum from SymbolSupplier.
> 
> Also, NO_MODULE doesn't seem like a symbol status, really.

Done.

http://breakpad.appspot.com/459002/diff/11002/src/google_breakpad/processor/s...
src/google_breakpad/processor/source_line_resolver_helper.h:68:
SourceLineResolverInterface *resolver);
On 2012/10/01 16:01:07, Ted Mielczarek wrote:
> Would it be worthwhile to add a subclass of this like
> BaseSourceLineResolverHelper that instantiates its own SimpleSymbolSupplier
and
> BasicSourceLineResolver?

I don't think there's too much benefit, probably saving of 2 lines code in
minidump_stackwalk?

http://breakpad.appspot.com/459002/diff/11002/src/google_breakpad/processor/s...
src/google_breakpad/processor/source_line_resolver_helper.h:87: virtual void
Reset() { no_symbol_modules_.clear(); }
On 2012/10/01 16:01:07, Ted Mielczarek wrote:
> This doesn't seem super-useful in this base class. I assume you have a more
> compelling use case for your subclass?
> 
> I experimented with something very similar to this a while ago, where I had a
> long-lived BasicSourceLineResolver, and I think I understand what issues this
> solves (keeping too many symbols in memory).

The no_symbol_modules_ is a list of modules that are known to miss their
symbols.  Prior to this change, it belongs to stack walker, which only has the
lifetime spanned for a single minidump (stackwalker is instantiated in
MinidumpProcessor::Process).

However, the helper object would have much longer lifetime (MinidumpProcessor
takes the ref to the helper in constructor).  So to keep the old behavior
unchanged, MinidumpProcessor::Process() needs to reset the no_symbol_modules_.

I think in my use case, I will manage the list of known missing symbols
differently (with much longer lifespan), and override Reset() with no-op.

http://breakpad.appspot.com/459002/diff/11002/src/google_breakpad/processor/s...
src/google_breakpad/processor/source_line_resolver_helper.h:97: // one minidump.
On 2012/10/01 16:01:07, Ted Mielczarek wrote:
> I know this is probably copy/pasted from somewhere else, but please fix the
> comment to be third-person (no "we") since you're adding this as new code.

Done.

http://breakpad.appspot.com/459002/diff/11002/src/processor/source_line_resol...
File src/processor/source_line_resolver_helper.cc (right):

http://breakpad.appspot.com/459002/diff/11002/src/processor/source_line_resol...
src/processor/source_line_resolver_helper.cc:50: SourceLineResolverInterface
*resolver) : supplier_(supplier),
On 2012/10/01 16:01:07, Ted Mielczarek wrote:
> Put the * next to the type name in new code.

Done.

http://breakpad.appspot.com/459002/diff/11002/src/processor/source_line_resol...
src/processor/source_line_resolver_helper.cc:57: SymbolStatus *symbol_status) {
On 2012/10/01 16:01:07, Ted Mielczarek wrote:
> It seems a little strange to have this method return void but have an outparam
> for status. Why not return SymbolStatus instead?
> 
> Related, "SymbolStatus" seems misleading here, since you're reporting the
> combination of status from the SymbolSupplier and the SourceLineResolver.

Done.

http://breakpad.appspot.com/459002/diff/11002/src/processor/source_line_resol...
src/processor/source_line_resolver_helper.cc:98: *symbol_status = load_success ?
LOADED : LOAD_ERROR;
On 2012/10/01 16:01:07, Ted Mielczarek wrote:
> Interestingly, we probably have a long-standing bug here where if you have a
> symbol file but it fails to load the stackwalker will repeatedly attempt to
load
> it for every frame in that module. We should probably cache "tried to load
> symbols but failed" on a per-module basis.

Done.

http://breakpad.appspot.com/459002/diff/11002/src/processor/stackwalker.cc
File src/processor/stackwalker.cc (right):

http://breakpad.appspot.com/459002/diff/11002/src/processor/stackwalker.cc#ne...
src/processor/stackwalker.cc:90: frame.get(), &symbol_status);
On 2012/10/01 16:01:07, Ted Mielczarek wrote:
> This is going to crash if resolver_helper_ is NULL.

I decide to assert whether resolver_helper_ is NULL.  I updated the comment in
the constructor that resolver_helper MUST NOT be NULL.  I think that's a fair
requirement for stack walker.

http://breakpad.appspot.com/459002/diff/11002/src/processor/stackwalker.cc#ne...
src/processor/stackwalker.cc:181: if (!resolver_helper_->resolver() ||
!resolver_helper_->supplier()) {
On 2012/10/01 16:01:07, Ted Mielczarek wrote:
> I think you could refactor this by adding a bool method to ResolverHelper that
> just returns "supplier_ && resolver_". I'm not sure what to call it that would
> make sense in subclasses, maybe bool has_implementation?
> 
> Also if you handle NULL resolver_helper_ above you should handle that here as
> well. 

Done.

Curious what's the use case to override this bool method in subclasses?

http://breakpad.appspot.com/459002/diff/11002/src/processor/stackwalker.cc#ne...
src/processor/stackwalker.cc:188: // we don't have symbols, but we're inside a
loaded module
On 2012/10/01 16:01:07, Ted Mielczarek wrote:
> Fix the "we" in the comments here while you're at it. (Sorry, I wrote that
> comment.)

Done.

http://breakpad.appspot.com/459002/diff/11002/src/processor/stackwalker_amd64...
File src/processor/stackwalker_amd64_unittest.cc (right):

http://breakpad.appspot.com/459002/diff/11002/src/processor/stackwalker_amd64...
src/processor/stackwalker_amd64_unittest.cc:144: &resolver_helper);
On 2012/10/01 16:01:07, Ted Mielczarek wrote:
> These tests aren't quite as valid as before. They do test that passing NULL
> supplier/resolver arguments to SourceLineResolverHelper doesn't crash, but
> you're not testing that passing a NULL resolver_helper doesn't crash (which
I'm
> pretty sure it would).

I added assert(resolver_helper_) in stack walker, and updated the comment to
make sure resolver_helper_ must not be null.

http://breakpad.appspot.com/459002/diff/11002/src/processor/stackwalker_arm_u...
File src/processor/stackwalker_arm_unittest.cc (right):

http://breakpad.appspot.com/459002/diff/11002/src/processor/stackwalker_arm_u...
src/processor/stackwalker_arm_unittest.cc:138: SourceLineResolverHelper
resolver_helper(NULL, NULL);
On 2012/10/01 16:01:07, Ted Mielczarek wrote:
> Likewise here.

Added assert(resolver_helper_) in constructor of stack walker instead.

http://breakpad.appspot.com/459002/diff/11002/src/processor/stackwalker_x86_u...
File src/processor/stackwalker_x86_unittest.cc (right):

http://breakpad.appspot.com/459002/diff/11002/src/processor/stackwalker_x86_u...
src/processor/stackwalker_x86_unittest.cc:152: SourceLineResolverHelper
resolver_helper(NULL, NULL);
On 2012/10/01 16:01:07, Ted Mielczarek wrote:
> And here.

ditto

[Ted Mielczarek]

http://breakpad.appspot.com/459002/diff/11002/src/processor/stackwalker.cc
File src/processor/stackwalker.cc (right):

http://breakpad.appspot.com/459002/diff/11002/src/processor/stackwalker.cc#ne...
src/processor/stackwalker.cc:181: if (!resolver_helper_->resolver() ||
!resolver_helper_->supplier()) {
On 2012/10/02 22:29:13, SiyangXie wrote:
> On 2012/10/01 16:01:07, Ted Mielczarek wrote:
> > I think you could refactor this by adding a bool method to ResolverHelper
that
> > just returns "supplier_ && resolver_". I'm not sure what to call it that
would
> > make sense in subclasses, maybe bool has_implementation?
> > 
> > Also if you handle NULL resolver_helper_ above you should handle that here
as
> > well. 
> 
> Done.
> 
> Curious what's the use case to override this bool method in subclasses?  

I was imagining a subclass of this class that doesn't actually use the
SymbolSupplier or SourceLineResolver classes, but handles all that logic itself
internally, so this test wouldn't really make sense (perhaps it'd always return
true).

[SiyangXie]

On 2012/10/03 00:31:34, Ted Mielczarek wrote:
> http://breakpad.appspot.com/459002/diff/11002/src/processor/stackwalker.cc
> File src/processor/stackwalker.cc (right):
> 
>
http://breakpad.appspot.com/459002/diff/11002/src/processor/stackwalker.cc#ne...
> src/processor/stackwalker.cc:181: if (!resolver_helper_->resolver() ||
> !resolver_helper_->supplier()) {
> On 2012/10/02 22:29:13, SiyangXie wrote:
> > On 2012/10/01 16:01:07, Ted Mielczarek wrote:
> > > I think you could refactor this by adding a bool method to ResolverHelper
> that
> > > just returns "supplier_ && resolver_". I'm not sure what to call it that
> would
> > > make sense in subclasses, maybe bool has_implementation?
> > > 
> > > Also if you handle NULL resolver_helper_ above you should handle that here
> as
> > > well. 
> > 
> > Done.
> > 
> > Curious what's the use case to override this bool method in subclasses?  
> 
> I was imagining a subclass of this class that doesn't actually use the
> SymbolSupplier or SourceLineResolver classes, but handles all that logic
itself
> internally, so this test wouldn't really make sense (perhaps it'd always
return
> true).

Hi Ted, have you had chance to take a look at my last edits based on your review
comments?  Thanks!

[Ted Mielczarek]

On Mon, Oct 8, 2012 at 1:47 PM,  <SiyangXie@gmail.com> wrote:
> Hi Ted, have you had chance to take a look at my last edits based on
> your review comments?  Thanks!

Sorry, didn't realize there was an updated patch. I'll take a look today.

[Ted Mielczarek]

This LGTM with some minor nits addressed.

http://breakpad.appspot.com/459002/diff/9005/src/google_breakpad/processor/mi...
File src/google_breakpad/processor/minidump_processor.h (right):

http://breakpad.appspot.com/459002/diff/9005/src/google_breakpad/processor/mi...
src/google_breakpad/processor/minidump_processor.h:110:
MinidumpProcessor(StackFrameSymbolizer *stack_frame_symbolizer,
The * should be next to the type name, here and elsewhere:
"StackFrameSymbolizer* stack_frame_symbolizer"

http://breakpad.appspot.com/459002/diff/9005/src/google_breakpad/processor/st...
File src/google_breakpad/processor/stackwalker.h (right):

http://breakpad.appspot.com/459002/diff/9005/src/google_breakpad/processor/st...
src/google_breakpad/processor/stackwalker.h:89: // associated with. 
frame_symbolizer, is a StackFrameSymbolizer object that
unnecessary comma.

http://breakpad.appspot.com/459002/diff/9005/src/google_breakpad/processor/st...
src/google_breakpad/processor/stackwalker.h:97: StackFrameSymbolizer
*frame_symbolizer);
* next to type name, here and below. (you could fix the formatting of the rest
of the arguments to this constructor while you're here.)

http://breakpad.appspot.com/459002/diff/9005/src/processor/stackwalker.cc
File src/processor/stackwalker.cc (right):

http://breakpad.appspot.com/459002/diff/9005/src/processor/stackwalker.cc#new...
src/processor/stackwalker.cc:63: StackFrameSymbolizer *frame_symbolizer)
* next to type name, here and below.

http://breakpad.appspot.com/459002/diff/9005/src/processor/stackwalker_amd64.cc
File src/processor/stackwalker_amd64.cc (right):

http://breakpad.appspot.com/459002/diff/9005/src/processor/stackwalker_amd64....
src/processor/stackwalker_amd64.cc:96: StackFrameSymbolizer *resolver_helper)
* next to type name.

http://breakpad.appspot.com/459002/diff/9005/src/processor/stackwalker_amd64.h
File src/processor/stackwalker_amd64.h (right):

http://breakpad.appspot.com/459002/diff/9005/src/processor/stackwalker_amd64....
src/processor/stackwalker_amd64.h:62: StackFrameSymbolizer *frame_symbolizer);
* next to type name.

http://breakpad.appspot.com/459002/diff/9005/src/processor/stackwalker_arm.cc
File src/processor/stackwalker_arm.cc (right):

http://breakpad.appspot.com/459002/diff/9005/src/processor/stackwalker_arm.cc...
src/processor/stackwalker_arm.cc:53: StackFrameSymbolizer *resolver_helper)
* next to type name.

http://breakpad.appspot.com/459002/diff/9005/src/processor/stackwalker_arm.h
File src/processor/stackwalker_arm.h (right):

http://breakpad.appspot.com/459002/diff/9005/src/processor/stackwalker_arm.h#...
src/processor/stackwalker_arm.h:62: StackFrameSymbolizer *frame_symbolizer);
* next to type name.

http://breakpad.appspot.com/459002/diff/9005/src/processor/stackwalker_ppc.cc
File src/processor/stackwalker_ppc.cc (right):

http://breakpad.appspot.com/459002/diff/9005/src/processor/stackwalker_ppc.cc...
src/processor/stackwalker_ppc.cc:50: StackFrameSymbolizer *resolver_helper)
* next to type name.

http://breakpad.appspot.com/459002/diff/9005/src/processor/stackwalker_ppc.h
File src/processor/stackwalker_ppc.h (right):

http://breakpad.appspot.com/459002/diff/9005/src/processor/stackwalker_ppc.h#...
src/processor/stackwalker_ppc.h:60: StackFrameSymbolizer *frame_symbolizer);
* next to type name.

http://breakpad.appspot.com/459002/diff/9005/src/processor/stackwalker_sparc.cc
File src/processor/stackwalker_sparc.cc (right):

http://breakpad.appspot.com/459002/diff/9005/src/processor/stackwalker_sparc....
src/processor/stackwalker_sparc.cc:50: StackFrameSymbolizer *resolver_helper)
* next to type name.

http://breakpad.appspot.com/459002/diff/9005/src/processor/stackwalker_sparc.h
File src/processor/stackwalker_sparc.h (right):

http://breakpad.appspot.com/459002/diff/9005/src/processor/stackwalker_sparc....
src/processor/stackwalker_sparc.h:60: StackFrameSymbolizer *frame_symbolizer);
* next to type name.

http://breakpad.appspot.com/459002/diff/9005/src/processor/stackwalker_x86.cc
File src/processor/stackwalker_x86.cc (right):

http://breakpad.appspot.com/459002/diff/9005/src/processor/stackwalker_x86.cc...
src/processor/stackwalker_x86.cc:84: StackFrameSymbolizer *resolver_helper)
* next to type name.

http://breakpad.appspot.com/459002/diff/9005/src/processor/stackwalker_x86.h
File src/processor/stackwalker_x86.h (right):

http://breakpad.appspot.com/459002/diff/9005/src/processor/stackwalker_x86.h#...
src/processor/stackwalker_x86.h:65: StackFrameSymbolizer *frame_symbolizer);
* next to type name.

[SiyangXie]

Thanks for review!

I've cleaned up and passed all tests in make check.
Submitting it now...

http://breakpad.appspot.com/459002/diff/9005/src/google_breakpad/processor/mi...
File src/google_breakpad/processor/minidump_processor.h (right):

http://breakpad.appspot.com/459002/diff/9005/src/google_breakpad/processor/mi...
src/google_breakpad/processor/minidump_processor.h:110:
MinidumpProcessor(StackFrameSymbolizer *stack_frame_symbolizer,
On 2012/10/09 12:30:54, Ted Mielczarek wrote:
> The * should be next to the type name, here and elsewhere:
> "StackFrameSymbolizer* stack_frame_symbolizer"

Done.

http://breakpad.appspot.com/459002/diff/9005/src/google_breakpad/processor/st...
File src/google_breakpad/processor/stackwalker.h (right):

http://breakpad.appspot.com/459002/diff/9005/src/google_breakpad/processor/st...
src/google_breakpad/processor/stackwalker.h:89: // associated with. 
frame_symbolizer, is a StackFrameSymbolizer object that
On 2012/10/09 12:30:54, Ted Mielczarek wrote:
> unnecessary comma.

Done.

http://breakpad.appspot.com/459002/diff/9005/src/google_breakpad/processor/st...
src/google_breakpad/processor/stackwalker.h:97: StackFrameSymbolizer
*frame_symbolizer);
On 2012/10/09 12:30:54, Ted Mielczarek wrote:
> * next to type name, here and below. (you could fix the formatting of the rest
> of the arguments to this constructor while you're here.)

Done.

http://breakpad.appspot.com/459002/diff/9005/src/processor/stackwalker.cc
File src/processor/stackwalker.cc (right):

http://breakpad.appspot.com/459002/diff/9005/src/processor/stackwalker.cc#new...
src/processor/stackwalker.cc:63: StackFrameSymbolizer *frame_symbolizer)
On 2012/10/09 12:30:54, Ted Mielczarek wrote:
> * next to type name, here and below.

Done.

http://breakpad.appspot.com/459002/diff/9005/src/processor/stackwalker_amd64.cc
File src/processor/stackwalker_amd64.cc (right):

http://breakpad.appspot.com/459002/diff/9005/src/processor/stackwalker_amd64....
src/processor/stackwalker_amd64.cc:96: StackFrameSymbolizer *resolver_helper)
On 2012/10/09 12:30:54, Ted Mielczarek wrote:
> * next to type name.

Done.

http://breakpad.appspot.com/459002/diff/9005/src/processor/stackwalker_amd64.h
File src/processor/stackwalker_amd64.h (right):

http://breakpad.appspot.com/459002/diff/9005/src/processor/stackwalker_amd64....
src/processor/stackwalker_amd64.h:62: StackFrameSymbolizer *frame_symbolizer);
On 2012/10/09 12:30:54, Ted Mielczarek wrote:
> * next to type name.

Done.

http://breakpad.appspot.com/459002/diff/9005/src/processor/stackwalker_arm.cc
File src/processor/stackwalker_arm.cc (right):

http://breakpad.appspot.com/459002/diff/9005/src/processor/stackwalker_arm.cc...
src/processor/stackwalker_arm.cc:53: StackFrameSymbolizer *resolver_helper)
On 2012/10/09 12:30:54, Ted Mielczarek wrote:
> * next to type name.

Done.

http://breakpad.appspot.com/459002/diff/9005/src/processor/stackwalker_arm.h
File src/processor/stackwalker_arm.h (right):

http://breakpad.appspot.com/459002/diff/9005/src/processor/stackwalker_arm.h#...
src/processor/stackwalker_arm.h:62: StackFrameSymbolizer *frame_symbolizer);
On 2012/10/09 12:30:54, Ted Mielczarek wrote:
> * next to type name.

Done.

http://breakpad.appspot.com/459002/diff/9005/src/processor/stackwalker_ppc.cc
File src/processor/stackwalker_ppc.cc (right):

http://breakpad.appspot.com/459002/diff/9005/src/processor/stackwalker_ppc.cc...
src/processor/stackwalker_ppc.cc:50: StackFrameSymbolizer *resolver_helper)
On 2012/10/09 12:30:54, Ted Mielczarek wrote:
> * next to type name.

Done.

http://breakpad.appspot.com/459002/diff/9005/src/processor/stackwalker_ppc.h
File src/processor/stackwalker_ppc.h (right):

http://breakpad.appspot.com/459002/diff/9005/src/processor/stackwalker_ppc.h#...
src/processor/stackwalker_ppc.h:60: StackFrameSymbolizer *frame_symbolizer);
On 2012/10/09 12:30:54, Ted Mielczarek wrote:
> * next to type name.

Done.

http://breakpad.appspot.com/459002/diff/9005/src/processor/stackwalker_sparc.cc
File src/processor/stackwalker_sparc.cc (right):

http://breakpad.appspot.com/459002/diff/9005/src/processor/stackwalker_sparc....
src/processor/stackwalker_sparc.cc:50: StackFrameSymbolizer *resolver_helper)
On 2012/10/09 12:30:54, Ted Mielczarek wrote:
> * next to type name.

Done.

http://breakpad.appspot.com/459002/diff/9005/src/processor/stackwalker_sparc.h
File src/processor/stackwalker_sparc.h (right):

http://breakpad.appspot.com/459002/diff/9005/src/processor/stackwalker_sparc....
src/processor/stackwalker_sparc.h:60: StackFrameSymbolizer *frame_symbolizer);
On 2012/10/09 12:30:54, Ted Mielczarek wrote:
> * next to type name.

Done.

http://breakpad.appspot.com/459002/diff/9005/src/processor/stackwalker_x86.cc
File src/processor/stackwalker_x86.cc (right):

http://breakpad.appspot.com/459002/diff/9005/src/processor/stackwalker_x86.cc...
src/processor/stackwalker_x86.cc:84: StackFrameSymbolizer *resolver_helper)
On 2012/10/09 12:30:54, Ted Mielczarek wrote:
> * next to type name.

Done.

http://breakpad.appspot.com/459002/diff/9005/src/processor/stackwalker_x86.h
File src/processor/stackwalker_x86.h (right):

http://breakpad.appspot.com/459002/diff/9005/src/processor/stackwalker_x86.h#...
src/processor/stackwalker_x86.h:65: StackFrameSymbolizer *frame_symbolizer);
On 2012/10/09 12:30:54, Ted Mielczarek wrote:
> * next to type name.

Done.