Issue 347001: Creating minidump for uncaught exception on iOS. (Closed)
|
Created:
12 years, 9 months ago by qsr Modified:
12 years, 9 months ago Reviewers:
Mark Mentovai CC:
google-breakpad-dev_googlegroups.com Base URL:
http://google-breakpad.googlecode.com/svn/trunk/ Visibility:
Public. |
DescriptionCreating minidump for uncaught exception on iOS. This CL adds a minidump_generator that can write a minidump from a NSException on iOS on an ARM cpu. This CL also install an uncaught exception handler on iOS, and use the previous generator to write minidumps for any uncaught exception. Committed: https://code.google.com/p/google-breakpad/source/detail?r=916 Patch Set 1 #
Total comments: 20
Patch Set 2 : '' #
Total comments: 6
Patch Set 3 : '' #
Total comments: 14
Patch Set 4 : '' #
Total comments: 10
Patch Set 5 : '' #
MessagesTotal messages: 10
https://breakpad.appspot.com/347001/diff/1/src/client/ios/handler/ios_excepti... File src/client/ios/handler/ios_exception_minidump_generator.h (right): https://breakpad.appspot.com/347001/diff/1/src/client/ios/handler/ios_excepti... src/client/ios/handler/ios_exception_minidump_generator.h:44: IosExceptionMinidumpGenerator(NSException *exception); Mark this explicit. https://breakpad.appspot.com/347001/diff/1/src/client/ios/handler/ios_excepti... src/client/ios/handler/ios_exception_minidump_generator.h:49: private: Blank line before this. https://breakpad.appspot.com/347001/diff/1/src/client/ios/handler/ios_excepti... src/client/ios/handler/ios_exception_minidump_generator.h:50: NSArray *returnAddresses_; This is a C++ class, so follow C++ naming rules: return_addresses_. https://breakpad.appspot.com/347001/diff/1/src/client/ios/handler/ios_excepti... File src/client/ios/handler/ios_exception_minidump_generator.mm (right): https://breakpad.appspot.com/347001/diff/1/src/client/ios/handler/ios_excepti... src/client/ios/handler/ios_exception_minidump_generator.mm:36: void appendToMemory(std::vector<uint8_t>* memory, uint32_t sp, uint32_t data) { For style, this should be AppendToMemory. Also, you should have a blank line between the namespace { and this, and again between the closing } for the function and the closing } for the namespace. https://breakpad.appspot.com/347001/diff/1/src/client/ios/handler/ios_excepti... src/client/ios/handler/ios_exception_minidump_generator.mm:45: NSException *exception) : MinidumpGenerator(mach_task_self(), 0) { Put the : on the next line. https://breakpad.appspot.com/347001/diff/1/src/client/ios/handler/ios_excepti... src/client/ios/handler/ios_exception_minidump_generator.mm:65: size_t nb_element = [returnAddresses_ count]; The style guide says not to abbreviate, and “nb” isn’t an abbreviation an English speaker would recognize in any event. I would normally write element_count or frame_count. https://breakpad.appspot.com/347001/diff/1/src/client/ios/handler/ios_excepti... src/client/ios/handler/ios_exception_minidump_generator.mm:75: for (int index = nb_element - 2; index >= 0; --index) { index isn’t that great a name for a variable, either. https://breakpad.appspot.com/347001/diff/1/src/client/ios/handler/ios_excepti... src/client/ios/handler/ios_exception_minidump_generator.mm:77: sp-=4; Put spaces around operators like -=. https://breakpad.appspot.com/347001/diff/1/src/client/ios/handler/ios_excepti... src/client/ios/handler/ios_exception_minidump_generator.mm:97: context_ptr->iregs[i] = 0; This won’t zero out iregs[12], but I bet you intended to hit that one. https://breakpad.appspot.com/347001/diff/1/src/client/ios/handler/ios_excepti... src/client/ios/handler/ios_exception_minidump_generator.mm:109: return MinidumpGenerator::WriteThreadListStream(thread_list_stream); I guess this is OK…
Sign in to reply to this message.
https://breakpad.appspot.com/347001/diff/1/src/client/ios/handler/ios_excepti... File src/client/ios/handler/ios_exception_minidump_generator.h (right): https://breakpad.appspot.com/347001/diff/1/src/client/ios/handler/ios_excepti... src/client/ios/handler/ios_exception_minidump_generator.h:44: IosExceptionMinidumpGenerator(NSException *exception); On 2012/02/09 16:12:08, Mark Mentovai wrote: > Mark this explicit. Done. https://breakpad.appspot.com/347001/diff/1/src/client/ios/handler/ios_excepti... src/client/ios/handler/ios_exception_minidump_generator.h:49: private: On 2012/02/09 16:12:08, Mark Mentovai wrote: > Blank line before this. Done. https://breakpad.appspot.com/347001/diff/1/src/client/ios/handler/ios_excepti... src/client/ios/handler/ios_exception_minidump_generator.h:50: NSArray *returnAddresses_; On 2012/02/09 16:12:08, Mark Mentovai wrote: > This is a C++ class, so follow C++ naming rules: return_addresses_. Done. https://breakpad.appspot.com/347001/diff/1/src/client/ios/handler/ios_excepti... File src/client/ios/handler/ios_exception_minidump_generator.mm (right): https://breakpad.appspot.com/347001/diff/1/src/client/ios/handler/ios_excepti... src/client/ios/handler/ios_exception_minidump_generator.mm:36: void appendToMemory(std::vector<uint8_t>* memory, uint32_t sp, uint32_t data) { On 2012/02/09 16:12:08, Mark Mentovai wrote: > For style, this should be AppendToMemory. Also, you should have a blank line > between the namespace { and this, and again between the closing } for the > function and the closing } for the namespace. Done. https://breakpad.appspot.com/347001/diff/1/src/client/ios/handler/ios_excepti... src/client/ios/handler/ios_exception_minidump_generator.mm:45: NSException *exception) : MinidumpGenerator(mach_task_self(), 0) { On 2012/02/09 16:12:08, Mark Mentovai wrote: > Put the : on the next line. Done. https://breakpad.appspot.com/347001/diff/1/src/client/ios/handler/ios_excepti... src/client/ios/handler/ios_exception_minidump_generator.mm:65: size_t nb_element = [returnAddresses_ count]; On 2012/02/09 16:12:08, Mark Mentovai wrote: > The style guide says not to abbreviate, and “nb” isn’t an abbreviation an > English speaker would recognize in any event. > > I would normally write element_count or frame_count. Done. https://breakpad.appspot.com/347001/diff/1/src/client/ios/handler/ios_excepti... src/client/ios/handler/ios_exception_minidump_generator.mm:75: for (int index = nb_element - 2; index >= 0; --index) { On 2012/02/09 16:12:08, Mark Mentovai wrote: > index isn’t that great a name for a variable, either. Done. https://breakpad.appspot.com/347001/diff/1/src/client/ios/handler/ios_excepti... src/client/ios/handler/ios_exception_minidump_generator.mm:77: sp-=4; On 2012/02/09 16:12:08, Mark Mentovai wrote: > Put spaces around operators like -=. Done. https://breakpad.appspot.com/347001/diff/1/src/client/ios/handler/ios_excepti... src/client/ios/handler/ios_exception_minidump_generator.mm:97: context_ptr->iregs[i] = 0; On 2012/02/09 16:12:08, Mark Mentovai wrote: > This won’t zero out iregs[12], but I bet you intended to hit that one. Done. https://breakpad.appspot.com/347001/diff/1/src/client/ios/handler/ios_excepti... src/client/ios/handler/ios_exception_minidump_generator.mm:109: return MinidumpGenerator::WriteThreadListStream(thread_list_stream); This code is never called, unless someone decide to use IosExceptionMinidumpGenerator outside of the usual framework. The other solution is to return false, but in both case what will be generated is not usefull.
Sign in to reply to this message.
LG otherwise https://breakpad.appspot.com/347001/diff/2002/src/client/ios/handler/ios_exce... File src/client/ios/handler/ios_exception_minidump_generator.mm (right): https://breakpad.appspot.com/347001/diff/2002/src/client/ios/handler/ios_exce... src/client/ios/handler/ios_exception_minidump_generator.mm:39: for (int i = 0; i < 4; i++) (*memory)[sp + i - 4] = *(byte++); I’m not convinced that this needs to be a loop. https://breakpad.appspot.com/347001/diff/2002/src/client/ios/handler/ios_exce... src/client/ios/handler/ios_exception_minidump_generator.mm:101: for (int i = 0; i < 16; ++i) arraysize or MD_CONTEXT_ARM_GPR_COUNT?
Sign in to reply to this message.
PTAL Change my way of doing things -> publish all threads and make sure the thread that catch the exception is considered a crashing thread. https://breakpad.appspot.com/347001/diff/2002/src/client/ios/handler/ios_exce... File src/client/ios/handler/ios_exception_minidump_generator.mm (right): https://breakpad.appspot.com/347001/diff/2002/src/client/ios/handler/ios_exce... src/client/ios/handler/ios_exception_minidump_generator.mm:39: for (int i = 0; i < 4; i++) (*memory)[sp + i - 4] = *(byte++); On 2012/02/09 18:40:54, Mark Mentovai wrote: > I’m not convinced that this needs to be a loop. Sorry, but I do not understand your comment. You want me to replace that to 4 statements? Won't the compiler unroll the loop for me? Or it the 4 the problem? I guess I can use sizeof(data) instead. https://breakpad.appspot.com/347001/diff/2002/src/client/ios/handler/ios_exce... src/client/ios/handler/ios_exception_minidump_generator.mm:101: for (int i = 0; i < 16; ++i) Changed that to use memset, that will be more efficient, smaller to write, and less error prone.
Sign in to reply to this message.
http://breakpad.appspot.com/347001/diff/2002/src/client/ios/handler/ios_excep... File src/client/ios/handler/ios_exception_minidump_generator.mm (right): http://breakpad.appspot.com/347001/diff/2002/src/client/ios/handler/ios_excep... src/client/ios/handler/ios_exception_minidump_generator.mm:39: for (int i = 0; i < 4; i++) (*memory)[sp + i - 4] = *(byte++); qsr wrote: > On 2012/02/09 18:40:54, Mark Mentovai wrote: > > I’m not convinced that this needs to be a loop. > > Sorry, but I do not understand your comment. You want me to replace that to 4 > statements? Won't the compiler unroll the loop for me? Or it the 4 the problem? > I guess I can use sizeof(data) instead. The compiler unrolls the loop which is good for the machine code, but the source code you’ve written here is still more obtuse than it needs to be from a readability perspective. You’re just moving 32 bits from data to memory (with an sp offset). Why should you need to write a loop to do that? http://breakpad.appspot.com/347001/diff/1007/src/client/ios/Breakpad.mm File src/client/ios/Breakpad.mm (right): http://breakpad.appspot.com/347001/diff/1007/src/client/ios/Breakpad.mm#newco... src/client/ios/Breakpad.mm:540: // 2- If the application crash will trying to handle this exception, a usual I don’t understand what you mean here. The word “crash” seems misplaced. http://breakpad.appspot.com/347001/diff/1007/src/client/ios/Breakpad.xcodepro... File src/client/ios/Breakpad.xcodeproj/project.pbxproj (right): http://breakpad.appspot.com/347001/diff/1007/src/client/ios/Breakpad.xcodepro... src/client/ios/Breakpad.xcodeproj/project.pbxproj:358: attributes = { Can you edit this addition out of your diff? Looks like Xcode 4.2 gunk. http://breakpad.appspot.com/347001/diff/1007/src/client/ios/handler/ios_excep... File src/client/ios/handler/ios_exception_minidump_generator.h (right): http://breakpad.appspot.com/347001/diff/1007/src/client/ios/handler/ios_excep... src/client/ios/handler/ios_exception_minidump_generator.h:33: #ifndef CLIENT_IOS_HANDLER_IOS_EXCEPTION_MINIDUMP_GENERATOR_H__ The current recommendation from the style guide is to only use one trailing underscore for the #include guards, because two trailing underscores are reserved for the implementation. In Breakpad, we’ve been usually following the new rule for new files, but not updating old files. http://breakpad.appspot.com/347001/diff/1007/src/client/ios/handler/ios_excep... src/client/ios/handler/ios_exception_minidump_generator.h:54: // Write a virtual thread context for the crashing site. Blank line before this one. http://breakpad.appspot.com/347001/diff/1007/src/client/ios/handler/ios_excep... File src/client/ios/handler/ios_exception_minidump_generator.mm (right): http://breakpad.appspot.com/347001/diff/1007/src/client/ios/handler/ios_excep... src/client/ios/handler/ios_exception_minidump_generator.mm:123: return MinidumpGenerator::WriteThreadStream(thread_id, thread); Good catch! http://breakpad.appspot.com/347001/diff/1007/src/client/mac/handler/minidump_... File src/client/mac/handler/minidump_generator.h (right): http://breakpad.appspot.com/347001/diff/1007/src/client/mac/handler/minidump_... src/client/mac/handler/minidump_generator.h:180: protected: Don’t put a protected section after a private section. Move this stuff up into the new protected section above. http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Class_... http://breakpad.appspot.com/347001/diff/1007/src/client/mac/handler/minidump_... src/client/mac/handler/minidump_generator.h:184: private: When you do that, you won’t need to add this private:. http://breakpad.appspot.com/347001/diff/1007/src/client/mac/handler/minidump_... src/client/mac/handler/minidump_generator.h:209: protected: And again.
Sign in to reply to this message.
https://breakpad.appspot.com/347001/diff/2002/src/client/ios/handler/ios_exce... File src/client/ios/handler/ios_exception_minidump_generator.mm (right): https://breakpad.appspot.com/347001/diff/2002/src/client/ios/handler/ios_exce... src/client/ios/handler/ios_exception_minidump_generator.mm:39: for (int i = 0; i < 4; i++) (*memory)[sp + i - 4] = *(byte++); Ok. Changed that to use a usual array, and memcpy instead. Clearer and more concise. Did add a stupid assert just in case ios is not 32 bits only in some future time. https://breakpad.appspot.com/347001/diff/1007/src/client/ios/Breakpad.mm File src/client/ios/Breakpad.mm (right): https://breakpad.appspot.com/347001/diff/1007/src/client/ios/Breakpad.mm#newc... src/client/ios/Breakpad.mm:540: // 2- If the application crash will trying to handle this exception, a usual On 2012/02/11 05:20:07, Mark Mentovai wrote: > I don’t understand what you mean here. The word “crash” seems misplaced. Done. https://breakpad.appspot.com/347001/diff/1007/src/client/ios/Breakpad.xcodepr... File src/client/ios/Breakpad.xcodeproj/project.pbxproj (right): https://breakpad.appspot.com/347001/diff/1007/src/client/ios/Breakpad.xcodepr... src/client/ios/Breakpad.xcodeproj/project.pbxproj:358: attributes = { On 2012/02/11 05:20:07, Mark Mentovai wrote: > Can you edit this addition out of your diff? Looks like Xcode 4.2 gunk. Done. https://breakpad.appspot.com/347001/diff/1007/src/client/ios/handler/ios_exce... File src/client/ios/handler/ios_exception_minidump_generator.h (right): https://breakpad.appspot.com/347001/diff/1007/src/client/ios/handler/ios_exce... src/client/ios/handler/ios_exception_minidump_generator.h:33: #ifndef CLIENT_IOS_HANDLER_IOS_EXCEPTION_MINIDUMP_GENERATOR_H__ On 2012/02/11 05:20:07, Mark Mentovai wrote: > The current recommendation from the style guide is to only use one trailing > underscore for the #include guards, because two trailing underscores are > reserved for the implementation. In Breakpad, we’ve been usually following the > new rule for new files, but not updating old files. Done. https://breakpad.appspot.com/347001/diff/1007/src/client/ios/handler/ios_exce... src/client/ios/handler/ios_exception_minidump_generator.h:54: // Write a virtual thread context for the crashing site. On 2012/02/11 05:20:07, Mark Mentovai wrote: > Blank line before this one. Done. https://breakpad.appspot.com/347001/diff/1007/src/client/ios/handler/ios_exce... File src/client/ios/handler/ios_exception_minidump_generator.mm (right): https://breakpad.appspot.com/347001/diff/1007/src/client/ios/handler/ios_exce... src/client/ios/handler/ios_exception_minidump_generator.mm:123: return MinidumpGenerator::WriteThreadStream(thread_id, thread); On 2012/02/11 05:20:07, Mark Mentovai wrote: > Good catch! Not getting it sorry... Or you missed something. I changed my way of doing things, previous iteration was rewriting all the threads steam and only send one stack (the one of the exception), now I'm only rewriting the current one with the one of the exception and using the usual memory introspection for other threads. I'm also now sending an exception stream with the id of the current thread and a custom exception code so that the server consider that a thread did crash and compute a correct stack signature. https://breakpad.appspot.com/347001/diff/1007/src/client/mac/handler/minidump... File src/client/mac/handler/minidump_generator.h (right): https://breakpad.appspot.com/347001/diff/1007/src/client/mac/handler/minidump... src/client/mac/handler/minidump_generator.h:180: protected: The problem is that order of declaration does count for initialization purposes. If I order everything as the style guide wants, I'll have to expose more things to the subclass. Is that wanted?
Sign in to reply to this message.
https://breakpad.appspot.com/347001/diff/2005/src/client/ios/handler/ios_exce... File src/client/ios/handler/ios_exception_minidump_generator.h (right): https://breakpad.appspot.com/347001/diff/2005/src/client/ios/handler/ios_exce... src/client/ios/handler/ios_exception_minidump_generator.h:53: You put the blank line in the wrong spot! There should be no blank between the “Get the crashing” comment and GetPCFromException. There should be a blank line between GetPCFromException and the “Write a virtual thread context” comment (that belongs with WriteCrashingContext). https://breakpad.appspot.com/347001/diff/2005/src/client/ios/handler/ios_exce... File src/client/ios/handler/ios_exception_minidump_generator.mm (right): https://breakpad.appspot.com/347001/diff/2005/src/client/ios/handler/ios_exce... src/client/ios/handler/ios_exception_minidump_generator.mm:30: #include <vector> Unused now? https://breakpad.appspot.com/347001/diff/2005/src/client/ios/handler/ios_exce... src/client/ios/handler/ios_exception_minidump_generator.mm:41: // Append the given 4 byte value to the memory represented by the vector. No more vector? https://breakpad.appspot.com/347001/diff/2005/src/client/ios/handler/ios_exce... src/client/ios/handler/ios_exception_minidump_generator.mm:44: memcpy(memory + sp, &data, sizeof(data)); Precisely! https://breakpad.appspot.com/347001/diff/2005/src/client/ios/handler/ios_exce... src/client/ios/handler/ios_exception_minidump_generator.mm:130: uint8_t stack_memory[size]; A dynamic stack array is bad. It’s also forbidden by the style guide. http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Variable-Lengt... You can switch back to the vector or use “new[]” (as long as you also use delete[], possibly via a scoper).
Sign in to reply to this message.
https://breakpad.appspot.com/347001/diff/2005/src/client/ios/handler/ios_exce... File src/client/ios/handler/ios_exception_minidump_generator.h (right): https://breakpad.appspot.com/347001/diff/2005/src/client/ios/handler/ios_exce... src/client/ios/handler/ios_exception_minidump_generator.h:53: On 2012/02/13 17:11:50, Mark Mentovai wrote: > You put the blank line in the wrong spot! > > There should be no blank between the “Get the crashing” comment and > GetPCFromException. > > There should be a blank line between GetPCFromException and the “Write a virtual > thread context” comment (that belongs with WriteCrashingContext). Done. https://breakpad.appspot.com/347001/diff/2005/src/client/ios/handler/ios_exce... File src/client/ios/handler/ios_exception_minidump_generator.mm (right): https://breakpad.appspot.com/347001/diff/2005/src/client/ios/handler/ios_exce... src/client/ios/handler/ios_exception_minidump_generator.mm:30: #include <vector> On 2012/02/13 17:11:50, Mark Mentovai wrote: > Unused now? Done. https://breakpad.appspot.com/347001/diff/2005/src/client/ios/handler/ios_exce... src/client/ios/handler/ios_exception_minidump_generator.mm:41: // Append the given 4 byte value to the memory represented by the vector. On 2012/02/13 17:11:50, Mark Mentovai wrote: > No more vector? Done. https://breakpad.appspot.com/347001/diff/2005/src/client/ios/handler/ios_exce... src/client/ios/handler/ios_exception_minidump_generator.mm:44: memcpy(memory + sp, &data, sizeof(data)); On 2012/02/13 17:11:50, Mark Mentovai wrote: > Precisely! Done. https://breakpad.appspot.com/347001/diff/2005/src/client/ios/handler/ios_exce... src/client/ios/handler/ios_exception_minidump_generator.mm:130: uint8_t stack_memory[size]; On 2012/02/13 17:11:50, Mark Mentovai wrote: > A dynamic stack array is bad. It’s also forbidden by the style guide. > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Variable-Lengt... > > You can switch back to the vector or use “new[]” (as long as you also use > delete[], possibly via a scoper). Done.
Sign in to reply to this message.
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
