Closed Bug 1472132 Opened 2 years ago Closed 2 years ago

|new Array| inlining in Ion needs to check new.target

Categories

(Core :: JavaScript Engine: JIT, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla63
Tracking Status
firefox63 --- fixed

People

(Reporter: jandem, Assigned: jandem)

Details

Attachments

(1 file, 1 obsolete file)

The test below fails with --no-threads:

test.js:4:9 Error: Assertion failed: got [], expected ({})

--
function f() {
    for (var i = 0; i < 20000; i++) {
        var a = Reflect.construct(Array, [], Object);
        assertEq(a.__proto__, Object.prototype);
    }
}
f();
--

Adding this to IonBuilder::inlineArray fixes it:

    if (callInfo.constructing() && callInfo.getNewTarget() != callInfo.fun())
        return InliningStatus_NotInlined;

We probably want to add a check like that in the caller so it works for all natives we inline..
Similar issue when inlining String:
---
function f() {
    for (var i = 0; i < 20000; i++) {
        var a = Reflect.construct(String, [""], Object);
        assertEq(a.__proto__, Object.prototype);
    }
}
f();
---


And TypedArrays:
---
function f() {
    for (var i = 0; i < 20000; i++) {
        var a = Reflect.construct(Int32Array, [0], Object);
        assertEq(a.__proto__, Object.prototype);
    }
}
f();
---
I should probably just fix this (later today) instead of rewriting the test I wrote in the other bug..
Flags: needinfo?(jdemooij)
Attached patch Patch (obsolete) — Splinter Review
I added the new.target != callee check to makeInliningDecision so it works for both native functions and inlining of Class hooks (inlineNonFunctionCall).
Assignee: nobody → jdemooij
Status: NEW → ASSIGNED
Flags: needinfo?(jdemooij)
Attachment #8989042 - Flags: review?(andrebargull)
Comment on attachment 8989042 [details] [diff] [review]
Patch

Review of attachment 8989042 [details] [diff] [review]:
-----------------------------------------------------------------

I guess that works, but I'd expected that MCallOptimize rejects inlining when |new.target| doesn't match the expected value instead of making that decision at an earlier level. Well, if this approach turns out to be too coarse grained we can still change it in the future.

::: js/src/jit/IonBuilder.cpp
@@ +4004,5 @@
> +        callInfo.getNewTarget() != callInfo.fun() &&
> +        (!targetArg->is<JSFunction>() ||
> +         !targetArg->as<JSFunction>().isInterpreted()))
> +    {
> +        return InliningDecision_DontInline;

Does it make sense to call |trackOptimizationOutcome| and/or |DontInline| here to track the inlining failure?
Attachment #8989042 - Flags: review?(andrebargull) → review+
How do you feel about moving the check to IonBuilder::inlineNativeCall and IonBuilder::inlineNonFunctionCall? I can do that if you prefer it.
That sounds good to me, because it makes it easier to spot when we reject to inline a native resp. non-function constructor call.
Attached patch Patch v2Splinter Review
Attachment #8989042 - Attachment is obsolete: true
Attachment #8989196 - Flags: review?(andrebargull)
Comment on attachment 8989196 [details] [diff] [review]
Patch v2

Review of attachment 8989196 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks!
Attachment #8989196 - Flags: review?(andrebargull) → review+
Pushed by jandemooij@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/83ca4ebfd22e
Don't inline non-scripted functions in Ion when constructing and new.target != the callee. r=anba
https://hg.mozilla.org/mozilla-central/rev/83ca4ebfd22e
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
You need to log in before you can comment on or make changes to this bug.