MIPS support for Stackwalker module

MIPS support for Stackwalker module

[deepthi]

MIPS support for Stackwalker module

[deepthi]

Chris, Please review the patch

[Ted Mielczarek]

I have a few comments, but I didn't thoroughly review the actual stack walking
bits.

http://breakpad.appspot.com/469002/diff/1/src/google_breakpad/processor/stack...
File src/google_breakpad/processor/stack_frame_cpu.h (right):

http://breakpad.appspot.com/469002/diff/1/src/google_breakpad/processor/stack...
src/google_breakpad/processor/stack_frame_cpu.h:243: // Adding structure for
MIPS.Replica of StackFrameX86 structure modified
This comment is not useful, please remove it. Also you've added quite a few
extra blank lines in this file, please don't do that.

http://breakpad.appspot.com/469002/diff/1/src/google_breakpad/processor/stack...
src/google_breakpad/processor/stack_frame_cpu.h:249: // The whole enum present
here is replaced for MIPS N64 architecture
This comment doesn't make much sense to me. Are you trying to say that this is
really only true for MIPS with the N64 calling convention? If so perhaps we
should be using N64 in the names. (Or are the other calling conventions
obsolete?)

http://breakpad.appspot.com/469002/diff/1/src/google_breakpad/processor/stack...
src/google_breakpad/processor/stack_frame_cpu.h:254: enum ContextValidity {
Two things here:
1) As per the other architectures this should be a set of bit flags, not just a
series of incremented integers. These are ORed together into a bitfield.
2) Do you really need to have a validity flag for every single register? Per the
Wikipedia link above most of these are not callee-save, and except for ARM none
of the other architecture implementations in this file have flags for every
register.

http://breakpad.appspot.com/469002/diff/1/src/google_breakpad/processor/stack...
src/google_breakpad/processor/stack_frame_cpu.h:327: //  This function is added
to cope with the ARM architecture of MIPS
I understand what this function does, but this comment is confusing.

http://breakpad.appspot.com/469002/diff/1/src/google_breakpad/processor/stack...
src/google_breakpad/processor/stack_frame_cpu.h:351: // These may be NULL if
there is no such information for that address.
I know you copy/pasted this from above in this file, but please remove the "we".
Comments should be written in third-person. You could rewrite this to say "Any
stack walking information that was found..."

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

http://breakpad.appspot.com/469002/diff/1/src/processor/minidump_processor.cc...
src/processor/minidump_processor.cc:341: case MD_CPU_ARCHITECTURE_MIPS: {
nit: blank line above this case statement to match the previous cases.

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

http://breakpad.appspot.com/469002/diff/1/src/processor/minidump_stackwalk.cc...
src/processor/minidump_stackwalk.cc:258: 
This blank line is not necessary.

http://breakpad.appspot.com/469002/diff/1/src/processor/minidump_stackwalk.cc...
src/processor/minidump_stackwalk.cc:262: sequence);
You should be checking context_validity for each of these registers before
printing them, as per the other architectures.

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

http://breakpad.appspot.com/469002/diff/1/src/processor/stackwalker.cc#newcode56
src/processor/stackwalker.cc:56: #include
"google_breakpad/common/minidump_cpu_mips.h"
This include shouldn't be necessary.

http://breakpad.appspot.com/469002/diff/1/src/processor/stackwalker_MIPS.cc
File src/processor/stackwalker_MIPS.cc (right):

http://breakpad.appspot.com/469002/diff/1/src/processor/stackwalker_MIPS.cc#n...
src/processor/stackwalker_MIPS.cc:1: // Copyright (c) 2010 Google Inc.
1) The filename of this (and the matching header file) should be all lowercase:
stackwalker_mips.{cc,h}.
2) You should probably set the copyright year to 2012 here.

http://breakpad.appspot.com/469002/diff/1/src/processor/stackwalker_MIPS.cc#n...
src/processor/stackwalker_MIPS.cc:34: // Author: Tata Elxsi
Do you have an email address for this person? It's convention to have the
author's email address in new files.

http://breakpad.appspot.com/469002/diff/1/src/processor/stackwalker_MIPS.cc#n...
src/processor/stackwalker_MIPS.cc:36: #include
"processor/postfix_evaluator-inl.h"
Include lines should be ordered by filename  unless there are specific ordering
requirements. (I know there are instances in other files where this is broken,
but if you're adding a new file you should do it right.)

http://breakpad.appspot.com/469002/diff/1/src/processor/stackwalker_MIPS.cc#n...
src/processor/stackwalker_MIPS.cc:48: #include
"google_breakpad/common/minidump_cpu_mips.h"
This include shouldn't be necessary.

http://breakpad.appspot.com/469002/diff/1/src/processor/stackwalker_MIPS.cc#n...
src/processor/stackwalker_MIPS.cc:55: StackwalkerMIPS::StackwalkerMIPS (const
SystemInfo *system_info,
Here (and elsewhere), the * should be next to the type name, like "const
SystemInfo* system_info".

http://breakpad.appspot.com/469002/diff/1/src/processor/stackwalker_MIPS.cc#n...
src/processor/stackwalker_MIPS.cc:67: memory_ = NULL;
You're asserting that the stack addresses will always be 32-bit. Is that really
what you want?

http://breakpad.appspot.com/469002/diff/1/src/processor/stackwalker_MIPS.cc#n...
src/processor/stackwalker_MIPS.cc:109: // of CFI of current frame is required.
On doing this, we will get all
Don't use "we" in comments.

http://breakpad.appspot.com/469002/diff/1/src/processor/stackwalker_MIPS.cc#n...
src/processor/stackwalker_MIPS.cc:119: StackFrameMIPS
*StackwalkerMIPS::GetCallerByCFIFrameInfo (
I don't have the mental energy to review the actual stack walking implementation
here right now. Either someone else will have to review it or I'll find a better
time to do so.

http://breakpad.appspot.com/469002/diff/1/src/processor/stackwalker_MIPS.cc#n...
src/processor/stackwalker_MIPS.cc:135: #endif
These debug messages don't seem terribly valuable to keep around.

http://breakpad.appspot.com/469002/diff/1/src/processor/stackwalker_MIPS.h
File src/processor/stackwalker_MIPS.h (right):

http://breakpad.appspot.com/469002/diff/1/src/processor/stackwalker_MIPS.h#ne...
src/processor/stackwalker_MIPS.h:33: // corresponding to a MIPSstack.This is
made with x_86 stackwalker as base
You don't need the last sentence here.

http://breakpad.appspot.com/469002/diff/1/src/processor/stackwalker_MIPS.h#ne...
src/processor/stackwalker_MIPS.h:51: //Adding structure for MIPS.Replica of
StackWalkerX86 structure modified
This comment isn't useful.

http://breakpad.appspot.com/469002/diff/1/src/processor/stackwalker_MIPS.h#ne...
src/processor/stackwalker_MIPS.h:67: // A STACK CFI-driven frame walker for
mips.
This comment no longer applies.

[digit]

http://breakpad.appspot.com/469002/diff/1/Makefile.am
File Makefile.am (right):

http://breakpad.appspot.com/469002/diff/1/Makefile.am#newcode191
Makefile.am:191: src/processor/stackwalker_MIPS.cc \
I don't see a good reason to introduce random capitalization in source file
names, i.e. can you rename these files to _mips.cc instead?

http://breakpad.appspot.com/469002/diff/1/src/google_breakpad/processor/stack...
File src/google_breakpad/processor/stack_frame_cpu.h (right):

http://breakpad.appspot.com/469002/diff/1/src/google_breakpad/processor/stack...
src/google_breakpad/processor/stack_frame_cpu.h:249: // The whole enum present
here is replaced for MIPS N64 architecture
Yes, please be specific when naming types. Keep in mind that Android/MIPS is
still using the O32 MIPS ABI, so ensure your changes would allow to support this
in the future too.

For example, in the client code, test for __mips__ only when you want to check
for a MIPS architecture, use _ABIO32 or _ABIN64 when checking for a specific
MIPS ABI, etc...

http://breakpad.appspot.com/469002/diff/1/src/google_breakpad/processor/stack...
src/google_breakpad/processor/stack_frame_cpu.h:327: //  This function is added
to cope with the ARM architecture of MIPS
I don't understand any thing in this comment :)

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

http://breakpad.appspot.com/469002/diff/1/src/processor/minidump_stackwalk.cc...
src/processor/minidump_stackwalk.cc:257: } else {
this should be:
  } else if (cpu == "mips") {

http://breakpad.appspot.com/469002/diff/1/src/processor/stackwalker_MIPS.cc
File src/processor/stackwalker_MIPS.cc (right):

http://breakpad.appspot.com/469002/diff/1/src/processor/stackwalker_MIPS.cc#n...
src/processor/stackwalker_MIPS.cc:97: static const char *register_names[] = {
Please use:

  static const char* const register_names[] = {