609502 - TM: "Assertion failure: hasInt32Repr(*vp)"

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Paul Biggar]

Attachment: test case - causes segfault

$ cat ./newbug.js 
for (var i = 0; i < 10; i++)
{
    var y = new Array(4096*1024*1024-1).length;
}

 $ ./build_fix_582161_DBG.OBJ/js -j ./newbug.js 
Assertion failure: hasInt32Repr(*vp), at ../jstracer.cpp:3966
Segmentation fault

Comment 1

[Jan de Mooij [:jandem]]

Another test case, same assertion failure:

for(var i=0; i<20; i++) {
    x = ''.charCodeAt(NaN);
}

Comment 2

[Nicholas Nethercote [inactive]]

I'll take a look at this early next week.

Comment 3

[Nicholas Nethercote [inactive]]

pbiggar asked to take this one.

FWIW, the program in comment 0 doesn't cause the assertion for me, but the one in comment 1 does.

Comment 5

[Paul Biggar]

(In reply to comment #3)
> pbiggar asked to take this one.
> 
> FWIW, the program in comment 0 doesn't cause the assertion for me, but the one
> in comment 1 does.

comment 0's testcase no longer fails since I updated to tip. The testcase from 616715 still fails:

{
    function a() {}
}
Math.floor(Math.d)
function c() {}
c()
for each(let b in [0, 0, 0, 0, 0, 0, 0, -2147483648]) {
    print(Math.abs(b))
}

Comment 6

[Paul Biggar]

comment 0's test case was fixed in bug https://bugzilla.mozilla.org/show_bug.cgi?id=563125.

Comment 7

[Paul Biggar]

Attachment: Fix for missing unsigned

Looks like all these bugs are from int/double conversions around the uint or int boundaries. I don't really know how these work, so going to spend a bit more time poking around and testing.

Comment 8

[Nicholas Nethercote [inactive]]

Comment on attachment 495763 [details] [diff] [review]
Fix for missing unsigned

>         v_ins = w.lduiObjPrivate(obj_ins);
>-        if (obj->getArrayLength() <= JSVAL_INT_MAX) {
>-            guard(true, w.leui(v_ins, w.immi(JSVAL_INT_MAX)), BRANCH_EXIT);
>-            v_ins = w.i2d(v_ins);
>-        } else {
>-            v_ins = w.ui2d(v_ins);
>-        }
>+        v_ins = w.ui2d(v_ins);

See bug 563125 comment 9 and comment 10 for why this is no good.  Similar problems may apply to your other changes.

Comment 9

[Paul Biggar]

(In reply to comment #8)
> Comment on attachment 495763 [details] [diff] [review]

> See bug 563125 comment 9 and comment 10 for why this is no good.  Similar
> problems may apply to your other changes.

I saw the comment, and was testing it out. What's the implication of isPromoteInt failing? Should I be looking to fix what dvander's talking about in bug 563125 comment 12?

Comment 10

[Nicholas Nethercote [inactive]]

(In reply to comment #9)
> (In reply to comment #8)
> > Comment on attachment 495763 [details] [diff] [review] [details]
> 
> > See bug 563125 comment 9 and comment 10 for why this is no good.  Similar
> > problems may apply to your other changes.
> 
> I saw the comment, and was testing it out. What's the implication of
> isPromoteInt failing?

I think some numbers will end up being doubles instead of ints.  You should run with TMFLAGS=afterdce to see what the LIR looks like before and after the change.
(That's a good idea in general:  http://blog.mozilla.com/nnethercote/2010/01/21/safer-refactoring-in-nanojit/)

> Should I be looking to fix what dvander's talking about
> in bug 563125 comment 12?

Maybe?  Not sure.

Comment 12

[Paul Biggar]

Attachment: Handle number edges cases in tracer

Ostensibly, this looks like just adding the two edge cases and not handling the more general problem, but I think it's the right solution. In the first case:

-                if (IsPromoteInt(a)) {
+                if (IsPromoteInt(a) && vp[2].toNumber() != INT_MIN) {

INT_MIN is the only value which can't be converted to a signed integer (we're in Math.abs).

In the second case:

                     jsdouble i = vp[2].toNumber();
+                    if (JSDOUBLE_IS_NaN(i))
+                      i = 0;
                     if (i < 0 || i >= str->length())
                         RETURN_STOP("charCodeAt out of bounds");


NaN is the only number which doesn't compare correctly (it may be more correct to use js_DoubleToInteger, I'm not sure).

Comment 13

[Nicholas Nethercote [inactive]]

Comment on attachment 496827 [details] [diff] [review]
Handle number edges cases in tracer

>             } else if (native == js_math_abs) {
>                 LIns* a = get(&vp[2]);
>-                if (IsPromoteInt(a)) {
>+                if (IsPromoteInt(a) && vp[2].toNumber() != INT_MIN) {
>                     a = w.demote(a);
>                     /* abs(INT_MIN) can't be done using integers;  exit if we see it. */
>                     LIns* intMin_ins = w.name(w.immi(0x80000000), "INT_MIN");
>                     LIns* isIntMin_ins = w.name(w.eqi(a, intMin_ins), "isIntMin");
>                     guard(false, isIntMin_ins, MISMATCH_EXIT);

My immediate thought was:  oh, you can't do that, you're conflating record-time and run-time -- the number might be INT_MIN at record-time but it could be something else during subsequent runs of the compiled code.  But then, the guard handles that case and you're just trying to prevent the assertion at record-time.  (There's a similar thing in the charCodeAt case, where the run-time test is generated by makeNumberInt32.)

So I think this is ok.  But it's odd, and not at all clear just by reading the code why the record-time test is necessary when the run-time test is also there, so I'm asking Gal to check as well.


>                     LIns* neg_ins = w.negi(a);
>                     LIns* isNeg_ins = w.name(w.ltiN(a, 0), "isNeg");
>                     LIns* abs_ins = w.name(w.cmovi(isNeg_ins, neg_ins, a), "abs");

>@@ -11199,16 +11199,18 @@ TraceRecorder::callNative(uintN argc, JS
>                     LIns* idx_ins = get(&vp[2]);
>                     LIns* char_ins;
>                     CHECK_STATUS(getCharAt(str, str_ins, idx_ins, mode, &char_ins));
>                     set(&vp[0], char_ins);
>                     pendingSpecializedNative = IGNORE_NATIVE_CALL_COMPLETE_CALLBACK;
>                     return RECORD_CONTINUE;
>                 } else if (native == js_str_charCodeAt) {
>                     jsdouble i = vp[2].toNumber();
>+                    if (JSDOUBLE_IS_NaN(i))
>+                      i = 0;

You'll need to do the same thing for js_str_charAt, which is immediately above.

Comment 14

[Paul Biggar]

http://hg.mozilla.org/tracemonkey/rev/e5090c9f6be3


(In reply to comment #13)
> >                     LIns* neg_ins = w.negi(a);
> >                     LIns* isNeg_ins = w.name(w.ltiN(a, 0), "isNeg");
> >                     LIns* abs_ins = w.name(w.cmovi(isNeg_ins, neg_ins, a), "abs");
> 
> >@@ -11199,16 +11199,18 @@ TraceRecorder::callNative(uintN argc, JS
> >                     LIns* idx_ins = get(&vp[2]);
> >                     LIns* char_ins;
> >                     CHECK_STATUS(getCharAt(str, str_ins, idx_ins, mode, &char_ins));
> >                     set(&vp[0], char_ins);
> >                     pendingSpecializedNative = IGNORE_NATIVE_CALL_COMPLETE_CALLBACK;
> >                     return RECORD_CONTINUE;
> >                 } else if (native == js_str_charCodeAt) {
> >                     jsdouble i = vp[2].toNumber();
> >+                    if (JSDOUBLE_IS_NaN(i))
> >+                      i = 0;
> 
> You'll need to do the same thing for js_str_charAt, which is immediately above.


OK. I didn't initially because I couldn't write a test case which caused a crash. But you're right, it clearly should be handled with the same code, even if it's already magically handled somehow already. I added a test, and the code copy.

Comment 15

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/e5090c9f6be3