Add PPC64 support to breakpad processor.

Add PPC64 support to breakpad processor.

[jijia]

Hi Lei,

This is my CL to add support of PPC64 to breakpad because we need PPC64 support
for our project.

Jia Ji

[Lei Zhang (chromium)]

You need to add entries for processor/stackwalker_ppc64.* in Makefile.am and
Makefile.in. It's probably sufficient to just copy every reference to
stackwalker_ppc.*.

https://breakpad.appspot.com/557002/diff/1006/src/processor/stackwalker_ppc64.cc
File src/processor/stackwalker_ppc64.cc (right):

https://breakpad.appspot.com/557002/diff/1006/src/processor/stackwalker_ppc64...
src/processor/stackwalker_ppc64.cc:53: }
extra brace -> code doesn't compile.

https://breakpad.appspot.com/557002/diff/1006/src/processor/stackwalker_ppc64...
src/processor/stackwalker_ppc64.cc:97: u_int64_t stack_pointer;
uint64_t, ditto for |instruction| on line 109.

[jijia]

https://breakpad.appspot.com/557002/diff/1006/src/processor/stackwalker_ppc64.cc
File src/processor/stackwalker_ppc64.cc (right):

https://breakpad.appspot.com/557002/diff/1006/src/processor/stackwalker_ppc64...
src/processor/stackwalker_ppc64.cc:53: }
On 2013/04/12 19:56:19, thestig wrote:
> extra brace -> code doesn't compile.

Done.

https://breakpad.appspot.com/557002/diff/1006/src/processor/stackwalker_ppc64...
src/processor/stackwalker_ppc64.cc:97: u_int64_t stack_pointer;
On 2013/04/12 19:56:19, thestig wrote:
> uint64_t, ditto for |instruction| on line 109.

Done.

[Lei Zhang (chromium)]

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

https://breakpad.appspot.com/557002/diff/1006/src/processor/minidump.cc#newco...
src/processor/minidump.cc:412: else if (expected_size ==
sizeof(MDRawContextPPC64)) {
Since MDRawContextPPC64 has its |context_flags| at the beginning of the struct,
unlike MDRawContextAMD64, you can probably put this code inside the else case,
rather than adding an else if case.

https://breakpad.appspot.com/557002/diff/1006/src/processor/minidump.cc#newco...
src/processor/minidump.cc:413: u_int64_t context_flags;
u_int* -> uint*

[Lei Zhang (chromium)]

+mark

[jijia]

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

https://breakpad.appspot.com/557002/diff/1006/src/processor/minidump.cc#newco...
src/processor/minidump.cc:412: else if (expected_size ==
sizeof(MDRawContextPPC64)) {
I put this outside the else case because the |context_flags| of
MDRawContextPPC64 is 64 bits, but in the else case |context_flags| is 32 bits.

On 2013/04/12 20:48:23, thestig wrote:
> Since MDRawContextPPC64 has its |context_flags| at the beginning of the
struct,
> unlike MDRawContextAMD64, you can probably put this code inside the else case,
> rather than adding an else if case.

[Mark Mentovai]

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

https://breakpad.appspot.com/557002/diff/12002/src/processor/minidump.cc#newc...
src/processor/minidump.cc:307: // at the beginning of the structure, so special
case it here.
This comment explains why the AMD64 context is handled here in this conditional
instead of in the switch statement below. I don’t believe it applies to PPC64 at
all. PPC64 should be handled in the switch statement.

https://breakpad.appspot.com/557002/diff/12002/src/processor/minidump.cc#newc...
src/processor/minidump.cc:517: switch (cpu_type) {
This is the switch statement I’m talking about.

https://breakpad.appspot.com/557002/diff/12002/src/processor/minidump.cc#newc...
src/processor/minidump.cc:972: if (system_info_cpu_type ==
MD_CPU_ARCHITECTURE_PPC)
Why is there no MD_CPU_ARCHITECTURE_PPC64?

https://breakpad.appspot.com/557002/diff/12002/src/processor/stackwalker_ppc6...
File src/processor/stackwalker_ppc64.cc (right):

https://breakpad.appspot.com/557002/diff/12002/src/processor/stackwalker_ppc6...
src/processor/stackwalker_ppc64.cc:1: // Copyright (c) 2013 Google Inc.
I assume this file was copied from stackwalker_ppc.cc with some struct names
changed and some 4s changed to 8s. Correct me if that’s wrong.

https://breakpad.appspot.com/557002/diff/12002/src/processor/stackwalker_ppc6...
src/processor/stackwalker_ppc64.cc:30: // stackwalker_ppc64.cc: ppc-specific
stackwalker.
ppc64-specific

https://breakpad.appspot.com/557002/diff/12002/src/processor/stackwalker_ppc64.h
File src/processor/stackwalker_ppc64.h (right):

https://breakpad.appspot.com/557002/diff/12002/src/processor/stackwalker_ppc6...
src/processor/stackwalker_ppc64.h:1: // Copyright (c) 2013 Google Inc.
Same question in this file, relative to stackwalker_ppc.h.

[jijia]

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

https://breakpad.appspot.com/557002/diff/1006/src/processor/minidump.cc#newco...
src/processor/minidump.cc:413: u_int64_t context_flags;
On 2013/04/12 20:48:23, thestig wrote:
> u_int* -> uint*

Done.

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

https://breakpad.appspot.com/557002/diff/12002/src/processor/minidump.cc#newc...
src/processor/minidump.cc:517: switch (cpu_type) {
As my previous comment, i put it outside of this else case because the
|context_flags| for ppc64 for 64 bits, but inside the else case, |context_flags|
are considered 32 bits. Do I still have to put it inside the switch statement?

On 2013/04/12 21:12:18, Mark Mentovai wrote:
> This is the switch statement I’m talking about.

https://breakpad.appspot.com/557002/diff/12002/src/processor/minidump.cc#newc...
src/processor/minidump.cc:972: if (system_info_cpu_type ==
MD_CPU_ARCHITECTURE_PPC)
OK. I'll add it.

On 2013/04/12 21:12:18, Mark Mentovai wrote:
> Why is there no MD_CPU_ARCHITECTURE_PPC64?

https://breakpad.appspot.com/557002/diff/12002/src/processor/stackwalker_ppc6...
File src/processor/stackwalker_ppc64.cc (right):

https://breakpad.appspot.com/557002/diff/12002/src/processor/stackwalker_ppc6...
src/processor/stackwalker_ppc64.cc:1: // Copyright (c) 2013 Google Inc.
Yes, it is. Also deleted the memory range check in StackwalkerPPC64.

On 2013/04/12 21:12:18, Mark Mentovai wrote:
> I assume this file was copied from stackwalker_ppc.cc with some struct names
> changed and some 4s changed to 8s. Correct me if that’s wrong.

https://breakpad.appspot.com/557002/diff/12002/src/processor/stackwalker_ppc6...
src/processor/stackwalker_ppc64.cc:30: // stackwalker_ppc64.cc: ppc-specific
stackwalker.
On 2013/04/12 21:12:18, Mark Mentovai wrote:
> ppc64-specific

Done.

https://breakpad.appspot.com/557002/diff/12002/src/processor/stackwalker_ppc64.h
File src/processor/stackwalker_ppc64.h (right):

https://breakpad.appspot.com/557002/diff/12002/src/processor/stackwalker_ppc6...
src/processor/stackwalker_ppc64.h:1: // Copyright (c) 2013 Google Inc.
Yes, it is.

On 2013/04/12 21:12:18, Mark Mentovai wrote:
> Same question in this file, relative to stackwalker_ppc.h.

https://breakpad.appspot.com/557002/diff/24001/src/google_breakpad/common/min...
File src/google_breakpad/common/minidump_format.h (right):

https://breakpad.appspot.com/557002/diff/24001/src/google_breakpad/common/min...
src/google_breakpad/common/minidump_format.h:607: MD_CPU_ARCHITECTURE_PPC64    
= 11,
Is there PROCESSOR_ARCHITECTURE_PPC64? And where can I find this number?

[Mark Mentovai]

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

https://breakpad.appspot.com/557002/diff/12002/src/processor/minidump.cc#newc...
src/processor/minidump.cc:517: switch (cpu_type) {
jijia wrote:
> As my previous comment, i put it outside of this else case because the
> |context_flags| for ppc64 for 64 bits, but inside the else case,
|context_flags|
> are considered 32 bits. Do I still have to put it inside the switch statement?

I see. This definitely needs a comment then.

[jijia]

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

https://breakpad.appspot.com/557002/diff/12002/src/processor/minidump.cc#newc...
src/processor/minidump.cc:517: switch (cpu_type) {
On 2013/04/12 21:49:58, Mark Mentovai wrote:
> jijia wrote:
> > As my previous comment, i put it outside of this else case because the
> > |context_flags| for ppc64 for 64 bits, but inside the else case,
> |context_flags|
> > are considered 32 bits. Do I still have to put it inside the switch
statement?
> 
> I see. This definitely needs a comment then.

Done.

[Mark Mentovai]

LG otherwise

https://breakpad.appspot.com/557002/diff/20002/src/google_breakpad/common/min...
File src/google_breakpad/common/minidump_format.h (right):

https://breakpad.appspot.com/557002/diff/20002/src/google_breakpad/common/min...
src/google_breakpad/common/minidump_format.h:607: MD_CPU_ARCHITECTURE_PPC64    
= 11,
Unless Microsoft has a definition for this, Breakpad-defined values should be in
the 0x8000 range, like SPARC below.

[Lei Zhang (chromium)]

LGTM with a couple of small nits. Please fix them and I'll land the patch for
you.

https://breakpad.appspot.com/557002/diff/27002/src/google_breakpad/common/min...
File src/google_breakpad/common/minidump_cpu_ppc64.h (right):

https://breakpad.appspot.com/557002/diff/27002/src/google_breakpad/common/min...
src/google_breakpad/common/minidump_cpu_ppc64.h:126:
MD_CONTEXT_PPC64_FLOATING_POINT | \
nit: indent 1 more space

https://breakpad.appspot.com/557002/diff/27002/src/google_breakpad/common/min...
File src/google_breakpad/common/minidump_format.h (right):

https://breakpad.appspot.com/557002/diff/27002/src/google_breakpad/common/min...
src/google_breakpad/common/minidump_format.h:607: MD_CPU_ARCHITECTURE_PPC64    
= 0x8000,
Can you comment it just like SPARC below?

[jijia]

https://breakpad.appspot.com/557002/diff/27002/src/google_breakpad/common/min...
File src/google_breakpad/common/minidump_cpu_ppc64.h (right):

https://breakpad.appspot.com/557002/diff/27002/src/google_breakpad/common/min...
src/google_breakpad/common/minidump_cpu_ppc64.h:126:
MD_CONTEXT_PPC64_FLOATING_POINT | \
On 2013/04/12 22:58:32, thestig wrote:
> nit: indent 1 more space

Done.

https://breakpad.appspot.com/557002/diff/27002/src/google_breakpad/common/min...
File src/google_breakpad/common/minidump_format.h (right):

https://breakpad.appspot.com/557002/diff/27002/src/google_breakpad/common/min...
src/google_breakpad/common/minidump_format.h:607: MD_CPU_ARCHITECTURE_PPC64    
= 0x8000,
On 2013/04/12 22:58:32, thestig wrote:
> Can you comment it just like SPARC below?

Done.

[Mark Mentovai]

I'm sure there's a reason we started at 0x8001 and not 0x8000. Please use
0x8002.
On Apr 12, 2013 7:02 PM, <jijia@google.com> wrote:

>
> https://breakpad.appspot.com/**557002/diff/27002/src/google_**
>
breakpad/common/minidump_cpu_**ppc64.h<https://breakpad.appspot.com/557002/diff/27002/src/google_breakpad/common/minidump_cpu_ppc64.h>
> File src/google_breakpad/common/**minidump_cpu_ppc64.h (right):
>
> https://breakpad.appspot.com/**557002/diff/27002/src/google_**
>
breakpad/common/minidump_cpu_**ppc64.h#newcode126<https://breakpad.appspot.com/557002/diff/27002/src/google_breakpad/common/minidump_cpu_ppc64.h#newcode126>
> src/google_breakpad/common/**minidump_cpu_ppc64.h:126:
> MD_CONTEXT_PPC64_FLOATING_**POINT | \
> On 2013/04/12 22:58:32, thestig wrote:
>
>> nit: indent 1 more space
>>
>
> Done.
>
> https://breakpad.appspot.com/**557002/diff/27002/src/google_**
>
breakpad/common/minidump_**format.h<https://breakpad.appspot.com/557002/diff/27002/src/google_breakpad/common/minidump_format.h>
> File src/google_breakpad/common/**minidump_format.h (right):
>
> https://breakpad.appspot.com/**557002/diff/27002/src/google_**
>
breakpad/common/minidump_**format.h#newcode607<https://breakpad.appspot.com/557002/diff/27002/src/google_breakpad/common/minidump_format.h#newcode607>
> src/google_breakpad/common/**minidump_format.h:607:
> MD_CPU_ARCHITECTURE_PPC64     = 0x8000,
> On 2013/04/12 22:58:32, thestig wrote:
>
>> Can you comment it just like SPARC below?
>>
>
> Done.
>
> https://breakpad.appspot.com/**557002/<https://breakpad.appspot.com/557002/>
>

[Lei Zhang (chromium)]

On 2013/04/13 01:52:46, Mark Mentovai wrote:
> I'm sure there's a reason we started at 0x8001 and not 0x8000. Please use
> 0x8002.

I was wondering about that, but since you didn't object earlier... anyway,
https://breakpad.appspot.com/559002