[webkit-reviews] review granted: [Bug 183391] Emit code to zero the stack frame on function entry : [Attachment 335165] Patch

[bugzilla-daemon at webkit.org]

Mark Lam <mark.lam at apple.com> has granted Michael Saboff <msaboff at apple.com>'s
request for review:
Bug 183391: Emit code to zero the stack frame on function entry
https://bugs.webkit.org/show_bug.cgi?id=183391

Attachment 335165: Patch

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




--- Comment #4 from Mark Lam <mark.lam at apple.com> ---
Comment on attachment 335165
  --> https://bugs.webkit.org/attachment.cgi?id=335165
Patch

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

r=me with issues addressed.

> Source/JavaScriptCore/ChangeLog:9
> +	   The default setting of the aoption is off.

typo: /aoption/option/

> Source/JavaScriptCore/b3/air/AirCode.cpp:46
> +	   jit.addPtr(MacroAssembler::TrustedImm32(-code.frameSize()),
MacroAssembler::stackPointerRegister, MacroAssembler::stackPointerRegister);

Did you mean for the second arg to be MacroAssembler::framePointerRegister?  I
think at this point right after the prologue, cfr and sp have the same values. 
So, it doesn't really matter, but you might as well be consistent with the rest
of your code below.

On the other hand, would using the ternary form result in code generated on
X86_64 to first move cfr into sp, followed by a binary instruction to add
-code.frameSize() to sp?  If so, keeping the original binary form would be more
efficient.  Ditto for all the case below as well.

> Source/JavaScriptCore/dfg/DFGThunks.cpp:127
> +

There's no other change in this file.  I suggest removing this.

> Source/JavaScriptCore/jit/JIT.cpp:691
> +    if (Options::zeroStackFrame())
> +	   clearStackFrame(stackPointerRegister, regT1, regT0, maxFrameSize);
> +

You should do this clearing after setting the sp below (clear from cfr to sp
like you do elsewhere).  Otherwise, an interrupt frame can come on and
overwrite the zeros with values.

> Source/JavaScriptCore/llint/LowLevelInterpreter.asm:1090
> +	   move t0, t2

I think this should be "move cfr, t2".	Moving t0 means t2 has the same value
as sp, and the loop below will never execute.

> Source/JavaScriptCore/llint/LowLevelInterpreter.asm:1093
> +	   subp PtrSize, [t2]

This should be "subp PtrSize, t2".  You want to decrement t2, not what it
points to.

> Source/JavaScriptCore/llint/LowLevelInterpreter.asm:1094
> +	   storep 0, t2

This should be "storep 0, [t2]".  You want to clear the stack location, not the
pointer.

> Source/JavaScriptCore/yarr/YarrJIT.cpp:626
> +		   if (callFrameSize <= 128) {

Please add an assert above this that (callFrameSize % stackAlignmentBytes() ==
0).

> Source/JavaScriptCore/yarr/YarrJIT.cpp:629
> +		       for (unsigned offset = 0; offset < callFrameSize; offset
+= sizeof(intptr_t))
> +			   storePtr(TrustedImm32(0),
Address(stackPointerRegister, -8 - offset));
> +		       subPtr(Imm32(alignCallFrameSizeInBytes(callFrameSize)),
stackPointerRegister);

I think you should adjust sp first and store 0s using a positive offset from sp
... because of potential interrupt stack activity.

> Source/JavaScriptCore/yarr/YarrJIT.cpp:632
> +		      
addPtr(TrustedImm32(-alignCallFrameSizeInBytes(callFrameSize)),
stackPointerRegister, stackPointerRegister);

Use binary form?

> Source/JavaScriptCore/yarr/YarrJIT.cpp:640
>	       subPtr(Imm32(alignCallFrameSizeInBytes(callFrameSize)),
stackPointerRegister);

Might as well indent this by 4 for better readability.