BaselineCompiler: Compile JSOP_THIS

RESOLVED FIXED

Status

()

RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: djvj, Unassigned)

Tracking

(Blocks: 1 bug)

17 Branch
x86
Mac OS X
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

6 years ago
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.
(Reporter)

Comment 1

6 years ago
Created attachment 689381 [details] [diff] [review]
Handle JSOP_THIS (wip)

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.
Attachment #689381 - Flags: review?(jdemooij)
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().
Attachment #689381 - Flags: review?(jdemooij)
(Reporter)

Updated

6 years ago
Blocks: 805868
No longer depends on: 805868
(Reporter)

Updated

6 years ago
Depends on: 819393
(Reporter)

Comment 3

6 years ago
(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.
(Reporter)

Comment 4

6 years ago
Created attachment 689894 [details] [diff] [review]
Handle JSOP_THIS

Revised patch with comments addressed.  Should be good to go.
Attachment #689381 - Attachment is obsolete: true
Attachment #689894 - Flags: review?(jdemooij)
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
Attachment #689894 - Flags: review?(jdemooij) → review+
(Reporter)

Comment 6

6 years ago
(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
(Reporter)

Updated

6 years ago
Depends on: 820084
(Reporter)

Updated

6 years ago
Depends on: 820152
(Reporter)

Updated

6 years ago
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.