Add support for JSOP_THIS. Handling is basically the same as for JSOP_GETARG - we just push a special stackval and tell the BaselineFrameInfo how to deal with it.

This gets us basically there, save for one bug I'll have to take a look at tomorrow. Putting up the patch anyway so that review can progress in the meantime. JSOP_THIS is simple for strict-mode and self-hosted functions: we just push the thisvalue directly. Otherwise, we do an inline test for whether |thisv| is an object, and if so, just return the thisvalue directly. If not, we enter the ICThis_Fallback stub, passing |thisv| in R0 and a pointer to the frame's CalleeToken in R1.scratchReg(). The fallback handler performs the object materialization, and returns. The mainline code then overwrites the |thisv| in the frame with the newly materialized object and proceeds. The bug I'm running into relates to the behaviour of this code on top-level uses of |this|. The following top-level code: print(this); Has the following output: [BaselineScripts] Baseline compiling script /tmp/foo.js:1 (0x7fb474810160) [BaselineOp] Compiling op: callgname [BaselineOp] Compiling op: undefined [BaselineOp] Compiling op: notearg [BaselineOp] Compiling op: this [BaselineOp] Compiling op: notearg [BaselineOp] Compiling op: call [BaselineOp] Compiling op: pop [BaselineOp] Compiling op: stop [BaselineScripts] Created BaselineScript 0x1552ce0 (raw 0x7fb476345d80) for /tmp/foo.js:1 6.95320022781746e-310 Cearly we're doing some reification of some random stack contents for |thisv|, which is showing up as double. Need to track this down. Otherwise seems to work well. To accomodate this code, BaselineCompiler has been extended to hold a JSFunction pointer.

Comment on attachment 689381 [details] [diff] [review] Handle JSOP_THIS (wip) Review of attachment 689381 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/ion/BaselineCompiler.cpp @@ +482,5 @@ > + // Keep this value in R0 > + frame.pushThis(); > + > + // In strict mode function or self-hosted function, |this| is left alone. > + if (function && (function->inStrictMode() || function->isSelfHostedBuiltin())) Nit: looking at JM, you can also return early if we are not in a function, since this is always an object. ::: js/src/ion/BaselineIC.cpp @@ +416,5 @@ > + * Check for SynthesizeFrame poisoning and fast constructors which > + * didn't check their callee properly. > + */ > + if (thisv.isNullOrUndefined()) { > + Rooted<GlobalObject*> global(cx, NULL); I think we should refactor BoxNonStrictThis on m-c so that we can just call it here, so that we don't have to duplicate its logic. A signature like the following should work. bool BoxNonStrictThis(JSContext *cx, MutableHandleValue thisv); Also note that we can get the global from cx->global() now. ::: js/src/ion/shared/BaselineCompiler-shared.h @@ +19,5 @@ > class BaselineCompilerShared > { > protected: > + JSContext * cx; > + HeapPtrScript script; Good catch. This should be RootedScript though since BaselineCompiler is always stack-allocated. @@ +20,5 @@ > { > protected: > + JSContext * cx; > + HeapPtrScript script; > + HeapPtrFunction function; We don't have to store the function explicitly, you can get it from script->function().

(In reply to Jan de Mooij [:jandem] from comment #2) > Comment on attachment 689381 [details] [diff] [review] > Handle JSOP_THIS (wip) > > Review of attachment 689381 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: js/src/ion/BaselineCompiler.cpp > @@ +482,5 @@ > > + // Keep this value in R0 > > + frame.pushThis(); > > + > > + // In strict mode function or self-hosted function, |this| is left alone. > > + if (function && (function->inStrictMode() || function->isSelfHostedBuiltin())) > > Nit: looking at JM, you can also return early if we are not in a function, > since this is always an object. Fun fact: enterJIT IonCode for top-level scripts seems to be broken on the |this| front. Breakpointing at the beginning of the baseline jitcode prologue and inspecting the stack shows that the the |thisv| is just bogus. The value at the location is something like: 0x00007fffffffdd0c (on x64). Seems to be pointerish to something on stack. Ion never sees this problem because it refuses to compile JSOP_THIS in scripts without functions.

Revised patch with comments addressed. Should be good to go.

Comment on attachment 689894 [details] [diff] [review] Handle JSOP_THIS Review of attachment 689894 [details] [diff] [review]: ----------------------------------------------------------------- Looks good. I think the problem with top-level scripts is that EnterBaseline doesn't pass the "this" Value to EnterJIT. Should be easy to fix, but can be a follow-up patch. ::: js/src/ion/BaselineCompiler.cpp @@ +509,5 @@ > + if (!addICLoadLabel(patchOffset)) > + return false; > + > + // Overwrite |this| value with the result of the IC, but only if we're not > + // compiling for a top-level script. Nit: if we are compiling a top-level script we shouldn't get here due to the !function() test above. ::: js/src/ion/BaselineCompiler.h @@ +103,5 @@ > class BaselineCompiler : public BaselineCompilerSpecific > { > FixedList<Label> labels_; > HeapLabel *return_; > + bool aborted_; Nit: unused and the baseline compiler shouldn't have to abort. ::: js/src/ion/BaselineIC.h @@ +988,5 @@ > + ICThis_Fallback(IonCode *stubCode) > + : ICFallbackStub(ICStub::This_Fallback, stubCode) {} > + > + public: > + static const uint32_t MAX_OPTIMIZED_STUBS = 8; Nit: don't need this. ::: js/src/ion/arm/IonFrames-arm.h @@ +68,5 @@ > return calleeToken_; > } > + void replaceCalleeToken(void *calleeToken) { > + calleeToken_ = calleeToken; > + } Nit: can remove this method

(In reply to Jan de Mooij [:jandem] from comment #5) > ::: js/src/ion/arm/IonFrames-arm.h > @@ +68,5 @@ > > return calleeToken_; > > } > > + void replaceCalleeToken(void *calleeToken) { > > + calleeToken_ = calleeToken; > > + } > > Nit: can remove this method replaceCalleeToken is not a new method, it's just being moved up a bit within the class definition. It's used by Ion bailout code. Other changes made and pushed: https://hg.mozilla.org/projects/ionmonkey/rev/fd94cdea9dad