Bug 124634 – Introducing VMEntryScope to update the VM stack limit

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Mark Lam]

The VMEntryScope will also replace the DynamicGlobalObjectScope and take care of stack bounds checks at VM entry points (i.e. all the Interpreter::execute..() functions).  In the current implementation, the code that sets the stack limit is disabled until we actually move to using the C stack for JS frames.

[Mark Lam]

*** Bug 56291 has been marked as a duplicate of this bug. ***

[Mark Lam]

Created attachment 217386 [details]
the patch.

[Build Bot]

Comment on attachment 217386 [details]
the patch.

Attachment 217386 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/29828069

[EFL EWS Bot]

Comment on attachment 217386 [details]
the patch.

Attachment 217386 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/29848105

[EFL EWS Bot]

Comment on attachment 217386 [details]
the patch.

Attachment 217386 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/29768081

[Build Bot]

Comment on attachment 217386 [details]
the patch.

Attachment 217386 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/29768076

[Mark Lam]

Comment on attachment 217386 [details]
the patch.

Should have tested with a release build.  Will fix.

[Mark Lam]

Created attachment 217399 [details]
patch 2: fixed release build.

[EFL EWS Bot]

Comment on attachment 217399 [details]
patch 2: fixed release build.

Attachment 217399 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/30058020

[Mark Lam]

Created attachment 217455 [details]
patch 3: another try at fixing EWS issues.

[Geoffrey Garen]

Comment on attachment 217455 [details]
patch 3: another try at fixing EWS issues.

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

> Source/JavaScriptCore/interpreter/JSStackInlines.h:158
> +    m_vm.setStackLimit(reinterpret_cast<void*>(newEnd));

No need to reinterpret_cast to void*.

> Source/JavaScriptCore/runtime/VMEntryScope.cpp:42
> +#if ENABLE(ASSEMBLE)

Should be ASSEMBLER.

I like to use copy-and-paste for tasks like this, to avoid typos like this.

> Source/JavaScriptCore/runtime/VMEntryScope.h:43
> +    bool hasEnoughStackToEnterVM() const { return m_stackBounds.isSafeToRecurse(); }

You should remove VMStackBounds::isSafeToRecurse() and its uses, including this use. Plain comparison against m_stackBounds.stackLimit() needs to become the one true way to measure the extent of the stack. Otherwise, we allow subtle differences between the stack limit check in C++ and the same check in JIT code.

> Source/JavaScriptCore/runtime/VMEntryScope.h:52
> +    void* m_prevStackLimit;

This needs a comment explaining that the previous limit might belong to another thread's stack.

> Source/WebCore/bindings/js/JSCryptoAlgorithmBuilder.cpp:74
> +    ASSERT(vm.entryScope);

I don't think this ASSERT adds much. If it's false, we'll crash on the next  line anyway.

[Mark Lam]

(In reply to comment #11)
> > Source/JavaScriptCore/runtime/VMEntryScope.h:43
> > +    bool hasEnoughStackToEnterVM() const { return m_stackBounds.isSafeToRecurse(); }
> 
> You should remove VMStackBounds::isSafeToRecurse() and its uses, including this use. Plain comparison against m_stackBounds.stackLimit() needs to become the one true way to measure the extent of the stack. Otherwise, we allow subtle differences between the stack limit check in C++ and the same check in JIT code.

I can't do this.  The BytecodeGenerator, Parser, and StringRecursionChecker still relies on VMStackBounds::isSafeToRecurse().  So, instead, I opted to keep a unified implementation of how we do stack checks in VMStackBounds::isSafeToRecurse()  instead.

[Mark Lam]

(In reply to comment #12)
> (In reply to comment #11)
> > You should remove VMStackBounds::isSafeToRecurse() and its uses, including this use. Plain comparison against m_stackBounds.stackLimit() needs to become the one true way to measure the extent of the stack. Otherwise, we allow subtle differences between the stack limit check in C++ and the same check in JIT code.
> 
> I can't do this.  The BytecodeGenerator, Parser, and StringRecursionChecker still relies on VMStackBounds::isSafeToRecurse().  So, instead, I opted to keep a unified implementation of how we do stack checks in VMStackBounds::isSafeToRecurse()  instead.

Per our offline conversation, I'll re-implement the C++ code native stack check to be a comparison against the VM's stackLimit pointer.

The only added functionality that VMStackBounds add over the WTF::StackBounds is that it takes into consideration whether we're in the midst of handling a JS exception or not when determining how much stack capacity is required in the stack checks.  Revisiting the current users of VMStackBounds, I see no reason why this required stack capacity value should change since the last time the stackLimit was set in the VM.  Hence, these users should be able to do their stack checks against the VM's stackLimit pointer without any adjustments since the required stack capacity won't change in the period of their use.

With that, I ended up adding a VM::isSafeToRecurse() function, and removed VMStackBounds altogether.  VMEntryScope now only updates the VM stackLimit, but does not offer any mechanism to check it.  The only stack check mechanism for the native stack is now VM::isSafeToRecurse() which implements the pointer comparison against VM::m_stackLimit.

Updated patch coming soon.

[Mark Lam]

Created attachment 217503 [details]
patch 4: addresses Geoff's feedback.

[Geoffrey Garen]

Comment on attachment 217503 [details]
patch 4: addresses Geoff's feedback.

r=me

[Mark Lam]

Thanks for the review.  Landed in r159605: <http://trac.webkit.org/r159605>.