Closed Bug 584603 Opened 15 years ago Closed 14 years ago

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

Categories

(Core :: JavaScript Engine, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
Tracking Status
blocking2.0 --- beta8+

People

(Reporter: jruderman, Assigned: dmandelin)

References

Details

(Keywords: testcase, Whiteboard: fixed-in-tracemonkey)

Attachments

(1 file)

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: --- → ?
blocking2.0: ? → beta8+
Blocks: 605891
Blocks: 605874
Blocks: 557407
Depends on: 609832
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);
Attached 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)
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+
Whiteboard: fixed-in-tracemonkey
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.
Status: ASSIGNED → RESOLVED
Closed: 15 years ago14 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: 618007
No longer depends on: 585536
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: