Allow minidump generator to generate ARM minidumps (for iOS on a device).

Allow minidump generator to generate ARM minidumps (for iOS on a device).

[Mark Mentovai]

http://breakpad.appspot.com/307002/diff/1/2
File src/client/mac/handler/dynamic_images.cc (right):

http://breakpad.appspot.com/307002/diff/1/2#newcode368
Line 368: static bool HasTaskInfo() {
This is a misnomer. task_info is a standard part of Mach that’s available on all
versions of Mac OS X. This should be called HasTaskDyldInfo, because only the
TASK_DYLD_INFO task_info flavor is new in 10.6.

http://breakpad.appspot.com/307002/diff/1/2#newcode369
Line 369: return true;
I’m not convinced that this is right. I’m sure there’s a minimum iOS version for
this too.

[Ted Mielczarek]

I figured the porting effort here wouldn't be huge. One thing I'm interested to
know, however. I've read that Apple has rejected apps from the iOS App Store for
using exc_server, since it's not a documented API. Clearly Breakpad can't
function without it, so do you think this is really feasible for apps to use?

http://breakpad.appspot.com/307002/diff/1/6
File src/client/mac/handler/minidump_generator.cc (right):

http://breakpad.appspot.com/307002/diff/1/6#newcode457
Line 457: #define AddReg(a) context_ptr->a =
REGISTER_FROM_THREADSTATE(machine_state, a)
You only use AddReg once. Doesn't seem worth the #define.

http://breakpad.appspot.com/307002/diff/1/6#newcode481
Line 481: context_ptr->cpsr = 0;
Why are you zeroing this out when you set it above?

http://breakpad.appspot.com/307002/diff/1/7
File src/client/mac/handler/minidump_generator.h (right):

http://breakpad.appspot.com/307002/diff/1/7#newcode50
Line 50: #if TARGET_OS_IPHONE && defined(__arm__)
I think #if defined(__arm__) is probably sufficient here, no? (And #elif
defined(__i386__) || defined(__x86_64__) for HAS_X86_SUPPORT.)

[Mark Mentovai]

This is really good. Some minor comments.

http://breakpad.appspot.com/307002/diff/1/4
File src/client/mac/handler/exception_handler.cc (right):

http://breakpad.appspot.com/307002/diff/1/4#newcode39
Line 39: #if TARGET_OS_IPHONE
This should be inside the “#ifndef USE_PROTECTED_ALLOCATIONS”

I’m don’t even think the code works when USE_PROTECTED_ALLOCATIONS is 0, but I
don’t want to tear all of that out right now.

http://breakpad.appspot.com/307002/diff/1/5
File src/client/mac/handler/exception_handler.h (right):

http://breakpad.appspot.com/307002/diff/1/5#newcode42
Line 42: #include <TargetConditionals.h>
C system headers before C++ system headers. This belongs in the block above with
<mach/mach.h>.

http://breakpad.appspot.com/307002/diff/1/6
File src/client/mac/handler/minidump_generator.cc (right):

http://breakpad.appspot.com/307002/diff/1/6#newcode481
Line 481: context_ptr->cpsr = 0;
You can’t recover this from state->cpsr?

http://breakpad.appspot.com/307002/diff/1/6#newcode482
Line 482: 
Wow, we never save any floating-point state for any CPU on the Mac.

Oh well.

[qsr]

Followed your review.

 My only problem is I cannot find mac test to run on that. I have a xcodeproject
called test, but it doesn't compile, complaining about missing cc files. Is
there some documentation on how to run tests?

 Thanks.

http://breakpad.appspot.com/307002/diff/1/2
File src/client/mac/handler/dynamic_images.cc (right):

http://breakpad.appspot.com/307002/diff/1/2#newcode368
Line 368: static bool HasTaskInfo() {
On 2011/10/06 14:37:53, Mark Mentovai wrote:
> This is a misnomer. task_info is a standard part of Mach that’s available on
all
> versions of Mac OS X. This should be called HasTaskDyldInfo, because only the
> TASK_DYLD_INFO task_info flavor is new in 10.6.

Done.

http://breakpad.appspot.com/307002/diff/1/2#newcode369
Line 369: return true;
> I’m not convinced that this is right. I’m sure there’s a minimum iOS version
for
> this too.

 The problem is that I do not know which it is, and I do not even know how to
look for which it is.

http://breakpad.appspot.com/307002/diff/1/4
File src/client/mac/handler/exception_handler.cc (right):

http://breakpad.appspot.com/307002/diff/1/4#newcode39
Line 39: #if TARGET_OS_IPHONE
On 2011/10/06 14:58:58, Mark Mentovai wrote:
> This should be inside the “#ifndef USE_PROTECTED_ALLOCATIONS”
> 
> I’m don’t even think the code works when USE_PROTECTED_ALLOCATIONS is 0, but I
> don’t want to tear all of that out right now.

Done.

http://breakpad.appspot.com/307002/diff/1/5
File src/client/mac/handler/exception_handler.h (right):

http://breakpad.appspot.com/307002/diff/1/5#newcode42
Line 42: #include <TargetConditionals.h>
On 2011/10/06 14:58:58, Mark Mentovai wrote:
> C system headers before C++ system headers. This belongs in the block above
with
> <mach/mach.h>.

Done.

http://breakpad.appspot.com/307002/diff/1/6
File src/client/mac/handler/minidump_generator.cc (right):

http://breakpad.appspot.com/307002/diff/1/6#newcode457
Line 457: #define AddReg(a) context_ptr->a =
REGISTER_FROM_THREADSTATE(machine_state, a)
On 2011/10/06 14:56:09, Ted Mielczarek wrote:
> You only use AddReg once. Doesn't seem worth the #define.

Done.

http://breakpad.appspot.com/307002/diff/1/6#newcode481
Line 481: context_ptr->cpsr = 0;
 Thanks. I let that slipped when I was trying to understand why my dumps were
wrong, and I blindly copied that from the linux implementation.

http://breakpad.appspot.com/307002/diff/1/6#newcode482
Line 482: 
 No we don't. I guessed that I won't try new things at this point :)

[Mark Mentovai]

minidump_generator_test, handler_test, and others are built from
src/client/mac/Breakpad.xcodeproj. I think we can just remove
src/client/mac/handler/minidump_test.xcodeproj. It’s outdated and
Breakpad.xcodeproj should build everything that minidump_test.xcodeproj used to
build.

[Mark Mentovai]

LGTM but send it out for another round if you need to change anything once you
get the tests running.

[qsr]

Added some #ifdef on src/client/mac/handler/minidump_generator.cc because
MacOSX14 doesn't know anything about arm.

[Mark Mentovai]

LGTM