Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now) Reporter
	Description • 15 years ago

x = true;
(function() {
  for each(let c in [0, x]) {
    (arguments)[4] *= c
  }
})()

asserts js debug shell on JM tip with -m and -j at Assertion failed: LIR type error (start of writer pipeline): arg 1 of 'fmul' is 'int' which has type int32 (expected float64): 0 (../nanojit/LIR.cpp:2610)

	Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now) Reporter
	Comment 1 • 15 years ago

A variant testcase asserts differently:

var c;
(function() {
  for each(e in [0, 0]) {
    (arguments)[1] *= c
  }
})()


Assertion failure: m != TT_INT32 || isInt32(*vp), at ../jstracer.cpp:3796

	David Anderson [:dvander] - inactive, e-mail if emergency
	Comment 2 • 15 years ago

This affects tm-tip too.

	David Anderson [:dvander] - inactive, e-mail if emergency
	Comment 3 • 15 years ago

Yuck. The problem is that the tracer expects arguments[n] to be undefined if |n| is out of range. In this case, it is _not_ undefined, it is NaN, because the interpreter did this:

  arguments[4] = undefined * 0

	Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now) Reporter
	Comment 4 • 15 years ago

You're right - it occurs on TM tip with -j. In the past, it used to assert at:

Assertion failed: ((unsigned)r)<8 (../nanojit/Nativei386.cpp:427)

or

Assertion failed: s0->isQuad() && s1->isQuad() (../nanojit/LIR.cpp:2182)

before morphing to the current form.

autoBisect shows this is probably related to bug 453730:

The first bad revision is:
changeset:   30020:c76558a87dd9
user:        David Mandelin
date:        Wed Jul 08 11:16:41 2009 -0700
summary:     Bug 453730: trace JSOP_ARGUMENTS, r=gal

	David Mandelin [:dmandelin]
	Comment 5 • 15 years ago

dvander and I talked this over, and we think all that it takes to fix this is to abort tracing a getelem with arguments (args[expr]) if the index is a constant that is out of range. The tracer already aborts if the index is a non-constant that is out of range, so this just makes things consistent. 

ETA = patch up for review later today.

	David Mandelin [:dmandelin]
	Comment 7 • 15 years ago

Adding bug 547299 as a dep because it needs to land on 1.9.2 before the patch for this bug can land there.

	Jason Orendorff [:jorendorff]
	Comment 8 • 15 years ago

Comment on attachment 437988 [details] [diff] [review]
Patch

The patch looks good.

Please put newlines on the last line of the two test files.

	David Mandelin [:dmandelin]
	Comment 9 • 14 years ago

http://hg.mozilla.org/tracemonkey/rev/bbdddef55da3

	Robert Sayre Assignee
	Comment 10 • 14 years ago

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

	Daniel Veditz [:dveditz]
	Comment 11 • 14 years ago

(In reply to comment #7)
> Adding bug 547299 as a dep because it needs to land on 1.9.2 before the patch
> for this bug can land there.

That one's fixed in 1.9.2.4, please add a 1.9.2 patch for this bug (or request approval on the trunk patch if it applies)

	Reed Loden [:reed]
	Comment 12 • 14 years ago

Why did this just land on 1.9.2 without approval? This isn't a new policy, and I've said this before. All branch patches must have explicit approval before landing. blocking+ is not enough.

	Robert Sayre Assignee
	Comment 13 • 14 years ago

(In reply to comment #12)
> Why did this just land on 1.9.2 without approval? This isn't a new policy, and
> I've said this before. All branch patches must have explicit approval before
> landing. blocking+ is not enough.

It's set to blocking 1.9.2.11. I don't even know why am answering this nonsense.

	Robert Sayre Assignee
	Comment 14 • 14 years ago

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/2c360dcaf7d5

	Christian Holler (:decoder)
	Comment 15 • 13 years ago

Tracer has been long gone on trunk, marking verified.

	Christian Holler (:decoder)
	Comment 16 • 12 years ago

A testcase for this bug was automatically identified at js/src/jit-test/tests/arguments/bug554670-1.js.