Issue 664002: Generate minidumps for 64-bit ARM apps on iOS. (Closed)
|
Created:
11 years, 1 month ago by blundell Modified:
11 years ago CC:
google-breakpad-dev_googlegroups.com Base URL:
http://google-breakpad.googlecode.com/svn/trunk Visibility:
Public. |
DescriptionGenerate minidumps for 64-bit ARM apps on iOS. Adds an ARM64-specific definition of MDRawContext and support for writing out a minidump when running on ARM64. Additionally, extends the iOS minidump generator for NSExceptions to work on ARM64 as well as ARM. Patch Set 1 #Patch Set 2 : Remove debugging output #Patch Set 3 : Nit #
Total comments: 14
Patch Set 4 : Fixes #
Total comments: 63
Patch Set 5 : Response to review #
Total comments: 14
Patch Set 6 : Respond to review #
Total comments: 2
Patch Set 7 : Response to review #MessagesTotal messages: 11
Hi Mark and Ben, With the changes in this CL, I am now generating minidumpson ARM 64 for iOS. I'd like you to comment on these changes while I concurrently start working on the stackwalker. The basis for minidump_cpu_arm64.h is _STRUCT_ARM_THREAD_STATE64 in /Applications/Xcode.app/Contents/Developer/Platforms/iPhoneOS.platform/Developer/SDKs/iPhoneOS7.0.sdk/usr/include/mach/arm/_structs.h; arm_thread_state64_t is typedef'd to the former. https://breakpad.appspot.com/664002/diff/30001/src/client/mac/handler/excepti... File src/client/mac/handler/exception_handler.cc (right): https://breakpad.appspot.com/664002/diff/30001/src/client/mac/handler/excepti... src/client/mac/handler/exception_handler.cc:459: fprintf(stderr, "** Exception default\n"); You can ignore these changes for the purposes of this discussion...I had found that when Breakpad wasn't generating a minidump (i.e. when false was passed to HandleBreakpadCallback in Breakpad.mm, the app wasn't dying as it should be. This problem went away once I got Breakpad to generate the minidump. I will investigate this at a later point. https://breakpad.appspot.com/664002/diff/30001/src/google_breakpad/common/min... File src/google_breakpad/common/minidump_cpu_arm64.h (right): https://breakpad.appspot.com/664002/diff/30001/src/google_breakpad/common/min... src/google_breakpad/common/minidump_cpu_arm64.h:140: #define MD_CONTEXT_ARM64_OLD 0x00000040 I don't know what to use for this value or the value of MD_CONTEXT_ARM64 below.
Sign in to reply to this message.
https://breakpad.appspot.com/664002/diff/30001/src/client/mac/handler/minidum... File src/client/mac/handler/minidump_generator.cc (right): https://breakpad.appspot.com/664002/diff/30001/src/client/mac/handler/minidum... src/client/mac/handler/minidump_generator.cc:569: #undef AddReg AddReg was never defined, you can remove the undef (and the same on the previous method, as this was a mistake) https://breakpad.appspot.com/664002/diff/30001/src/client/mac/handler/minidum... src/client/mac/handler/minidump_generator.cc:895: case ((cpu_type_t) CPU_TYPE_ARM64): Is the cast mandatory? https://breakpad.appspot.com/664002/diff/30001/src/client/mac/handler/minidum... src/client/mac/handler/minidump_generator.cc:1146: // TODO(blundell): Does this need to be separated out for ARM64? This is used on the processor side to indicate the type of the CPU. It is already missing some information in the arm case, generating cpu type: ARM armv0 instead of getting the right information. You probably want to give more information there because if the difference between 6, 7, 7s etc is maybe not that mandatory, the difference between 32 and 64 bits might well be. https://breakpad.appspot.com/664002/diff/30001/src/google_breakpad/common/min... File src/google_breakpad/common/minidump_cpu_arm64.h (right): https://breakpad.appspot.com/664002/diff/30001/src/google_breakpad/common/min... src/google_breakpad/common/minidump_cpu_arm64.h:70: #define MD_FLOATINGSAVEAREA_ARM64_FPEXTRA_COUNT 8 Did you check those values? https://breakpad.appspot.com/664002/diff/30001/src/google_breakpad/common/min... src/google_breakpad/common/minidump_cpu_arm64.h:137: * Therefore, Breakpad defines its own value for ARM CPUs. The comment is completely wrong. Windows CE never handled 64 bits CPU. https://breakpad.appspot.com/664002/diff/30001/src/google_breakpad/common/min... src/google_breakpad/common/minidump_cpu_arm64.h:140: #define MD_CONTEXT_ARM64_OLD 0x00000040 On 2013/10/25 16:07:02, blundell wrote: > I don't know what to use for this value or the value of MD_CONTEXT_ARM64 below. You either need to find documentation, or you need to examine the real context at a point where you know what it should contain. You then look at its type, and find out what the bits mean.
Sign in to reply to this message.
Hi Mark, Ben is OOO for the next couple weeks; could you take over this review? I have a companion processor-side CL that I am cleaning up for review and will send out to you shortly. I can combine and/or subdivide these CLs if you would find it useful to do so. I have successfully generated and processed (via minidump_stackwalk) crashes on an iPhone 5s for the following: - Null pointer dereference - kill(getpid(), SIGABRT) - raised NSException Are there other types of crashes that it would be useful to test? One that I can think of is a raised C++ exception when not in a default iOS runloop; I will do that testing. In addition to the questions that I posed for you in the comments, I was wondering how to run the client unittests: - Can I run them on ARM/ARM64? - I wasn't able to build any of the targets in client/mac/Breakpad.xcodeproj on OS X 10.8. Are there any secrets I'm missing? https://breakpad.appspot.com/664002/diff/30001/src/client/mac/handler/minidum... File src/client/mac/handler/minidump_generator.cc (right): https://breakpad.appspot.com/664002/diff/30001/src/client/mac/handler/minidum... src/client/mac/handler/minidump_generator.cc:569: #undef AddReg On 2013/10/28 10:25:38, qsr wrote: > AddReg was never defined, you can remove the undef (and the same on the previous > method, as this was a mistake) Done. https://breakpad.appspot.com/664002/diff/30001/src/client/mac/handler/minidum... src/client/mac/handler/minidump_generator.cc:895: case ((cpu_type_t) CPU_TYPE_ARM64): On 2013/10/28 10:25:38, qsr wrote: > Is the cast mandatory? Done. https://breakpad.appspot.com/664002/diff/30001/src/client/mac/handler/minidum... src/client/mac/handler/minidump_generator.cc:1146: // TODO(blundell): Does this need to be separated out for ARM64? On 2013/10/28 10:25:38, qsr wrote: > This is used on the processor side to indicate the type of the CPU. It is > already missing some information in the arm case, generating cpu type: ARM armv0 > instead of getting the right information. > > You probably want to give more information there because if the difference > between 6, 7, 7s etc is maybe not that mandatory, the difference between 32 and > 64 bits might well be. Done. https://breakpad.appspot.com/664002/diff/30001/src/google_breakpad/common/min... File src/google_breakpad/common/minidump_cpu_arm64.h (right): https://breakpad.appspot.com/664002/diff/30001/src/google_breakpad/common/min... src/google_breakpad/common/minidump_cpu_arm64.h:70: #define MD_FLOATINGSAVEAREA_ARM64_FPEXTRA_COUNT 8 On 2013/10/28 10:25:38, qsr wrote: > Did you check those values? Done. https://breakpad.appspot.com/664002/diff/30001/src/google_breakpad/common/min... src/google_breakpad/common/minidump_cpu_arm64.h:137: * Therefore, Breakpad defines its own value for ARM CPUs. On 2013/10/28 10:25:38, qsr wrote: > The comment is completely wrong. Windows CE never handled 64 bits CPU. Done. https://breakpad.appspot.com/664002/diff/30001/src/google_breakpad/common/min... src/google_breakpad/common/minidump_cpu_arm64.h:140: #define MD_CONTEXT_ARM64_OLD 0x00000040 I believe that that MD_CONTEXT_ARM64 is Breakpad-defined. Updated to reflect that. On 2013/10/28 10:25:38, qsr wrote: > On 2013/10/25 16:07:02, blundell wrote: > > I don't know what to use for this value or the value of MD_CONTEXT_ARM64 > below. > > You either need to find documentation, or you need to examine the real context > at a point where you know what it should contain. You then look at its type, and > find out what the bits mean. https://breakpad.appspot.com/664002/diff/250001/src/client/ios/handler/ios_ex... File src/client/ios/handler/ios_exception_minidump_generator.h (right): https://breakpad.appspot.com/664002/diff/250001/src/client/ios/handler/ios_ex... src/client/ios/handler/ios_exception_minidump_generator.h:54: unsigned long GetPCFromException(); The values in question in the iOS exception minidump generator are pointer values, i.e., 4 bytes on ARM and 8 bytes on ARM64. A long is guaranteed to be 8 bytes on ARM64 on iOS (this is specified in the iOS ARMv8 documentation). What do you think of this solution? https://breakpad.appspot.com/664002/diff/250001/src/client/mac/handler/except... File src/client/mac/handler/exception_handler.cc (right): https://breakpad.appspot.com/664002/diff/250001/src/client/mac/handler/except... src/client/mac/handler/exception_handler.cc:456: switch (target_behavior & ~MACH_EXCEPTION_CODES) { This is a high-order bit that is set on arm 64 to indicate that 64-bit code and subcode are sent in the header. https://breakpad.appspot.com/664002/diff/250001/src/client/mac/handler/uconte... File src/client/mac/handler/ucontext_compat.h (right): https://breakpad.appspot.com/664002/diff/250001/src/client/mac/handler/uconte... src/client/mac/handler/ucontext_compat.h:35: #if defined(__arm64__) This file is needed because the signal handler receives a ucontext64_t instead of a ucontext_t on ARM64. I was wondering whether this should also encompass PPC64 -- I imagine that it should. https://breakpad.appspot.com/664002/diff/250001/src/google_breakpad/common/mi... File src/google_breakpad/common/minidump_cpu_arm64.h (right): https://breakpad.appspot.com/664002/diff/250001/src/google_breakpad/common/mi... src/google_breakpad/common/minidump_cpu_arm64.h:127: * context stored in the structure. MD_CONTEXT_ARM64 is Breakpad-defined. I couldn't find any definition of MD_CONTEXT_ARM64 on the web; I assume that it doesn't have one but don't know how to be sure of that. https://breakpad.appspot.com/664002/diff/250001/src/google_breakpad/common/mi... File src/google_breakpad/common/minidump_format.h (right): https://breakpad.appspot.com/664002/diff/250001/src/google_breakpad/common/mi... src/google_breakpad/common/minidump_format.h:640: MD_CPU_ARCHITECTURE_ARM64 = 0x8003, /* Breakpad-defined value for ARM64 */ Same question re: not being 100% sure that Breakpad is free to define this value itself.
Sign in to reply to this message.
blundell@chromium.org wrote: > Ben is OOO for the next couple weeks; could you take over this review? > I’ll do my best. My review queue isn’t very deep but everything in it is pretty dense. I’ll try to take a look at this in the afternoon. I have a companion processor-side CL that I am cleaning up for review > and will send out to you shortly. I can combine and/or subdivide these > CLs if you would find it useful to do so. I have successfully generated > and processed (via minidump_stackwalk) crashes on an iPhone 5s for the > following: > - Null pointer dereference > - kill(getpid(), SIGABRT) > - raised NSException > Ideally you’ll make sure this works for all of the possible machine crash types. Here’s a good start: null pointer dereference dereference something from an unmapped page (if you’re not threaded, mach_vm_allocate and mach_vm_deallocate a page for testing) dereference something from a mapped but inaccessible page (use mprotect or mach_vm_protect) execute an illegal instruction execute on an unmapped page execute on an unexecutable page integer division by zero do something with unaligned instructions or data (only on architectures where alignment checks are enforced) In addition to sending your own SIGABRT, make sure abort works too, in case the standard library version of abort (that most code will be inclined to use) implements things differently. Are there other types of crashes that it would be useful to test? One > that I can think of is a raised C++ exception when not in a default iOS > runloop; I will do that testing. > > In addition to the questions that I posed for you in the comments, I was > wondering how to run the client unittests: > - Can I run them on ARM/ARM64? > I’m not sure about that at all. - I wasn't able to build any of the targets in > client/mac/Breakpad.xcodeproj on OS X 10.8. Are there any secrets I'm > missing? > Sadly, the only secret is that the xcodeproj is not kept up to date with code changes and is routinely broken. There are probably also incompatibilities with newer SDKs, since recent Xcodes don’t include older SDKs anymore. If I have a chance, I’ll take a look at this later too.
Sign in to reply to this message.
https://breakpad.appspot.com/664002/diff/250001/src/client/ios/Breakpad.xcode... File src/client/ios/Breakpad.xcodeproj/project.pbxproj (right): https://breakpad.appspot.com/664002/diff/250001/src/client/ios/Breakpad.xcode... src/client/ios/Breakpad.xcodeproj/project.pbxproj:1: // !$*UTF8*$! I guess this file lists nearby headers. Wanna add your new ucontext_compat.h here? https://breakpad.appspot.com/664002/diff/250001/src/client/ios/handler/ios_ex... File src/client/ios/handler/ios_exception_minidump_generator.h (right): https://breakpad.appspot.com/664002/diff/250001/src/client/ios/handler/ios_ex... src/client/ios/handler/ios_exception_minidump_generator.h:54: unsigned long GetPCFromException(); blundell wrote: > The values in question in the iOS exception minidump generator are pointer > values, i.e., 4 bytes on ARM and 8 bytes on ARM64. A long is guaranteed to be 8 > bytes on ARM64 on iOS (this is specified in the iOS ARMv8 documentation). What > do you think of this solution? iOS dump generation is always in-process, right? So you don’t have to worry about a 64-bit crash victim being examined by a 32-bit dump writer or vice-versa? A program counter register contains a pointer, so I think you want uintptr_t. Same on line 57. This will also influence the type choice in the .mm file and perhaps elsewhere. https://breakpad.appspot.com/664002/diff/250001/src/client/ios/handler/ios_ex... src/client/ios/handler/ios_exception_minidump_generator.h:62: bool WriteCrashingContextARM64(MDLocationDescriptor *register_location); Then I see this, where you’re building a function for both ARM and ARM64 into the same object, and I think that maybe iOS dump generation isn’t always in-process? If that’s the case, the uintptr_t and unsigned long would both be wrong, because how would you express a full 64 bits of register content from a crash victim process in a 32-bit dump writer? https://breakpad.appspot.com/664002/diff/250001/src/client/ios/handler/ios_ex... File src/client/ios/handler/ios_exception_minidump_generator.mm (right): https://breakpad.appspot.com/664002/diff/250001/src/client/ios/handler/ios_ex... src/client/ios/handler/ios_exception_minidump_generator.mm:40: const unsigned long kExpectedFinalFp = sizeof(long); Technically the types should match, so whatever you use here is what you take the sizeof of. Since you’re using unsigned long now, it should be unsigned long on the right side too. If it changes to some other type, the right side should also obviously change. https://breakpad.appspot.com/664002/diff/250001/src/client/ios/handler/ios_ex... src/client/ios/handler/ios_exception_minidump_generator.mm:75: return WriteCrashingContextARM64(register_location); OK, so now that I see this, it looks like we’re back to only doing 64-bit dumps in a 64-bit process, and 32-bit dumps in a 32-bit process. To make that clearer… #ifdef __arm64__ around the declarations and the implementations too. There’s no reason to build code that’s just going to be dead (even if you want to dead-strip it away). Another option is to just have the #ifdef live in this function which wraps the two inline implementations. That’d be fine, but it’s also fine to leave them in separate functions, as long as you #ifdef everything properly. Your call. If the two separate functions are very different, it would support keeping them distinct. If the two functions are almost entirely the same, except maybe for a few constants and types here and there, a better answer would be to have a single implementation, sticking just the constants and typedefs into an #ifdef, and letting both bitnesses share the guts of the code. Which applies here? https://breakpad.appspot.com/664002/diff/250001/src/client/ios/handler/ios_ex... src/client/ios/handler/ios_exception_minidump_generator.mm:80: assert(false); Seems like you really just want this to be #error "This file requires ARM" compile-time error, not run-time assertion failure. I see this pattern show up elsewhere in the file, you can clean those up too or just get rid of ’em as long as it’s checked in at least one spot. Unless there’s a risk of that one spot disappearing and the check getting lost. https://breakpad.appspot.com/664002/diff/250001/src/client/ios/handler/ios_ex... src/client/ios/handler/ios_exception_minidump_generator.mm:116: context_ptr->iregs[29] = kExpectedFinalFp; // FP Maybe instead of these magic numbers, you can use the values from minidump_cpu_arm64.h? And minidump_cpu_arm.h for the ARM non-ARM64 ones? That doesn’t prevent you from needing an #ifdef to pick the right names, but it’s less “magical” to use constants with names, assuming they’re correct. https://breakpad.appspot.com/664002/diff/250001/src/client/ios/handler/ios_ex... src/client/ios/handler/ios_exception_minidump_generator.mm:171: size_t data_size = sizeof(long); I don’t like the name “data_size.” Is it just word_size or native_word_size or something along those lines? “data” sounds like something generic that could be larger (or even smaller) than a native word. https://breakpad.appspot.com/664002/diff/250001/src/client/ios/handler/ios_ex... src/client/ios/handler/ios_exception_minidump_generator.mm:176: scoped_array<uint8_t> stack_memory(new uint8_t[stack_size]); It kinda seems to me like you want an array of structs here that you later treat as bytes. https://breakpad.appspot.com/664002/diff/250001/src/client/mac/handler/except... File src/client/mac/handler/exception_handler.cc (right): https://breakpad.appspot.com/664002/diff/250001/src/client/mac/handler/except... src/client/mac/handler/exception_handler.cc:345: bool ExceptionHandler::WriteMinidumpWithException(int exception_type, If you’re gonna put the arguments on a continuation line like this so they’re all lined up, move exception_type to the next line so it lines up with them too. I was looking for what callers were passing to this function for the breakpad_ucontext_t* argument, and was erroneously looking for the third argument, because I didn’t see exception_type hanging out all alone up here. https://breakpad.appspot.com/664002/diff/250001/src/client/mac/handler/except... src/client/mac/handler/exception_handler.cc:353: There’s whitespace on this line. https://breakpad.appspot.com/664002/diff/250001/src/client/mac/handler/mach_v... File src/client/mac/handler/mach_vm_compat.h (right): https://breakpad.appspot.com/664002/diff/250001/src/client/mac/handler/mach_v... src/client/mac/handler/mach_vm_compat.h:39: #define mach_vm_address_t vm_address_t Can this, and the one for mach_vm_size_t, be typedefs? https://breakpad.appspot.com/664002/diff/250001/src/client/mac/handler/mach_v... src/client/mac/handler/mach_vm_compat.h:43: #define mach_vm_region_recurse vm_region_recurse_64 Can’t you just use vm_region_recurse_64 even for 32-bit? If not, you should technically also switch the info type between vm_region_submap_info and vm_region_submap_info_64. But the _64 variant should work everywhere. And, in fact, so should the non-_64 variant. This is tricky and you wouldn’t necessarily notice any problems if you got the struct types and calls confused, so please go back and make sure that the data you’re expecting to see in any vm_region_submap_info[_64] struct you get back from the system is actually what you expect it to be. https://breakpad.appspot.com/664002/diff/250001/src/client/mac/handler/minidu... File src/client/mac/handler/minidump_generator.cc (right): https://breakpad.appspot.com/664002/diff/250001/src/client/mac/handler/minidu... src/client/mac/handler/minidump_generator.cc:371: case CPU_TYPE_ARM64: As before, it looks like for ARM (as opposed to ppc and x86), you really only ever support dumping the same-bittedness, since everything’s in-process. You should refine this stuff with #ifdefs to avoid building unnecessary code. https://breakpad.appspot.com/664002/diff/250001/src/client/mac/handler/uconte... File src/client/mac/handler/ucontext_compat.h (left): https://breakpad.appspot.com/664002/diff/250001/src/client/mac/handler/uconte... src/client/mac/handler/ucontext_compat.h:30: #include "client/linux/log/log.h" This file looks like a copy, but it’s really not, it’s a brand new file. The git default here is bad. Can you futz with your git “similarity” thing so that it doesn’t show up as a file copy? https://breakpad.appspot.com/664002/diff/250001/src/client/mac/handler/uconte... File src/client/mac/handler/ucontext_compat.h (right): https://breakpad.appspot.com/664002/diff/250001/src/client/mac/handler/uconte... src/client/mac/handler/ucontext_compat.h:1: // Copyright 2013, Google Inc. Nit: no comma between the year and the copyright holder’s name. https://breakpad.appspot.com/664002/diff/250001/src/client/mac/handler/uconte... src/client/mac/handler/ucontext_compat.h:33: #include <TargetConditionals.h> You don’t need this. Seriously. Macros like __arm64__ are compiler-predefined. You don’t need to #include anything to use them. https://breakpad.appspot.com/664002/diff/250001/src/client/mac/handler/uconte... src/client/mac/handler/ucontext_compat.h:35: #if defined(__arm64__) blundell wrote: > This file is needed because the signal handler receives a ucontext64_t instead > of a ucontext_t on ARM64. Great comment! Why is it only a code review comment and not part of this file? > I was wondering whether this should also encompass > PPC64 -- I imagine that it should. Not that anyone cares about ppc really anymore, but based on my read of the 10.6 SDK’s <ppc/_structs.h>, yes. Sucks that Apple did the right thing for the 32-bit x86/x86_64 pair but didn’t carry that forward to ARM (or back to PPC…although in that case, maybe they just didn’t want to break source compatibility). Actually, since on 32-bit x86, the ucontext_t struct and its uc_mcontext member are correct, and on 64-bit x86_64, the ucontext64_t and its uc_mcontext64 member are the same as ucontext_t and uc_mcontext, you can use the breakpad_* definitions in the same way on that platform pair as well. Instead of checking for and naming specific 64-bit CPUs, you would just write this #ifdef around __LP64__. https://breakpad.appspot.com/664002/diff/250001/src/client/mac/handler/uconte... src/client/mac/handler/ucontext_compat.h:36: #include <sys/_types/_ucontext64.h> Why dig this deep into scary directories with underscores in their names? Why not just #include <sys/ucontext.h>, which should include the scary path for you? Also, do the #include outside of the CPU-specific #ifdef, so that you get the same #include behavior regardless of CPU. Otherwise you’ll wind up with problems where it’s an error to forget to #include <sys/ucontext.h> on non-ARM64, but you won’t see that error on ARM64. Having different rules like that is a bummer. https://breakpad.appspot.com/664002/diff/250001/src/client/mac/handler/uconte... src/client/mac/handler/ucontext_compat.h:37: #define breakpad_ucontext_t ucontext64_t Ew, gross! Use a typedef, not a #define. https://breakpad.appspot.com/664002/diff/250001/src/client/mac/handler/uconte... src/client/mac/handler/ucontext_compat.h:40: #define breakpad_ucontext_t ucontext_t The typedef will also require you to properly #include the system header outside of any CPU-specific preprocessor branch too. https://breakpad.appspot.com/664002/diff/250001/src/google_breakpad/common/mi... File src/google_breakpad/common/minidump_cpu_arm64.h (right): https://breakpad.appspot.com/664002/diff/250001/src/google_breakpad/common/mi... src/google_breakpad/common/minidump_cpu_arm64.h:1: /* Copyright 2013, Google Inc. Same comma nit applies here. https://breakpad.appspot.com/664002/diff/250001/src/google_breakpad/common/mi... src/google_breakpad/common/minidump_cpu_arm64.h:127: * context stored in the structure. MD_CONTEXT_ARM64 is Breakpad-defined. blundell wrote: > I couldn't find any definition of MD_CONTEXT_ARM64 on the web; I assume that it > doesn't have one but don't know how to be sure of that. You wouldn’t find it by that name because MD_ is a prefix we made up for Breakpad. Microsoft would have just called it CONTEXT_whatever. You still might not find anything. https://breakpad.appspot.com/664002/diff/250001/src/google_breakpad/common/mi... src/google_breakpad/common/minidump_cpu_arm64.h:130: #define MD_CONTEXT_ARM64 0x80000000 If we’re gonna keep treating these as bitfields, I guess we’re gonna run out of numbers soon. At least numbers “avoiding likely conflicts.” Not really our problem today, though. https://breakpad.appspot.com/664002/diff/250001/src/google_breakpad/common/mi... File src/google_breakpad/common/minidump_format.h (right): https://breakpad.appspot.com/664002/diff/250001/src/google_breakpad/common/mi... src/google_breakpad/common/minidump_format.h:640: MD_CPU_ARCHITECTURE_ARM64 = 0x8003, /* Breakpad-defined value for ARM64 */ blundell wrote: > Same question re: not being 100% sure that Breakpad is free to define this value > itself. Well the 0x8000 range is something we picked to use for our own use. If you were to find a non-Breakpad version of this already in existence, it’d be something like CPU_ARCHITECTURE_ARM64 or CPU_ARCHITECTURE_ARMV8. I don’t know if Microsoft has ever shipped anything on armv8 yet. I wouldn’t rule it out for the future, but we can’t really be expected to read their minds here. I think 0x8003 is fine.
Sign in to reply to this message.
Hi Mark, Thanks! All comments addressed. I'm working my way through the list of crashes that you suggested. https://breakpad.appspot.com/664002/diff/250001/src/client/ios/Breakpad.xcode... File src/client/ios/Breakpad.xcodeproj/project.pbxproj (right): https://breakpad.appspot.com/664002/diff/250001/src/client/ios/Breakpad.xcode... src/client/ios/Breakpad.xcodeproj/project.pbxproj:1: // !$*UTF8*$! On 2013/11/07 21:26:45, Mark Mentovai wrote: > I guess this file lists nearby headers. Wanna add your new ucontext_compat.h > here? Done. https://breakpad.appspot.com/664002/diff/250001/src/client/ios/handler/ios_ex... File src/client/ios/handler/ios_exception_minidump_generator.h (right): https://breakpad.appspot.com/664002/diff/250001/src/client/ios/handler/ios_ex... src/client/ios/handler/ios_exception_minidump_generator.h:54: unsigned long GetPCFromException(); Correct, iOS dump generation is always in-process. Changed to use uintptr_t. On 2013/11/07 21:26:45, Mark Mentovai wrote: > blundell wrote: > > The values in question in the iOS exception minidump generator are pointer > > values, i.e., 4 bytes on ARM and 8 bytes on ARM64. A long is guaranteed to be > 8 > > bytes on ARM64 on iOS (this is specified in the iOS ARMv8 documentation). What > > do you think of this solution? > > iOS dump generation is always in-process, right? So you don’t have to worry > about a 64-bit crash victim being examined by a 32-bit dump writer or > vice-versa? > > A program counter register contains a pointer, so I think you want uintptr_t. > Same on line 57. > > This will also influence the type choice in the .mm file and perhaps elsewhere. https://breakpad.appspot.com/664002/diff/250001/src/client/ios/handler/ios_ex... src/client/ios/handler/ios_exception_minidump_generator.h:62: bool WriteCrashingContextARM64(MDLocationDescriptor *register_location); It is always in-process; I originally ifdef-d the declaration and definition of these functions but then took that out based on looking at the existing pattern. From other comments that you've made, I now understand why that pattern has to be in place for other platforms. Ifdef-ing applied. To apply the ifdef-ing, I changed HAS_ARM_SUPPORT back to being true only for 32-bit ARM and added a new HAS_ARM64_SUPPORT. On 2013/11/07 21:26:45, Mark Mentovai wrote: > Then I see this, where you’re building a function for both ARM and ARM64 into > the same object, and I think that maybe iOS dump generation isn’t always > in-process? If that’s the case, the uintptr_t and unsigned long would both be > wrong, because how would you express a full 64 bits of register content from a > crash victim process in a 32-bit dump writer? https://breakpad.appspot.com/664002/diff/250001/src/client/ios/handler/ios_ex... File src/client/ios/handler/ios_exception_minidump_generator.mm (right): https://breakpad.appspot.com/664002/diff/250001/src/client/ios/handler/ios_ex... src/client/ios/handler/ios_exception_minidump_generator.mm:40: const unsigned long kExpectedFinalFp = sizeof(long); On 2013/11/07 21:26:45, Mark Mentovai wrote: > Technically the types should match, so whatever you use here is what you take > the sizeof of. Since you’re using unsigned long now, it should be unsigned long > on the right side too. If it changes to some other type, the right side should > also obviously change. Done. https://breakpad.appspot.com/664002/diff/250001/src/client/ios/handler/ios_ex... src/client/ios/handler/ios_exception_minidump_generator.mm:75: return WriteCrashingContextARM64(register_location); Done (added a HAS_ARM64_SUPPORT define and used that everywhere). I think it makes sense to leave them in separate functions but could be convinced otherwise...they're conceptually similar but just about every line references concrete types. On 2013/11/07 21:26:45, Mark Mentovai wrote: > OK, so now that I see this, it looks like we’re back to only doing 64-bit dumps > in a 64-bit process, and 32-bit dumps in a 32-bit process. To make that clearer… > > #ifdef __arm64__ around the declarations and the implementations too. There’s no > reason to build code that’s just going to be dead (even if you want to > dead-strip it away). > > Another option is to just have the #ifdef live in this function which wraps the > two inline implementations. That’d be fine, but it’s also fine to leave them in > separate functions, as long as you #ifdef everything properly. Your call. > > If the two separate functions are very different, it would support keeping them > distinct. > > If the two functions are almost entirely the same, except maybe for a few > constants and types here and there, a better answer would be to have a single > implementation, sticking just the constants and typedefs into an #ifdef, and > letting both bitnesses share the guts of the code. > > Which applies here? https://breakpad.appspot.com/664002/diff/250001/src/client/ios/handler/ios_ex... src/client/ios/handler/ios_exception_minidump_generator.mm:80: assert(false); On 2013/11/07 21:26:45, Mark Mentovai wrote: > Seems like you really just want this to be > > #error "This file requires ARM" > > compile-time error, not run-time assertion failure. > > I see this pattern show up elsewhere in the file, you can clean those up too or > just get rid of ’em as long as it’s checked in at least one spot. Unless there’s > a risk of that one spot disappearing and the check getting lost. Done. https://breakpad.appspot.com/664002/diff/250001/src/client/ios/handler/ios_ex... src/client/ios/handler/ios_exception_minidump_generator.mm:116: context_ptr->iregs[29] = kExpectedFinalFp; // FP On 2013/11/07 21:26:45, Mark Mentovai wrote: > Maybe instead of these magic numbers, you can use the values from > minidump_cpu_arm64.h? And minidump_cpu_arm.h for the ARM non-ARM64 ones? That > doesn’t prevent you from needing an #ifdef to pick the right names, but it’s > less “magical” to use constants with names, assuming they’re correct. Done. https://breakpad.appspot.com/664002/diff/250001/src/client/ios/handler/ios_ex... src/client/ios/handler/ios_exception_minidump_generator.mm:171: size_t data_size = sizeof(long); It's the size of a pointer (arm64 still defines a word as 4 bytes). Changed. On 2013/11/07 21:26:45, Mark Mentovai wrote: > I don’t like the name “data_size.” Is it just word_size or native_word_size or > something along those lines? “data” sounds like something generic that could be > larger (or even smaller) than a native word. https://breakpad.appspot.com/664002/diff/250001/src/client/ios/handler/ios_ex... src/client/ios/handler/ios_exception_minidump_generator.mm:176: scoped_array<uint8_t> stack_memory(new uint8_t[stack_size]); Assuming that you mean an array of {fp, lr} pairs, it's complicated by the fact that there's one extra pointer stored in the memory. I'd prefer to leave such a refactoring to a separate CL as there's so much going on in this CL as it is if that's OK. On 2013/11/07 21:26:45, Mark Mentovai wrote: > It kinda seems to me like you want an array of structs here that you later treat > as bytes. https://breakpad.appspot.com/664002/diff/250001/src/client/mac/handler/except... File src/client/mac/handler/exception_handler.cc (right): https://breakpad.appspot.com/664002/diff/250001/src/client/mac/handler/except... src/client/mac/handler/exception_handler.cc:345: bool ExceptionHandler::WriteMinidumpWithException(int exception_type, On 2013/11/07 21:26:45, Mark Mentovai wrote: > If you’re gonna put the arguments on a continuation line like this so they’re > all lined up, move exception_type to the next line so it lines up with them too. > I was looking for what callers were passing to this function for the > breakpad_ucontext_t* argument, and was erroneously looking for the third > argument, because I didn’t see exception_type hanging out all alone up here. Done. https://breakpad.appspot.com/664002/diff/250001/src/client/mac/handler/except... src/client/mac/handler/exception_handler.cc:353: On 2013/11/07 21:26:45, Mark Mentovai wrote: > There’s whitespace on this line. Done. https://breakpad.appspot.com/664002/diff/250001/src/client/mac/handler/except... src/client/mac/handler/exception_handler.cc:456: switch (target_behavior & ~MACH_EXCEPTION_CODES) { Moved this comment into the code. On 2013/11/07 16:41:26, blundell wrote: > This is a high-order bit that is set on arm 64 to indicate that 64-bit code and > subcode are sent in the header. https://breakpad.appspot.com/664002/diff/250001/src/client/mac/handler/mach_v... File src/client/mac/handler/mach_vm_compat.h (right): https://breakpad.appspot.com/664002/diff/250001/src/client/mac/handler/mach_v... src/client/mac/handler/mach_vm_compat.h:39: #define mach_vm_address_t vm_address_t Not as far as I can see; the compiler complains on armV7 as mach_vm_address_t is 64 bits and vm_address_t is 32 bits. On 2013/11/07 21:26:45, Mark Mentovai wrote: > Can this, and the one for mach_vm_size_t, be typedefs? https://breakpad.appspot.com/664002/diff/250001/src/client/mac/handler/mach_v... src/client/mac/handler/mach_vm_compat.h:43: #define mach_vm_region_recurse vm_region_recurse_64 I tested, and vm_region_recurse_64 at least compiles/links fine on 32-bit; I didn't test the operation of Breakpad in that case. I would prefer to leave such a change to a separate CL if you're OK with that. The linker actually doesn't find vm_region_recurse on ARM64. I looked at Breakpad's usage of this function, and in fact we're using it with vm_region_submap_info_64 everywhere. So if there *is* a compatibility problem, it would actually be a pre-existing one with 32-bit ARM rather than a new one with 64-bit ARM (unless I'm missing something). Would you be OK with me filing a bug to investigate this issue on 32-bit ARM? On 2013/11/07 21:26:45, Mark Mentovai wrote: > Can’t you just use vm_region_recurse_64 even for 32-bit? If not, you should > technically also switch the info type between vm_region_submap_info and > vm_region_submap_info_64. But the _64 variant should work everywhere. And, in > fact, so should the non-_64 variant. > > This is tricky and you wouldn’t necessarily notice any problems if you got the > struct types and calls confused, so please go back and make sure that the data > you’re expecting to see in any vm_region_submap_info[_64] struct you get back > from the system is actually what you expect it to be. https://breakpad.appspot.com/664002/diff/250001/src/client/mac/handler/minidu... File src/client/mac/handler/minidump_generator.cc (right): https://breakpad.appspot.com/664002/diff/250001/src/client/mac/handler/minidu... src/client/mac/handler/minidump_generator.cc:371: case CPU_TYPE_ARM64: On 2013/11/07 21:26:45, Mark Mentovai wrote: > As before, it looks like for ARM (as opposed to ppc and x86), you really only > ever support dumping the same-bittedness, since everything’s in-process. You > should refine this stuff with #ifdefs to avoid building unnecessary code. Done. https://breakpad.appspot.com/664002/diff/250001/src/client/mac/handler/uconte... File src/client/mac/handler/ucontext_compat.h (left): https://breakpad.appspot.com/664002/diff/250001/src/client/mac/handler/uconte... src/client/mac/handler/ucontext_compat.h:30: #include "client/linux/log/log.h" I experimented here. Unfortunately this file is more similar to the file git thinks it's a copy of than minidump_cpu_arm64.h is to minidump_cpu_arm.h, so I can't fix this problem without breaking that association as well. I assume we want to keep that? On 2013/11/07 21:26:45, Mark Mentovai wrote: > This file looks like a copy, but it’s really not, it’s a brand new file. The git > default here is bad. Can you futz with your git “similarity” thing so that it > doesn’t show up as a file copy? https://breakpad.appspot.com/664002/diff/250001/src/client/mac/handler/uconte... File src/client/mac/handler/ucontext_compat.h (right): https://breakpad.appspot.com/664002/diff/250001/src/client/mac/handler/uconte... src/client/mac/handler/ucontext_compat.h:1: // Copyright 2013, Google Inc. On 2013/11/07 21:26:45, Mark Mentovai wrote: > Nit: no comma between the year and the copyright holder’s name. Done. https://breakpad.appspot.com/664002/diff/250001/src/client/mac/handler/uconte... src/client/mac/handler/ucontext_compat.h:33: #include <TargetConditionals.h> On 2013/11/07 21:26:45, Mark Mentovai wrote: > You don’t need this. Seriously. > > Macros like __arm64__ are compiler-predefined. You don’t need to #include > anything to use them. Done. https://breakpad.appspot.com/664002/diff/250001/src/client/mac/handler/uconte... src/client/mac/handler/ucontext_compat.h:35: #if defined(__arm64__) On 2013/11/07 21:26:45, Mark Mentovai wrote: > blundell wrote: > > This file is needed because the signal handler receives a ucontext64_t instead > > of a ucontext_t on ARM64. > > Great comment! Why is it only a code review comment and not part of this file? Done. > > > I was wondering whether this should also encompass > > PPC64 -- I imagine that it should. > > Not that anyone cares about ppc really anymore, but based on my read of the 10.6 > SDK’s <ppc/_structs.h>, yes. > > Sucks that Apple did the right thing for the 32-bit x86/x86_64 pair but didn’t > carry that forward to ARM (or back to PPC…although in that case, maybe they just > didn’t want to break source compatibility). > > Actually, since on 32-bit x86, the ucontext_t struct and its uc_mcontext member > are correct, and on 64-bit x86_64, the ucontext64_t and its uc_mcontext64 member > are the same as ucontext_t and uc_mcontext, you can use the breakpad_* > definitions in the same way on that platform pair as well. Instead of checking > for and naming specific 64-bit CPUs, you would just write this #ifdef around > __LP64__. Done. It looks to me like on x86_64 ucontext_t and ucontext64_t are distinct types with very slight differences (ucontext_t has an extra member if _XOPEN_SOURCE is defined), but ucontext_t's uc_mcontext member is defined to be the 64-bit flavor of mcontext. It looks like that's enough for us (given Breakpad's usage) to be confident that this change won't break anything for x86_64? https://breakpad.appspot.com/664002/diff/250001/src/client/mac/handler/uconte... src/client/mac/handler/ucontext_compat.h:36: #include <sys/_types/_ucontext64.h> I had tried including <sys/ucontext.h> first; bizarrely, that include does not bring in a definition of ucontext64_t on ARM64. I spent a bunch of time trying to track down why this was the case but was unsuccessful. It looks like |__need_ucontext64_t| is not getting defined, but I couldn't figure out where that define should be coming from. Moved the file includes out of the conditional. On 2013/11/07 21:26:45, Mark Mentovai wrote: > Why dig this deep into scary directories with underscores in their names? Why > not just #include <sys/ucontext.h>, which should include the scary path for you? > > Also, do the #include outside of the CPU-specific #ifdef, so that you get the > same #include behavior regardless of CPU. Otherwise you’ll wind up with problems > where it’s an error to forget to #include <sys/ucontext.h> on non-ARM64, but you > won’t see that error on ARM64. Having different rules like that is a bummer. https://breakpad.appspot.com/664002/diff/250001/src/client/mac/handler/uconte... src/client/mac/handler/ucontext_compat.h:37: #define breakpad_ucontext_t ucontext64_t On 2013/11/07 21:26:45, Mark Mentovai wrote: > Ew, gross! Use a typedef, not a #define. Done. https://breakpad.appspot.com/664002/diff/250001/src/client/mac/handler/uconte... src/client/mac/handler/ucontext_compat.h:40: #define breakpad_ucontext_t ucontext_t On 2013/11/07 21:26:45, Mark Mentovai wrote: > The typedef will also require you to properly #include the system header outside > of any CPU-specific preprocessor branch too. Done. https://breakpad.appspot.com/664002/diff/250001/src/google_breakpad/common/mi... File src/google_breakpad/common/minidump_cpu_arm64.h (right): https://breakpad.appspot.com/664002/diff/250001/src/google_breakpad/common/mi... src/google_breakpad/common/minidump_cpu_arm64.h:1: /* Copyright 2013, Google Inc. On 2013/11/07 21:26:45, Mark Mentovai wrote: > Same comma nit applies here. Done. https://breakpad.appspot.com/664002/diff/250001/src/google_breakpad/common/mi... src/google_breakpad/common/minidump_cpu_arm64.h:127: * context stored in the structure. MD_CONTEXT_ARM64 is Breakpad-defined. Thanks. As far as I can tell, there is no official CONTEXT_ARM64 definition. On 2013/11/07 21:26:45, Mark Mentovai wrote: > blundell wrote: > > I couldn't find any definition of MD_CONTEXT_ARM64 on the web; I assume that > it > > doesn't have one but don't know how to be sure of that. > > You wouldn’t find it by that name because MD_ is a prefix we made up for > Breakpad. Microsoft would have just called it CONTEXT_whatever. > > You still might not find anything. https://breakpad.appspot.com/664002/diff/250001/src/google_breakpad/common/mi... File src/google_breakpad/common/minidump_format.h (right): https://breakpad.appspot.com/664002/diff/250001/src/google_breakpad/common/mi... src/google_breakpad/common/minidump_format.h:640: MD_CPU_ARCHITECTURE_ARM64 = 0x8003, /* Breakpad-defined value for ARM64 */ Thanks. A Google search reveals nothing. On 2013/11/07 21:26:45, Mark Mentovai wrote: > blundell wrote: > > Same question re: not being 100% sure that Breakpad is free to define this > value > > itself. > > Well the 0x8000 range is something we picked to use for our own use. If you were > to find a non-Breakpad version of this already in existence, it’d be something > like CPU_ARCHITECTURE_ARM64 or CPU_ARCHITECTURE_ARMV8. I don’t know if Microsoft > has ever shipped anything on armv8 yet. I wouldn’t rule it out for the future, > but we can’t really be expected to read their minds here. I think 0x8003 is > fine.
Sign in to reply to this message.
https://breakpad.appspot.com/664002/diff/250001/src/client/ios/handler/ios_ex... File src/client/ios/handler/ios_exception_minidump_generator.mm (right): https://breakpad.appspot.com/664002/diff/250001/src/client/ios/handler/ios_ex... src/client/ios/handler/ios_exception_minidump_generator.mm:171: size_t data_size = sizeof(long); blundell wrote: > It's the size of a pointer (arm64 still defines a word as 4 bytes). Changed. I usually mean “native register size” (which is usually the same as the pointer size) when I say “word.” Architectures define “word” in their own technical documentation to all kinds of stupid things. Like an x86_64 “word,” that’s still 16 bits, but this is clearly ridiculous. It’s the same mentality that gave us High Frequency, and then when there was something higher it had to be Very High, and then Ultra High, all the way up to Tremendously High Frequency. https://breakpad.appspot.com/664002/diff/250001/src/client/ios/handler/ios_ex... src/client/ios/handler/ios_exception_minidump_generator.mm:176: scoped_array<uint8_t> stack_memory(new uint8_t[stack_size]); blundell wrote: > Assuming that you mean an array of {fp, lr} pairs, it's complicated by the fact > that there's one extra pointer stored in the memory. I'd prefer to leave such a > refactoring to a separate CL as there's so much going on in this CL as it is if > that's OK. Separate CL plan approved. https://breakpad.appspot.com/664002/diff/250001/src/client/mac/handler/mach_v... File src/client/mac/handler/mach_vm_compat.h (right): https://breakpad.appspot.com/664002/diff/250001/src/client/mac/handler/mach_v... src/client/mac/handler/mach_vm_compat.h:43: #define mach_vm_region_recurse vm_region_recurse_64 blundell wrote: > I tested, and vm_region_recurse_64 at least compiles/links fine on 32-bit; I > didn't test the operation of Breakpad in that case. I would prefer to leave such > a change to a separate CL if you're OK with that. I am. It’ll be easier to test if you don’t have all of the moving parts of this CL distracting you. But, like the other stuff that’ll be in a followup, please try not to forget. With this one especially, it’s kinda dangerous, as you point out here: > I looked at Breakpad's usage of this function, and in fact we're using it with > vm_region_submap_info_64 everywhere. So if there *is* a compatibility problem, > it would actually be a pre-existing one with 32-bit ARM rather than a new one > with 64-bit ARM (unless I'm missing something). Precisely. The compatibility problem is an old one, your new usage is actually correct. > Would you be OK with me filing a > bug to investigate this issue on 32-bit ARM? Good call. https://breakpad.appspot.com/664002/diff/250001/src/client/mac/handler/uconte... File src/client/mac/handler/ucontext_compat.h (left): https://breakpad.appspot.com/664002/diff/250001/src/client/mac/handler/uconte... src/client/mac/handler/ucontext_compat.h:30: #include "client/linux/log/log.h" blundell wrote: > I experimented here. Unfortunately this file is more similar to the file git > thinks it's a copy of than minidump_cpu_arm64.h is to minidump_cpu_arm.h, so I > can't fix this problem without breaking that association as well. I assume we > want to keep that? Ugh, both of the options kind of suck here. It’s one thing to mess this up in a pure git world where it does its own gittish thing, but the bad detection here propagates to the svn server, which does properly track copies, and having the similarity messed up makes the svn history look kind of bad. Bad either way you do it. :/ https://breakpad.appspot.com/664002/diff/250001/src/client/mac/handler/uconte... File src/client/mac/handler/ucontext_compat.h (right): https://breakpad.appspot.com/664002/diff/250001/src/client/mac/handler/uconte... src/client/mac/handler/ucontext_compat.h:35: #if defined(__arm64__) blundell wrote: > Done. It looks to me like on x86_64 ucontext_t and ucontext64_t are distinct > types with very slight differences (ucontext_t has an extra member if > _XOPEN_SOURCE is defined), but ucontext_t's uc_mcontext member is defined to be > the 64-bit flavor of mcontext. It looks like that's enough for us (given > Breakpad's usage) to be confident that this change won't break anything for > x86_64? Exactly! The structs on x86_64 are interchangeable. The extra member doesn’t impact anything if you ignore it. https://breakpad.appspot.com/664002/diff/250001/src/client/mac/handler/uconte... src/client/mac/handler/ucontext_compat.h:36: #include <sys/_types/_ucontext64.h> blundell wrote: > I had tried including <sys/ucontext.h> first; bizarrely, that include does not > bring in a definition of ucontext64_t on ARM64. That’s a regression in the 10.9 SDK! (Which doesn’t apply to ARM64, sure.) https://breakpad.appspot.com/664002/diff/420002/src/client/ios/handler/ios_ex... File src/client/ios/handler/ios_exception_minidump_generator.mm (right): https://breakpad.appspot.com/664002/diff/420002/src/client/ios/handler/ios_ex... src/client/ios/handler/ios_exception_minidump_generator.mm:42: #endif This file currently also makes the assumption that only one of the above are defined, so can you add an #error if more than one is defined? I’m not positive it will break in a constructive and obvious way at compile time if both are defined, so let’s be explicit about the check up here. https://breakpad.appspot.com/664002/diff/420002/src/client/mac/handler/except... File src/client/mac/handler/exception_handler.cc (right): https://breakpad.appspot.com/664002/diff/420002/src/client/mac/handler/except... src/client/mac/handler/exception_handler.cc:459: switch (target_behavior & ~MACH_EXCEPTION_CODES) { Now that the comment’s where it belongs, I can see this line for what it’s actually doing. NAK on it. :( If you do this, and something had MACH_EXCEPTION_CODES set, then our forwarded exception is going to appear incorrect to the handler that receives the forwarded exception. You would need to use mach_exception_raise. And there’s no client/user stub for that, so you’d need to mig it to get mach_excUser.c. Also, the comment’s kind of off. MACH_EXCEPTION_CODES can be set on any architecture, 64 or not. It expands the size of the code field in a way that’s really only useful on a 64-bit system, but it can be specified on 32-bit systems as well. https://breakpad.appspot.com/664002/diff/420002/src/client/mac/handler/minidu... File src/client/mac/handler/minidump_generator.cc (right): https://breakpad.appspot.com/664002/diff/420002/src/client/mac/handler/minidu... src/client/mac/handler/minidump_generator.cc:539: #define AddGPR(a) context_ptr->iregs[a] = REGISTER_FROM_THREADSTATE(machine_state, x[a]) Can you use a \ so that this doesn’t overflow past 80 columns? https://breakpad.appspot.com/664002/diff/420002/src/client/mac/handler/uconte... File src/client/mac/handler/ucontext_compat.h (right): https://breakpad.appspot.com/664002/diff/420002/src/client/mac/handler/uconte... src/client/mac/handler/ucontext_compat.h:34: #include <sys/_types/_ucontext64.h> This won’t do either. <sys/_types> doesn’t exist prior to the 10.9 SDK (and probably its iOS equivalent). I guess the most generic thing you can do with wide SDK compatibility is #include <sys/_structs.h>? https://breakpad.appspot.com/664002/diff/420002/src/client/mac/handler/uconte... src/client/mac/handler/ucontext_compat.h:45: #endif // defined(__arm64__) This should say __LP64__ now. https://breakpad.appspot.com/664002/diff/420002/src/google_breakpad/common/mi... File src/google_breakpad/common/minidump_cpu_arm64.h (right): https://breakpad.appspot.com/664002/diff/420002/src/google_breakpad/common/mi... src/google_breakpad/common/minidump_cpu_arm64.h:123: MD_CONTEXT_ARM64_REG_PC = 32, Some compiler might have gotten upset about the trailing comma here before. (This file is supposed to work as straight C.) I bring it up because I see that the file you copied from, minidump_cpu_arm.h, doesn’t have a comma after the last member of its equivalent enum.
Sign in to reply to this message.
Thanks for the review! https://breakpad.appspot.com/664002/diff/250001/src/client/ios/handler/ios_ex... File src/client/ios/handler/ios_exception_minidump_generator.mm (right): https://breakpad.appspot.com/664002/diff/250001/src/client/ios/handler/ios_ex... src/client/ios/handler/ios_exception_minidump_generator.mm:176: scoped_array<uint8_t> stack_memory(new uint8_t[stack_size]); Filed https://code.google.com/p/google-breakpad/issues/detail?id=548. On 2013/11/08 23:25:52, Mark Mentovai wrote: > blundell wrote: > > Assuming that you mean an array of {fp, lr} pairs, it's complicated by the > fact > > that there's one extra pointer stored in the memory. I'd prefer to leave such > a > > refactoring to a separate CL as there's so much going on in this CL as it is > if > > that's OK. > > Separate CL plan approved. https://breakpad.appspot.com/664002/diff/250001/src/client/mac/handler/mach_v... File src/client/mac/handler/mach_vm_compat.h (right): https://breakpad.appspot.com/664002/diff/250001/src/client/mac/handler/mach_v... src/client/mac/handler/mach_vm_compat.h:43: #define mach_vm_region_recurse vm_region_recurse_64 Filed https://code.google.com/p/google-breakpad/issues/detail?id=549. On 2013/11/08 23:25:52, Mark Mentovai wrote: > blundell wrote: > > I tested, and vm_region_recurse_64 at least compiles/links fine on 32-bit; I > > didn't test the operation of Breakpad in that case. I would prefer to leave > such > > a change to a separate CL if you're OK with that. > > I am. It’ll be easier to test if you don’t have all of the moving parts of this > CL distracting you. But, like the other stuff that’ll be in a followup, please > try not to forget. With this one especially, it’s kinda dangerous, as you point > out here: > > > I looked at Breakpad's usage of this function, and in fact we're using it with > > vm_region_submap_info_64 everywhere. So if there *is* a compatibility problem, > > it would actually be a pre-existing one with 32-bit ARM rather than a new one > > with 64-bit ARM (unless I'm missing something). > > Precisely. The compatibility problem is an old one, your new usage is actually > correct. > > > Would you be OK with me filing a > > bug to investigate this issue on 32-bit ARM? > > Good call. https://breakpad.appspot.com/664002/diff/420002/src/client/ios/handler/ios_ex... File src/client/ios/handler/ios_exception_minidump_generator.mm (right): https://breakpad.appspot.com/664002/diff/420002/src/client/ios/handler/ios_ex... src/client/ios/handler/ios_exception_minidump_generator.mm:42: #endif On 2013/11/08 23:25:53, Mark Mentovai wrote: > This file currently also makes the assumption that only one of the above are > defined, so can you add an #error if more than one is defined? I’m not positive > it will break in a constructive and obvious way at compile time if both are > defined, so let’s be explicit about the check up here. Done. https://breakpad.appspot.com/664002/diff/420002/src/client/mac/handler/except... File src/client/mac/handler/exception_handler.cc (right): https://breakpad.appspot.com/664002/diff/420002/src/client/mac/handler/except... src/client/mac/handler/exception_handler.cc:459: switch (target_behavior & ~MACH_EXCEPTION_CODES) { I see. The issue is that |code| gets stuffed into a 32-bit int by |exception_raise|, IIUC. :( I actually only came across this in very early testing when I had things set up totally incorrectly and something was triggering this code path with such an exception (and indeed, IIRC, behavior got super-wonky once I put in this addition). What do you recommend I do here? Thanks! On 2013/11/08 23:25:53, Mark Mentovai wrote: > Now that the comment’s where it belongs, I can see this line for what it’s > actually doing. NAK on it. :( > > If you do this, and something had MACH_EXCEPTION_CODES set, then our forwarded > exception is going to appear incorrect to the handler that receives the > forwarded exception. You would need to use mach_exception_raise. And there’s no > client/user stub for that, so you’d need to mig it to get mach_excUser.c. > > Also, the comment’s kind of off. MACH_EXCEPTION_CODES can be set on any > architecture, 64 or not. It expands the size of the code field in a way that’s > really only useful on a 64-bit system, but it can be specified on 32-bit systems > as well. https://breakpad.appspot.com/664002/diff/420002/src/client/mac/handler/minidu... File src/client/mac/handler/minidump_generator.cc (right): https://breakpad.appspot.com/664002/diff/420002/src/client/mac/handler/minidu... src/client/mac/handler/minidump_generator.cc:539: #define AddGPR(a) context_ptr->iregs[a] = REGISTER_FROM_THREADSTATE(machine_state, x[a]) On 2013/11/08 23:25:53, Mark Mentovai wrote: > Can you use a \ so that this doesn’t overflow past 80 columns? Done. https://breakpad.appspot.com/664002/diff/420002/src/client/mac/handler/uconte... File src/client/mac/handler/ucontext_compat.h (right): https://breakpad.appspot.com/664002/diff/420002/src/client/mac/handler/uconte... src/client/mac/handler/ucontext_compat.h:34: #include <sys/_types/_ucontext64.h> Thanks. <sys/_structs.h> doesn't include the necessary file, as __need_struct_ucontext64 isn't defined. I moved this inclusion back underneath the __LP64__ with a comment; let me know if you have a better solution. On 2013/11/08 23:25:53, Mark Mentovai wrote: > This won’t do either. <sys/_types> doesn’t exist prior to the 10.9 SDK (and > probably its iOS equivalent). > > I guess the most generic thing you can do with wide SDK compatibility is > #include <sys/_structs.h>? https://breakpad.appspot.com/664002/diff/420002/src/client/mac/handler/uconte... src/client/mac/handler/ucontext_compat.h:45: #endif // defined(__arm64__) On 2013/11/08 23:25:53, Mark Mentovai wrote: > This should say __LP64__ now. Done. https://breakpad.appspot.com/664002/diff/420002/src/google_breakpad/common/mi... File src/google_breakpad/common/minidump_cpu_arm64.h (right): https://breakpad.appspot.com/664002/diff/420002/src/google_breakpad/common/mi... src/google_breakpad/common/minidump_cpu_arm64.h:123: MD_CONTEXT_ARM64_REG_PC = 32, On 2013/11/08 23:25:53, Mark Mentovai wrote: > Some compiler might have gotten upset about the trailing comma here before. > (This file is supposed to work as straight C.) I bring it up because I see that > the file you copied from, minidump_cpu_arm.h, doesn’t have a comma after the > last member of its equivalent enum. Done.
Sign in to reply to this message.
I will LG this once we decide how to deal with the MACH_EXCEPTION_CODES thing. Awaiting input from you. https://breakpad.appspot.com/664002/diff/420002/src/client/mac/handler/except... File src/client/mac/handler/exception_handler.cc (right): https://breakpad.appspot.com/664002/diff/420002/src/client/mac/handler/except... src/client/mac/handler/exception_handler.cc:459: switch (target_behavior & ~MACH_EXCEPTION_CODES) { blundell wrote: > I see. The issue is that |code| gets stuffed into a 32-bit int by > |exception_raise|, IIUC. :( Well, it’s more than that. With MACH_EXCEPTION_CODES, a handler is expecting to have its catch_mach_exception_raise routine called. A client calling exception_raise triggers catch_exception_raise in the handler instead. A handler might not even implement that routine, might not implement it in a way that’s properly reachable from a port that it had registered with MACH_EXCEPTION_CODES, or might not implement it in a functional way. Sending an exception to a handler routine that might not be expecting it or might not even be implemented doesn’t really seem better than dropping the exception with the “** Unknown exception behavior” message, does it? > I actually only came across this in very early testing when I had things set up > totally incorrectly and something was triggering this code path with such an > exception (and indeed, IIRC, behavior got super-wonky once I put in this > addition). What do you recommend I do here? Thanks! Are we allowed to mig mach_exc.defs and include mach_excUser.c in what we ship on iOS here? Or does Apple not want us using that API? This is already a problem on desktop Mac OS X too, by the way. But we slide by because, since 10.5, there is no task-level exception handler for the exception mask we’re interested in. Nothing to forward to here. (And I think we do leave behind “** Unknown exception behavior” turds.) Honestly, now that I look at it, the existing code here is so cheesey that maybe it hasn’t actually really even been used in a long time. Maybe there’s no problem here at all. Do you ever reach this code on iOS? If so, what’s are the values of current.masks[found], target_port, target_behavior, and current.flavors[found]? https://breakpad.appspot.com/664002/diff/190003/src/client/mac/handler/uconte... File src/client/mac/handler/ucontext_compat.h (right): https://breakpad.appspot.com/664002/diff/190003/src/client/mac/handler/uconte... src/client/mac/handler/ucontext_compat.h:38: #if defined(__LP64__) I would make this defined(__arm64__) then, since that’s the platform where it’s proven to be necessary.
Sign in to reply to this message.
Thanks! I can't land this myself, BTW. https://breakpad.appspot.com/664002/diff/420002/src/client/mac/handler/except... File src/client/mac/handler/exception_handler.cc (right): https://breakpad.appspot.com/664002/diff/420002/src/client/mac/handler/except... src/client/mac/handler/exception_handler.cc:459: switch (target_behavior & ~MACH_EXCEPTION_CODES) { I dug into this issue: - This flow is entered on iOS only when the minidump has not successfully been written (if the minidump has successfully been written, Breakpad exits in the bottom of WriteMinidumpWithException(), which is called in WaitForMessage() before WaitForMessage() calls ForwardException() via breakpad_exc_server()). Hence my encountering this flow early on in this process when I was testing minidump generation with ARM64 in a totally broken state. - |target_behavior| has MACH_EXCEPTION_CODES set on 32-bit ARM for iOS in both iOS7 and iOS6, i.e., this issue is actually a pre-existing one (as you had indicated). Here is representative info on the values you asked for: Mask: 4e Port: 14083 Behavior: 80000001 Flavor: 5 I filed https://code.google.com/p/google-breakpad/issues/detail?id=551. I don't know the answer offhand to the question of generating the required code. On 2013/11/20 14:51:33, Mark Mentovai wrote: > blundell wrote: > > I see. The issue is that |code| gets stuffed into a 32-bit int by > > |exception_raise|, IIUC. :( > > Well, it’s more than that. With MACH_EXCEPTION_CODES, a handler is expecting to > have its catch_mach_exception_raise routine called. A client calling > exception_raise triggers catch_exception_raise in the handler instead. A handler > might not even implement that routine, might not implement it in a way that’s > properly reachable from a port that it had registered with MACH_EXCEPTION_CODES, > or might not implement it in a functional way. > > Sending an exception to a handler routine that might not be expecting it or > might not even be implemented doesn’t really seem better than dropping the > exception with the “** Unknown exception behavior” message, does it? > > > I actually only came across this in very early testing when I had things set > up > > totally incorrectly and something was triggering this code path with such an > > exception (and indeed, IIRC, behavior got super-wonky once I put in this > > addition). What do you recommend I do here? Thanks! > > Are we allowed to mig mach_exc.defs and include mach_excUser.c in what we ship > on iOS here? Or does Apple not want us using that API? > > This is already a problem on desktop Mac OS X too, by the way. But we slide by > because, since 10.5, there is no task-level exception handler for the exception > mask we’re interested in. Nothing to forward to here. (And I think we do leave > behind “** Unknown exception behavior” turds.) > > Honestly, now that I look at it, the existing code here is so cheesey that maybe > it hasn’t actually really even been used in a long time. Maybe there’s no > problem here at all. > > Do you ever reach this code on iOS? If so, what’s are the values of > current.masks[found], target_port, target_behavior, and current.flavors[found]? https://breakpad.appspot.com/664002/diff/190003/src/client/mac/handler/uconte... File src/client/mac/handler/ucontext_compat.h (right): https://breakpad.appspot.com/664002/diff/190003/src/client/mac/handler/uconte... src/client/mac/handler/ucontext_compat.h:38: #if defined(__LP64__) On 2013/11/20 14:51:34, Mark Mentovai wrote: > I would make this defined(__arm64__) then, since that’s the platform where it’s > proven to be necessary. Done.
Sign in to reply to this message.
|
