[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