Closed Bug 593554 Opened 9 years ago Closed 9 years ago

JM: incorrect results for (num >> obj)

Categories

(Core :: JavaScript Engine, defect)

x86_64
Linux
defect
Not set

Tracking

()

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

People

(Reporter: jruderman, Unassigned)

References

(Blocks 2 open bugs)

Details

(Keywords: regression, testcase, Whiteboard: fixed-in-tracemonkey)

Attachments

(1 file, 2 obsolete files)

var b = 7;
var a = [];
for (var j = 0; j < 7; ++j) {
    var d = {};
    a.push(b >> d);
}
print(a);

Interpreter: 7,7,7,7,7,7,7
Method JIT:  0,7,0,7,0,7,0 (or something similar)

The first bad revision is:
  changeset:   52401:27b2c8b23ff3
  user:        Chris Leary <cdleary@mozilla.com>
  date:        Thu Jul 29 11:32:36 2010 -0700
  summary:     Bug 582900: JM: Missing rsh type set. (r=dvander)
blocking2.0: --- → ?
Summary: Method JIT gives incorrect results for (num >> obj) → JM: incorrect results for (num >> obj)
Attached patch Fix isNotType() usage in rsh. (obsolete) — Splinter Review
The problem is that jsop_rsh_unknown_any() was using lhs->isNotType(JSVAL_TYPE_INT32) to mean "is the type unknown or an int32" when isNotType() is defined to return true iff the value is known and is not an int32.

So one solution is to check (!isTypeKnown() || isNotType()). The attached patch does this.

I think this may be confusing, however. It reads much more clearly if we define isNotType() to return true if the type is unknown. Your call.
Attachment #472178 - Flags: review?(dvander)
Comment on attachment 472178 [details] [diff] [review]
Fix isNotType() usage in rsh.


>+    if (!rhs->isTypeKnown() || rhs->isNotType(JSVAL_TYPE_INT32)) {
>         rhsType.setReg(frame.tempRegForType(rhs));
>         frame.pinReg(rhsType.reg());

This path confuses me - the isNotType is a non-sequitor because tempRegForType would assert-botch. At the top of jsop_rsh is a guard:

    if (lhs->isNotType(JSVAL_TYPE_INT32) || rhs->isNotType(JSVAL_TYPE_INT32)) {

Which goes to an always-taken slow path. Do we just not need the second half of that condition? Actually, do we need any part of this check at all?
Attachment #472178 - Flags: review?(dvander)
You're right -- jsop_rhs_unknown_any() is used only in the case that both types are unknown. Must be some legacy code that suffered from a renaming.

This version does the right check for unknown types, and asserts that jsop_rsh_unknown_any() is only used when, indeed, the types are both unknown.
Attachment #472178 - Attachment is obsolete: true
Attachment #472859 - Flags: review?(dvander)
Attachment #472859 - Flags: review?(dvander) → review+
Assert botches on jaeger/bug582900.js, backed out as 51c060e00161

> [].x >>= a | 0

Sorry, I misled here - we can know the type of the RHS, it's just the first part of the condition that wasn't needed.

(PS - you want to run trace-tests with --jitflags=mj or else it won't cover either JIT)
Whiteboard: fixed-in-tracemonkey
That is, second part of the condition... the isNotType(JSVAL_TYPE_INT32)
blocking2.0: ? → betaN+
Removed the extra check.
Attachment #472859 - Attachment is obsolete: true
Attachment #474222 - Flags: review?(dvander)
Attachment #474222 - Flags: review?(dvander) → review+
http://hg.mozilla.org/mozilla-central/rev/5a9241e86ecc
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Depends on: 600129
You need to log in before you can comment on or make changes to this bug.