Closed Bug 455748 Opened 13 years ago Closed 13 years ago

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

Categories

(Core :: JavaScript Engine, defect, P1)

x86
macOS
defect

Tracking

()

VERIFIED FIXED
mozilla1.9.1b1

People

(Reporter: jruderman, Assigned: gal)

References

Details

(Keywords: assertion, testcase, verified1.9.1)

Attachments

(1 file, 3 obsolete files)

$ ./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)
David is taking over from here.
Assignee: general → danderson
Assignee: danderson → gal
Flags: blocking1.9.1?
Priority: -- → P1
Target Milestone: --- → mozilla1.9.1b1
Duplicate of this bug: 456652
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).
Attachment #339345 - Attachment is obsolete: true
Attachment #340304 - Flags: review?(brendan)
Attachment #340304 - Attachment is patch: true
Attachment #340304 - Attachment mime type: application/octet-stream → text/plain
Attachment #340304 - Flags: review?(danderson)
Attachment #340304 - Attachment is obsolete: true
Attachment #340305 - Flags: review?(brendan)
Attachment #340304 - Flags: review?(danderson)
Attachment #340304 - Flags: review?(brendan)
Attachment #340305 - Flags: review?(danderson)
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.
Attachment #340305 - Flags: review?(danderson) → review+
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
Attached patch v2Splinter Review
Attachment #340305 - Attachment is obsolete: true
Attachment #340341 - Flags: review?(brendan)
Attachment #340305 - Flags: review?(brendan)
Attachment #340341 - Flags: review?(brendan) → review+
Comment on attachment 340341 [details] [diff] [review]
v2

Looks good, thanks.

/be
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.
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
/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
Flags: in-testsuite+
Flags: in-litmus-
Flags: wanted1.9.2+
Keywords: fixed1.9.1
Flags: wanted1.9.2+
Flags: blocking1.9.1?
Flags: blocking1.9.1+
v 1.9.1, 1.9.2
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.