[webkit-reviews] review granted: [Bug 173427] Add logging to MachineStackMarker to try to diagnose crashes in the wild : [Attachment 312998] Patch

[bugzilla-daemon at webkit.org]

Mark Lam <mark.lam at apple.com> has granted Keith Miller
<keith_miller at apple.com>'s request for review:
Bug 173427: Add logging to MachineStackMarker to try to diagnose crashes in the
wild
https://bugs.webkit.org/show_bug.cgi?id=173427

Attachment 312998: Patch

https://bugs.webkit.org/attachment.cgi?id=312998&action=review




--- Comment #4 from Mark Lam <mark.lam at apple.com> ---
Comment on attachment 312998
  --> https://bugs.webkit.org/attachment.cgi?id=312998
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".