826124 - Differential Testing: Getting different output w/without --ion-eager with JM Inlining

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

try {
    x = Number
    eval("function f(g){functional.x}")
    Number.toString = (function(j) {
        f(j)
    })
    print(x)
} catch (e) {}
try {
    print(x)
} catch (e) {}
try {
    function f() {}
    print(x)
    functional = this
} catch (e) {}

shows the following output on js opt shell on m-c changeset a812ef63de87 without any CLI arguments:

undefined

but with --no-ion -a shows no output.

autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   104398:55639ec64ed2
user:        Nicholas Nethercote
date:        Thu Aug 30 19:32:07 2012 -0700
summary:     Bug 787866 (part 2) - Don't call function-only methods on non-function SharedContexts.  r=luke.

Comment 1

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

njn, is the regressing changeset in comment 0 correct?

Comment 2

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

functional is just a variable name in this case - the following testcase is more applicable:

try {
    x = Number
    eval("function f(g){y.x}")
    Number.toString = (function(j) {
        f(j)
    })
    print(x)
} catch (e) {}
try {
    print(x)
} catch (e) {}
try {
    function f() {}
    print(x)
    y = this
} catch (e) {}

Comment 3

[Nicholas Nethercote [inactive]]

> njn, is the regressing changeset in comment 0 correct?

I don't think so.  IonMonkey hadn't even landed on trunk at that point.

Comment 4

[Jesse Ruderman]

var x = {};
function f() { y.prop; }
x.toString = function () { f(); };
try {
    f();
} catch (e) { }
try {
    "" + x;
} catch (e) { }
try {
    function f() { print("The empty 'f' got called") }
    "" + x;
} catch (e) { print("Threw"); }
y = 0;

> ./js                        test.js
The empty 'f' got called

> ./js --no-ion -a            test.js
Threw

> ./js --no-ion -a --no-ti    test.js
The empty 'f' got called

Comment 5

[Nicolas B. Pierron [:nbp]]

var x = {};
function f() { y.prop; }
x.toStr = function () { f(); };
print(1);
try {
    f();
} catch (e) { }
print(2);
try {
    x.toStr();
} catch (e) { }
print(3);
try {
    function f() { print("The empty 'f' got called") }
    x.toStr();
} catch (e) { print("Threw"); }
print(4);
y = 0;


toString is not the reason and JM spew show that the last function is not invalidated when we enter the try block containing the "f" function.  Ion does this invalidation.

This bug only matters when JM + TI inlines some functions.  AFAIK, b2g is using JM without TI, and Ion forbids JM inlining.  This bug still matters as people can disable Ion, and that the chrome is not using Ion at all.

Comment 6

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

I re-ran autoBisect on the testcase in comment 5:

autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   106779:d16c4404e8c4
parent:      106778:666bf90824f8
parent:      104497:233441ff53af
user:        David Anderson
date:        Thu Sep 06 18:28:59 2012 -0700
summary:     Merge from mozilla-central.

Seems like this was a bad merge or something?

Comment 7

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Jesse and I made some modifications to autoBisect and it pointed back at bug 787866 again:

autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   104398:55639ec64ed2
user:        Nicholas Nethercote
date:        Thu Aug 30 19:32:07 2012 -0700
summary:     Bug 787866 (part 2) - Don't call function-only methods on non-function SharedContexts.  r=luke.


> I don't think so.  IonMonkey hadn't even landed on trunk at that point.

njn: These testcases don't involve IonMonkey. If I run a js shell compiled against revision 55639ec64ed2 with the testcase in comment 5,

./js -m -n -a comment5.js

it outputs:

1
2
3
Threw
4

but ./js comment5.js outputs:

1
2
3
The empty 'f' got called
4

Comment 8

[Nicholas Nethercote [inactive]]

> If I run a js shell compiled
> against revision 55639ec64ed2 with the testcase in comment 5,
> 
> ./js -m -n -a comment5.js
> 
> it outputs:
> 
> 1
> 2
> 3
> Threw
> 4
> 
> but ./js comment5.js outputs:
> 
> 1
> 2
> 3
> The empty 'f' got called
> 4

But the current trunk gives the 2nd output ("The empty 'f' got called") in both cases, in my testing.

I think Jesse's test case from comment 4 is the best one to focus on in this bug, because it's the shortest.

Comment 9

[Nicholas Nethercote [inactive]]

Attachment: patch showing that mightAliasLocals is the problem

This patch fixes the problem (albeit in an unacceptable way).  It shows that the setting of mightAliasLocals is somehow causing the problem.

Comment 10

[Nicholas Nethercote [inactive]]

Attachment: better patch demonstrating the problem

Here is a patch that fixes the problem in a less blunt way, but which is still unacceptable.  It also shows that mightAliasLocals is the problem.

Comment 11

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Nick, thanks for your analysis here.

Luke, any thoughts on Nick's findings?

Comment 12

[Luke Wagner [:luke]]

Nope; although similarly named, mightAliasLocals predates my aliasedvar work and I don't exactly understand what it does.  IIIRC, it had pretty sketchy semantics and I wanted to remove it but never found the time...

Comment 13

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

(In reply to Luke Wagner [:luke] from comment #12)
> Nope; although similarly named, mightAliasLocals predates my aliasedvar work
> and I don't exactly understand what it does.  IIIRC, it had pretty sketchy
> semantics and I wanted to remove it but never found the time...

Thanks Luke. Wondering if jorendorff or brendan can add some thoughts then.

(Nick's analysis on mightAliasLocals in the couple of comments above might help)

Comment 14

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

http://hg.mozilla.org/mozilla-central/rev/4bee0517d440 was the related change that landed the line that njn was pointing out in comment 10.

It points to: Bug 568593 - Refactor SharedContext; r=jorendorff

Comment 15

[Jason Orendorff [:jorendorff]]

This bug is gone with JM. I have a patch to add a regression test for it.

Comment 16

[Jason Orendorff [:jorendorff]]

Btw, the changeset mentioned in comment 14 probably just moved it around; the bug was not in the front end.

Also, Luke's right; mightAliasLocals is bogus. Killing it off is on my todo list.

Comment 17

[Luke Wagner [:luke]]

\o/

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/71c097c1ec7a