604996 - JM: Function returns -65535 instead of 1

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Duncan Loveday]

User-Agent:       Mozilla/5.0 (Windows NT 5.1; rv:2.0b8pre) Gecko/20101017 Firefox/4.0b8pre
Build Identifier: 

The function below should return 1, however I on the Fx 4 trunk I see it returning -65535. Strangely, commenting out the second line "var dummy=1 * n;" makes it work correctly even though the value of dummy is not used.

function f() {
    var n = Math.pow(10, 0);
    var dummy=1 * n;
    return n/1;
}


Reproducible: Always

Steps to Reproduce:
1. Load the test case, click the button
2.
3.
Actual Results:  
The alert I see reads "function f returned -65535" when it should read "function f returned 1"

Comment 1

[Duncan Loveday]

Attachment: test case

Comment 2

[Duncan Loveday]

Attachment: Test case v2

Test case v2. In fact Math.pow() is not necessary because when I try to work around this bug using multiplication in a loop I get the same thing.

Comment 3

[Duncan Loveday]

This only happens when method JIT is on

Comment 4

[Dave Garrett]

Confirmed on Linux using latest Minefield and latest Tracemonkey branch.

Removal or commenting out of the dummy line does produce the correct result.

Comment 5

[Sean Stangl [:sstangl]]

This appears to be x86-specific; x64 correctly returns 1.

Comment 6

[Duncan Loveday]

I can work around this bug by changing "return n/1;" to "var ret=n/1; return ret;"

In the example below, removing the loop also produces the correct result even though the loop is never entered at run time


function g() {
    var n=1; for (var m=0;m<0;m++) n*=10;
    var dummy=1 * n;
    return n/1;
}

Comment 7

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

This is register allocation bug in FrameState::loadTo(), tempRegFor*() are called in succession without pinning, and the first register is clobbered. There's a trivial fix but it results in an extra store, I'd like to try and make that go away.

Comment 8

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

This should block b7. It's more than a correctness issue. It could cause random crashes anywhere in the method JIT, and in an evil way since it happens in a method that is just about to return.

If we're going to ship with this though, it should be security sensitive.

Comment 9

[Damon Sicore (:damons)]

dvander, eta here?

Comment 10

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

Patch in a few minutes.

Comment 11

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

Attachment: fix

Thanks for the great test case, Duncan. This patch removes register evictions from the return path. Instead, it first pins registers if they are available, and then figures out the optimal way to either load from memory or shuffle the right registers into place.

Comment 12

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

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

Comment 13

[Robert Sayre]

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

Comment 14

[Duncan Loveday]

This is working fine for me now (using current trunk nightly) so marking as VERIFIED