Bug 173427 – Add logging to MachineStackMarker to try to diagnose crashes in the wild

• Component: New Bugs
• Status: RESOLVED FIXED

[Keith Miller]

Add logging to MachineStackMarker to try to diagnose crashes in the wild

[Radar WebKit Bug Importer]

<rdar://problem/32797302>

[Keith Miller]

Created attachment 312998 [details]
Patch

[Build Bot]

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.

[Mark Lam]

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".

[Mark Lam]

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.

[Keith Miller]

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.

[Keith Miller]

Created attachment 313000 [details]
Patch for landing

[Keith Miller]

Committed r218350: <http://trac.webkit.org/changeset/218350>