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

RESOLVED FIXED

Status

()

RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: paul.biggar, Assigned: paul.biggar)

Tracking

Trunk
x86
Mac OS X
Points:
---

Firefox Tracking Flags

(blocking2.0 betaN+)

Details

(Whiteboard: [fixed-in-tracemonkey])

Attachments

(3 attachments)

(Assignee)

Description

8 years ago
Created attachment 488090 [details]
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
blocking2.0: --- → ?

Updated

8 years ago
blocking2.0: ? → betaN+
Another test case, same assertion failure:

for(var i=0; i<20; i++) {
    x = ''.charCodeAt(NaN);
}
I'll take a look at this early next week.
Assignee: pbiggar → nnethercote
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.
Assignee: nnethercote → pbiggar
(Assignee)

Updated

8 years ago
Duplicate of this bug: 616715
(Assignee)

Comment 5

8 years ago
(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))
}
Summary: Double-length arrays cause tracer crash → TM: "Assertion failure: hasInt32Repr(*vp)"
(Assignee)

Comment 6

8 years ago
comment 0's test case was fixed in bug https://bugzilla.mozilla.org/show_bug.cgi?id=563125.
(Assignee)

Comment 7

8 years ago
Created attachment 495763 [details] [diff] [review]
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 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.
(Assignee)

Comment 9

8 years ago
(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?
(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.
(Assignee)

Updated

8 years ago
Duplicate of this bug: 617770
(Assignee)

Comment 12

8 years ago
Created attachment 496827 [details] [diff] [review]
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).
Attachment #496827 - Flags: review?(nnethercote)
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.
Attachment #496827 - Flags: review?(nnethercote)
Attachment #496827 - Flags: review?(gal)
Attachment #496827 - Flags: review+

Updated

8 years ago
Attachment #496827 - Flags: review?(gal) → review+
(Assignee)

Comment 14

8 years ago
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.
Whiteboard: [fixed-in-tracemonkey]

Comment 15

8 years ago
http://hg.mozilla.org/mozilla-central/rev/e5090c9f6be3
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.