593554 - JM: incorrect results for (num >> obj)

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jesse Ruderman]

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 <[e-mail]>
  date:        Thu Jul 29 11:32:36 2010 -0700
  summary:     Bug 582900: JM: Missing rsh type set. (r=dvander)

Comment 1

[Sean Stangl [:sstangl]]

Attachment: 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.

Comment 2

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

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?

Comment 3

[Sean Stangl [:sstangl]]

Attachment: 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.

Comment 4

[Sean Stangl [:sstangl]]

http://hg.mozilla.org/tracemonkey/rev/45f147e08faf

Comment 5

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

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)

Comment 6

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

That is, second part of the condition... the isNotType(JSVAL_TYPE_INT32)

Comment 7

[Sean Stangl [:sstangl]]

Attachment: Same as before, plus assertions, minus gratuitous check.

Removed the extra check.

Comment 8

[Sean Stangl [:sstangl]]

http://hg.mozilla.org/tracemonkey/rev/5a9241e86ecc

Comment 9

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/5a9241e86ecc