Bug 183391 – Emit code to zero the stack frame on function entry

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Michael Saboff]

...

[Michael Saboff]

<rdar://problem/38203302>

[Michael Saboff]

Created attachment 335165 [details]
Patch

[EWS Watchlist]

Attachment 335165 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/jit/AssemblyHelpers.h:461:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
ERROR: Source/JavaScriptCore/jit/AssemblyHelpers.h:472:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 2 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Mark Lam]

Comment on attachment 335165 [details]
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.

[EWS Watchlist]

Comment on attachment 335165 [details]
Patch

Attachment 335165 [details] did not pass win-ews (win):
Output: http://webkit-queues.webkit.org/results/6835528

New failing tests:
http/wpt/resource-timing/rt-initiatorType-media.html

[EWS Watchlist]

Created attachment 335173 [details]
Archive of layout-test-results from ews205 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews205  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit

[Michael Saboff]

Comment on attachment 335165 [details]
Patch

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

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

Change the second arg to framePointerRegister.  The ternary form uses lea on X86, while the binary form uses addq.  According to Intel lea is preferred.

>> Source/JavaScriptCore/dfg/DFGThunks.cpp:127
>> +
> 
> There's no other change in this file.  I suggest removing this.

Done.

>> Source/JavaScriptCore/jit/JIT.cpp:691
>> +
> 
> 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.

Fixed

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

Fixed.

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

Fixed.

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

Fixed.

>> Source/JavaScriptCore/yarr/YarrJIT.cpp:626
>> +                if (callFrameSize <= 128) {
> 
> Please add an assert above this that (callFrameSize % stackAlignmentBytes() == 0).

I refactored to move sp down by alignCallFrameSizeInBytes(callFrameSize), thus eliminating the need for the ASSERT.

>> Source/JavaScriptCore/yarr/YarrJIT.cpp:629
>> +                    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.

Done.

>> Source/JavaScriptCore/yarr/YarrJIT.cpp:640
>>              subPtr(Imm32(alignCallFrameSizeInBytes(callFrameSize)), stackPointerRegister);
> 
> Might as well indent this by 4 for better readability.

Moved to immediately after the if (callFrameSize)

[Michael Saboff]

Comment on attachment 335165 [details]
Patch

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

>>> Source/JavaScriptCore/yarr/YarrJIT.cpp:629
>>> +                    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.
> 
> Done.

I hoisted moving the stack pointer down to before the frame size if, saving the previous sp in a temp.  I then use that temp for the base register.

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

Done when I hoisted the setting sp for before the if.

>>> Source/JavaScriptCore/yarr/YarrJIT.cpp:640
>>>              subPtr(Imm32(alignCallFrameSizeInBytes(callFrameSize)), stackPointerRegister);
>> 
>> Might as well indent this by 4 for better readability.
> 
> Moved to immediately after the if (callFrameSize)

I ended up indenting the subPtr().

[Michael Saboff]

Created attachment 335374 [details]
Patch for landing after make suggested changes

[Michael Saboff]

Committed r229444: <https://trac.webkit.org/changeset/229444>

[Saam Barati]

Was this perf neutral?