[MIPS] Add support for Breakpad for MIPS

[MIPS] Add support for Breakpad for MIPS

This change adds support for MIPS in the breakpad.
Based on original patches from Chris Dearman.

BUG: None
TEST: Running manual tests including unittests

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

[Lei Zhang (chromium)]

Here's a first pass. Mostly trying to help get the style right.

https://breakpad.appspot.com/614002/diff/16001/configure
File configure (right):

https://breakpad.appspot.com/614002/diff/16001/configure#newcode1
configure:1: #! /bin/sh
Are you actually making any changes to this file, or is this all just changes
from using a different version of autoconf?

https://breakpad.appspot.com/614002/diff/16001/src/client/linux/handler/excep...
File src/client/linux/handler/exception_handler.cc (right):

https://breakpad.appspot.com/614002/diff/16001/src/client/linux/handler/excep...
src/client/linux/handler/exception_handler.cc:401: // FP state is part of struct
ucontext for MIPS Linux
did you mean to say "not part of" ?

https://breakpad.appspot.com/614002/diff/16001/src/client/linux/handler/excep...
File src/client/linux/handler/exception_handler_unittest.cc (right):

https://breakpad.appspot.com/614002/diff/16001/src/client/linux/handler/excep...
src/client/linux/handler/exception_handler_unittest.cc:76:
__builtin___clear_cache(const_cast< char*>(memory), const_cast<char*>(memory) +
memory_size);
nit: no space in const_cast< char*>

https://breakpad.appspot.com/614002/diff/16001/src/client/linux/handler/excep...
src/client/linux/handler/exception_handler_unittest.cc:453: const unsigned char
instructions[] = { 0x48, 0x00, 0x00, 0x48 };
Would you mind renaming |instructions| to "kIllegalInstruction" here and below.
I realize this is pre-existing code, but it would be nice to not add more code
that violates the Google C++ Style Guide.

I would also just move it to the anonymous namespace above so you don't have to
declare it twice.

https://breakpad.appspot.com/614002/diff/16001/src/client/linux/minidump_writ...
File src/client/linux/minidump_writer/minidump_writer.cc (right):

https://breakpad.appspot.com/614002/diff/16001/src/client/linux/minidump_writ...
src/client/linux/minidump_writer/minidump_writer.cc:407: static void
CPUFillFromUContext(MDRawContextMIPS *out, const ucontext *uc,
nit: be consistent with "Foo* bar" vs "Foo *bar"

Ditto with ++i and i++.

Google C++ style guide prefers the former but lots of code in Breakpad uses the
latter.

https://breakpad.appspot.com/614002/diff/16001/src/client/linux/minidump_writ...
src/client/linux/minidump_writer/minidump_writer.cc:1312: #elif
defined(__mips__)
nit: Put this first so x86 and amd64 stays together.

https://breakpad.appspot.com/614002/diff/16001/src/common/linux/dump_symbols.cc
File src/common/linux/dump_symbols.cc (right):

https://breakpad.appspot.com/614002/diff/16001/src/common/linux/dump_symbols....
src/common/linux/dump_symbols.cc:581: elf_header->e_machine == EM_MIPS ?
Just do this once and use the result in all 3 calls.

https://breakpad.appspot.com/614002/diff/16001/src/google_breakpad/common/min...
File src/google_breakpad/common/minidump_cpu_mips.h (right):

https://breakpad.appspot.com/614002/diff/16001/src/google_breakpad/common/min...
src/google_breakpad/common/minidump_cpu_mips.h:1: /* Copyright (c) 2012, Google
Inc.
It's 2013.

https://breakpad.appspot.com/614002/diff/16001/src/google_breakpad/processor/...
File src/google_breakpad/processor/stack_frame_cpu.h (right):

https://breakpad.appspot.com/614002/diff/16001/src/google_breakpad/processor/...
src/google_breakpad/processor/stack_frame_cpu.h:310: /*
Remove this block. Add it when you are ready.

https://breakpad.appspot.com/614002/diff/16001/src/google_breakpad/processor/...
src/google_breakpad/processor/stack_frame_cpu.h:333: if (n>=INDEX_MIPS_REG_S0 &&
n <=INDEX_MIPS_REG_S7)
nit: spacing in this entireh function is a bit funny.

https://breakpad.appspot.com/614002/diff/16001/src/google_breakpad/processor/...
src/google_breakpad/processor/stack_frame_cpu.h:344: : context(),
indent 2 more spaces

https://breakpad.appspot.com/614002/diff/16001/src/google_breakpad/processor/...
src/google_breakpad/processor/stack_frame_cpu.h:352: // context_validity.
Refer to the variable as |context_validity|. Ditto below.

https://breakpad.appspot.com/614002/diff/16001/src/google_breakpad/processor/...
src/google_breakpad/processor/stack_frame_cpu.h:355: // context_validity is
actually ContextValidity, but int is used because
s/int/it/

https://breakpad.appspot.com/614002/diff/16001/src/google_breakpad/processor/...
src/google_breakpad/processor/stack_frame_cpu.h:360: // Any stack walking
information that is found describing this.instruction.
this.instruction -> this instruction

https://breakpad.appspot.com/614002/diff/16001/src/processor/minidump_unittes...
File src/processor/minidump_unittest.cc (right):

https://breakpad.appspot.com/614002/diff/16001/src/processor/minidump_unittes...
src/processor/minidump_unittest.cc:1194: 0x1234abcd, // thread id
nit: two spaces before //

https://breakpad.appspot.com/614002/diff/16001/src/processor/minidump_unittes...
src/processor/minidump_unittest.cc:1232: & MD_CONTEXT_MIPS_INTEGER));
nit: fits on previous line. I realize this is copy + pasted.

https://breakpad.appspot.com/614002/diff/16001/src/processor/stackwalker.cc
File src/processor/stackwalker.cc (right):

https://breakpad.appspot.com/614002/diff/16001/src/processor/stackwalker.cc#n...
src/processor/stackwalker.cc:219: memory,modules, frame_symbolizer);
nit: space after comma

https://breakpad.appspot.com/614002/diff/16001/src/processor/stackwalker_mips.cc
File src/processor/stackwalker_mips.cc (right):

https://breakpad.appspot.com/614002/diff/16001/src/processor/stackwalker_mips...
src/processor/stackwalker_mips.cc:49: using std::hex;
not needed? same with std::dec.

https://breakpad.appspot.com/614002/diff/16001/src/processor/stackwalker_mips...
src/processor/stackwalker_mips.cc:53: StackwalkerMIPS::StackwalkerMIPS (const
SystemInfo* system_info,
nit: no space before (

Ditto below.

https://breakpad.appspot.com/614002/diff/16001/src/processor/stackwalker_mips...
src/processor/stackwalker_mips.cc:62: BPLOG(ERROR) << "Memory out of range for
stackwalking: " <<
nit: BPLOG(ERROR) << "foo "
                  << "and "
                  << "bar."

https://breakpad.appspot.com/614002/diff/16001/src/processor/stackwalker_mips...
src/processor/stackwalker_mips.cc:71: if (cfi_frame_info)
Where is this variable defined?

https://breakpad.appspot.com/614002/diff/16001/src/processor/stackwalker_mips...
src/processor/stackwalker_mips.cc:100: // TODO: add float point save registers?
TODO(developer_name)

https://breakpad.appspot.com/614002/diff/16001/src/processor/stackwalker_mips...
src/processor/stackwalker_mips.cc:123:
CFIFrameInfo::RegisterValueMap<uint32_t>::iterator entry;
Not used? There's a shadow variable on line 161.

https://breakpad.appspot.com/614002/diff/16001/src/processor/stackwalker_mips...
src/processor/stackwalker_mips.cc:140:
CFIFrameInfo::RegisterValueMap<u_int32_t>::iterator entry1 =
Can |entry1| have a better name?

https://breakpad.appspot.com/614002/diff/16001/src/processor/stackwalker_mips...
src/processor/stackwalker_mips.cc:140:
CFIFrameInfo::RegisterValueMap<u_int32_t>::iterator entry1 =
You can use a const_iterator

https://breakpad.appspot.com/614002/diff/16001/src/processor/stackwalker_mips...
src/processor/stackwalker_mips.cc:140:
CFIFrameInfo::RegisterValueMap<u_int32_t>::iterator entry1 =
nit: be consistent with uint32_t vs u_int32_t. Ditto elsewhere.

https://breakpad.appspot.com/614002/diff/16001/src/processor/stackwalker_mips...
src/processor/stackwalker_mips.cc:141: caller_registers.find(".cfa");
nit: 4 space indent

https://breakpad.appspot.com/614002/diff/16001/src/processor/stackwalker_mips...
src/processor/stackwalker_mips.cc:161:
CFIFrameInfo::RegisterValueMap<u_int32_t>::iterator entry =
const_iterator

https://breakpad.appspot.com/614002/diff/16001/src/processor/stackwalker_mips...
src/processor/stackwalker_mips.cc:162: caller_registers.find(register_names[i]);
nit: indentation

https://breakpad.appspot.com/614002/diff/16001/src/processor/stackwalker_mips...
src/processor/stackwalker_mips.cc:168: } else if ((i>=INDEX_MIPS_REG_S0 &&
i<=INDEX_MIPS_REG_S7
nit:

if (foo &&
    bar)

https://breakpad.appspot.com/614002/diff/16001/src/processor/stackwalker_mips...
src/processor/stackwalker_mips.cc:177: //frame->context_validity |=
StackFrameMIPS::RegisterValidFlag(i);
remove? uncomment?

https://breakpad.appspot.com/614002/diff/16001/src/processor/stackwalker_mips...
src/processor/stackwalker_mips.cc:200: if (!new_frame.get()) {
This statement will always be true. Just put a '{' on this line.

https://breakpad.appspot.com/614002/diff/16001/src/processor/stackwalker_mips...
src/processor/stackwalker_mips.cc:209: if (!new_frame.get())  {
nit: extra space before '{', ditto below.

https://breakpad.appspot.com/614002/diff/16001/src/processor/stackwalker_mips...
src/processor/stackwalker_mips.cc:218: if (new_frame->context.epc== 0)  {
nit: space before ==, also before <= on line 225.

https://breakpad.appspot.com/614002/diff/16001/src/processor/stackwalker_mips...
src/processor/stackwalker_mips.cc:226:
last_frame->context.iregs[MD_CONTEXT_MIPS_REG_SP]){
nit: align, space before '{'

https://breakpad.appspot.com/614002/diff/16001/src/processor/stackwalker_mips...
src/processor/stackwalker_mips.cc:237: #define MAX_FRAME_STACK_SIZE 1024
Replace with: const uint32_t kMaxFrameStackSize = 1024; inside the method?

https://breakpad.appspot.com/614002/diff/16001/src/processor/stackwalker_mips...
src/processor/stackwalker_mips.cc:255: {
why this extra set of braces?

https://breakpad.appspot.com/614002/diff/16001/src/processor/stackwalker_mips...
src/processor/stackwalker_mips.cc:260: int count =
MAX_FRAME_STACK_SIZE/sizeof(caller_epc);
nit: spaces around '/'

https://breakpad.appspot.com/614002/diff/16001/src/processor/stackwalker_mips...
src/processor/stackwalker_mips.cc:267: last_esp +=  4*sizeof(caller_epc);
nit: spacing - last_esp += 4 * sizeof(caller_epc);

https://breakpad.appspot.com/614002/diff/16001/src/processor/stackwalker_mips...
src/processor/stackwalker_mips.cc:270: {
put this on the previous line. Ditto below.

https://breakpad.appspot.com/614002/diff/16001/src/processor/stackwalker_mips...
src/processor/stackwalker_mips.cc:279: if
(!memory_->GetMemoryAtAddress(caller_esp-sizeof(caller_epc),
nit: spaces around minus operator.

https://breakpad.appspot.com/614002/diff/16001/src/processor/stackwalker_mips...
src/processor/stackwalker_mips.cc:286: count = count - ( caller_esp -
last_esp)/sizeof(caller_epc);
nit: fix spacing.

https://breakpad.appspot.com/614002/diff/16001/src/processor/stackwalker_mips...
src/processor/stackwalker_mips.cc:289: }while (caller_efp - caller_esp >=
MAX_FRAME_STACK_SIZE && count > 0);
space before while

extra parenthesis may help readability.

https://breakpad.appspot.com/614002/diff/16001/src/processor/stackwalker_mips...
src/processor/stackwalker_mips.cc:305: trust = StackFrame::FRAME_TRUST_SCAN;
If it's always this value, then just put it on line 311 and get rid of |trust|.

https://breakpad.appspot.com/614002/diff/16001/src/processor/stackwalker_mips.h
File src/processor/stackwalker_mips.h (right):

https://breakpad.appspot.com/614002/diff/16001/src/processor/stackwalker_mips...
src/processor/stackwalker_mips.h:79: int context_validity;
nit: context_validity_

Also, is this used?

https://breakpad.appspot.com/614002/diff/16001/src/tools/linux/md2core/minidu...
File src/tools/linux/md2core/minidump-2-core.cc (right):

https://breakpad.appspot.com/614002/diff/16001/src/tools/linux/md2core/minidu...
src/tools/linux/md2core/minidump-2-core.cc:444: #elif defined(__mips__)
not needed?

[Ben Chan]

https://breakpad.appspot.com/614002/diff/26001/src/client/linux/minidump_writ...
File src/client/linux/minidump_writer/linux_ptrace_dumper.cc (right):

https://breakpad.appspot.com/614002/diff/26001/src/client/linux/minidump_writ...
src/client/linux/minidump_writer/linux_ptrace_dumper.cc:214:
(void*)(DSP_BASE+(i*2)), &info->hi[i]);
use c++ style cast

https://breakpad.appspot.com/614002/diff/26001/src/client/linux/minidump_writ...
src/client/linux/minidump_writer/linux_ptrace_dumper.cc:216:
(void*)(DSP_BASE+(i*2)+1), &info->lo[i]);
use c++ style cast

https://breakpad.appspot.com/614002/diff/26001/src/client/linux/minidump_writ...
src/client/linux/minidump_writer/linux_ptrace_dumper.cc:219: (void*)DSP_CONTROL,
&info->dsp_control);
use c++ style cast

https://breakpad.appspot.com/614002/diff/26001/src/client/linux/minidump_writ...
File src/client/linux/minidump_writer/minidump_writer.cc (right):

https://breakpad.appspot.com/614002/diff/26001/src/client/linux/minidump_writ...
src/client/linux/minidump_writer/minidump_writer.cc:402: out->float_save.regs[i]
= info.fpregs.regs[i];
add a line break, to make it obvious that the following two lines are not part
of the loop

https://breakpad.appspot.com/614002/diff/26001/src/client/linux/minidump_writ...
src/client/linux/minidump_writer/minidump_writer.cc:431: out->float_save.regs[i]
= uc->uc_mcontext.fpregs.fp_r.fp_dregs[i];
add a line break

https://breakpad.appspot.com/614002/diff/26001/src/common/dwarf/dwarf2reader.cc
File src/common/dwarf/dwarf2reader.cc (right):

https://breakpad.appspot.com/614002/diff/26001/src/common/dwarf/dwarf2reader....
src/common/dwarf/dwarf2reader.cc:1525: Handler::kCFARegister))
indent

https://breakpad.appspot.com/614002/diff/26001/src/processor/minidump.cc
File src/processor/minidump.cc (right):

https://breakpad.appspot.com/614002/diff/26001/src/processor/minidump.cc#newc...
src/processor/minidump.cc:818: for (unsigned int ireg_index = 0;
per style guide, use 'int' instead of 'unsigned int'

https://breakpad.appspot.com/614002/diff/26001/src/processor/minidump.cc#newc...
src/processor/minidump.cc:825: for (unsigned int dsp_index = 0;
dotto

https://breakpad.appspot.com/614002/diff/26001/src/processor/minidump.cc#newc...
src/processor/minidump.cc:836: for (unsigned int fpr_index = 0;
ditto

https://breakpad.appspot.com/614002/diff/26001/src/processor/minidump.cc#newc...
src/processor/minidump.cc:1369: for (unsigned int ireg_index = 0;
ditto

https://breakpad.appspot.com/614002/diff/26001/src/processor/minidump.cc#newc...
src/processor/minidump.cc:1379: for (unsigned int dsp_index = 0;
ditto

https://breakpad.appspot.com/614002/diff/26001/src/processor/minidump.cc#newc...
src/processor/minidump.cc:1398: for (unsigned int fpr_index = 0;
ditto

https://breakpad.appspot.com/614002/diff/26001/src/processor/stackwalker_mips.cc
File src/processor/stackwalker_mips.cc (right):

https://breakpad.appspot.com/614002/diff/26001/src/processor/stackwalker_mips...
src/processor/stackwalker_mips.cc:88: static const char* const register_names[]
= {
constants should be named as kRegisterNames

https://breakpad.appspot.com/614002/diff/26001/src/processor/stackwalker_mips...
src/processor/stackwalker_mips.cc:274: count = count - ( caller_esp - last_esp)
/ sizeof(caller_epc);
remove the space between ( and caller_esp

https://breakpad.appspot.com/614002/diff/26001/src/processor/synth_minidump.cc
File src/processor/synth_minidump.cc (right):

https://breakpad.appspot.com/614002/diff/26001/src/processor/synth_minidump.c...
src/processor/synth_minidump.cc:199: : Section(dump) {
indent two more spaces

https://breakpad.appspot.com/614002/diff/26001/src/processor/synth_minidump.c...
src/processor/synth_minidump.cc:212: D32(context.hi[i]);
line break

https://breakpad.appspot.com/614002/diff/26001/src/processor/synth_minidump.c...
src/processor/synth_minidump.cc:214: D32(context.lo[i]);
line break

https://breakpad.appspot.com/614002/diff/26001/src/processor/synth_minidump.c...
src/processor/synth_minidump.cc:224: D64(context.float_save.regs[i]);
line break

https://breakpad.appspot.com/614002/diff/26001/src/tools/linux/md2core/minidu...
File src/tools/linux/md2core/minidump-2-core.cc (right):

https://breakpad.appspot.com/614002/diff/26001/src/tools/linux/md2core/minidu...
src/tools/linux/md2core/minidump-2-core.cc:367: thread->regs.regs[i] =
rawregs->iregs[i];
add a line break

https://breakpad.appspot.com/614002/diff/26001/src/tools/linux/md2core/minidu...
src/tools/linux/md2core/minidump-2-core.cc:376: thread->fpregs.regs[i] =
rawregs->float_save.regs[i];
add a line break

[Lei Zhang (chromium)]

https://breakpad.appspot.com/614002/diff/39002/src/client/linux/handler/excep...
File src/client/linux/handler/exception_handler.cc (right):

https://breakpad.appspot.com/614002/diff/39002/src/client/linux/handler/excep...
src/client/linux/handler/exception_handler.cc:402: // and 'float_state' is not a
memeber of CrashContext.
typo: member

https://breakpad.appspot.com/614002/diff/39002/src/client/linux/minidump_writ...
File src/client/linux/minidump_writer/minidump_writer.cc (right):

https://breakpad.appspot.com/614002/diff/39002/src/client/linux/minidump_writ...
src/client/linux/minidump_writer/minidump_writer.cc:427: out->badvaddr = 0; //
Not reported in signal context.
nit: double spaces before //

https://breakpad.appspot.com/614002/diff/39002/src/google_breakpad/processor/...
File src/google_breakpad/processor/stack_frame_cpu.h (right):

https://breakpad.appspot.com/614002/diff/39002/src/google_breakpad/processor/...
src/google_breakpad/processor/stack_frame_cpu.h:274: // MIPS callee save
registers are:
indent comments 2 spaces.

https://breakpad.appspot.com/614002/diff/39002/src/google_breakpad/processor/...
src/google_breakpad/processor/stack_frame_cpu.h:321: else
you can omit this line.

https://breakpad.appspot.com/614002/diff/39002/src/processor/minidump_unittes...
File src/processor/minidump_unittest.cc (right):

https://breakpad.appspot.com/614002/diff/39002/src/processor/minidump_unittes...
src/processor/minidump_unittest.cc:1216: ASSERT_EQ(0x1234abcd, thread_id);
0x1234abcdU

https://breakpad.appspot.com/614002/diff/39002/src/processor/minidump_unittes...
src/processor/minidump_unittest.cc:1222: EXPECT_EQ(0x0919a9b9,
0x09a9a9b9U

https://breakpad.appspot.com/614002/diff/39002/src/tools/linux/md2core/minidu...
File src/tools/linux/md2core/minidump-2-core.cc (right):

https://breakpad.appspot.com/614002/diff/39002/src/tools/linux/md2core/minidu...
src/tools/linux/md2core/minidump-2-core.cc:362:
ParseThreadRegisters(CrashedProcess::Thread* thread,
nit: this goes on the previous line.

static void ParseThreadRegisters(CrashedProcess::Thread* thread,

https://breakpad.appspot.com/614002/diff/39002/src/tools/linux/md2core/minidu...
src/tools/linux/md2core/minidump-2-core.cc:446: #elif !defined(__mips__)
Shouldn't you add a block and check |sysinfo->processor_architecture| like all
the other archs above?

[Lei Zhang (chromium)]

https://breakpad.appspot.com/614002/diff/39002/src/google_breakpad/common/min...
File src/google_breakpad/common/minidump_cpu_mips.h (right):

https://breakpad.appspot.com/614002/diff/39002/src/google_breakpad/common/min...
src/google_breakpad/common/minidump_cpu_mips.h:178: #endif  /*
GOOGLE_BREAKPAD_COMMON_MINIDUMP_CPU_MIPS_H__ */
nit: #endif  // ...

https://breakpad.appspot.com/614002/diff/39002/src/processor/stackwalker_mips.cc
File src/processor/stackwalker_mips.cc (right):

https://breakpad.appspot.com/614002/diff/39002/src/processor/stackwalker_mips...
src/processor/stackwalker_mips.cc:132:
CFIFrameInfo::RegisterValueMap<uint32_t>::iterator entry =
const_iterator?

https://breakpad.appspot.com/614002/diff/39002/src/processor/stackwalker_mips...
src/processor/stackwalker_mips.cc:189: const vector<StackFrame*> &frames =
*stack->frames();
nit: const foo& bar...

https://breakpad.appspot.com/614002/diff/39002/src/processor/stackwalker_mips...
src/processor/stackwalker_mips.cc:228: const uint32_t kMaxFrameStackSize = 1024;
move this down 3 lines so it has the minimal necessary scope?

[vapier]

https://breakpad.appspot.com/614002/diff/63002/src/client/linux/handler/excep...
File src/client/linux/handler/exception_handler_unittest.cc (right):

https://breakpad.appspot.com/614002/diff/63002/src/client/linux/handler/excep...
src/client/linux/handler/exception_handler_unittest.cc:76:
__builtin___clear_cache(const_cast<char*>(memory),
i thought mips provided __NR_cacheflush ?  glibc/uClibc provide a cacheflush()
func right in the C library itself via sys/cachectl.h.  is there a reason for
preferring the builtin (which looks like is new to gcc-4.3) over the C library
func (which has been around since the initial glibc port) ?

https://breakpad.appspot.com/614002/diff/63002/src/client/linux/handler/excep...
src/client/linux/handler/exception_handler_unittest.cc:443: const unsigned char
kIllegalInstruction[] = { 0x48, 0x00, 0x00, 0x48 };
do we discourage inline ifdefs ?  otherwise i'd think this would be better
written as:

   const unsigned char kIllegalInstruction[] = {
 #if defined(__mips__)
     // mfc2 zero,Impl - usually illegal in userspace.
     0x48, 0x00, 0x00, 0x48
 #else
     // This crashes with SIGILL on x86/x86-64/arm.
     0xff, 0xff, 0xff, 0xff
 #endif
   };

https://breakpad.appspot.com/614002/diff/63002/src/client/linux/minidump_writ...
File src/client/linux/minidump_writer/linux_dumper.h (right):

https://breakpad.appspot.com/614002/diff/63002/src/client/linux/minidump_writ...
src/client/linux/minidump_writer/linux_dumper.h:57: #if defined(__i386) ||
defined(__ARM_EABI__) || defined(__mips__)
this doesn't seem right.  aren't you assuming like o32 ABI here ?  n32/n64 ABI
will use Elf64_auxv_t won't it ?

https://breakpad.appspot.com/614002/diff/63002/src/client/linux/minidump_writ...
src/client/linux/minidump_writer/linux_dumper.h:92: user_regs_struct regs;
is this correct for all mips ABIs ?  if it's assuming a specific one, you should
add a comment, or fix it so it works for all ABIs.

[cjhopman]

Adding ivan.penkov@gmail.com and mark@chromium.org as reviewers.

[mankapura]

Add additional reviewers.

[Mark Mentovai]

https://breakpad.appspot.com/614002/diff/63002/src/client/linux/handler/excep...
File src/client/linux/handler/exception_handler_unittest.cc (right):

https://breakpad.appspot.com/614002/diff/63002/src/client/linux/handler/excep...
src/client/linux/handler/exception_handler_unittest.cc:443: const unsigned char
kIllegalInstruction[] = { 0x48, 0x00, 0x00, 0x48 };
vapier wrote:
> do we discourage inline ifdefs ?  otherwise i'd think this would be better
> written as:
> 
>    const unsigned char kIllegalInstruction[] = {
>  #if defined(__mips__)
>      // mfc2 zero,Impl - usually illegal in userspace.
>      0x48, 0x00, 0x00, 0x48
>  #else
>      // This crashes with SIGILL on x86/x86-64/arm.
>      0xff, 0xff, 0xff, 0xff
>  #endif
>    };

x86/x86_64 has a reserved undefined opcode for this, ud2, { 0x0f 0x0b }. Since
this is doing something per-architecture, it should use that for x86.

https://breakpad.appspot.com/614002/diff/63002/src/client/linux/handler/excep...
src/client/linux/handler/exception_handler_unittest.cc:443: const unsigned char
kIllegalInstruction[] = { 0x48, 0x00, 0x00, 0x48 };
Definitely don’t indent this, it makes it look like it’s in some {scope} that it
isn’t. You can do what vapier suggests.

https://breakpad.appspot.com/614002/diff/63002/src/google_breakpad/common/min...
File src/google_breakpad/common/minidump_cpu_mips.h (right):

https://breakpad.appspot.com/614002/diff/63002/src/google_breakpad/common/min...
src/google_breakpad/common/minidump_cpu_mips.h:30: /* minidump_format.h: A
cross-platform reimplementation of minidump-related
?

[Ivan Penkov]

The main issue for me is that the MIPS stackwalker is missing unittests.

https://breakpad.appspot.com/614002/diff/63002/src/processor/minidump.cc
File src/processor/minidump.cc (right):

https://breakpad.appspot.com/614002/diff/63002/src/processor/minidump.cc#newc...
src/processor/minidump.cc:784: case MD_CONTEXT_MIPS: {
Maybe it's time to break this method into smaller junks?

https://breakpad.appspot.com/614002/diff/63002/src/processor/stackwalker_mips.h
File src/processor/stackwalker_mips.h (right):

https://breakpad.appspot.com/614002/diff/63002/src/processor/stackwalker_mips...
src/processor/stackwalker_mips.h:50: class StackwalkerMIPS : public Stackwalker
{
I don't see tests for this class.  For examples on how to add tests, take a look
at existing tests for the other architectures:
stackwalker_(x86|amd64|arm)_unittest.cc

https://breakpad.appspot.com/614002/diff/63002/src/processor/stackwalker_mips...
src/processor/stackwalker_mips.h:63: virtual StackFrame* GetContextFrame();
Provide short description for all methods.  No need to duplicate the same
description in the .cc file.

[Ivan Penkov]

LGTM.  The unit tests for the MIPS stack walker look good.