617609 - Upvar analysis marks functions as NULL_CLOSURE incorrectly

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jim Blandy :jimb]

In the following code (from bug 554955):

function f(s) {
    eval(s);
    return function(a) {
        var d;
        let (c = 3) {
            d = function() { a; };
            return b;
        };
    };
}

the inner function gets marked as a NULL_CLOSURE. But since it can refer to a binding created by the eval in f, that's not correct.

This code happens to work because, for null closures, we simply grab the current frame's scope chain to use as the function's parent, because that's more convenient than walking down to get the global, so the closure does end up enclosing f's call object, and the JSOP_NAME finds the right binding.

But it's my understanding that NULL_CLOSURE means that a function doesn't need anything but the global on its scope chain; that's clearly incorrect here.

Comment 1

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

Nominating because bug 561359 depends on this and that bug is marked as final+ and a softblocker.

Comment 2

[Jason Orendorff [:jorendorff]]

Attachment: WIP 1

This works, but it deoptimizes a little more than I think we need to.

Instead I bet I can make DeoptimizeUsesWithin mark all functions that directly contain deoptimized uses.

Comment 3

[Jason Orendorff [:jorendorff]]

Comment on attachment 505509 [details] [diff] [review]
WIP 1

Correction. It looks like getting from pnu to its containing function in DeoptimizeUsesWithin is more trouble than it's worth. So let's try v1.

The upshot of this is:

function f(s) {  // HEAVYWEIGHT
    eval(s);
    return function(a) {  // HEAVYWEIGHT
        var d;
        let (c = 3) {
            d = function() { a; };  // neither heavyweight nor null closure
            return b;
        };
    };
}

Comment 4

[Brendan Eich [:brendan]]

Comment on attachment 505509 [details] [diff] [review]
WIP 1

Seems fine.

/be

Comment 5

[Jason Orendorff [:jorendorff]]

https://hg.mozilla.org/tracemonkey/rev/33f07113de5f

Comment 6

[Chris Leary [:cdleary] (not checking bugmail)]

http://hg.mozilla.org/mozilla-central/rev/33f07113de5f