455748 - TM: Assertion failed: "Should not move data from GPR to XMM" with [1][-0]

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Jesse Ruderman]

$ ./js -j 
js> for (var j = 0; j < 5; ++j) { if([1][-0]) { } }

Assertion failed: "Should not move data from GPR to XMM": false (nanojit/Nativei386.cpp:1192)

Comment 1

[Andreas Gal :gal]

Attachment: Deal with floating point arguments to String[] and remove bogus IndexToId check.

Comment 2

[Andreas Gal :gal]

David is taking over from here.

Comment 4

[Andreas Gal :gal]

Attachment: Rewrite GETELEM, SETELEM and ::elem().

Rewrite and cleanup GETELEM and SETELEM.
- Only support integer indexes. Double indexes are converted to int and if its not a whole number we side exit. According to Brendan this is super rare.
- Properly reject (and check at runtime) for aliasing the global object (which is not safe to do).
- Don't modify the interpreter stack (id).

Comment 5

[Andreas Gal :gal]

Attachment: Getting tired. Right patch this time.

Comment 6

[David Anderson [:dvander] - inactive, e-mail if emergency]

Comment on attachment 340305 [details] [diff] [review]
Getting tired. Right patch this time.

Looks much better than the old code, though someone with more knowledge of property access should take a closer look than I can.

Comment 7

[Brendan Eich [:brendan]]

Comment on attachment 340305 [details] [diff] [review]
Getting tired. Right patch this time.

>+LIns* TraceRecorder::guardThatNumberIsInt32(LIns* f)

Drop the "That" deadwood-preposition, we don't use it elsewhere (including your guardNotGlobalObject).

>+{
>+    JS_ASSERT(f->isQuad());
>+    LIns* x;
>+    if (!isPromote(f)) {
>+        guard(true, lir->ins2(LIR_feq, f, lir->ins1(LIR_i2f, x = f2i(f))), MISMATCH_EXIT);
>+    } else x = ::demote(lir, f);

Wacky style, suggest braced if-else with the x = f2i(f) nested assignment pulled out and above the guard so it can be seen.

Substantive point: shouldn't this be guarding that the instruction generates a uint32, given how it's used in GETELEM and SETELEM and the built-ins?

> TraceRecorder::record_JSOP_SETELEM()
> {
>     jsval& v = stackval(-1);
[skipped lines...]
>+    if (!JSVAL_IS_INT(idx))
>         ABORT_TRACE("non-string, non-int JSOP_SETELEM index");
> 
[deleted lines...]
>+    if (!guardNotGlobalObject(JSVAL_TO_OBJECT(obj), obj_ins))
>+        return false;

You don't do this for int-idx case in GETPROP, and don't need to given lack of imports for this[1], e.g., global indexed prop refs. Why do it here for the int-idx case?

/be

Comment 8

[Andreas Gal :gal]

Attachment: v2

Comment 9

[Brendan Eich [:brendan]]

Comment on attachment 340341 [details] [diff] [review]
v2

Looks good, thanks.

/be

Comment 10

[Andreas Gal :gal]

http://hg.mozilla.org/tracemonkey/rev/34c61673341a

Pushed into tracemonkey. Since this is not a user-reported bug I will close immediately. Merge into m-c will happen today.

Comment 11

[Bob Clary [:bc] (inactive)]

/cvsroot/mozilla/js/tests/js1_5/Regress/regress-455748.js,v  <--  regress-455748.js
initial revision: 1.1

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

Comment 12

[Bob Clary [:bc] (inactive)]

v 1.9.1, 1.9.2