Bug 126393 – CStack: Get the C Loop LLINT to build again.

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

[Mark Lam]

As a first step towards resurrecting the C Loop LLINT, let’s get it to build again.  The goal here is simply to resolve build issues, and not necessarily to get it to run yet.

[Mark Lam]

Created attachment 220241 [details]
the patch.

[Mark Lam]

Comment on attachment 220241 [details]
the patch.

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

> Source/JavaScriptCore/llint/LLIntCLoop.h:44
> +    static JSValue execute(OpcodeID entryOpcodeID, void* executableAddress, VM*, ProtoCallFrame*, bool isInitializationPass = false);    

Will remove excess spaces at the end before landing.

[Mark Lam]

Landed in r161219 on the jsCStack branch: <http://trac.webkit.org/r161219>.

[Michael Saboff]

Comment on attachment 220241 [details]
the patch.

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

r-

It seems to me that callToJavaScript should be calling the C Loop interpreter and not the other way around.  By having CLoop::execute() call into callToJavaScript (renamed for the CLoop in this patch to _llint_call_to_javascript) the CLoop is diverging from the LLInt.  CLoop::execute() is the entry point for the C Loop LLInt and that is what callToJavaScript should call.

This patch also breaks 4 tests when compiled X86-64:
** The following JSC stress test failures have been introduced:
	jsc-layout-tests.yaml/js/script-tests/function-apply-aliased.js.layout
	jsc-layout-tests.yaml/js/script-tests/function-apply-aliased.js.layout-no-llint
	jsc-layout-tests.yaml/js/script-tests/function-apply-aliased.js.layout-no-cjit
	jsc-layout-tests.yaml/js/script-tests/function-apply-aliased.js.layout-dfg-eager-no-cjit

> Source/JavaScriptCore/llint/LLIntThunks.cpp:97
> +EncodedJSValue callToJavaScript(void* executableAddress, VM* vm, ProtoCallFrame* protoCallFrame)

This should be renamed to cLoopCallToJavaScript() or callToJavaScriptCLoop() and don't rename callToJavaScript().

> Source/JavaScriptCore/llint/LLIntThunks.cpp:103
> +EncodedJSValue callToNativeFunction(void* executableAddress, VM* vm, ProtoCallFrame* protoCallFrame)

Same comment as callToJavaScript above.  Rename this function not the one in LowLevelInterpreter.asm

> Source/JavaScriptCore/llint/LowLevelInterpreter.asm:475
> +_llint_call_to_javascript:

Why a different name for the C Loop?

> Source/JavaScriptCore/llint/LowLevelInterpreter.asm:483
> +_llint_call_to_native_function:

Ditto.

> Source/JavaScriptCore/llint/LowLevelInterpreter.cpp:247
> +JSValue CLoop::execute(OpcodeID entryOpcodeID, void* executableAddress, VM* vm, ProtoCallFrame* protoCallFrame, bool isInitializationPass)

I don't like that the C Loop interpreter uses the ProtoCallFrame.  We want it to use a CallFrame just like the LLInt and other engines.

> Source/JavaScriptCore/offlineasm/cloop.rb:74
> +            "t0"

Move the when "t0" to here as well.

> Source/JavaScriptCore/offlineasm/cloop.rb:75
>          when "a1"

Move the when "t1" to here as well.

[Mark Lam]

(In reply to comment #4)
> (From update of attachment 220241 [details])
> It seems to me that callToJavaScript should be calling the C Loop interpreter and not the other way around.  By having CLoop::execute() call into callToJavaScript (renamed for the CLoop in this patch to _llint_call_to_javascript) the CLoop is diverging from the LLInt.  CLoop::execute() is the entry point for the C Loop LLInt and that is what callToJavaScript should call.

The devil is in the details:

1. Interpreter.cpp and JITCode.cpp calls callToJavaScript and callToNativeFunction directly.  I can either turn callToJavaScript into C functions that know how to enter the C Loop LLINT, or I can add #if ENABLE(LLINT_C_LOOP) to all the places that currently expect callToJavaScript and callToNativeFunction to be call-able.  I opted for the former, which I think is cleaner and keep the LLINT implementation details (and I consider the C Loop LLINT part of the LLINT) in the LLINT.  I don’t like adding #if’s outside the LLINT for this if I can help it.  In the end, the decision is subjective.  Do you insist that I add #if ENABLE(LLINT_C_LOOP) in Interpreter.cpp and JITCode.cpp instead?

2. callToJavaScript does a lot of initialization work that I don’t want to duplicate in a C function.  This way, if we fix the code in LLINT assembly, it is fixed, and I won’t have to redo the work.  It also means that the CLoop LLINT will more closely match the semantics of the assembly LLINT, which is good for all sorts of reasons.

In order to execute the “assembly” code in callToJavaScript, that necessarily means that CLoop::execute() should be calling callToJavaScript instead of the other way around.  Note: the correct way to think of CLoop::execute() is that it is a CPU emulator that can execute all of the “assembly” code in the LowLevelInterpreter*.asm files. 


> This patch also breaks 4 tests when compiled X86-64:
> ** The following JSC stress test failures have been introduced:
>     jsc-layout-tests.yaml/js/script-tests/function-apply-aliased.js.layout
>     jsc-layout-tests.yaml/js/script-tests/function-apply-aliased.js.layout-no-llint
>     jsc-layout-tests.yaml/js/script-tests/function-apply-aliased.js.layout-no-cjit
>     jsc-layout-tests.yaml/js/script-tests/function-apply-aliased.js.layout-dfg-eager-no-cjit

The breakage is not due to this patch, though this patch did uncover the latent bug that caused it.  The breakage is fixed in https://bugs.webkit.org/show_bug.cgi?id=126405.

> > Source/JavaScriptCore/llint/LLIntThunks.cpp:97
> > +EncodedJSValue callToJavaScript(void* executableAddress, VM* vm, ProtoCallFrame* protoCallFrame)
> 
> This should be renamed to cLoopCallToJavaScript() or callToJavaScriptCLoop() and don't rename callToJavaScript().

See reason above.  I think we will want to preserve the semantics of being able to call callToJavaScript().  In order to do that, I need to rename it.

> > Source/JavaScriptCore/llint/LLIntThunks.cpp:103
> > +EncodedJSValue callToNativeFunction(void* executableAddress, VM* vm, ProtoCallFrame* protoCallFrame)
> 
> Same comment as callToJavaScript above.  Rename this function not the one in LowLevelInterpreter.asm

See reason above.  I think we will want to preserve the semantics of being able to call callToNativeFunction().  In order to do that, I need to rename it.
 
> > Source/JavaScriptCore/llint/LowLevelInterpreter.asm:475
> > +_llint_call_to_javascript:
> 
> Why a different name for the C Loop?

See reason above.  I think we will want to preserve the semantics of being able to call callToJavaScript().  In order to do that, I need to rename it.
 
> > Source/JavaScriptCore/llint/LowLevelInterpreter.asm:483
> > +_llint_call_to_native_function:
> 
> Ditto.

See reason above.  I think we will want to preserve the semantics of being able to call callToJavaScript().  In order to do that, I need to rename it.
 
> > Source/JavaScriptCore/llint/LowLevelInterpreter.cpp:247
> > +JSValue CLoop::execute(OpcodeID entryOpcodeID, void* executableAddress, VM* vm, ProtoCallFrame* protoCallFrame, bool isInitializationPass)
> 
> I don't like that the C Loop interpreter uses the ProtoCallFrame.  We want it to use a CallFrame just like the LLInt and other engines.

At the entry point to the LLINT and other engines, we pass in the ProtoCallFrame.  doCallToJavaScript then takes care of doing the stack checks and populating the CallFrame.  I am doing the exact same thing here.

Again, the goal is so that the C Loop LLINT works as much the same way as the assembly LLINT as possible.  In this case, I’m trying to make it use the code for doCallToJavaScript and not rewrite a mirrored version in C++.

> > Source/JavaScriptCore/offlineasm/cloop.rb:74
> > +            "t0"
> 
> Move the when "t0" to here as well.
> 
> > Source/JavaScriptCore/offlineasm/cloop.rb:75
> >          when "a1"
> 
> Move the when "t1" to here as well.

OK, will fix these.

[Mark Lam]

Created attachment 220259 [details]
follow up patch to address feedback for cloop.rb

[Mark Lam]

Thanks for the review.  Review status update and follow up patch landed in r161238: <http://trac.webkit.org/r161238>.