JM: function-statement can disappear if it shadows a subsequent var declaration

RESOLVED FIXED

Status

()

defect
RESOLVED FIXED
9 years ago
5 years ago

People

(Reporter: jruderman, Assigned: dmandelin)

Tracking

(Blocks 2 bugs, {testcase})

Trunk
x86
macOS
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking2.0 beta8+)

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(1 attachment)

Reporter

Description

9 years ago
var spots = 0;
(function() {
  for (var j = 0; j < 3; ++j) {
    if (j == 0) {
      function x(){}
    } else if (x) {
      ++spots;
    }
  }
})(); var x;
print(spots);

I put "var x" on the same line so the testcase shows the bug even when pasted into an interactive shell.

Result:
  ./js     2
  ./js -j  2
  ./js -m  1   <-- probably wrong
  ./jsc    2
wrong bug, sorry
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
The problem is that we've predicted "x" in the "else if" to be a global variable, but it's really not. It's a potentially aliased in the Call object because of the named function definition snuggled in a block.

This is broken in our test suite already. I added an assert to jsinterp.cpp after js_FindIdentifierBase in JSOP_NAME:

    JS_ASSERT_IF(op == JSOP_GETGNAME || op == JSOP_CALLGNAME, obj == obj->getGlobal());

Then doMath.js fails (and another, filed as bug 585524). Turning the DEFFUN into DEFLOCALFUN would break our current (though out-of-spec) semantics. Instead maybe this and bug 585524 should share a common solution of marking which names cannot be optimized.
Assignee: general → dvander
Status: REOPENED → ASSIGNED
blocking2.0: --- → ?

Updated

9 years ago
blocking2.0: ? → beta8+
Blocks: 557407
Depends on: 609832

Updated

9 years ago
Duplicate of this bug: 605874
A simple test case is (which throws 'too much recursion') is:

function f(x) {
    if (x) {
        function f() { return 2; }
        assertEq(f(true), 2);
    }
}
f(true);
Assignee

Comment 6

9 years ago
Posted patch PatchSplinter Review
This patch avoids the JSOP_GETGLOBAL optimization if the JSOP_DEFFUN op occurs. Summarizing earlier discussion, this is necessary because a JSOP_DEFFUN can introduce a shadowing definition, but it will not be seen in the parser because that shadowing is not recorded in the parser; it is only introduced at runtime when the JSOP_DEFFUN is executed.
Assignee: dvander → dmandelin
Attachment #490011 - Flags: review?(dvander)
Assignee

Comment 7

9 years ago
I forgot to say in comment 6 that I verified that the deoptimization does not occur in Sunspider, V8, or Kraken.
Attachment #490011 - Flags: review?(dvander) → review+
Assignee

Comment 8

9 years ago
http://hg.mozilla.org/tracemonkey/rev/96a84cd98d84
Whiteboard: fixed-in-tracemonkey
Assignee

Updated

9 years ago
Duplicate of this bug: 605774
Assignee

Updated

9 years ago
Duplicate of this bug: 599668
This still seems to be not working quite right.

function g(v) {
  with (v) {
    var f = function() { print("HI"); }
    f();
  }
  f();
}
g({});

The 'f' function is called twice here, but on disassembly the first call is a CALLNAME and the second is a CALLGNAME.

Comment 12

9 years ago
http://hg.mozilla.org/mozilla-central/rev/96a84cd98d84
Status: ASSIGNED → RESOLVED
Closed: 9 years ago9 years ago
Resolution: --- → FIXED
(In reply to comment #11)
> This still seems to be not working quite right.
> 
> function g(v) {
>   with (v) {
>     var f = function() { print("HI"); }
>     f();
>   }
>   f();
> }
> g({});
> 
> The 'f' function is called twice here, but on disassembly the first call is a
> CALLNAME and the second is a CALLGNAME.

David, doesn't this need a followup bug?

/be
Was about to file, but just realized: that test case is different, and is bug 585524.
Depends on: 616715
Reporter

Updated

9 years ago
Depends on: 618007
No longer depends on: 585536
You need to log in before you can comment on or make changes to this bug.