1233176 - Scalar Replacement of the scope chain does not restore constant pre-init value.

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Nicolas B. Pierron [:nbp]]

Bug 1143758 added a test case which fails when branch pruning is enabled by default.  My guess is that the problem comes from Scalar Replacement of Objects, which remove the allocation of the scope chain.

The problem is that the scope chain produced by scalar replacement of object contains Undefined values in all properties instead of the pre-initialized value for constants.

Thus, in the following test case (which does not require branch pruning), we can observe the effect, that the first g() call inisde the if branch unexpectedly succeed, while it fails on the next iteration:


var uceFault = function (i) {
    if (i == 1500)
        uceFault = function (i) { return true; };
    return false;
};

function f(i) {
    if (uceFault(i) || uceFault(i))
        g();
    const x = 42;
    function g() {
        return x;
    }
    return g;
}

var caught = false;
var i;
try {
    for (i = 0; i < 2000; i++)
        assertEq(f(i)(), 42);
} catch(e) {
    assertEq(e instanceof ReferenceError, true);
    assertEq(i, 1500);
    caught = true;
}
assertEq(caught, true);



ion/lexical-check-2.1.js:27:5 Error: Assertion failed: got 1501, expected 1500

This test case is similar to ion/lexical-check-2.js test case.

Comment 1

[Nicolas B. Pierron [:nbp]]

Attachment: Scalar Replacement: Initialize properties with the default value of the template object.

Comment 2

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8699458 [details] [diff] [review]
Scalar Replacement: Initialize properties with the default value of the template object.

Running the test suite highlighted a few other issues related to the fact that we have objects in the template object.

Comment 3

[Nicolas B. Pierron [:nbp]]

Attachment: Scalar Replacement: Initialize properties with the default value of the template object.

Fix the object allocation, to replicate what is currently done with the
template object allocation.

Comment 4

[Shu-yu Guo [:shu]]

Comment on attachment 8699495 [details] [diff] [review]
Scalar Replacement: Initialize properties with the default value of the template object.

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

Thanks for the fix!

::: js/src/jit-test/tests/ion/lexical-check-2.js
@@ +1,4 @@
> +// This function uses UCE to test when the if branch is removed by
> +// IonMonkey.  Some optimization such as Scalar Replacement are able to remove
> +// the scope chain, which can cause issues when the scope chain properties are
> +// not initialized properly.

This seems like a different test. Worth keeping lexical-check-2 as is and putting this as lexical-check-6 or something?

::: js/src/jit/MIR.cpp
@@ +4080,5 @@
> +    if (templateObject->is<UnboxedPlainObject>()) {
> +        UnboxedPlainObject& unboxedObject = templateObject->as<UnboxedPlainObject>();
> +        const UnboxedLayout& layout = unboxedObject.layoutDontCheckGeneration();
> +
> +        // 0 is used as an error code.

I don't understand this comment.

Comment 5

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8699495 [details] [diff] [review]
Scalar Replacement: Initialize properties with the default value of the template object.

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

::: js/src/jit/MIR.cpp
@@ +4080,5 @@
> +    if (templateObject->is<UnboxedPlainObject>()) {
> +        UnboxedPlainObject& unboxedObject = templateObject->as<UnboxedPlainObject>();
> +        const UnboxedLayout& layout = unboxedObject.layoutDontCheckGeneration();
> +
> +        // 0 is used as an error code.

I have no idea from where this comment comes from.
I'll remove it.

Comment 6

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5a2ba7a9b571

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/5a2ba7a9b571