JM: incorrect results for (num >> obj)

RESOLVED FIXED

Status

()

Core
JavaScript Engine
RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: Jesse Ruderman, Unassigned)

Tracking

(Blocks: 2 bugs, {regression, testcase})

Trunk
x86_64
Linux
regression, testcase
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking2.0 betaN+)

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

8 years ago
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: --- → ?
(Reporter)

Updated

8 years ago
Summary: Method JIT gives incorrect results for (num >> obj) → JM: incorrect results for (num >> obj)
Created attachment 472178 [details] [diff] [review]
Fix isNotType() usage in rsh.

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)
Created attachment 472859 [details] [diff] [review]
Remove isNotType() usage in jsop_rsh_unknown_any().

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+
http://hg.mozilla.org/tracemonkey/rev/45f147e08faf
Whiteboard: fixed-in-tracemonkey
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)

Updated

8 years ago
blocking2.0: ? → betaN+
Created attachment 474222 [details] [diff] [review]
Same as before, plus assertions, minus gratuitous check.

Removed the extra check.
Attachment #472859 - Attachment is obsolete: true
Attachment #474222 - Flags: review?(dvander)
Attachment #474222 - Flags: review?(dvander) → review+

Comment 9

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