Add logging to MachineStackMarker to try to diagnose crashes in the wild
<rdar://problem/32797302>
Created attachment 312998 [details] Patch
Attachment 312998 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/heap/MachineStackMarker.cpp:104: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/heap/MachineStackMarker.cpp:105: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/heap/MachineStackMarker.cpp:106: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/heap/MachineStackMarker.cpp:107: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/heap/MachineStackMarker.cpp:108: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/heap/MachineStackMarker.cpp:109: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/heap/MachineStackMarker.cpp:110: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/heap/MachineStackMarker.cpp:111: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/heap/MachineStackMarker.cpp:112: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/heap/MachineStackMarker.cpp:117: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/heap/MachineStackMarker.cpp:118: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/heap/MachineStackMarker.cpp:119: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/heap/MachineStackMarker.cpp:120: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/heap/MachineStackMarker.cpp:121: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/heap/MachineStackMarker.cpp:122: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/heap/MachineStackMarker.cpp:123: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/heap/MachineStackMarker.cpp:124: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/heap/MachineStackMarker.cpp:125: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/heap/MachineStackMarker.cpp:126: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/heap/MachineStackMarker.cpp:127: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/heap/MachineStackMarker.cpp:128: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/heap/MachineStackMarker.cpp:129: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Total errors found: 22 in 2 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 312998 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=312998&action=review Did you confirm that this does not conflict with registers used in the function? Did you confirm that the callee saved registers will be restored to their original value by the C++ compiler before returning from the constructor? r=me assuming questions and issues are addressed. > Source/JavaScriptCore/ChangeLog:10 > + not support os_log_info my hope is that if we set all the callee please add a comma after "os_log_info". > Source/JavaScriptCore/heap/MachineStackMarker.cpp:103 > + asm volatile \ > + ( \ I suggest moving the ( after volatile. > Source/JavaScriptCore/heap/MachineStackMarker.cpp:111 > + "movq $0xc0defefe000000" # number ", %%rbx;" \ > + "movq $0xc0defefe000000" # number ", %%r12;" \ > + "movq $0xc0defefe000000" # number ", %%r13;" \ > + "movq $0xc0defefe000000" # number ", %%r14;" \ > + "movq $0xc0defefe000000" # number ", %%r15;" \ > + : \ > + : \ > + : "%rbx", "%r12", "%r13", "%r14", "%r15" \ why indent by 5 spaces? I suggest indenting by 3 more to make this 8. > Source/JavaScriptCore/heap/MachineStackMarker.cpp:112 > + ); I suggest changing indentation here to 4 instead of 5. > Source/JavaScriptCore/heap/MachineStackMarker.cpp:129 > + asm volatile \ > + ( \ > + "movq $0xc0defefe000000" # number ", %%rax;" \ > + "movq $0xc0defefe000000" # number ", %%rdi;" \ > + "movq $0xc0defefe000000" # number ", %%rsi;" \ > + "movq $0xc0defefe000000" # number ", %%rdx;" \ > + "movq $0xc0defefe000000" # number ", %%rcx;" \ > + "movq $0xc0defefe000000" # number ", %%r8;" \ > + "movq $0xc0defefe000000" # number ", %%r9;" \ > + "movq $0xc0defefe000000" # number ", %%r10;" \ > + "movq $0xc0defefe000000" # number ", %%r11;" \ > + : \ > + : \ > + : "%rax", "%rdi", "%rsi", "%rdx", "%rcx", "%r8", "%r9", "%r10", "%r11" \ > + ); I suggest applying same changes as asm statement above. > Source/JavaScriptCore/heap/MachineStackMarker.cpp:131 > +bool truth = true; Remove. > Source/JavaScriptCore/heap/MachineStackMarker.cpp:138 > + FILL_CALLEE_SAVES_FOR_CRASH_INFO(1); I suggest passing "01" instead of 1. This way, the value you'll store is $0xc0defefe00000001 (16 digits) instead of $0xc0defefe0000001 (15 digits). > Source/JavaScriptCore/heap/MachineStackMarker.cpp:142 > + FILL_CALLEE_SAVES_FOR_CRASH_INFO(2); Ditto. Pass "02". > Source/JavaScriptCore/heap/MachineStackMarker.cpp:146 > + if (truth) > + CRASH(); Remove. > Source/JavaScriptCore/heap/MachineStackMarker.cpp:148 > + FILL_CALLER_SAVES_FOR_CRASH_INFO(3); Ditto. Pass "03".
Comment on attachment 312998 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=312998&action=review > Source/JavaScriptCore/heap/MachineStackMarker.cpp:137 > +#if CPU(X86_64) && OS(DARWIN) nit: I would move this #if + #endif up around the definition of FILL_CALLEE_SAVES_FOR_CRASH_INFO instead of duplicating it here for all 3 instances. Also provide a stubbed FILL_CALLEE_SAVES_FOR_CRASH_INFO for the #else case there.
Comment on attachment 312998 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=312998&action=review >> Source/JavaScriptCore/ChangeLog:10 >> + not support os_log_info my hope is that if we set all the callee > > please add a comma after "os_log_info". Fixed. >> Source/JavaScriptCore/heap/MachineStackMarker.cpp:103 >> + ( \ > > I suggest moving the ( after volatile. Changed. >> Source/JavaScriptCore/heap/MachineStackMarker.cpp:111 >> + : "%rbx", "%r12", "%r13", "%r14", "%r15" \ > > why indent by 5 spaces? I suggest indenting by 3 more to make this 8. Changed. >> Source/JavaScriptCore/heap/MachineStackMarker.cpp:112 >> + ); > > I suggest changing indentation here to 4 instead of 5. Changed. >> Source/JavaScriptCore/heap/MachineStackMarker.cpp:131 >> +bool truth = true; > > Remove. Whoops, I deleted this locally but I guess that didn't get updated. >> Source/JavaScriptCore/heap/MachineStackMarker.cpp:137 >> +#if CPU(X86_64) && OS(DARWIN) > > nit: I would move this #if + #endif up around the definition of FILL_CALLEE_SAVES_FOR_CRASH_INFO instead of duplicating it here for all 3 instances. Also provide a stubbed FILL_CALLEE_SAVES_FOR_CRASH_INFO for the #else case there. Changed. >> Source/JavaScriptCore/heap/MachineStackMarker.cpp:138 >> + FILL_CALLEE_SAVES_FOR_CRASH_INFO(1); > > I suggest passing "01" instead of 1. This way, the value you'll store is $0xc0defefe00000001 (16 digits) instead of $0xc0defefe0000001 (15 digits). Changed. >> Source/JavaScriptCore/heap/MachineStackMarker.cpp:146 >> + CRASH(); > > Remove. Done.
Created attachment 313000 [details] Patch for landing
Committed r218350: <http://trac.webkit.org/changeset/218350>