SH4 support

SH4 support

[Ted Mielczarek]

For as invasive as new-architecture patches are they're surprisingly not that
hard to read. This looks mostly good, there's just one showstopper (the
unimplemented swap() case in minidump.cc). Also it would be good if you added a
stackwalker_sh4_unittest.cc so you can verify that stackwalking works like you
expect without having to test on-device.

Have you signed a Google CLA? You'll need to have one of those on file for us to
accept significant contributions: https://cla.developers.google.com/clas

https://breakpad.appspot.com/6814002/diff/20001/src/client/linux/dump_writer_...
File src/client/linux/dump_writer_common/thread_info.cc (right):

https://breakpad.appspot.com/6814002/diff/20001/src/client/linux/dump_writer_...
src/client/linux/dump_writer_common/thread_info.cc:287: {
nit: brace on wrong line

https://breakpad.appspot.com/6814002/diff/20001/src/client/linux/dump_writer_...
File src/client/linux/dump_writer_common/thread_info.h (right):

https://breakpad.appspot.com/6814002/diff/20001/src/client/linux/dump_writer_...
src/client/linux/dump_writer_common/thread_info.h:63: struct user regs;
The struct is really named 'user'? That's weird.

https://breakpad.appspot.com/6814002/diff/20001/src/client/linux/dump_writer_...
File src/client/linux/dump_writer_common/ucontext_reader.cc (right):

https://breakpad.appspot.com/6814002/diff/20001/src/client/linux/dump_writer_...
src/client/linux/dump_writer_common/ucontext_reader.cc:276: {
style nit: Google C++ style has braces on the end of the previous line.

The style guide is here if you haven't seen it:
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html

https://breakpad.appspot.com/6814002/diff/20001/src/client/linux/handler/exce...
File src/client/linux/handler/exception_handler.h (right):

https://breakpad.appspot.com/6814002/diff/20001/src/client/linux/handler/exce...
src/client/linux/handler/exception_handler.h:195: #if !defined(__ARM_EABI__) &&
!defined(__mips__) && !defined(__SH4__)
At this point it might be nice to move this up to the top of a header and do
something like:
#if defined(__ARM_EABI__) || defined(__mips__) || defined(__SH4__)
#define NO_FLOAT_STATE
#endif

...and use that everywhere you're touching.

https://breakpad.appspot.com/6814002/diff/20001/src/client/linux/minidump_wri...
File src/client/linux/minidump_writer/linux_ptrace_dumper.cc (right):

https://breakpad.appspot.com/6814002/diff/20001/src/client/linux/minidump_wri...
src/client/linux/minidump_writer/linux_ptrace_dumper.cc:275:
reinterpret_cast<uint8_t*>(info->regs.regs.regs[MD_CONTEXT_SH4_REG_SP]);
I can't say I understand why the other archs are using my_memcpy here, or why
mips is doing a cast, but it would be good for these all to be consistent.

https://breakpad.appspot.com/6814002/diff/20001/src/client/linux/minidump_wri...
File src/client/linux/minidump_writer/minidump_writer.cc (right):

https://breakpad.appspot.com/6814002/diff/20001/src/client/linux/minidump_wri...
src/client/linux/minidump_writer/minidump_writer.cc:801: bool
WriteCPUInformation(MDRawSystemInfo* sys_info) {
Have you verified that you get useful information out of this function on sh4?
It was producing bogus results on ARM, hence the alternate ARM implementation.

https://breakpad.appspot.com/6814002/diff/20001/src/common/dwarf_cfi_to_modul...
File src/common/dwarf_cfi_to_module.cc (right):

https://breakpad.appspot.com/6814002/diff/20001/src/common/dwarf_cfi_to_modul...
src/common/dwarf_cfi_to_module.cc:148: "r8",  "r9",  "r10", "r11", "r12", "r13",
 "r14",  "r15",
Style nit: the other architectures have the array entries lined up nicely, can
you do that here as well?

https://breakpad.appspot.com/6814002/diff/20001/src/google_breakpad/common/mi...
File src/google_breakpad/common/minidump_cpu_sh4.h (right):

https://breakpad.appspot.com/6814002/diff/20001/src/google_breakpad/common/mi...
src/google_breakpad/common/minidump_cpu_sh4.h:75: * are not exactly minidumps.
You cargo-culted this from minidump_cpu_arm, but this comment mostly doesn't
apply. I'd keep the first sentence and drop the rest.

https://breakpad.appspot.com/6814002/diff/20001/src/google_breakpad/common/mi...
src/google_breakpad/common/minidump_cpu_sh4.h:122: #define MD_CONTEXT_SHX_OLD  
0x000000c0  /* CONTEXT_SH4 (Super-H, includes SH3) */
I would just drop this constant, it's not actually used anywhere. We kept the
ARM_OLD for transition purposes because we were generating dumps with that value
before we realized that Microsoft had started using that bit for context flags.

https://breakpad.appspot.com/6814002/diff/20001/src/google_breakpad/common/mi...
src/google_breakpad/common/minidump_cpu_sh4.h:124: #define MD_CONTEXT_SHX       
   0x02000000
I kind of wish these constants were all back in minidump_format.h for ease of
reading (and adding new ones), but that's not your fault.

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

https://breakpad.appspot.com/6814002/diff/20001/src/processor/minidump.cc#new...
src/processor/minidump.cc:1122: BPLOG(ERROR) << "XXX TODO";
You'll need to fix this before landing. I don't know what endianness SH-4 chips
are in practice, but if they're ever big-endian and you try to process the dump
on a little-endian system it won't work without this code.

https://breakpad.appspot.com/6814002/diff/20001/src/processor/stackwalker_sh4.cc
File src/processor/stackwalker_sh4.cc (right):

https://breakpad.appspot.com/6814002/diff/20001/src/processor/stackwalker_sh4...
src/processor/stackwalker_sh4.cc:139: if (i >= 8 && i <= 15 &&
(last_frame->context_validity &StackFrameSH4::RegisterValidFlag(i)))
nit: use braces here since you have a multi-line conditional (even if it has its
own set of braces)

https://breakpad.appspot.com/6814002/diff/20001/src/processor/stackwalker_sh4.h
File src/processor/stackwalker_sh4.h (right):

https://breakpad.appspot.com/6814002/diff/20001/src/processor/stackwalker_sh4...
src/processor/stackwalker_sh4.h:55: // context is an arm context object that
gives access to arm-specific
s/arm/SH-4/

[Ben Chan]

https://breakpad.appspot.com/6814002/diff/20001/src/google_breakpad/common/mi...
File src/google_breakpad/common/minidump_cpu_sh4.h (right):

https://breakpad.appspot.com/6814002/diff/20001/src/google_breakpad/common/mi...
src/google_breakpad/common/minidump_cpu_sh4.h:1: /* Copyright (c) 2015, Google
Inc.
this is a new file but the header including the license text seems to be copied
from other files. It should probably be updated accordingly.

[christian.bruel]

Hi,

On Thursday, April 2, 2015 at 2:24:35 PM UTC+2, Ted Mielczarek wrote:
>
> For as invasive as new-architecture patches are they're surprisingly not 
> that hard to read. This looks mostly good, there's just one showstopper 
> (the unimplemented swap() case in minidump.cc). 


Ah yes, I forgot this one.
 

> Also it would be good if 
> you added a stackwalker_sh4_unittest.cc so you can verify that 
> stackwalking works like you expect without having to test on-device. 
>

ok
 

>
> Have you signed a Google CLA? You'll need to have one of those on file 
> for us to accept significant contributions: 
> https://cla.developers.google.com/clas 
>
>
>
We do have signed the document entitled "Google Inc. Software Grant and 
Corporate Contributor License Agreement ("Agreement"), v1.1". in May 2, 
2012. 

...
>

thanks for your review,

[christian.bruel]

with sh4_unittest

[christian.bruel]

Hi,

I've posted a new patch set version with the sh4 unittests

thanks,


Le jeudi 2 avril 2015 14:24:35 UTC+2, Ted Mielczarek a écrit :
>
> For as invasive as new-architecture patches are they're surprisingly not 
> that hard to read. This looks mostly good, there's just one showstopper 
> (the unimplemented swap() case in minidump.cc). Also it would be good if 
> you added a stackwalker_sh4_unittest.cc so you can verify that 
> stackwalking works like you expect without having to test on-device. 
>
> Have you signed a Google CLA? You'll need to have one of those on file 
> for us to accept significant contributions: 
> https://cla.developers.google.com/clas 
>
>
>
>
>
https://breakpad.appspot.com/6814002/diff/20001/src/client/linux/dump_writer_...

> File src/client/linux/dump_writer_common/thread_info.cc (right): 
>
>
https://breakpad.appspot.com/6814002/diff/20001/src/client/linux/dump_writer_...

>
> src/client/linux/dump_writer_common/thread_info.cc:287 
>
<https://breakpad.appspot.com/6814002/diff/20001/src/client/linux/dump_writer_...

> { 
> nit: brace on wrong line 
>
>
>
https://breakpad.appspot.com/6814002/diff/20001/src/client/linux/dump_writer_...

> File src/client/linux/dump_writer_common/thread_info.h (right): 
>
>
https://breakpad.appspot.com/6814002/diff/20001/src/client/linux/dump_writer_...

>
> src/client/linux/dump_writer_common/thread_info.h:63 
>
<https://breakpad.appspot.com/6814002/diff/20001/src/client/linux/dump_writer_...

> struct user regs; 
> The struct is really named 'user'? That's weird. 
>
>
>
https://breakpad.appspot.com/6814002/diff/20001/src/client/linux/dump_writer_...

> File src/client/linux/dump_writer_common/ucontext_reader.cc (right): 
>
>
https://breakpad.appspot.com/6814002/diff/20001/src/client/linux/dump_writer_...

>
> src/client/linux/dump_writer_common/ucontext_reader.cc:276 
>
<https://breakpad.appspot.com/6814002/diff/20001/src/client/linux/dump_writer_...

> { 
> style nit: Google C++ style has braces on the end of the previous line. 
>
> The style guide is here if you haven't seen it: 
> http://google-styleguide.googlecode.com/svn/trunk/cppguide.html 
>
>
>
https://breakpad.appspot.com/6814002/diff/20001/src/client/linux/handler/exce...

> File src/client/linux/handler/exception_handler.h (right): 
>
>
https://breakpad.appspot.com/6814002/diff/20001/src/client/linux/handler/exce...

>
> src/client/linux/handler/exception_handler.h:195 
>
<https://breakpad.appspot.com/6814002/diff/20001/src/client/linux/handler/exce...

> #if 
> !defined(__ARM_EABI__) && !defined(__mips__) && !defined(__SH4__) 
> At this point it might be nice to move this up to the top of a header 
> and do something like: 
> #if defined(__ARM_EABI__) || defined(__mips__) || defined(__SH4__) 
> #define NO_FLOAT_STATE 
> #endif 
>
> ...and use that everywhere you're touching. 
>
>
>
https://breakpad.appspot.com/6814002/diff/20001/src/client/linux/minidump_wri...

> File src/client/linux/minidump_writer/linux_ptrace_dumper.cc (right): 
>
>
https://breakpad.appspot.com/6814002/diff/20001/src/client/linux/minidump_wri...

>
> src/client/linux/minidump_writer/linux_ptrace_dumper.cc:275 
>
<https://breakpad.appspot.com/6814002/diff/20001/src/client/linux/minidump_wri...

>
> reinterpret_cast<uint8_t*>(info->regs.regs.regs[MD_CONTEXT_SH4_REG_SP]); 
> I can't say I understand why the other archs are using my_memcpy here, 
> or why mips is doing a cast, but it would be good for these all to be 
> consistent. 
>
>
>
https://breakpad.appspot.com/6814002/diff/20001/src/client/linux/minidump_wri...

> File src/client/linux/minidump_writer/minidump_writer.cc (right): 
>
>
https://breakpad.appspot.com/6814002/diff/20001/src/client/linux/minidump_wri...

>
> src/client/linux/minidump_writer/minidump_writer.cc:801 
>
<https://breakpad.appspot.com/6814002/diff/20001/src/client/linux/minidump_wri...

> bool 
> WriteCPUInformation(MDRawSystemInfo* sys_info) { 
> Have you verified that you get useful information out of this function 
> on sh4? It was producing bogus results on ARM, hence the alternate ARM 
> implementation. 
>
>
>
https://breakpad.appspot.com/6814002/diff/20001/src/common/dwarf_cfi_to_modul...

> File src/common/dwarf_cfi_to_module.cc (right): 
>
>
https://breakpad.appspot.com/6814002/diff/20001/src/common/dwarf_cfi_to_modul...

>
> src/common/dwarf_cfi_to_module.cc:148 
>
<https://breakpad.appspot.com/6814002/diff/20001/src/common/dwarf_cfi_to_modul...

> "r8",  "r9",  "r10", "r11", 
> "r12", "r13",  "r14",  "r15", 
> Style nit: the other architectures have the array entries lined up 
> nicely, can you do that here as well? 
>
>
>
https://breakpad.appspot.com/6814002/diff/20001/src/google_breakpad/common/mi...

> File src/google_breakpad/common/minidump_cpu_sh4.h (right): 
>
>
https://breakpad.appspot.com/6814002/diff/20001/src/google_breakpad/common/mi...

>
> src/google_breakpad/common/minidump_cpu_sh4.h:75 
>
<https://breakpad.appspot.com/6814002/diff/20001/src/google_breakpad/common/mi...

> * are not exactly 
> minidumps. 
> You cargo-culted this from minidump_cpu_arm, but this comment mostly 
> doesn't apply. I'd keep the first sentence and drop the rest. 
>
>
https://breakpad.appspot.com/6814002/diff/20001/src/google_breakpad/common/mi...

>
> src/google_breakpad/common/minidump_cpu_sh4.h:122 
>
<https://breakpad.appspot.com/6814002/diff/20001/src/google_breakpad/common/mi...

> #define 
> MD_CONTEXT_SHX_OLD   0x000000c0  /* CONTEXT_SH4 (Super-H, includes SH3) 
> */ 
> I would just drop this constant, it's not actually used anywhere. We 
> kept the ARM_OLD for transition purposes because we were generating 
> dumps with that value before we realized that Microsoft had started 
> using that bit for context flags. 
>
>
https://breakpad.appspot.com/6814002/diff/20001/src/google_breakpad/common/mi...

>
> src/google_breakpad/common/minidump_cpu_sh4.h:124 
>
<https://breakpad.appspot.com/6814002/diff/20001/src/google_breakpad/common/mi...

> #define 
> MD_CONTEXT_SHX           0x02000000 
> I kind of wish these constants were all back in minidump_format.h for 
> ease of reading (and adding new ones), but that's not your fault. 
>
> https://breakpad.appspot.com/6814002/diff/20001/src/processor/minidump.cc 
> File src/processor/minidump.cc (right): 
>
>
https://breakpad.appspot.com/6814002/diff/20001/src/processor/minidump.cc#new...

>
> src/processor/minidump.cc:1122 
>
<https://breakpad.appspot.com/6814002/diff/20001/src/processor/minidump.cc#new...

> BPLOG(ERROR) << "XXX TODO"; 
> You'll need to fix this before landing. I don't know what endianness 
> SH-4 chips are in practice, but if they're ever big-endian and you try 
> to process the dump on a little-endian system it won't work without this 
> code. 
>
>
>
https://breakpad.appspot.com/6814002/diff/20001/src/processor/stackwalker_sh4.cc

> File src/processor/stackwalker_sh4.cc (right): 
>
>
https://breakpad.appspot.com/6814002/diff/20001/src/processor/stackwalker_sh4...

>
> src/processor/stackwalker_sh4.cc:139 
>
<https://breakpad.appspot.com/6814002/diff/20001/src/processor/stackwalker_sh4...

> if (i >= 8 && i <= 15 && 
> (last_frame->context_validity &StackFrameSH4::RegisterValidFlag(i))) 
> nit: use braces here since you have a multi-line conditional (even if it 
> has its own set of braces) 
>
>
>
https://breakpad.appspot.com/6814002/diff/20001/src/processor/stackwalker_sh4.h

> File src/processor/stackwalker_sh4.h (right): 
>
>
https://breakpad.appspot.com/6814002/diff/20001/src/processor/stackwalker_sh4...

>
> src/processor/stackwalker_sh4.h:55 
>
<https://breakpad.appspot.com/6814002/diff/20001/src/processor/stackwalker_sh4...

> // context is an arm context object 
> that gives access to arm-specific 
> s/arm/SH-4/ 
>
> https://breakpad.appspot.com/6814002/ 
>

[christian.bruel]

with sh4_unittest

[christian.bruel]

could you please review this version. It should fix the aforementioned 
comments. Mostly:

 - the few slyle/nits
 - new macro CONTEXT_FLOAT_STATE for archs not having float refs in the 
ucontext
 - use endianness conversion macros
 - the sh4 unitests

thanks,

Le vendredi 17 avril 2015 14:56:49 UTC+2, christi...@st.com a écrit :
>
> Hi,
>
> I've posted a new patch set version with the sh4 unittests
>
> thanks,
>
>
> Le jeudi 2 avril 2015 14:24:35 UTC+2, Ted Mielczarek a écrit :
>>
>> For as invasive as new-architecture patches are they're surprisingly not 
>> that hard to read. This looks mostly good, there's just one showstopper 
>> (the unimplemented swap() case in minidump.cc). Also it would be good if 
>> you added a stackwalker_sh4_unittest.cc so you can verify that 
>> stackwalking works like you expect without having to test on-device. 
>>
>> Have you signed a Google CLA? You'll need to have one of those on file 
>> for us to accept significant contributions: 
>> https://cla.developers.google.com/clas 
>>
>>
>>
>>
>>
https://breakpad.appspot.com/6814002/diff/20001/src/client/linux/dump_writer_...

>> File src/client/linux/dump_writer_common/thread_info.cc (right): 
>>
>>
https://breakpad.appspot.com/6814002/diff/20001/src/client/linux/dump_writer_...

>>
>> src/client/linux/dump_writer_common/thread_info.cc:287 
>>
<https://breakpad.appspot.com/6814002/diff/20001/src/client/linux/dump_writer_...

>> { 
>> nit: brace on wrong line 
>>
>>
>>
https://breakpad.appspot.com/6814002/diff/20001/src/client/linux/dump_writer_...

>> File src/client/linux/dump_writer_common/thread_info.h (right): 
>>
>>
https://breakpad.appspot.com/6814002/diff/20001/src/client/linux/dump_writer_...

>>
>> src/client/linux/dump_writer_common/thread_info.h:63 
>>
<https://breakpad.appspot.com/6814002/diff/20001/src/client/linux/dump_writer_...

>> struct user regs; 
>> The struct is really named 'user'? That's weird. 
>>
>>
>>
https://breakpad.appspot.com/6814002/diff/20001/src/client/linux/dump_writer_...

>> File src/client/linux/dump_writer_common/ucontext_reader.cc (right): 
>>
>>
https://breakpad.appspot.com/6814002/diff/20001/src/client/linux/dump_writer_...

>>
>> src/client/linux/dump_writer_common/ucontext_reader.cc:276 
>>
<https://breakpad.appspot.com/6814002/diff/20001/src/client/linux/dump_writer_...

>> { 
>> style nit: Google C++ style has braces on the end of the previous line. 
>>
>> The style guide is here if you haven't seen it: 
>> http://google-styleguide.googlecode.com/svn/trunk/cppguide.html 
>>
>>
>>
https://breakpad.appspot.com/6814002/diff/20001/src/client/linux/handler/exce...

>> File src/client/linux/handler/exception_handler.h (right): 
>>
>>
https://breakpad.appspot.com/6814002/diff/20001/src/client/linux/handler/exce...

>>
>> src/client/linux/handler/exception_handler.h:195 
>>
<https://breakpad.appspot.com/6814002/diff/20001/src/client/linux/handler/exce...

>> #if 
>> !defined(__ARM_EABI__) && !defined(__mips__) && !defined(__SH4__) 
>> At this point it might be nice to move this up to the top of a header 
>> and do something like: 
>> #if defined(__ARM_EABI__) || defined(__mips__) || defined(__SH4__) 
>> #define NO_FLOAT_STATE 
>> #endif 
>>
>> ...and use that everywhere you're touching. 
>>
>>
>>
https://breakpad.appspot.com/6814002/diff/20001/src/client/linux/minidump_wri...

>> File src/client/linux/minidump_writer/linux_ptrace_dumper.cc (right): 
>>
>>
https://breakpad.appspot.com/6814002/diff/20001/src/client/linux/minidump_wri...

>>
>> src/client/linux/minidump_writer/linux_ptrace_dumper.cc:275 
>>
<https://breakpad.appspot.com/6814002/diff/20001/src/client/linux/minidump_wri...

>>
>> reinterpret_cast<uint8_t*>(info->regs.regs.regs[MD_CONTEXT_SH4_REG_SP]); 
>> I can't say I understand why the other archs are using my_memcpy here, 
>> or why mips is doing a cast, but it would be good for these all to be 
>> consistent. 
>>
>>
>>
https://breakpad.appspot.com/6814002/diff/20001/src/client/linux/minidump_wri...

>> File src/client/linux/minidump_writer/minidump_writer.cc (right): 
>>
>>
https://breakpad.appspot.com/6814002/diff/20001/src/client/linux/minidump_wri...

>>
>> src/client/linux/minidump_writer/minidump_writer.cc:801 
>>
<https://breakpad.appspot.com/6814002/diff/20001/src/client/linux/minidump_wri...

>> bool 
>> WriteCPUInformation(MDRawSystemInfo* sys_info) { 
>> Have you verified that you get useful information out of this function 
>> on sh4? It was producing bogus results on ARM, hence the alternate ARM 
>> implementation. 
>>
>>
>>
https://breakpad.appspot.com/6814002/diff/20001/src/common/dwarf_cfi_to_modul...

>> File src/common/dwarf_cfi_to_module.cc (right): 
>>
>>
https://breakpad.appspot.com/6814002/diff/20001/src/common/dwarf_cfi_to_modul...

>>
>> src/common/dwarf_cfi_to_module.cc:148 
>>
<https://breakpad.appspot.com/6814002/diff/20001/src/common/dwarf_cfi_to_modul...

>> "r8",  "r9",  "r10", "r11", 
>> "r12", "r13",  "r14",  "r15", 
>> Style nit: the other architectures have the array entries lined up 
>> nicely, can you do that here as well? 
>>
>>
>>
https://breakpad.appspot.com/6814002/diff/20001/src/google_breakpad/common/mi...

>> File src/google_breakpad/common/minidump_cpu_sh4.h (right): 
>>
>>
https://breakpad.appspot.com/6814002/diff/20001/src/google_breakpad/common/mi...

>>
>> src/google_breakpad/common/minidump_cpu_sh4.h:75 
>>
<https://breakpad.appspot.com/6814002/diff/20001/src/google_breakpad/common/mi...

>> * are not exactly 
>> minidumps. 
>> You cargo-culted this from minidump_cpu_arm, but this comment mostly 
>> doesn't apply. I'd keep the first sentence and drop the rest. 
>>
>>
https://breakpad.appspot.com/6814002/diff/20001/src/google_breakpad/common/mi...

>>
>> src/google_breakpad/common/minidump_cpu_sh4.h:122 
>>
<https://breakpad.appspot.com/6814002/diff/20001/src/google_breakpad/common/mi...

>> #define 
>> MD_CONTEXT_SHX_OLD   0x000000c0  /* CONTEXT_SH4 (Super-H, includes SH3) 
>> */ 
>> I would just drop this constant, it's not actually used anywhere. We 
>> kept the ARM_OLD for transition purposes because we were generating 
>> dumps with that value before we realized that Microsoft had started 
>> using that bit for context flags. 
>>
>>
https://breakpad.appspot.com/6814002/diff/20001/src/google_breakpad/common/mi...

>>
>> src/google_breakpad/common/minidump_cpu_sh4.h:124 
>>
<https://breakpad.appspot.com/6814002/diff/20001/src/google_breakpad/common/mi...

>> #define 
>> MD_CONTEXT_SHX           0x02000000 
>> I kind of wish these constants were all back in minidump_format.h for 
>> ease of reading (and adding new ones), but that's not your fault. 
>>
>> https://breakpad.appspot.com/6814002/diff/20001/src/processor/minidump.cc 
>> File src/processor/minidump.cc (right): 
>>
>>
https://breakpad.appspot.com/6814002/diff/20001/src/processor/minidump.cc#new...

>>
>> src/processor/minidump.cc:1122 
>>
<https://breakpad.appspot.com/6814002/diff/20001/src/processor/minidump.cc#new...

>> BPLOG(ERROR) << "XXX TODO"; 
>> You'll need to fix this before landing. I don't know what endianness 
>> SH-4 chips are in practice, but if they're ever big-endian and you try 
>> to process the dump on a little-endian system it won't work without this 
>> code. 
>>
>>
>>
https://breakpad.appspot.com/6814002/diff/20001/src/processor/stackwalker_sh4.cc

>> File src/processor/stackwalker_sh4.cc (right): 
>>
>>
https://breakpad.appspot.com/6814002/diff/20001/src/processor/stackwalker_sh4...

>>
>> src/processor/stackwalker_sh4.cc:139 
>>
<https://breakpad.appspot.com/6814002/diff/20001/src/processor/stackwalker_sh4...

>> if (i >= 8 && i <= 15 && 
>> (last_frame->context_validity &StackFrameSH4::RegisterValidFlag(i))) 
>> nit: use braces here since you have a multi-line conditional (even if it 
>> has its own set of braces) 
>>
>>
>>
https://breakpad.appspot.com/6814002/diff/20001/src/processor/stackwalker_sh4.h

>> File src/processor/stackwalker_sh4.h (right): 
>>
>>
https://breakpad.appspot.com/6814002/diff/20001/src/processor/stackwalker_sh4...

>>
>> src/processor/stackwalker_sh4.h:55 
>>
<https://breakpad.appspot.com/6814002/diff/20001/src/processor/stackwalker_sh4...

>> // context is an arm context object 
>> that gives access to arm-specific 
>> s/arm/SH-4/ 
>>
>> https://breakpad.appspot.com/6814002/ 
>>
>

[Ted Mielczarek]

Just one thing that needs to be fixed before landing, you missed adding
stackwalker_sh4_unittest to Makefile.am.

Have you run the unit tests on a SH4 device?

https://breakpad.appspot.com/6814002/diff/250002/Makefile.am
File Makefile.am (right):

https://breakpad.appspot.com/6814002/diff/250002/Makefile.am#newcode354
Makefile.am:354: src/processor/stackwalker_x86_unittest \
You need to add stackwalker_sh4_unittest to this list as well.

https://breakpad.appspot.com/6814002/diff/250002/src/client/linux/handler/exc...
File src/client/linux/handler/exception_handler.h (right):

https://breakpad.appspot.com/6814002/diff/250002/src/client/linux/handler/exc...
src/client/linux/handler/exception_handler.h:51: #define CONTEXT_FLOAT_STATE
Thanks, this is nicer.

https://breakpad.appspot.com/6814002/diff/250002/src/client/linux/minidump_wr...
File src/client/linux/minidump_writer/minidump_writer.h (right):

https://breakpad.appspot.com/6814002/diff/250002/src/client/linux/minidump_wr...
src/client/linux/minidump_writer/minidump_writer.h:50: #elif
!defined(__ARM_EABI__) && !defined(__mips__) && !defined(__SH4__)
Oh hum, this gets included before the #define you added in exception_handler.h?
You could move the #define here, but I'm not that hung up on it.

https://breakpad.appspot.com/6814002/diff/250002/src/processor/stackwalker_sh...
File src/processor/stackwalker_sh4_unittest.cc (right):

https://breakpad.appspot.com/6814002/diff/250002/src/processor/stackwalker_sh...
src/processor/stackwalker_sh4_unittest.cc:2: // All rights reserved.
Thanks for adding tests. I know these are a very copy-paste affair, I wish it
was easier to write generic tests for this stuff.

https://breakpad.appspot.com/6814002/diff/250002/src/processor/stackwalker_sh...
src/processor/stackwalker_sh4_unittest.cc:70: class StackwalkerSH4Fixture {
We should probably move this out into a common header for all the tests to
subclass at some point.

https://breakpad.appspot.com/6814002/diff/250002/src/tools/linux/md2core/mini...
File src/tools/linux/md2core/minidump-2-core.cc (right):

https://breakpad.appspot.com/6814002/diff/250002/src/tools/linux/md2core/mini...
src/tools/linux/md2core/minidump-2-core.cc:78: #define ELF_ARCH  EM_SH
These changes are going to conflict with https://breakpad.appspot.com/10734002/
depending on who lands first.

[christian.bruel]

Hi Ted,

On 05/04/2015 01:39 PM, ted.mielczarek@gmail.com wrote:
> Just one thing that needs to be fixed before landing, you missed adding
> stackwalker_sh4_unittest to Makefile.am.

Interestingly, I saw PASS: src/processor/stackwalker_sh4_unittest
with make check, I wonder what consequence had this missing dependency

do I need to resend the patch or is it just "as obvious" ?

> 
> Have you run the unit tests on a SH4 device?
> 
> 

yes I did, they were ran on a SH4-STLinux distro:

machine		: hdk7108
processor	: 0
cpu family	: sh4
cpu variant	: st40-300
cpu type	: STx7108
cut		: 2.x
cpu flags	: fpu icbi synco fpchg
cache type	: split (harvard)
icache size	: 32KiB (2-way)
dcache size	: 32KiB (2-way)
address sizes	: 32 bits physical
bogomips	: 500.00

I also ran the build+tests on a x86_64-linux-gnu host.

thanks

Christian

> https://breakpad.appspot.com/6814002/diff/250002/Makefile.am
> File Makefile.am (right):
> 
> https://breakpad.appspot.com/6814002/diff/250002/Makefile.am#newcode354
> Makefile.am:354: src/processor/stackwalker_x86_unittest \
> You need to add stackwalker_sh4_unittest to this list as well.
> 
>
https://breakpad.appspot.com/6814002/diff/250002/src/client/linux/handler/exc...
> File src/client/linux/handler/exception_handler.h (right):
> 
>
https://breakpad.appspot.com/6814002/diff/250002/src/client/linux/handler/exc...
> src/client/linux/handler/exception_handler.h:51: #define
> CONTEXT_FLOAT_STATE
> Thanks, this is nicer.
> 
>
https://breakpad.appspot.com/6814002/diff/250002/src/client/linux/minidump_wr...
> File src/client/linux/minidump_writer/minidump_writer.h (right):
> 
>
https://breakpad.appspot.com/6814002/diff/250002/src/client/linux/minidump_wr...
> src/client/linux/minidump_writer/minidump_writer.h:50: #elif
> !defined(__ARM_EABI__) && !defined(__mips__) && !defined(__SH4__)
> Oh hum, this gets included before the #define you added in
> exception_handler.h? You could move the #define here, but I'm not that
> hung up on it.
> 
>
https://breakpad.appspot.com/6814002/diff/250002/src/processor/stackwalker_sh...
> File src/processor/stackwalker_sh4_unittest.cc (right):
> 
>
https://breakpad.appspot.com/6814002/diff/250002/src/processor/stackwalker_sh...
> src/processor/stackwalker_sh4_unittest.cc:2: // All rights reserved.
> Thanks for adding tests. I know these are a very copy-paste affair, I
> wish it was easier to write generic tests for this stuff.
> 
>
https://breakpad.appspot.com/6814002/diff/250002/src/processor/stackwalker_sh...
> src/processor/stackwalker_sh4_unittest.cc:70: class
> StackwalkerSH4Fixture {
> We should probably move this out into a common header for all the tests
> to subclass at some point.
> 
>
https://breakpad.appspot.com/6814002/diff/250002/src/tools/linux/md2core/mini...
> File src/tools/linux/md2core/minidump-2-core.cc (right):
> 
>
https://breakpad.appspot.com/6814002/diff/250002/src/tools/linux/md2core/mini...
> src/tools/linux/md2core/minidump-2-core.cc:78: #define ELF_ARCH  EM_SH
> These changes are going to conflict with
> https://breakpad.appspot.com/10734002/ depending on who lands first.
> 
> https://breakpad.appspot.com/6814002/
>