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

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, enhancement)

Description

[Jan de Mooij [:jandem]]

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..

Comment 1

[André Bargull [:anba]]

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();
---

Comment 2

[Jan de Mooij [:jandem]]

I should probably just fix this (later today) instead of rewriting the test I wrote in the other bug..

Comment 3

[Jan de Mooij [:jandem]]

Attachment: Patch

I added the new.target != callee check to makeInliningDecision so it works for both native functions and inlining of Class hooks (inlineNonFunctionCall).

Comment 4

[André Bargull [:anba]]

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?

Comment 5

[Jan de Mooij [:jandem]]

How do you feel about moving the check to IonBuilder::inlineNativeCall and IonBuilder::inlineNonFunctionCall? I can do that if you prefer it.

Comment 6

[André Bargull [:anba]]

That sounds good to me, because it makes it easier to spot when we reject to inline a native resp. non-function constructor call.

Comment 7

[Jan de Mooij [:jandem]]

Attachment: Patch v2

Comment 8

[André Bargull [:anba]]

Comment on attachment 8989196 [details] [diff] [review]
Patch v2

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

Thanks!

Comment 9

[Pulsebot]

Pushed by [e-mail]:
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

Comment 10

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/83ca4ebfd22e