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)
Tracking
()
RESOLVED
FIXED
Tracking | Status | |
---|---|---|
blocking2.0 | --- | beta8+ |
People
(Reporter: jruderman, Assigned: dmandelin)
References
Details
(Keywords: testcase, Whiteboard: fixed-in-tracemonkey)
Attachments
(1 file)
4.03 KB,
patch
|
dvander
:
review+
|
Details | Diff | Splinter Review |
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
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
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
![]() |
||
Updated•14 years ago
|
blocking2.0: --- → ?
Updated•14 years ago
|
blocking2.0: ? → beta8+
![]() |
||
Comment 5•14 years ago
|
||
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•14 years ago
|
||
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•14 years ago
|
||
I forgot to say in comment 6 that I verified that the deoptimization does not occur in Sunspider, V8, or Kraken.
![]() |
||
Updated•14 years ago
|
Attachment #490011 -
Flags: review?(dvander) → review+
Assignee | ||
Comment 8•14 years ago
|
||
Whiteboard: fixed-in-tracemonkey
Comment 11•14 years ago
|
||
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•14 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 15 years ago → 14 years ago
Resolution: --- → FIXED
Comment 13•14 years ago
|
||
(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.
You need to log in
before you can comment on or make changes to this bug.
Description
•