Process minidumps generated on ARM64 in iOS apps.

Process minidumps generated on ARM64 in iOS apps.

[blundell]

Hi Mark,

This is the processor-side companion to https://breakpad.appspot.com/664002/,
and is dependent on that CL. Again, if you could review it that would be great
as Ben is OOO.

Two questions:
- Do you think it makes sense to merge stackwalker_arm64.* into
stackwalker_arm.*, or to leave them separate as I've done here?
- I would like to add unittests of the ARM64 stackwalker code. Apologies for the
basic question, but how can I build these unittests on OS X 10.8?

Thanks,

Colin

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

https://breakpad.appspot.com/704002/diff/80001/src/processor/minidump.cc#newc...
src/processor/minidump.cc:551: context_flags_ = context_flags;
It looks like this was a bug in the PPC64 handling; would you like it to be part
of this CL or in its own CL?

https://breakpad.appspot.com/704002/diff/80001/src/processor/minidump.cc#newc...
src/processor/minidump.cc:553: uint64_t context_flags;
This has to be special-cased just like the PPC64 handling due to the different
size of context_flags.

https://breakpad.appspot.com/704002/diff/80001/src/processor/minidump.cc#newc...
src/processor/minidump.cc:598:
Normalize128(&context_arm64->float_save.regs[fpr_index], true);
I don't know how to know what value to pass in for the endian-ness here; ARM64
is bi-endian.

https://breakpad.appspot.com/704002/diff/80001/src/processor/minidump.cc#newc...
src/processor/minidump.cc:1515: // TODO(blundell): print the 128-bit quantities
in
Is there anything better to do here?

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

https://breakpad.appspot.com/704002/diff/80001/src/processor/stackwalker.cc#n...
src/processor/stackwalker.cc:205: int arm_fp_register = -1;
This change was necessary because the compiler complained once I put in the
ARM64 case after the ARM case.

https://breakpad.appspot.com/704002/diff/80001/src/processor/stackwalker_arm6...
File src/processor/stackwalker_arm64.cc (right):

https://breakpad.appspot.com/704002/diff/80001/src/processor/stackwalker_arm6...
src/processor/stackwalker_arm64.cc:81: // Obtaining the stack frame from CFI
info is not yet supported for ARM64.
As we discussed over email, the .o files and fat binary files for ARM64 don't
include CFI info in the arm64 slices, so I would have no way to test any
implementation (my understanding is that the CFI information is unusable on ARM
for some reason anyway).

https://breakpad.appspot.com/704002/diff/80001/src/processor/stackwalker_arm6...
src/processor/stackwalker_arm64.cc:208: frame->instruction =
frame->context.iregs[MD_CONTEXT_ARM64_REG_PC] - 2;
I am not 100% sure on whether this value should be changed for ARM64; I just
took it from ARM, and things seem to work fine.

[Mark Mentovai]

blundell@chromium.org wrote:

> - Do you think it makes sense to merge stackwalker_arm64.* into
> stackwalker_arm.*, or to leave them separate as I've done here?
>

There are distinct stackwalker implementation files for x86 and amd64, and
for ppc and ppc64, and in both of those cases, the 32-bit and 64-bit
architectures are pretty similar. I’d follow their lead, unless there’s a
compelling reason not to. Enhancing sharing of non-trivial code would start
to be compelling.

- I would like to add unittests of the ARM64 stackwalker code. Apologies
> for the basic question, but how can I build these unittests on OS X
> 10.8?
>

The processor has an autoconf-based build, and that will produce the unit
tests for you. If you build the processor with configure && make, you
should get the unit tests along with all of the processor libraries and
tools. If you want to add your own unit tests, you’ll be editing
Makefile.am (at the very least) and re-generating Makefile.in (and maybe
other files) with “autoreconf -i” before running configure. You should add
your new file to Makefile.am anyway.

[Mark Mentovai]

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

https://breakpad.appspot.com/704002/diff/80001/src/processor/minidump.cc#newc...
src/processor/minidump.cc:551: context_flags_ = context_flags;
blundell wrote:
> It looks like this was a bug in the PPC64 handling; would you like it to be
part
> of this CL or in its own CL?

It’s good here. Nice catch, thanks.

https://breakpad.appspot.com/704002/diff/80001/src/processor/minidump.cc#newc...
src/processor/minidump.cc:553: uint64_t context_flags;
blundell wrote:
> This has to be special-cased just like the PPC64 handling due to the different
> size of context_flags.

OK, good.

The AMD64 and PPC64 cases both have comments explaining why they have a special
case to handle this. Your comment exists only on the code review. It belongs in
the file.

Be sure that the sizeof(MDRawContextARM64) (and, for that matter, the sizes of
the AMD64 and PPC64 raw context structs) don’t conflict with the sizes of any of
the other non-special-cased context structs that are supposed to be handled in
the big else with the switch block around cpu_type below.

https://breakpad.appspot.com/704002/diff/80001/src/processor/minidump.cc#newc...
src/processor/minidump.cc:561: uint32_t cpu_type = context_flags &
MD_CONTEXT_CPU_MASK;
Do you want to check cpu_type against the expected value, MD_CONTEXT_ARM64 here?
The AMD64 case above does that, and it also comes with this handy comment:

      // TODO: fall through to switch below?
      // need a Tell method to be able to SeekSet back to beginning
      // http://code.google.com/p/google-breakpad/issues/detail?id=224

The PPC64 code doesn’t, but maybe it should also.

If you used the less-used PPC64 branch as the template for your special case
here, you might want to consult the AMD64 branch as well, because it’s probably
better-tested.

https://breakpad.appspot.com/704002/diff/80001/src/processor/minidump.cc#newc...
src/processor/minidump.cc:598:
Normalize128(&context_arm64->float_save.regs[fpr_index], true);
blundell wrote:
> I don't know how to know what value to pass in for the endian-ness here; ARM64
> is bi-endian.

Tell me more. How bi-endian is it? Does the CPU switch on the fly? Will the
registers contain a mix of big- and little-endian data at the same time? Is this
a problem for the GPRs too?

Assuming that switching endianness is a heavyweight operation, you won’t find
mixes of endianness in the register file, and an OS will have decided on a
single endianness, it’s fine to hard-code this to the endianness that iOS uses
for now. In the future, if we encounter a system that uses ARM64 in a different
endianness, we can deal with it by sticking a hint somewhere in the minidump.

https://breakpad.appspot.com/704002/diff/100001/src/google_breakpad/processor...
File src/google_breakpad/processor/minidump.h (right):

https://breakpad.appspot.com/704002/diff/100001/src/google_breakpad/processor...
src/google_breakpad/processor/minidump.h:214: MDRawContextARM64*   arm64;
Spacing: all of the other union member field names line up here, this should
too.

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

https://breakpad.appspot.com/704002/diff/100001/src/google_breakpad/processor...
src/google_breakpad/processor/stack_frame_cpu.h:293: CONTEXT_VALID_X16  = 1 <<
15,
!!!

Really, 15 for all of the rest of these? If that’s in fact the case, maybe you
should just have a single entry for CONTEXT_VALID_X15_THRU_X32.

Perhaps, though, you were just sloppy with updating the right-hand side of all
of these when you copied and expanded the entries from StackFrameARM. In that
case, do the right thing with these constants.

https://breakpad.appspot.com/704002/diff/100001/src/google_breakpad/processor...
src/google_breakpad/processor/stack_frame_cpu.h:309: CONTEXT_VALID_X32  = 1 <<
15,
See next comment.

https://breakpad.appspot.com/704002/diff/100001/src/google_breakpad/processor...
src/google_breakpad/processor/stack_frame_cpu.h:339: int context_validity;
Careful! Watch out for the number of registers and therefore bits that get used
in the ContextValidity enum. 1 << 32 won’t fit in an int.

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

https://breakpad.appspot.com/704002/diff/100001/src/processor/minidump.cc#new...
src/processor/minidump.cc:1519: // byte ordering.
Which CPU? If it’s just the host CPU but not the crash dump CPU, there are
macros you can use to tell you the byte ordering.

You can also take the unswapped data (or reswap it since it’s already swapped)
and print it out as a series of uint8_ts with only knowledge of the ARM’s byte
ordering. (And I’m assuming that this can be knowable, but I’m waiting for your
answer to that question.)

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

https://breakpad.appspot.com/704002/diff/100001/src/processor/stackwalker.cc#...
src/processor/stackwalker.cc:205: int arm_fp_register = -1;
Instead of moving this here, you can put it back where it was and put the
MD_CONTEXT_ARM case into {braces}, and even rename the variable back to
fp_register.

https://breakpad.appspot.com/704002/diff/100001/src/processor/stackwalker_arm...
File src/processor/stackwalker_arm64.cc (right):

https://breakpad.appspot.com/704002/diff/100001/src/processor/stackwalker_arm...
src/processor/stackwalker_arm64.cc:199: // callee. Set new_frame->instruction to
one less than the PC. This won't
The comment says one less but the code says two less.

What are the types of encodings that can be used for a call on ARM64? If they’re
all of uniform length, this can be improved so that frame->instruction always
points to the beginning of the call. Is thumb still a factor?

https://breakpad.appspot.com/704002/diff/100001/src/processor/stackwalker_arm...
File src/processor/stackwalker_arm64.h (right):

https://breakpad.appspot.com/704002/diff/100001/src/processor/stackwalker_arm...
src/processor/stackwalker_arm64.h:94: int fp_register_;
Not needed (at least not yet?) and not used for ARM64.

[Ted Mielczarek]

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

https://breakpad.appspot.com/704002/diff/80001/src/processor/minidump.cc#newc...
src/processor/minidump.cc:561: uint32_t cpu_type = context_flags &
MD_CONTEXT_CPU_MASK;
On 2013/11/07 22:13:20, Mark Mentovai wrote:
> The AMD64 case above does that, and it also comes with this handy comment:

FYI I implemented that Tell method ages ago but never fixed that TODO. (Shame on
me!)

[blundell]

Thanks for the review! Comments addressed.

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

https://breakpad.appspot.com/704002/diff/80001/src/processor/minidump.cc#newc...
src/processor/minidump.cc:553: uint64_t context_flags;
Added the comment in the code.

I added in a pretty hideous-looking check against sizes conflicting. Do you
think it's worth it?


On 2013/11/07 22:13:20, Mark Mentovai wrote:
> blundell wrote:
> > This has to be special-cased just like the PPC64 handling due to the
different
> > size of context_flags.
> 
> OK, good.
> 
> The AMD64 and PPC64 cases both have comments explaining why they have a
special
> case to handle this. Your comment exists only on the code review. It belongs
in
> the file.
> 
> Be sure that the sizeof(MDRawContextARM64) (and, for that matter, the sizes of
> the AMD64 and PPC64 raw context structs) don’t conflict with the sizes of any
of
> the other non-special-cased context structs that are supposed to be handled in
> the big else with the switch block around cpu_type below.

https://breakpad.appspot.com/704002/diff/80001/src/processor/minidump.cc#newc...
src/processor/minidump.cc:561: uint32_t cpu_type = context_flags &
MD_CONTEXT_CPU_MASK;
Thanks. I vetted the ARM64 case against the AMD64 case. I'd like to leave the
fixing of the TODO about falling through to the else clause for a separate CL if
that's OK, as I think the restructurings that would be necessary to cleanly
support that flow would introduce significant noise into this CL. Filed
https://code.google.com/p/google-breakpad/issues/detail?id=550.

On 2013/11/08 00:00:32, Ted Mielczarek wrote:
> On 2013/11/07 22:13:20, Mark Mentovai wrote:
> > The AMD64 case above does that, and it also comes with this handy comment:
> 
> FYI I implemented that Tell method ages ago but never fixed that TODO. (Shame
on
> me!)

https://breakpad.appspot.com/704002/diff/80001/src/processor/minidump.cc#newc...
src/processor/minidump.cc:598:
Normalize128(&context_arm64->float_save.regs[fpr_index], true);
Your assumption is correct in that iOS uses ARM64 in little-endian mode.
Hardcoded this to little-endian.

On 2013/11/07 22:13:20, Mark Mentovai wrote:
> blundell wrote:
> > I don't know how to know what value to pass in for the endian-ness here;
ARM64
> > is bi-endian.
> 
> Tell me more. How bi-endian is it? Does the CPU switch on the fly? Will the
> registers contain a mix of big- and little-endian data at the same time? Is
this
> a problem for the GPRs too?
> 
> Assuming that switching endianness is a heavyweight operation, you won’t find
> mixes of endianness in the register file, and an OS will have decided on a
> single endianness, it’s fine to hard-code this to the endianness that iOS uses
> for now. In the future, if we encounter a system that uses ARM64 in a
different
> endianness, we can deal with it by sticking a hint somewhere in the minidump.

https://breakpad.appspot.com/704002/diff/100001/src/google_breakpad/processor...
File src/google_breakpad/processor/minidump.h (right):

https://breakpad.appspot.com/704002/diff/100001/src/google_breakpad/processor...
src/google_breakpad/processor/minidump.h:214: MDRawContextARM64*   arm64;
On 2013/11/07 22:13:20, Mark Mentovai wrote:
> Spacing: all of the other union member field names line up here, this should
> too.

Done.

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

https://breakpad.appspot.com/704002/diff/100001/src/google_breakpad/processor...
src/google_breakpad/processor/stack_frame_cpu.h:293: CONTEXT_VALID_X16  = 1 <<
15,
Whoops! Great catch, thanks!

On 2013/11/07 22:13:20, Mark Mentovai wrote:
> !!!
> 
> Really, 15 for all of the rest of these? If that’s in fact the case, maybe you
> should just have a single entry for CONTEXT_VALID_X15_THRU_X32.
> 
> Perhaps, though, you were just sloppy with updating the right-hand side of all
> of these when you copied and expanded the entries from StackFrameARM. In that
> case, do the right thing with these constants.

https://breakpad.appspot.com/704002/diff/100001/src/google_breakpad/processor...
src/google_breakpad/processor/stack_frame_cpu.h:309: CONTEXT_VALID_X32  = 1 <<
15,
On 2013/11/07 22:13:20, Mark Mentovai wrote:
> See next comment.

Done.

https://breakpad.appspot.com/704002/diff/100001/src/google_breakpad/processor...
src/google_breakpad/processor/stack_frame_cpu.h:339: int context_validity;
Thanks again! Done.

On 2013/11/07 22:13:20, Mark Mentovai wrote:
> Careful! Watch out for the number of registers and therefore bits that get
used
> in the ContextValidity enum. 1 << 32 won’t fit in an int.

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

https://breakpad.appspot.com/704002/diff/100001/src/processor/minidump.cc#new...
src/processor/minidump.cc:1519: // byte ordering.
Full disclosure: I copied this comment from similar ones in the PPC64 printout
above. As I mentioned elsewhere, ARM64 for iOS is little-endian. I put in a
placeholder implementation here, but I'm not sure how the endianness of the host
CPU may need to be accounted for.

On 2013/11/07 22:13:20, Mark Mentovai wrote:
> Which CPU? If it’s just the host CPU but not the crash dump CPU, there are
> macros you can use to tell you the byte ordering.
> 
> You can also take the unswapped data (or reswap it since it’s already swapped)
> and print it out as a series of uint8_ts with only knowledge of the ARM’s byte
> ordering. (And I’m assuming that this can be knowable, but I’m waiting for
your
> answer to that question.)

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

https://breakpad.appspot.com/704002/diff/100001/src/processor/stackwalker.cc#...
src/processor/stackwalker.cc:205: int arm_fp_register = -1;
On 2013/11/07 22:13:20, Mark Mentovai wrote:
> Instead of moving this here, you can put it back where it was and put the
> MD_CONTEXT_ARM case into {braces}, and even rename the variable back to
> fp_register.

Done.

https://breakpad.appspot.com/704002/diff/100001/src/processor/stackwalker_arm...
File src/processor/stackwalker_arm64.cc (right):

https://breakpad.appspot.com/704002/diff/100001/src/processor/stackwalker_arm...
src/processor/stackwalker_arm64.cc:199: // callee. Set new_frame->instruction to
one less than the PC. This won't
According to the web, thumb is not a factor on ARM64, which has a uniform 4-byte
encoding. Adjusted.

On 2013/11/07 22:13:20, Mark Mentovai wrote:
> The comment says one less but the code says two less.
> 
> What are the types of encodings that can be used for a call on ARM64? If
they’re
> all of uniform length, this can be improved so that frame->instruction always
> points to the beginning of the call. Is thumb still a factor?

https://breakpad.appspot.com/704002/diff/100001/src/processor/stackwalker_arm...
File src/processor/stackwalker_arm64.h (right):

https://breakpad.appspot.com/704002/diff/100001/src/processor/stackwalker_arm...
src/processor/stackwalker_arm64.h:94: int fp_register_;
Oops, meant to kill this. Thanks, removed.

On 2013/11/07 22:13:20, Mark Mentovai wrote:
> Not needed (at least not yet?) and not used for ARM64.

[blundell]

Note: Please ignore patch sets 4 and 5, they're broken uploads that I can't
remove.

I added my new files to Makefile.am. I unfortunately still can't figure out the
unit tests: doing ./configure && make builds minidump_stackwalk but doesn't seem
to build the unit tests (note: in case you're wondering, I've been building with
ninja via Chromium's gypfile).

[blundell]

...And I was able to remove the broken patchsets immediately after sending the
last message. Apologies for the noise.

On 2013/11/19 17:32:01, blundell wrote:
> Note: Please ignore patch sets 4 and 5, they're broken uploads that I can't
> remove.
> 
> I added my new files to Makefile.am. I unfortunately still can't figure out
the
> unit tests: doing ./configure && make builds minidump_stackwalk but doesn't
seem
> to build the unit tests (note: in case you're wondering, I've been building
with
> ninja via Chromium's gypfile).

[Mark Mentovai]

Getting very close here too.

https://breakpad.appspot.com/704002/diff/50030/Makefile.am
File Makefile.am (right):

https://breakpad.appspot.com/704002/diff/50030/Makefile.am#newcode645
Makefile.am:645: src/processor/stackwalker_arm64.o \
Makefiles use tabs here, and you’ve used spaces.

Lines 738, 867, and 1024 too.

https://breakpad.appspot.com/704002/diff/50030/Makefile.in
File Makefile.in (right):

https://breakpad.appspot.com/704002/diff/50030/Makefile.in#newcode1
Makefile.in:1: # Makefile.in generated by automake 1.11.1 from Makefile.am.
Well let’s not go backwards. If we started with 1.11.3, you shouldn’t be
dropping to 1.11.1.

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

https://breakpad.appspot.com/704002/diff/50030/src/google_breakpad/processor/...
src/google_breakpad/processor/stack_frame_cpu.h:309: CONTEXT_VALID_X32  = 1 <<
32,
Careful! What’s the value of this? Compare it to CONTEXT_VALID_X31.

(GCC has a warning for this. Sadly, clang doesn’t.)

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

https://breakpad.appspot.com/704002/diff/50030/src/processor/minidump.cc#newc...
src/processor/minidump.cc:81: uint32_t num_matching_contexts = 0;
int is OK for this guy.

https://breakpad.appspot.com/704002/diff/50030/src/processor/minidump.cc#newc...
src/processor/minidump.cc:98: return (num_matching_contexts == 1);
Outer (parentheses) unnecessary.

https://breakpad.appspot.com/704002/diff/50030/src/processor/minidump_stackwa...
File src/processor/minidump_stackwalk.cc (right):

https://breakpad.appspot.com/704002/diff/50030/src/processor/minidump_stackwa...
src/processor/minidump_stackwalk.cc:404: PrintRegister64("sp",
frame_arm64->context.iregs[31], sequence);
x27 x28 fp sp lr pc

I would expect this to be the same as x27 x28 x29 x30 x31 x32, but it’s not. I
see that you matched the order that they’re printed for 32-bit ARM, but that
seems kind of arbitrary.

[blundell]

Thanks! Will respond to your comments. In the meantime, I got the processor
unittests working and ported most of the StackwalkerARM unittests to
StackwalkerARM64; that's what this patchset adds. The exceptions are the
CFI-related unittests; CFI support will have to wait for someone to do the Linux
ARM64 work.

The unittest shows up as a new file. Frustratingly, if I dial down the
similarity, it shows up as a copy from a different 64-bit platform's unittest
rather than from the ARM unittest. The fact that you can't tell git that file B
is a copy of file A is one of my biggest annoyances with it.

[Mark Mentovai]

blundell@chromium.org wrote:

> The unittest shows up as a new file. Frustratingly, if I dial down the
> similarity, it shows up as a copy from a different 64-bit platform's
> unittest rather than from the ARM unittest. The fact that you can't tell
> git that file B is a copy of file A is one of my biggest annoyances with
> it.


Mine too.

[blundell]

Added a comment to make an attempt at giving you context you would have had by
looking back and forth between the old and new files.

https://breakpad.appspot.com/704002/diff/240001/src/processor/stackwalker_arm...
File src/processor/stackwalker_arm64_unittest.cc (right):

https://breakpad.appspot.com/704002/diff/240001/src/processor/stackwalker_arm...
src/processor/stackwalker_arm64_unittest.cc:151: StackwalkerARM64
walker(&system_info, &raw_context, &stack_region, &modules,
Essentially the only changes that I made in these tests were to s/ARM/ARM64/g
and to s/32/64 in a lot of places (most notably, in the stacks being manually
constructed). I did not have to change any non-trivial logic; everything Just
Worked.

[blundell]

Thanks!

https://breakpad.appspot.com/704002/diff/50030/Makefile.am
File Makefile.am (right):

https://breakpad.appspot.com/704002/diff/50030/Makefile.am#newcode645
Makefile.am:645: src/processor/stackwalker_arm64.o \
Oof, it's been a while since I've done makefile editing :).

On 2013/11/20 15:10:08, Mark Mentovai wrote:
> Makefiles use tabs here, and you’ve used spaces.
> 
> Lines 738, 867, and 1024 too.

https://breakpad.appspot.com/704002/diff/50030/Makefile.in
File Makefile.in (right):

https://breakpad.appspot.com/704002/diff/50030/Makefile.in#newcode1
Makefile.in:1: # Makefile.in generated by automake 1.11.1 from Makefile.am.
On 2013/11/20 15:10:08, Mark Mentovai wrote:
> Well let’s not go backwards. If we started with 1.11.3, you shouldn’t be
> dropping to 1.11.1.

Done. Is moving the version forward OK? This is what macports updated me to.

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

https://breakpad.appspot.com/704002/diff/50030/src/google_breakpad/processor/...
src/google_breakpad/processor/stack_frame_cpu.h:309: CONTEXT_VALID_X32  = 1 <<
32,
D'oh! I decided to compress X0-X28 into one bit, since they're valid as a
package on iOS. I left a comment so that if anyone needs to specify them all
individually in the future (e.g., for Linux), they'll be aware that they'll have
to deal with this issue.

Let me know if there's another solution that you think would be better.

On 2013/11/20 15:10:08, Mark Mentovai wrote:
> Careful! What’s the value of this? Compare it to CONTEXT_VALID_X31.
> 
> (GCC has a warning for this. Sadly, clang doesn’t.)

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

https://breakpad.appspot.com/704002/diff/50030/src/processor/minidump.cc#newc...
src/processor/minidump.cc:81: uint32_t num_matching_contexts = 0;
On 2013/11/20 15:10:08, Mark Mentovai wrote:
> int is OK for this guy.

Done.

https://breakpad.appspot.com/704002/diff/50030/src/processor/minidump.cc#newc...
src/processor/minidump.cc:98: return (num_matching_contexts == 1);
On 2013/11/20 15:10:08, Mark Mentovai wrote:
> Outer (parentheses) unnecessary.

Done.

https://breakpad.appspot.com/704002/diff/50030/src/processor/minidump_stackwa...
File src/processor/minidump_stackwalk.cc (right):

https://breakpad.appspot.com/704002/diff/50030/src/processor/minidump_stackwa...
src/processor/minidump_stackwalk.cc:404: PrintRegister64("sp",
frame_arm64->context.iregs[31], sequence);
On 2013/11/20 15:10:08, Mark Mentovai wrote:
> x27 x28 fp sp lr pc
> 
> I would expect this to be the same as x27 x28 x29 x30 x31 x32, but it’s not. I
> see that you matched the order that they’re printed for 32-bit ARM, but that
> seems kind of arbitrary.

Done.

[Mark Mentovai]

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

https://breakpad.appspot.com/704002/diff/50030/src/google_breakpad/processor/...
src/google_breakpad/processor/stack_frame_cpu.h:309: CONTEXT_VALID_X32  = 1 <<
32,
blundell wrote:
> D'oh! I decided to compress X0-X28 into one bit, since they're valid as a
> package on iOS. I left a comment so that if anyone needs to specify them all
> individually in the future (e.g., for Linux), they'll be aware that they'll
have
> to deal with this issue.
> 
> Let me know if there's another solution that you think would be better.

Instead of an enum, you could use a series of const uint64_ts.

https://breakpad.appspot.com/704002/diff/330001/Makefile.am
File Makefile.am (right):

https://breakpad.appspot.com/704002/diff/330001/Makefile.am#newcode1
Makefile.am:1: ## Process this file with automake to produce Makefile.in
You didn’t re-run automake to get a new Makefile.in as you did in previous patch
sets.

https://breakpad.appspot.com/704002/diff/330001/aclocal.m4
File aclocal.m4 (right):

https://breakpad.appspot.com/704002/diff/330001/aclocal.m4#newcode1
aclocal.m4:1: # generated automatically by aclocal 1.14 -*- Autoconf -*-
This is fine.

[blundell]

Thanks! Changed the flags in StackFrameARM64 to be static const uint64_t
members. Don't know what to do about Makefile.in :(.

https://breakpad.appspot.com/704002/diff/330001/Makefile.am
File Makefile.am (right):

https://breakpad.appspot.com/704002/diff/330001/Makefile.am#newcode1
Makefile.am:1: ## Process this file with automake to produce Makefile.in
Aargh, I did and git cl upload didn't upload Makefile.in because the diff was
too large. Going in and adjusting the upload limit didn't help as I then got
what seemed like a server-side error that the uploaded file must be < 1MB.

Have you ever run across this before?


On 2013/11/22 14:57:31, Mark Mentovai wrote:
> You didn’t re-run automake to get a new Makefile.in as you did in previous
patch
> sets.

[Mark Mentovai]

No, I don’t recall that.

Since it’s just a generated file, I’ll run automake myself when I check it in.

LGTM.

[Mark Mentovai]

Committed r1236