Closed Bug 609502 Opened 9 years ago Closed 9 years ago

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

Categories

(Core :: JavaScript Engine, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
Tracking Status
blocking2.0 --- betaN+

People

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

References

Details

(Whiteboard: [fixed-in-tracemonkey])

Attachments

(3 files)

$ 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: --- → ?
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
Duplicate of this bug: 616715
(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)"
comment 0's test case was fixed in bug https://bugzilla.mozilla.org/show_bug.cgi?id=563125.
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.
(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.
Duplicate of this bug: 617770
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+
Attachment #496827 - Flags: review?(gal) → review+
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]
http://hg.mozilla.org/mozilla-central/rev/e5090c9f6be3
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.