[webkit-reviews] review granted: [Bug 175617] Enhance MacroAssembler::probe() to support an initializeStackFunction callback. : [Attachment 318278] proposed patch.

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Wed Aug 16 13:12:29 PDT 2017


JF Bastien <jfbastien at apple.com> has granted Mark Lam <mark.lam at apple.com>'s
request for review:
Bug 175617: Enhance MacroAssembler::probe() to support an
initializeStackFunction callback.
https://bugs.webkit.org/show_bug.cgi?id=175617

Attachment 318278: proposed patch.

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




--- Comment #4 from JF Bastien <jfbastien at apple.com> ---
Comment on attachment 318278
  --> https://bugs.webkit.org/attachment.cgi?id=318278
proposed patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=318278&action=review

r=me


On x86-64 I think you should do a follow-up patch to avoid setting eflags with
pushf / popf. It's slow, and historically has been the source of some exploits.
I have code and benchmark for it here:
https://github.com/jfbastien/benchmark-x86-flags  I'd use seto+lahf / addb+sahf
instead.

> Source/JavaScriptCore/ChangeLog:60
> +	      we're moving the ProbeContext, we're always be moving it to a
lower address.

"we're always be moving it"
*we'll ?

> Source/JavaScriptCore/assembler/MacroAssemblerARM64.cpp:358
> +    // Initialize ProbeContex::initializeStackFunction to zero.

ProbeContext

> Source/JavaScriptCore/assembler/MacroAssemblerARM64.cpp:370
> +    "add	  x2, x27, #" STRINGIZE_VALUE_OF(PROBE_SIZE_PLUS_EXTRAS +
OUT_SIZE) "\n" // End of ProveContext + buffer.

ProbeContext

> Source/JavaScriptCore/assembler/MacroAssemblerARM64.cpp:376
> +    "bic	  x1, x1, #0xf" "\n" // The ARM EABI specifies that the stack
needs to be 16 byte aligned.

IIUC x1 is:
RESULT_SP - (PROBE_SIZE_PLUS_EXTRA + OUT_SIZE)
Right?

So why do you need to align it? It seems like OUT_SIZE is the only thing that
could force misalignment, no? Why not assert it's always a value that won't
mis-align SP?

> Source/JavaScriptCore/assembler/MacroAssemblerARM64.cpp:377
> +    "mov	  sp, x1" "\n" // Set the new sp to protect that memory from
interrupts before we copy the ProbeContext.

I think you should also set FP here, so that backtraces will look fine.

> Source/JavaScriptCore/assembler/MacroAssemblerARM.cpp:264
>      "bic	  r3, r3, #0xf" "\n"

Ditto on mask.

> Source/JavaScriptCore/assembler/MacroAssemblerARM.cpp:265
> +    "mov	  sp, r3" "\n" // Set the sp to protect the ProbeContext from
interrupts before we initialize it.

Ditto on FP.

> Source/JavaScriptCore/assembler/MacroAssemblerARM.cpp:292
> +    "vstmia.64 ip!, { d16-d31 }" "\n"

Wait... The previous code was just wrong, lost half the values, overwrote the
other half, and kept garbage?

> Source/JavaScriptCore/assembler/MacroAssemblerARM.cpp:296
> +    // Initialize ProbeContex::initializeStackFunction to zero.

ProbeContext

> Source/JavaScriptCore/assembler/MacroAssemblerARM.cpp:314
> +    "bic	  r1, r1, #0xf" "\n" // The ARM EABI specifies that the stack
needs to be 16 byte aligned.

Ditto

> Source/JavaScriptCore/assembler/MacroAssemblerARM.cpp:328
> +    "str	  r4, [r6], #4" "\n"

You could use load / store pair here, but I'm not sure it's worth sweating the
details on ARM32. You could also use load / store multiple to avoid looping
entirely and have less code, but it's not necessarily faster.

> Source/JavaScriptCore/assembler/MacroAssemblerARMv7.cpp:235
> +    // Initialize ProbeContex::initializeStackFunction to zero.

ProbeContext

> Source/JavaScriptCore/assembler/MacroAssemblerARMv7.cpp:254
> +    "bic	  r1, r1, #0xf" "\n" // The ARM EABI specifies that the stack
needs to be 16 byte aligned.

Ditto


More information about the webkit-reviews mailing list