1175233 - RObjectState should convert to a native if the type of the value does not match.

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Nicolas B. Pierron [:nbp]]

This Bug is similar to Bug 1174322, except that Recover are expected to not be able to distinguish between Int32 types and Doubles types.

We should handle cases where the and Int32 operation is promoted into a Double operation.  A test case similar to the following should be able to trigger this issue:


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

function f(i) {
    var obj = {
      i: i,
      v: i + i
    };
    assertRecoveredOnBailout(obj, true);
    assertRecoveredOnBailout(obj.v, true);
    if (uceFault(i) || uceFault(i)) {
        // MObjectState::recover should neither fail,
        // nor coerce its result to an int32.
        assertEq(obj.v, 2 * i);
    }
    return 2 * obj.i;
}

var min = -100;
for (var j = min; j <= 31; ++j) {
    var i = (Math.pow(2, j) | 0;
    f(i);
}

Comment 1

[Nicolas B. Pierron [:nbp]]

Attachment: test case.

Comment 2

[Nicolas B. Pierron [:nbp]]

Attachment: RObjectState::recover: Handle cases where the property type does not match the recovered value.

Comment 3

[Brian Hackett [Laid off!]]

Comment on attachment 8623722 [details] [diff] [review]
RObjectState::recover: Handle cases where the property type does not match the recovered value.

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

::: js/src/jit/BaselineBailouts.cpp
@@ +1606,5 @@
> +
> +        JitSpew(JitSpew_BaselineBailouts, "Invalidating due to bounds check failure");
> +        if (!Invalidate(cx, outerScript))
> +            return false;
> +    }

Can you bundle up this logic into an InvalidateAfterBailout function?  That should be cleaner and will allow the new comment below to only appear in one place.

::: js/src/jit/Recover.cpp
@@ +1396,5 @@
>                  val.setBoolean(val.toInt32() != 0);
>  
> +            id = NameToId(properties[i].name);
> +            ObjectOpResult result;
> +            if (!js::SetProperty(cx, object, id, val, receiver, result))

no js:: needed

@@ +1399,5 @@
> +            ObjectOpResult result;
> +            if (!js::SetProperty(cx, object, id, val, receiver, result))
> +                return false;
> +            if (!result)
> +                return result.reportError(cx, object, id);

Maybe assert or comment here that the SetProperty call can only fail due to OOM.

Comment 4

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e613eb7895d6

Comment 5

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/e613eb7895d6