Last Comment Bug 452498 - (upvar2) TM: Can we jit heavyweight functions? (upvar, part deux)
(upvar2)
: TM: Can we jit heavyweight functions? (upvar, part deux)
Status: VERIFIED FIXED
fixed-in-tracemonkey
: verified1.9.1
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: All All
: P1 normal with 8 votes (vote)
: mozilla1.9.1
Assigned To: Brendan Eich [:brendan]
:
Mentors:
: 473271 (view as bug list)
Depends on: upvar1 486820 487209 487215 487251 487430 487563 487957 487967 488015 488029 488034 488050 488272 490339 490568 490818 491806 492714 493466 496134 496316 496422 496532 498395 499524 502604 502714 504590 505220 520513 527032 528082 531516 540243 541255 546615 633828 646820
Blocks: 460050 448605 456588 458838 471660 472450 472528 472703 473014 473271 477733 483179 485177 487202
  Show dependency treegraph
 
Reported: 2008-08-27 14:19 PDT by Boris Zbarsky [:bz]
Modified: 2011-09-19 15:18 PDT (History)
46 users (show)
sayrer: blocking1.9.1+
bob: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
js shell testcase (1.18 KB, text/plain)
2008-08-27 14:19 PDT, Boris Zbarsky [:bz]
no flags Details
backup of work in progress (143.58 KB, patch)
2009-01-05 19:25 PST, Brendan Eich [:brendan]
no flags Details | Diff | Review
backup, v2 (147.94 KB, patch)
2009-01-05 22:08 PST, Brendan Eich [:brendan]
no flags Details | Diff | Review
backup, v3 (196.47 KB, patch)
2009-01-11 02:46 PST, Brendan Eich [:brendan]
no flags Details | Diff | Review
backup, v4 (233.68 KB, patch)
2009-01-12 15:38 PST, Brendan Eich [:brendan]
no flags Details | Diff | Review
backup, v5 (242.58 KB, patch)
2009-01-12 18:03 PST, Brendan Eich [:brendan]
no flags Details | Diff | Review
backup, v6 (360.57 KB, patch)
2009-02-06 19:18 PST, Brendan Eich [:brendan]
no flags Details | Diff | Review
backup, v7 (389.19 KB, patch)
2009-02-09 00:18 PST, Brendan Eich [:brendan]
no flags Details | Diff | Review
backup, v7a (389.58 KB, patch)
2009-02-09 00:27 PST, Brendan Eich [:brendan]
no flags Details | Diff | Review
v8 (393.48 KB, patch)
2009-02-09 19:07 PST, Brendan Eich [:brendan]
no flags Details | Diff | Review
traces bz's testcase (396.48 KB, patch)
2009-02-10 01:27 PST, Brendan Eich [:brendan]
no flags Details | Diff | Review
working thru t/*.js, at date-format-tofte.js (382.81 KB, patch)
2009-02-10 19:01 PST, Brendan Eich [:brendan]
no flags Details | Diff | Review
compiles and runs ss, debugging v8/earley-boyer.js (387.39 KB, patch)
2009-02-11 11:51 PST, Brendan Eich [:brendan]
no flags Details | Diff | Review
fix all v8 but raytrace.js (interp only -- yeah, known jit bugs) (389.88 KB, patch)
2009-02-11 13:43 PST, Brendan Eich [:brendan]
no flags Details | Diff | Review
Version of the testcase that runs faster (by a factor of 150 or so) and detects correctness (8.54 KB, text/plain)
2009-02-11 14:32 PST, Boris Zbarsky [:bz]
no flags Details
bunch of fixes, still need to run js tests (393.33 KB, patch)
2009-02-11 21:49 PST, Brendan Eich [:brendan]
no flags Details | Diff | Review
passes trace-test.js interp (-j NanoAsserts in testSwitchUndefined) (403.24 KB, patch)
2009-02-12 02:48 PST, Brendan Eich [:brendan]
no flags Details | Diff | Review
passes trace-test.js with -j, passes bench.sh, passes v8 w/&w/o -j (407.18 KB, patch)
2009-02-13 00:53 PST, Brendan Eich [:brendan]
no flags Details | Diff | Review
fixes comment 50 bugs, plus some things not found by fuzzers (411.20 KB, patch)
2009-02-13 19:36 PST, Brendan Eich [:brendan]
no flags Details | Diff | Review
fixed more, through 3 of 5 in comment 53 -- 2/5 remain + comment 54 (416.37 KB, patch)
2009-02-16 01:38 PST, Brendan Eich [:brendan]
no flags Details | Diff | Review
fix bogus assertion (416.27 KB, patch)
2009-02-16 10:48 PST, Brendan Eich [:brendan]
no flags Details | Diff | Review
#ifdef __cplusplus in jsatom.h to avoid breaking stinking liveconnect (416.32 KB, patch)
2009-02-16 11:45 PST, Brendan Eich [:brendan]
no flags Details | Diff | Review
stacktrace+ for firefox startup assertion (4.49 KB, text/plain)
2009-02-16 12:07 PST, Jesse Ruderman
no flags Details
better, but browser doesn't render and 39 tests beyond baseline fail (420.29 KB, patch)
2009-02-18 03:29 PST, Brendan Eich [:brendan]
no flags Details | Diff | Review
getting there (424.16 KB, patch)
2009-02-19 01:24 PST, Brendan Eich [:brendan]
no flags Details | Diff | Review
fixes everything from gary's last comment (429.18 KB, patch)
2009-02-20 00:05 PST, Brendan Eich [:brendan]
no flags Details | Diff | Review
rebased (428.70 KB, patch)
2009-02-20 00:11 PST, Brendan Eich [:brendan]
no flags Details | Diff | Review
fixes for Jesse (const [d] and the for(let...) scope prob remain) (431.85 KB, patch)
2009-02-20 02:15 PST, Brendan Eich [:brendan]
no flags Details | Diff | Review
fixes comment 78 and comment 79 probs (432.48 KB, patch)
2009-02-20 10:55 PST, Brendan Eich [:brendan]
no flags Details | Diff | Review
passes js tests (491.84 KB, patch)
2009-02-26 12:10 PST, Brendan Eich [:brendan]
no flags Details | Diff | Review
rebased to tip of tm (491.90 KB, patch)
2009-02-26 12:41 PST, Brendan Eich [:brendan]
no flags Details | Diff | Review
Fix bug that bit sunspider-compare-results.js (493.14 KB, patch)
2009-02-26 15:33 PST, Brendan Eich [:brendan]
no flags Details | Diff | Review
really fix bug that bit sunspider-compare-results.js (493.86 KB, patch)
2009-02-26 17:31 PST, Brendan Eich [:brendan]
no flags Details | Diff | Review
fixes all comment 94 tests except the first (the -j) one (494.52 KB, patch)
2009-02-26 21:12 PST, Brendan Eich [:brendan]
no flags Details | Diff | Review
fix all but the -j one from comment 53, and the last two gary found (lexdep...) (494.54 KB, patch)
2009-02-27 00:29 PST, Brendan Eich [:brendan]
no flags Details | Diff | Review
fix all but the -j issue and comment 104 testcase (499.92 KB, patch)
2009-02-27 20:07 PST, Brendan Eich [:brendan]
no flags Details | Diff | Review
fix all reported bugs except for the -j ones (500.23 KB, patch)
2009-02-28 01:26 PST, Brendan Eich [:brendan]
no flags Details | Diff | Review
fixes all of comment 116&118, some of 117&119 (510.15 KB, patch)
2009-03-01 00:48 PST, Brendan Eich [:brendan]
no flags Details | Diff | Review
fixes all of comment 116&118, some of 117&119, without bogo-assert (510.33 KB, patch)
2009-03-01 01:45 PST, Brendan Eich [:brendan]
no flags Details | Diff | Review
the last two from comment 119 are still broken (515.52 KB, patch)
2009-03-02 00:18 PST, Brendan Eich [:brendan]
no flags Details | Diff | Review
fixes all but the three non-jit ones in comment 130 (521.82 KB, patch)
2009-03-13 19:13 PDT, Brendan Eich [:brendan]
no flags Details | Diff | Review
fix bug reported by Gary over IRC (522.05 KB, patch)
2009-03-13 22:07 PDT, Brendan Eich [:brendan]
no flags Details | Diff | Review
fix comment 130 hard cases (524.92 KB, patch)
2009-03-14 01:26 PDT, Brendan Eich [:brendan]
no flags Details | Diff | Review
fix all reported fuzzer-generated tests (525.86 KB, patch)
2009-03-14 11:12 PDT, Brendan Eich [:brendan]
no flags Details | Diff | Review
refresh to match tm tip (525.90 KB, patch)
2009-03-14 11:32 PDT, Brendan Eich [:brendan]
no flags Details | Diff | Review
js1_8_1/trace/regress-452498-01.js (10.86 KB, text/plain)
2009-03-15 11:27 PDT, Bob Clary [:bc:]
no flags Details
refresh to match tm tip (525.87 KB, patch)
2009-03-17 10:57 PDT, Brendan Eich [:brendan]
no flags Details | Diff | Review
assertion on browser startup (3.46 KB, text/plain)
2009-03-17 13:08 PDT, Robert Sayre
no flags Details
preliminary tests and results (10.70 KB, application/x-bzip2)
2009-03-17 16:45 PDT, Bob Clary [:bc:]
no flags Details
fix XDR of function script to not treat upvars as vars (526.53 KB, patch)
2009-03-17 22:57 PDT, Brendan Eich [:brendan]
no flags Details | Diff | Review
tests v2 w/ results log. (30.95 KB, application/x-bzip2)
2009-03-18 09:20 PDT, Bob Clary [:bc:]
no flags Details
latest and greatest (529.14 KB, patch)
2009-03-22 01:20 PDT, Brendan Eich [:brendan]
no flags Details | Diff | Review
fixes for bugs Gary found, and rebased (530.37 KB, patch)
2009-03-22 16:42 PDT, Brendan Eich [:brendan]
no flags Details | Diff | Review
fix TraceRecorder::record_JSOP_DSLOT, passing MochiKit tests now (531.52 KB, patch)
2009-03-23 08:31 PDT, Brendan Eich [:brendan]
no flags Details | Diff | Review
more fixes, now at jQuery tests, know what's wrong (532.16 KB, patch)
2009-03-23 19:30 PDT, Brendan Eich [:brendan]
no flags Details | Diff | Review
comment 168 fixes, plus deferred space policing in js_Interpret (535.80 KB, patch)
2009-03-24 00:46 PDT, Brendan Eich [:brendan]
no flags Details | Diff | Review
more fixes, still some issues mochitesting (539.11 KB, patch)
2009-03-24 19:37 PDT, Brendan Eich [:brendan]
no flags Details | Diff | Review
fix for good the constant-folding-leaves-dead-funbox bug (548.68 KB, patch)
2009-03-25 19:12 PDT, Brendan Eich [:brendan]
no flags Details | Diff | Review
fix bogus assertion in last patch (549.34 KB, patch)
2009-03-26 08:01 PDT, Brendan Eich [:brendan]
no flags Details | Diff | Review
refreshed and tweaked a bit (549.54 KB, patch)
2009-03-26 16:00 PDT, Brendan Eich [:brendan]
no flags Details | Diff | Review
fix the bug Jesse found (550.57 KB, patch)
2009-03-26 16:38 PDT, Brendan Eich [:brendan]
no flags Details | Diff | Review
Fix bug from comment 184 (550.94 KB, patch)
2009-03-26 22:35 PDT, Brendan Eich [:brendan]
no flags Details | Diff | Review
fix comment 186 bug, improve var vs. let, etc., error checking (551.05 KB, patch)
2009-03-27 01:45 PDT, Brendan Eich [:brendan]
no flags Details | Diff | Review
latest and greatest (551.07 KB, patch)
2009-03-27 10:22 PDT, Brendan Eich [:brendan]
no flags Details | Diff | Review
fix bug reported in comment 193 (552.83 KB, patch)
2009-03-28 14:05 PDT, Brendan Eich [:brendan]
no flags Details | Diff | Review
fix mochitest "FRAME_LENGTH is not defined" bug (552.97 KB, patch)
2009-03-30 00:47 PDT, Brendan Eich [:brendan]
no flags Details | Diff | Review
fix bug and bogo-assert reported in comment 196 (553.94 KB, patch)
2009-03-30 05:04 PDT, Brendan Eich [:brendan]
no flags Details | Diff | Review
refreshed patch (553.94 KB, patch)
2009-03-30 17:48 PDT, Brendan Eich [:brendan]
no flags Details | Diff | Review
fix unit test bug sayrer reported (554.41 KB, patch)
2009-03-31 17:55 PDT, Brendan Eich [:brendan]
no flags Details | Diff | Review
upvar with windows fixes (711.60 KB, patch)
2009-03-31 18:40 PDT, Robert Sayre
no flags Details | Diff | Review
KONST in honor of MSFT; VariablesParser will be better in a cleanup pass later (554.86 KB, patch)
2009-03-31 23:35 PDT, Brendan Eich [:brendan]
no flags Details | Diff | Review
fix lexdep redirection bug + better msvc/windows.h portability (554.86 KB, patch)
2009-04-01 17:41 PDT, Brendan Eich [:brendan]
no flags Details | Diff | Review
rebased (553.82 KB, patch)
2009-04-02 08:42 PDT, Brendan Eich [:brendan]
no flags Details | Diff | Review
fix bug reported in comment 215 (554.83 KB, patch)
2009-04-02 17:03 PDT, Brendan Eich [:brendan]
no flags Details | Diff | Review
followup fix to remove bogus off-by-1-level code in FindFunArgs (554.35 KB, patch)
2009-04-02 19:03 PDT, Brendan Eich [:brendan]
no flags Details | Diff | Review
fix hole in funarg escape analysis, and refresh to tm tip (560.21 KB, patch)
2009-04-03 02:35 PDT, Brendan Eich [:brendan]
no flags Details | Diff | Review
fix bug from comment 224 (560.26 KB, patch)
2009-04-03 12:40 PDT, Brendan Eich [:brendan]
mrbkap: review+
Details | Diff | Review
attempt to recover and refresh patch that was backed out (559.88 KB, patch)
2009-04-04 15:13 PDT, Brendan Eich [:brendan]
no flags Details | Diff | Review
plain diff of last two patches (4.60 KB, patch)
2009-04-04 15:20 PDT, Brendan Eich [:brendan]
no flags Details | Diff | Review
fix dumb bugs in analysis (560.43 KB, patch)
2009-04-05 14:57 PDT, Brendan Eich [:brendan]
no flags Details | Diff | Review
don't forget to check analyzeFunction's new bool return value (560.50 KB, patch)
2009-04-05 15:03 PDT, Brendan Eich [:brendan]
mrbkap: review+
Details | Diff | Review

Description Boris Zbarsky [:bz] 2008-08-27 14:19:06 PDT
On the attached testcase we fall off the jit because of the heavyweight function square().  Blake tells me making it not heavyweight is Hard, but would it be possible to jit it anyway?

If the answer is "no" I can live with that, since this isn't really the right way to get this code to be fast anyway.

js shell testcase is attached.  Takes a while to run, and runs slower with -j than without.
Comment 1 Boris Zbarsky [:bz] 2008-08-27 14:19:43 PDT
Created attachment 335770 [details]
js shell testcase
Comment 2 Brendan Eich [:brendan] 2008-08-27 14:56:39 PDT
Ah, here is the "upvar, part deux" bug I want. See 

I have a rotted mq patch, complete with .rej files being overwritten accidentally. Grrrr. Need to get it fixed and in.

/be
Comment 3 Brendan Eich [:brendan] 2008-08-27 15:09:54 PDT
(In reply to comment #2)
> Ah, here is the "upvar, part deux" bug I want. See 

Oops, should have been "See bug 445262."

/be
Comment 4 Igor Bukanov 2008-09-24 11:44:41 PDT
A note about display optimization: why the display is per context and not per frame? With the former it is hard to optimize escaping closures that overlived the caller frames, while with the latter this is pretty straightforward. 

The idea is to store in the display pointers to arg and slots arrays of the caller's frames. When the parent frame terminates, the pointers will be replaced by the pointers to the corresponding arrays from the call object.
Comment 5 Igor Bukanov 2008-09-25 07:29:26 PDT
Here is details of the display optimization that should work with escaping closures:

1. Extend JSScript with a bitmap indicating which upper frames the function or eval script accessing.

2. When a function is created, put it on a list for each upper frame it can access and put each upper frame to an array stored in JSFunction instance. When the upper frame terminates, it will use the list to replace the frame references in the nested functions with the reference to the call object.

3. When the function starts, the reference to the array of upper frames or call objects is stored in the frame itself for faster access. Similarly, when an eval script starts, the array of the upper frames is constructed and stored.
Comment 6 Andreas Gal :gal 2008-11-26 10:19:33 PST
mrbkap, danderson and I discuss this today and propose the following approach:

Introduce a new kind of function ("middleweight functions"). Consider the following code:

function foo() { 
    var x = 4;
    var f = function { return x++; }
    var g = function { return x++; }
    return [f,g];
}

This will be compiled as follows:

When translating f and g, we recognize that x does not refer to any locally defined variable, so we follow the static scope chain and try to locate x. We find it in foo. We count the hops from f to foo (1) and emit the following bytecodes to access x from f and g:

GETUPVAR up, slot
CALLUPVAR up, slot
SETUPVAR up, slot, value

f and g are lightweight. Using *UPVAR opcodes does not make a function middleweight or heavy weight.

x in foo is accessed from a scope further down and thus foo becomes middleweight. foo is flagged middleweight while compiling f and g (are they compiled first?). This means that foo sets scopeChain, and has a reduced call object. This call object is a JSObject, but it only contains upvars. Upvars are assigned fixed slot numbers in this reduced call object. 

In foo, x is allocated into the call object only, except if foo for some other reason becomes heavyweight, in which case we use the regular SETPROP access patterns and x becomes a stack var and part of the call object. If foo is middleweight only, then we use *UPVAR with up=0 in foo to access x.

Accessing x in foo is a bit slower than if foo was lightweight, but faster than we do it right now (since foo would become heavy weight atm).

Once foo is popped of the stack, *UPVAR continues to target the call object via scopeChain in f and g, and the call object is kept alive, and f and g still see the same slot that maps to x.

middleweight functions do not make their outer functions heavy weight. However, if we decided foo has to be heavyweight for some other reason (eval), we make it a full blown heavyweight function and that will propagate as usual.

mrbkap thinks he can implement the parser side of this and I think I can do the interpreter side. Brendan, do you mind if we steal this and give it a try? Want to comment and point out anything we might have overlooked?
Comment 7 Brendan Eich [:brendan] 2008-11-26 11:49:55 PST
I do mind stealing this since sayrer just sent mail about working on blockers that are either unshippable perf regressions or correctness bugs including crash and memory safety bugs.

Adding more cases of function "weight" and more opcodes is not necessary if the common cases do not involve assignment to outer vars. Display closures are strictly better (no scope chain). I will work on this bug as other priorities allow.

/be
Comment 8 Brendan Eich [:brendan] 2008-11-26 13:25:18 PST
(In reply to comment #6)
> mrbkap, danderson and I discuss this today and propose the following approach:
> 
> Introduce a new kind of function ("middleweight functions"). Consider the
> following code:
> 
> function foo() { 
>     var x = 4;
>     var f = function { return x++; }
>     var g = function { return x++; }
>     return [f,g];
> }
> 
> This will be compiled as follows:
> 
> When translating f and g, we recognize that x does not refer to any locally
> defined variable, so we follow the static scope chain and try to locate x. We
> find it in foo. We count the hops from f to foo (1) and emit the following
> bytecodes to access x from f and g:
> 
> GETUPVAR up, slot
> CALLUPVAR up, slot

These bytecodes exist already (see the bug this was spun off from). The skip (up) and slot members are not stored as immediates, since that bloats every reference with redundant info. They're in the script's upvar table. Don't reinvent the wheel.

> SETUPVAR up, slot, value

This is a hard case. If x is mutated then we have to share it among all closures such as f and g.

> f and g are lightweight. Using *UPVAR opcodes does not make a function
> middleweight or heavy weight.

That's true already too -- please to be reading current code! I do not mean only the UPVAR code used (currently) by eval called from a function activation. It's also true of any lightweight function that looks at outer names. Such a function is not heavyweight, but if it uses non-local names, its enclosing function will become heavyweight (and either uses-non-locals or heavyweight inner makes outer heavyweight, on up to the global).

This is sub-optimal for the case of outers that do not use eval. We grew hair on eval (unintended hair) that made even indirect eval reify the call object at runtime, so it was impossible to decide what variables were free in a given inner function. But we should break that and match ES3.1: indirect eval uses global scope and variable object only.

If we do that, then an inner using a non-local name should not force its immediately enclosing outer function to be heavyweight, if the non-local name is not bound lexically in any enclosing function. Such free variables must be globals, or else reference errors.

> x in foo is accessed from a scope further down and thus foo becomes
> middleweight. foo is flagged middleweight while compiling f and g (are they
> compiled first?). This means that foo sets scopeChain, and has a reduced call
> object. This call object is a JSObject, but it only contains upvars. Upvars are
> assigned fixed slot numbers in this reduced call object.

This is unnecessary since x is a var and vars (and formal args) have fixed slots in call objects. Please read existing code before duplicating effort! Igor already optimized call object layout in JS1.8 / Firefox 3.

The term "upvar" applies to the callee, not the caller, in any case. foo has vars which may be upvars in f, g, or the like. These are not foo's "upvars", which if they existed would be references to variables in an outer function.

> In foo, x is allocated into the call object only, except if foo for some other
> reason becomes heavyweight, in which case we use the regular SETPROP

SETLOCAL?

> access
> patterns and x becomes a stack var and part of the call object.

We don't need this case, since we have heavyweight machinery already and it is "optimal enough" in the particular case at hand as what you propose. Since x is not used in the body of foo, its stack slot is allocated and set to undefineda nd then to 4, but not otherwise used. The 4 is sync'ed back to x's call object slot when the activation of foo returns.

> If foo is
> middleweight only, then we use *UPVAR with up=0 in foo to access x.

This is suboptimal and code bloat compared to the existing situation where we can use SETLOCAL.
 
> Accessing x in foo is a bit slower than if foo was lightweight, but faster than
> we do it right now (since foo would become heavy weight atm).

This is premature optimization. You have a call object no cheaper than today's. You actually deoptimize the initializing SETLOCAL that results from var x = 4. This does not make sense.

> Once foo is popped of the stack, *UPVAR continues to target the call object via
> scopeChain in f and g, and the call object is kept alive, and f and g still see
> the same slot that maps to x.

This machinery already exists.

If we do all this, we still lose at 3d-raytrace.js because it has an upward funarg that uses only write-once "constants" (grey and green, upvars in the floorShader function in function raytraceScene). Our ability to trace is not helped if we have to reify call objects.

The answer for 3d-raytrace.js and most cases is display closures, which copy write-once, already-written upvars down into the closure's slots. This gives faster access through compile-time slot computation, without requiring call objects to be reified from frames that are not created as the JIT inlines.

But computing display closure slots requires assignment analysis, which I have a partial patch for in my mq. I'll dust it off as time allows, not starve it but not preempt more pressing bug work for it. Strongly suggest everyone do the same, or sayrer and I will come after you :-/.

/be
Comment 9 Andreas Gal :gal 2008-11-26 14:51:17 PST
(In reply to comment #8)
> (In reply to comment #6)
> > mrbkap, danderson and I discuss this today and propose the following approach:
> > 
> > Introduce a new kind of function ("middleweight functions"). Consider the
> > following code:
> > 
> > function foo() { 
> >     var x = 4;
> >     var f = function { return x++; }
> >     var g = function { return x++; }
> >     return [f,g];
> > }
> > 
> > This will be compiled as follows:
> > 
> > When translating f and g, we recognize that x does not refer to any locally
> > defined variable, so we follow the static scope chain and try to locate x. We
> > find it in foo. We count the hops from f to foo (1) and emit the following
> > bytecodes to access x from f and g:
> > 
> > GETUPVAR up, slot
> > CALLUPVAR up, slot
> 
> These bytecodes exist already (see the bug this was spun off from). The skip
> (up) and slot members are not stored as immediates, since that bloats every
> reference with redundant info. They're in the script's upvar table. Don't
> reinvent the wheel.

Yeah I know about the existing bytecodes. I was trying to describe the mechanism, not the implementation steps. I have no opinion on the upvar table. Its an indirection vs inline data. The notation below doesn't imply a preference for either approach.

> 
> > SETUPVAR up, slot, value
> 
> This is a hard case. If x is mutated then we have to share it among all
> closures such as f and g.
> 
> > f and g are lightweight. Using *UPVAR opcodes does not make a function
> > middleweight or heavy weight.
> 
> That's true already too -- please to be reading current code! I do not mean
> only the UPVAR code used (currently) by eval called from a function activation.
> It's also true of any lightweight function that looks at outer names. Such a
> function is not heavyweight, but if it uses non-local names, its enclosing
> function will become heavyweight (and either uses-non-locals or heavyweight
> inner makes outer heavyweight, on up to the global).
> 

I didn't mean to say the inner one is currently heavyweight. My point was that neither needs to be heavyweight.

> This is sub-optimal for the case of outers that do not use eval. We grew hair
> on eval (unintended hair) that made even indirect eval reify the call object at
> runtime, so it was impossible to decide what variables were free in a given
> inner function. But we should break that and match ES3.1: indirect eval uses
> global scope and variable object only.
> 
> If we do that, then an inner using a non-local name should not force its
> immediately enclosing outer function to be heavyweight, if the non-local name
> is not bound lexically in any enclosing function. Such free variables must be
> globals, or else reference errors.

Is it ok to bind to declared globals? Or would the global script in this case behave like a function and you have to make it heavyweight? Or in other words can a function escape the global script and survive?

> 
> > x in foo is accessed from a scope further down and thus foo becomes
> > middleweight. foo is flagged middleweight while compiling f and g (are they
> > compiled first?). This means that foo sets scopeChain, and has a reduced call
> > object. This call object is a JSObject, but it only contains upvars. Upvars are
> > assigned fixed slot numbers in this reduced call object.
> 
> This is unnecessary since x is a var and vars (and formal args) have fixed
> slots in call objects. Please read existing code before duplicating effort!
> Igor already optimized call object layout in JS1.8 / Firefox 3.

I have no strong opinion against a full call objects for functions with downvars (you made me not use upvars, so now you have to live with my randomly invented terms :) ), but it seems wasteful. We know exactly which variables have upvars pointing to them. But on the other hand the parsing order might require allocating all slots, and its probably not a significant overhead, so again either way would be fine I think.

> 
> The term "upvar" applies to the callee, not the caller, in any case. foo has
> vars which may be upvars in f, g, or the like. These are not foo's "upvars",
> which if they existed would be references to variables in an outer function.
> 
> > In foo, x is allocated into the call object only, except if foo for some other
> > reason becomes heavyweight, in which case we use the regular SETPROP
> 
> SETLOCAL?
> 
> > access
> > patterns and x becomes a stack var and part of the call object.
> 
> We don't need this case, since we have heavyweight machinery already and it is
> "optimal enough" in the particular case at hand as what you propose. Since x is
> not used in the body of foo, its stack slot is allocated and set to undefineda
> nd then to 4, but not otherwise used. The 4 is sync'ed back to x's call object
> slot when the activation of foo returns.

Yeah, we talked about this path. I wonder why its preferred? Parsing order might be an issue (if we don't see all upvars first then we can't decide where to generate different code), and SETLOCAL does it dynamically so parsing order is irrelevant. 

> 
> > If foo is
> > middleweight only, then we use *UPVAR with up=0 in foo to access x.
> 
> This is suboptimal and code bloat compared to the existing situation where we
> can use SETLOCAL.

*UPVAR seems faster to me than SETLOCAL since SETLOCAL updates locally as well. I am not sure how frequently downvars are used in the defining scopes. It might not matter much.

> 
> > Accessing x in foo is a bit slower than if foo was lightweight, but faster than
> > we do it right now (since foo would become heavy weight atm).
> 
> This is premature optimization. You have a call object no cheaper than today's.
> You actually deoptimize the initializing SETLOCAL that results from var x = 4.
> This does not make sense.

What exactly does this optimize for? Mostly read-only downvars where we can use the local to read but write through to the call object on set? Is that the most frequent use-case?

> 
> > Once foo is popped of the stack, *UPVAR continues to target the call object via
> > scopeChain in f and g, and the call object is kept alive, and f and g still see
> > the same slot that maps to x.
> 
> This machinery already exists.
> 
> If we do all this, we still lose at 3d-raytrace.js because it has an upward
> funarg that uses only write-once "constants" (grey and green, upvars in the
> floorShader function in function raytraceScene). Our ability to trace is not
> helped if we have to reify call objects.

Why not? We can trace *UPVAR like any_prop or array accesses and box/unbox on write/read. Its not optimal since we have to box, which might be especially bad for doubles (GC garbage accumulates). Otoh the types will likely be very stable so we don't need to split off a lot of traces. But tracing at all beats by a lot what we do now, which is aborting :(

> 
> The answer for 3d-raytrace.js and most cases is display closures, which copy
> write-once, already-written upvars down into the closure's slots. This gives
> faster access through compile-time slot computation, without requiring call
> objects to be reified from frames that are not created as the JIT inlines.

Absolutely. Display closure solve this more elegantly, but are not on the radar short-term.

> 
> But computing display closure slots requires assignment analysis, which I have
> a partial patch for in my mq. I'll dust it off as time allows, not starve it
> but not preempt more pressing bug work for it. Strongly suggest everyone do the
> same, or sayrer and I will come after you :-/.
> 
> /be

We didn't make progress on this in 3 month because the good solution is really hard. I think a simple solution that enables tracing would be nice. We can't meaningfully trace heavyweight functions, but we can trace call objects as long they are accessed in a somewhat more predictable way (*UPVAR). The current *UPVAR code only works at recording time and only for as many frames up as inliningDepth. I think improving that is worth it until we get copying closures.
Comment 10 Brendan Eich [:brendan] 2008-11-26 16:54:07 PST
(In reply to comment #9)
> I didn't mean to say the inner one is currently heavyweight. My point was that
> neither needs to be heavyweight.

Heavy or middle, you proposed a call object, we have a call object that copies locals into its slots when the frame goes away. So I claim we don't need a "middle" weight.

But, this is not enough for tracing in general.

> > is not bound lexically in any enclosing function. Such free variables must be
> > globals, or else reference errors.
> 
> Is it ok to bind to declared globals? Or would the global script in this case
> behave like a function and you have to make it heavyweight? Or in other words
> can a function escape the global script and survive?

Yes, a function can and often does escape its declaring script. You have to look up globals at runtime.

ES-Harmony has a "use lexical scope" (placeholder syntax, the semantics are the key idea, not the bikeshed color) proposal that fixes this to give immutable and purely lexical global bindings:

http://wiki.ecmascript.org/doku.php?id=strawman:lexical_scope

> I have no strong opinion against a full call objects for functions with
> downvars (you made me not use upvars, so now you have to live with my randomly
> invented terms :) ), but it seems wasteful.

It's what you proposed!

The locals have to be stored somewhere in the heap. That means an object. It should be done using available fslots+dslots. Igor's code does exactly this via the call object. What's the problem?

> We know exactly which variables
> have upvars pointing to them.

If we do more analysis in the front end, and change indirect eval to be global eval, then we do know the bindings and their assignments.

> But on the other hand the parsing order might
> require allocating all slots, and its probably not a significant overhead, so
> again either way would be fine I think.

Premature optimization is the root of all evil.

Also, we definitely need another pass to catch all uses including forward refs (var declaration after use in same scope).

> Yeah, we talked about this path. I wonder why its preferred? Parsing order
> might be an issue (if we don't see all upvars first then we can't decide where
> to generate different code), and SETLOCAL does it dynamically so parsing order
> is irrelevant.

We have two passes already, but only within each function. We do not have inter-function analysis yet, ignoring the limited UPVAR-from-eval-in-function hacking done for date-format-tofte.js. This bug is about doing the full analysis.

> *UPVAR seems faster to me than SETLOCAL since SETLOCAL updates locally as well.

No, SETLOCAL only updates the stack. Once the frame has popped, the code in the function itself is done running (generators save the frame in their private data).

An inner function will use SETNAME, but we aren't talking about that case here, since you wrote "we use *UPVAR with up=0 in foo to access x." The issue was foo using SETUPVAR vs. SETLOCAL. Any improvement to inner function code generation to use SETUPVAR instead of SETNAME is not at issue -- the foo function's opcode for its own var x = 4 is the issue.

> I am not sure how frequently downvars are used in the defining scopes. It might
> not matter much.

Good question, but we know for benchmarks what matters: the upvars for inner functions that escape their scopes are constant, so display closures win. Any call object overhead will hurt us compared to the competition.

> > > Accessing x in foo is a bit slower than if foo was lightweight, but faster than
> > > we do it right now (since foo would become heavy weight atm).
> > 
> > This is premature optimization. You have a call object no cheaper than today's.
> > You actually deoptimize the initializing SETLOCAL that results from var x = 4.
> > This does not make sense.
> 
> What exactly does this optimize for? Mostly read-only downvars where we can use
> the local to read but write through to the call object on set? Is that the most
> frequent use-case?

That's not the issue, because most upvar cases we care about only read (do not write) in the inner function after writing once in the outer. I was responding to your suggestion of *UPVAR with "up"=0, which adds code to cover a case we already cover with call objects.

Here's the sample program (with typo fixes):

$ cat /tmp/a.js
function foo() { 
    var x = 4;
    var f = function() { return x++; }
    var g = function() { return x++; }
    return [f,g];
}
$ ./Darwin_DBG.OBJ/js -f /tmp/a.js -
js> dis(foo)
flags: HEAVYWEIGHT INTERPRETED
main:
00000:  int8 4
00002:  setlocal 0
00005:  pop
00006:  anonfunobj (function () {return x++;})
00009:  setlocal 1
00012:  pop
00013:  anonfunobj (function () {return x++;})
00016:  setlocal 2
00019:  pop
00020:  newinit 3
00022:  zero
00023:  getlocal 1
00026:  initelem
00027:  one
00028:  getlocal 2
00031:  initelem
00032:  endinit
00033:  return
00034:  stop

Source notes:
  0:     0 [   0] newline 
  1:     2 [   2] decl     offset 0
  3:     6 [   4] newline 
  4:     9 [   3] decl     offset 0
  6:    13 [   4] newline 
  7:    16 [   3] decl     offset 0
  9:    20 [   4] newline 

It is true that we write a stack slot (setlocal 0) and then copy that value to a call object slot when foo returns. Optimizing this to one write to a middle-weight call object is premature at best, given the code added. OTOH the cost to all such functions in loss of the setlocal, etc. ops is potentially high.

There are some upward funarg examples in benchmarks. But there are many nested function examples in benchmarks, Ajax libraries, and web apps. We must not de-optimize the latter cases just in case it might help the upward funarg cases.

> > If we do all this, we still lose at 3d-raytrace.js because it has an upward
> > funarg that uses only write-once "constants" (grey and green, upvars in the
> > floorShader function in function raytraceScene). Our ability to trace is not
> > helped if we have to reify call objects.
> 
> Why not? We can trace *UPVAR like any_prop or array accesses and box/unbox on
> write/read.

We cannot build call objects on trace without frames, which we do not create when inlining while tracing.

In 3d-raytrace.js there is a single call object for raytraceScene, which is long-lived (raytraceScene is called from global code at test startup). So that would be helped but GETUPVAR, you're right. But we don't need SETUPVAR at all.

If all we cared about was 3d-raytrace.js, then throwing more code at GETUPVAR and eating the call object costs might be enough. I don't want to tune for the benchmark at this point, if I can do better.

> Its not optimal since we have to box, which might be especially bad
> for doubles (GC garbage accumulates). Otoh the types will likely be very stable
> so we don't need to split off a lot of traces. But tracing at all beats by a
> lot what we do now, which is aborting :(

In general this ignores the issue of where the boxed values would be stored: no call object would be created if the outer function were being inlined by the trace, not called outside the trace once only.

> Absolutely. Display closure solve this more elegantly, but are not on the radar
> short-term.

Why assume that? If we do anything we should do the right thing. It's still doable (more below).

We've had too many short-cuts backfire in TM, IMHO (I'm to blame for more than my share, I'm sure, so don't take this personally -- it's self-directed!).

At this point I'd rather work on fixing this bug than arguing unnecessarily. We can blow more ops on SETUPVAR, etc. but in practice these ops won't be needed (because upvar exclusive-read scenarios dominate in benchmarks). We still need enough analysis to find the outer bindings. This suggests to me that display closures are the better fix.

> We didn't make progress on this in 3 month because the good solution is really
> hard.

Not true. I didn't work on this because of intrinsic hardness; rather, I worked on higher priority bugs.

I agree with sayrer that we have too many bugs in this list:

https://bugzilla.mozilla.org/buglist.cgi?cmdtype=dorem&remaction=run&namedcmd=blocking1.9.1%20JS&sharer_id=20209

to be laying on more optimizations right now, but I intend to fix this bug once that list is much shorter, which is why I've kept it assigned.

/be
Comment 11 Brendan Eich [:brendan] 2008-11-26 17:22:43 PST
I wrote:
> In 3d-raytrace.js there is a single call object for raytraceScene, which is
> long-lived (raytraceScene is called from global code at test startup). So that
> would be helped by GETUPVAR, you're right.

The current tracer code for JSOP_NAME, etc. funnels control flow through TraceRecorder::activeCallOrGlobalSlot, which will get call object slots if the call object's frame is in range of callDepth, starting from cx->fp. To make GETUPVAR work from the current frame into a frame that was popped, we would follow the scope chain (static link), not the frame chain (dynamic link).

This analysis requires no front-end work if done at trace time, since scoping is mostly static in JS. We would need to be sure the call object up 1 on the scope chain (static link) is going to be the same object on trace. Guarding on function identity when inlining suffices for 3d-raytrace.js's floorShader nested in raytraceScene case.

There is a counter-example where we start tracing in the inner function and the outer could vary, so we can't embed the outer's call object as a constant in the JITted code. Is it sufficient to test callDepth == 0 to distinguish this hard case from the 3d-raytrace one?

/be
Comment 12 Brendan Eich [:brendan] 2008-11-26 17:26:32 PST
In general upward funargs can escape arbitrary call objects, be invoked either from trace-recorded callers or start recording in their own bodies, and the value of callDepth at recording time won't tell us whether we can hard-code the call object on trace. If callDepth > 0 we know recording inlined into the upward funarg but we don't know what outer function with which call object it escaped.

So we would need to navigate the scope chain in generated code. Sorry if this is obvious, I was hoping for better.

/be
Comment 13 Brendan Eich [:brendan] 2008-12-26 14:11:49 PST
Better is coming, ho ho ho.

/be
Comment 14 Christopher Blizzard (:blizzard) 2009-01-05 12:16:48 PST
Some kind of JavaScript Christmas miracle?
Comment 15 Brendan Eich [:brendan] 2009-01-05 19:25:41 PST
Created attachment 355512 [details] [diff] [review]
backup of work in progress
Comment 16 Brendan Eich [:brendan] 2009-01-05 22:08:58 PST
Created attachment 355527 [details] [diff] [review]
backup, v2
Comment 17 Brendan Eich [:brendan] 2009-01-11 02:46:30 PST
Created attachment 356403 [details] [diff] [review]
backup, v3
Comment 18 Brendan Eich [:brendan] 2009-01-12 15:38:56 PST
Created attachment 356611 [details] [diff] [review]
backup, v4

Getting into closure runtime finally -- JSOP_LAMBDA with JSOP_CALLEE replace JSOP_ANONFUNOBJ and JSOP_NAMEDFUNOBJ.

Draft ES3.1 fixes the ES3 bug whereby you can override Object and abuse a named function expression or a catch block, by making the named function expression bind its name lexically and immutably. 3.1 makes rebinding via an eval in the function, or a var declaration, a no-op in default mode and an error in strict mode.

This patch makes the var declaration of the function's name shadow the name, as ES3 did. For eval, we throw a redeclaration error, since the function name binding appears as if it were made using const, and you can't redeclare const.

I'm thinking about how to reconcile 3.1 and this patch. It isn't obvious to me that 3.1 has it right in pre-binding the name, and silently refusing to update its value (if non-strict), in both the var and eval-of-var cases. Comments?

/be
Comment 19 Brendan Eich [:brendan] 2009-01-12 18:03:46 PST
Created attachment 356637 [details] [diff] [review]
backup, v5

C++ rulez, C macros drool! Srsly, there was a bad macro actual parameter that was completely undetected in past patches (interdiff readers can spot it).

/be
Comment 20 Brendan Eich [:brendan] 2009-01-26 16:38:02 PST
*** Bug 458838 has been marked as a duplicate of this bug. ***
Comment 21 Mike Shaver (:shaver -- probably not reading bugmail closely) 2009-01-26 17:00:01 PST
Is there anything helpful that can be done to expedite the next patch?  I hear it's awesome, but we're in overtime for b3, and I don't really want to imagine a non-upvar FF3.1 too vividly. :-/
Comment 22 Brendan Eich [:brendan] 2009-01-26 17:38:39 PST
New patch coming soon, later tonight I hope. Testing help when it arrives would be great.

/be
Comment 23 Mike Shaver (:shaver -- probably not reading bugmail closely) 2009-01-27 07:57:59 PST
Awesome, operators are standing by.
Comment 24 Brendan Eich [:brendan] 2009-02-06 19:18:34 PST
Created attachment 361019 [details] [diff] [review]
backup, v6

Painful -- I just re-based (finally got .rej files due to FE changes by others).

More later tonight, I will get this up and passing the testsuite without emitting the new opcodes, then start turning them on.

/be
Comment 25 Brendan Eich [:brendan] 2009-02-09 00:18:31 PST
Created attachment 361232 [details] [diff] [review]
backup, v7

Very close now, will have testable patch tomorrow. Need sleep, sorry for delays -- whole-family flu finally going away.

/be
Comment 26 Brendan Eich [:brendan] 2009-02-09 00:27:57 PST
Created attachment 361236 [details] [diff] [review]
backup, v7a
Comment 27 Brendan Eich [:brendan] 2009-02-09 19:07:17 PST
Created attachment 361452 [details] [diff] [review]
v8

This is sort of working... lightly tested:

function f(x){function g(y)x+y;return g}

js> f
function f(x) {

    function g(y) x + y;

    return g;
}
js> dis(f)
flags: NULL_CLOSURE
main:
00000:  deflocalfun_fc 0 function g(y) x + y;
00005:  nop
00006:  getlocal 0
00009:  return
00010:  stop

Source notes:
  0:     5 [   5] funcdef  function 0 (function g(y) x + y;)
js> g = f(2)
function g(y) x + y;
js> g(3)
5
js> 

/be
Comment 28 Brendan Eich [:brendan] 2009-02-10 01:27:17 PST
Created attachment 361483 [details] [diff] [review]
traces bz's testcase

Nice speedup on the attached testcase: 895ms without -j, 394ms with -j in an opt js shell on my MBP. I got tired of waiting for the times from an unpatched opt js shell.

More testing and review needed, I see a few things in the patch that aren't quite right yet. I can't claim this is ready for fuzzers until it passes the suite and mochitests, but if you are looking for adventure, try it and find me on IRC at the first sign of trouble compared to a tm tip shell (this patch was just rebased).

/be
Comment 29 Mike Shaver (:shaver -- probably not reading bugmail closely) 2009-02-10 04:50:24 PST
Patch has a trivial (whitespace?) conflict with the opcountectomy, but also seems to remove imacros.c.out wholesale.  Clearly, I should have saved off a shell with which to rebuild imacros.c.out before I clobbered. :)
Comment 30 Mike Shaver (:shaver -- probably not reading bugmail closely) 2009-02-10 05:16:20 PST
Duplicate variable declarations trip this assertion:

js> function f() { var i = 0; var i = 5; }
Assertion failure: dn->pn_defn, at ../jsemit.cpp:1899
Trace/BPT trap

(Such duplication can be found in 3d-cube.js's CalcNormal, among other illustrious locations.)
Comment 31 Mike Shaver (:shaver -- probably not reading bugmail closely) 2009-02-10 05:48:30 PST
More early-morning results, dups in SS and v8-suite suppressed:

../t/3d-morph.js:33: TypeError: sin is not a function

(gdb) run ../t/3d-raytrace.js  # when analyzing 'intersect'
Program received signal EXC_BAD_ACCESS, Could not access memory.
Reason: KERN_PROTECTION_FAILURE at address: 0x0000001c
JSCompiler::analyzeFunctions (this=0xbffff48c, funpob=0x822440) at ../jsparse.cpp:1454
1454	                        while (afunpob->level != lexdep->frameLevel())
(gdb) p afunpob 
$1 = (JSParsedFunctionBox *) 0x0

(gdb) run ../t/crypto-md5.js 
Program received signal SIGTRAP, Trace/breakpoint trap.
JS_Assert (s=0x3c <Address 0x3c out of bounds>, file=0x3c <Address 0x3c out of bounds>, ln=60) at ../jsutil.cpp:63
63	    abort();
(gdb) up
#1  0x0009df2d in JSParseNode::append () at jsparse.h:3822
3822	                catchList->append(pnblock);

(gdb) run ../t/date-format-tofte.js # pn is formatDate :: a :: self
Assertion failure: FUN_KIND(cg->fun) == JSFUN_INTERPRETED, at ../jsemit.cpp:1932

../t/date-format-xparb.js:45: TypeError: code is undefined

../t/math-cordic.js:92: TypeError: end.getTime is not a function

(gdb) run ../t/math-partial-sums.js 
Assertion failure: slot < script->nslots, at ../jsinterp.cpp:5571

../t/string-fasta.js:53: TypeError: s is undefined

(gdb) run ../t/string-tagcloud.js 
Assertion failure: FUN_KIND(cg->fun) == JSFUN_INTERPRETED, at ../jsemit.cpp:1932
# referencing parseJSON :: walk within itself

(gdb) run ../t/string-unpack-code.js 
Assertion failure: OBJ_GET_PARENT(cx, obj) == parent, at ../jsinterp.cpp:5991
(gdb) p/x obj->fslots[1]
$13 = 0x1f8000
(gdb) p/x parent
$14 = 0x0

(gdb) run ../v8/earley-boyer.js 
Assertion failure: EmitEnterBlock, at ../jsemit.cpp:1851
1851	        JS_NOT_REACHED("EmitEnterBlock");
# catch block within sc_withHandlerLambda

../v8/richards.js:38: ReferenceError: BenchmarkSuite is not defined
Comment 32 Brendan Eich [:brendan] 2009-02-10 19:01:51 PST
Created attachment 361696 [details] [diff] [review]
working thru t/*.js, at date-format-tofte.js

Have to cope with forward upvar refs, in tofte's case to var self. Who invented this hoisting crap? :-P

/be
Comment 33 Gary Kwong [:gkw] [:nth10sd] 2009-02-10 19:17:57 PST
(In reply to comment #32)
> Created an attachment (id=361696) [details]
> working thru t/*.js, at date-format-tofte.js
> 
> Have to cope with forward upvar refs, in tofte's case to var self. Who invented
> this hoisting crap? :-P
> 
> /be

Perhaps you know this, fyi, jsfunfuzz's hitting this same assertion continuously with this patch: Assertion failure: EmitEnterBlock, at ../jsemit.cpp:1851 in debug TM, a "too much recursion" error in opt TM (I don't know if they're the same problem.

and it's also found in shaver's results.

(gdb) run ../v8/earley-boyer.js 
Assertion failure: EmitEnterBlock, at ../jsemit.cpp:1851
1851            JS_NOT_REACHED("EmitEnterBlock");
# catch block within sc_withHandlerLambda
Comment 34 Brendan Eich [:brendan] 2009-02-11 11:51:00 PST
Created attachment 361810 [details] [diff] [review]
compiles and runs ss, debugging v8/earley-boyer.js
Comment 35 Andreas Gal :gal 2009-02-11 12:58:23 PST
Brendan, the latest patch crashes in 3d-raytrace DBG and OPT with JIT enabled (its fine with jit off). The crash happens somewhere in the generated code.
Comment 36 Brendan Eich [:brendan] 2009-02-11 13:43:08 PST
Created attachment 361834 [details] [diff] [review]
fix all v8 but raytrace.js (interp only -- yeah, known jit bugs)

Or at least, I know there are jit bugs -- haven't debugged them. Please feel free to try this patch (rebased just now) and run ahead.

/be
Comment 37 Boris Zbarsky [:bz] 2009-02-11 14:32:50 PST
Created attachment 361852 [details]
Version of the testcase that runs faster (by a factor of 150 or so) and detects correctness
Comment 38 Jesse Ruderman 2009-02-11 15:03:47 PST
[0 for (a in [])]

Assertion failure: pn->pn_type == TOK_LET, at ../jsemit.cpp:1838
Comment 39 Jesse Ruderman 2009-02-11 15:05:12 PST
const e = 0; print(++e);

Without patch: 0
With patch: 1
Comment 40 Jesse Ruderman 2009-02-11 15:22:15 PST
This crashes in js_Interpret:

function m()
{
	function a() { }
	function b() { a(); }
	this.c = function () { b(); }
}
(new m).c();
Comment 41 Jesse Ruderman 2009-02-11 15:25:08 PST
Comment 40 is reduced from the mersenne twister implementation that is part of jsfunfuzz.  jsfunfuzz can't run at all unless I change it.
Comment 42 Blake Kaplan (:mrbkap) (please use needinfo!) 2009-02-11 17:10:11 PST
Comment on attachment 361834 [details] [diff] [review]
fix all v8 but raytrace.js (interp only -- yeah, known jit bugs)

>+                if (nupvars == 0) {
>+                    FUN_METER(onlyfreevar);
>+                    FUN_SET_KIND(fun, JSFUN_NULL_CLOSURE);
>+                } else if (!setUpvar) {
>+                    /*
>+                     * Algol-like functions can read upvars using the dynamic
>+                     * link (cx->fp/fp->down). They do not need to entrain and
>+                     * search their environment.
>+                     */
>+                    FUN_METER(display);
>+                    FUN_SET_KIND(fun, JSFUN_NULL_CLOSURE);

Is this a copy/paste bug? It looks like this wants to be a JSFUN_FLAT_CLOSURE. If I make it one, then Jesse's reduced testcase runs without crashing.
Comment 43 Blake Kaplan (:mrbkap) (please use needinfo!) 2009-02-11 17:22:16 PST
Actually, it looks like the problem is that we don't notice that |b| escapes through the anonymous function that is later assigned to |this.c|.
Comment 44 Brendan Eich [:brendan] 2009-02-11 18:19:26 PST
Right, the analysis is buggy but the NULL_CLOSURE judgment is correct -- no need for fat scope chain if the function does not escape but is only called with the frame stack holding its upvars.

/be
Comment 45 Andreas Gal :gal 2009-02-11 20:28:28 PST
We can bake in callee directly, don't look it up via the rp stack (which doesn't work for calldepth 0 anyway).

--- b/js/src/jstracer.cpp
+++ b/js/src/jstracer.cpp
@@ -7310,12 +7310,9 @@
 JS_REQUIRES_STACK bool
 TraceRecorder::record_JSOP_GETDSLOT()
 {
-    LIns* fi_ins = lir->insLoadi(lirbuf->rp, callDepth * sizeof(FrameInfo*));
-    LIns* callee_ins = lir->insLoadi(fi_ins, offsetof(FrameInfo, callee));
-
     unsigned index = GET_UINT16(cx->fp->regs->pc);
     LIns* dslots_ins = NULL;
-    LIns* v_ins = stobj_get_dslot(callee_ins, index, dslots_ins);
+    LIns* v_ins = stobj_get_dslot(INS_CONSTPTR(cx->fp->callee), index, dslots_ins);
 
     stack(0, v_ins);
     return true;
Comment 46 Brendan Eich [:brendan] 2009-02-11 21:49:17 PST
Created attachment 361939 [details] [diff] [review]
bunch of fixes, still need to run js tests
Comment 47 Brendan Eich [:brendan] 2009-02-12 02:48:57 PST
Created attachment 361971 [details] [diff] [review]
passes trace-test.js interp (-j NanoAsserts in testSwitchUndefined)

Must sleep, this is getting close -- no ceegar yet.

/be
Comment 48 Brendan Eich [:brendan] 2009-02-13 00:53:28 PST
Created attachment 362191 [details] [diff] [review]
passes trace-test.js with -j, passes bench.sh, passes v8 w/&w/o -j

Jesse, this does the right thing with the test reduced from Mersenne Twister, and variations mrbkap and I discussed today. I'm too tired to run the suite right now but you fuzzer kids, go nuts.

/be
Comment 49 Brendan Eich [:brendan] 2009-02-13 00:54:55 PST
(In reply to comment #47)
> Created an attachment (id=361971) [details]
> passes trace-test.js interp (-j NanoAsserts in testSwitchUndefined)

That NanoAssert was fixed by http://hg.mozilla.org/tracemonkey/rev/c31a7fa98db3.

Latest patch was just rebased.

/be
Comment 50 Jason Orendorff [:jorendorff] 2009-02-13 07:45:19 PST
Gary found some crashes:

// compiler bug when a block introduces no names
let ({}={}) {}

// compiler incorrectly emits GETLOCAL for first `x`,
// triggering decompiler assertion
while (x.y) { let x; }
Comment 51 Jason Orendorff [:jorendorff] 2009-02-13 08:10:47 PST
This one too:

// Assertion failure: UPVAR_FRAME_SKIP(uva->vector[i]) == 0
// at ../jsopcode.cpp:2791
//
// when decompiling the eval code, which is:
//
// main:
// 00000:  10  getupvar 0
// 00003:  10  getprop "y"
// 00006:  10  popv
// 00007:  10  stop
function f() { var x; eval("x.y"); }
f();
Comment 52 Jason Orendorff [:jorendorff] 2009-02-13 09:47:57 PST
Another batch, more to come.

// Crash in NoteLValue, called from BindDestructuringVar.
// NoteLValue assumes pn->pn_lexdef is non-null, but here
// pn is itself the definition of x.
for (var [x]=0 in null) ;

// This one only crashes when executed from a file.
// Assertion failure: pn != dn->dn_uses, at ../jsparse.cpp:1131
for (var f in null)
    ;
var f = 1;
(f)

// Assertion failure: pnu->pn_cookie == FREE_UPVAR_COOKIE, at ../jsemit.cpp:1815
// In EmitEnterBlock. x has one use, which is pnu here.
// pnu is indeed a name, but pnu->pn_cookie is 0.
let (x = 1) { var x; }

// Assertion failure: cg->upvars.lookup(atom), at ../jsemit.cpp:1992
// atom="x", upvars is empty.
(1 for each (x in x));

// Assertion failure: pn_arity == PN_FUNC || pn_arity == PN_NAME, at ../jsparse.h:444
// Here the function node has been morphed into a JSOP_TRUE node, but we're in
// FindFunArgs poking it anyway.
while(function(){});
Comment 53 Jason Orendorff [:jorendorff] 2009-02-13 12:58:37 PST
// Assertion failure: (slot) < (uint32)(obj)->dslots[-1]
// at ../jsobj.cpp:5559
// On the last line of BindLet, we have
//    JS_SetReservedSlot(cx, blockObj, index, PRIVATE_TO_JSVAL(pn));
// but this uses reserved slots as though they were unlimited.
// blockObj only has 2.
let (a=0, b=1, c=2) {}

// In RecycleTree at ../jsparse.cpp:315, we hit
//     JS_NOT_REACHED("RecycleUseDefKids");
// pn->pn_type is TOK_UNARYOP
// pn->pn_op   is JSOP_XMLNAME
// pn->pn_defn is 1
// pn->pn_used is 1
@foo; 0;

// Calls LinkUseToDef with pn->pn_defn == 1.
// 
// If you say "var x;" first, then run this case, it gets further,
// crashing in NoteLValue like the first case in comment 52.
//
for (var [x] = x in y) var x;

// Assertion failure: !pn2->pn_defn, at ../jsparse.h:461
// Another case where some optimization is going on.
if (true && @foo) ;

// Assertion failure: scope->object == ctor
// in js_FastNewObject at ../jsbuiltins.cpp:237
//
// In the interpreter, new-ing a function causes its .prototype property 
// to be resolved, which implies a call to js_GetMutableScope; the function
// gets its own scope.  Before this patch, we always executed that path
// in the interpreter before calling js_FastNewObject it on trace.
//
// With the patch, we're new-ing a different function each time, and the
// .prototype property is missing.
//
for (var z = 0; z < 3; z++) { (new function(){}); }
Comment 54 Jason Orendorff [:jorendorff] 2009-02-13 13:34:27 PST
OK, this is the last batch for now.

// Assertion failure: cg->flags & TCF_IN_FUNCTION
// at ../jsemit.cpp:1991
//
// Perhaps because the `let x` in the eval binds to
// the `var x` in the function, but the cg state
// doesn't indicate that we're lexically in a function.
//
// The code around this assertion is totally new and
// I don't understand it too well yet.
function f() { var x; eval("let x, x;"); }
f();

// Assertion failure: fp2->fun && fp2->script,
// at ../jsinterp.cpp:5589
//
// Here fp2 is the eval frame, so fp2->fun is NULL.
//
// But also: the function compiles incorrectly:
//   00000:   2  this
//   00001:   2  getupvar 0
//   00004:   2  pop
//   00005:   2  stop
// No assignment opcode.
//
eval("let(x) function(){ x = this; }()");
Comment 55 Brendan Eich [:brendan] 2009-02-13 19:36:30 PST
Created attachment 362358 [details] [diff] [review]
fixes comment 50 bugs, plus some things not found by fuzzers

More in a bit.

/be
Comment 56 Brendan Eich [:brendan] 2009-02-16 01:38:50 PST
Created attachment 362550 [details] [diff] [review]
fixed more, through 3 of 5 in comment 53 -- 2/5 remain + comment 54

Hoisting!!! Grrrr.

/be
Comment 57 Brendan Eich [:brendan] 2009-02-16 01:43:09 PST
The 2 of 5 from comment 53 that aren't fixed are:

for (var [x] = x in y) var x;

and (with -j):

for (var z = 0; z < 3; z++) { (new function(){}); }

/be
Comment 58 Gary Kwong [:gkw] [:nth10sd] 2009-02-16 03:19:06 PST
Keeping in mind the unfixed issues, I tried out the latest patch in comment #56 with TM tip, but it's making jsfunfuzz virtually unusable with this assertion:

function foo(x) { var x = x }

$ ./js-dbg-tm-intelmac 
js> function foo(x) { var x = x }
Assertion failure: dn->kind() == ((data->op == JSOP_DEFCONST) ? JSDefinition::CONST : JSDefinition::VAR), at ../jsparse.cpp:2595
Trace/BPT trap


The testcase is reduced from part of jsfunfuzz.
Comment 59 Brendan Eich [:brendan] 2009-02-16 10:48:54 PST
Created attachment 362598 [details] [diff] [review]
fix bogus assertion

Gary, sorry about that -- try this and find me on IRC if there are other such bogo-asserts.

/be
Comment 60 Brendan Eich [:brendan] 2009-02-16 11:45:45 PST
Created attachment 362606 [details] [diff] [review]
#ifdef __cplusplus in jsatom.h to avoid breaking stinking liveconnect

Jesse found a problem and worked around (I hope) with --disable-oji (I *wish* we could make that the default).

/be
Comment 61 Jesse Ruderman 2009-02-16 12:07:12 PST
Created attachment 362610 [details]
stacktrace+ for firefox startup assertion

Assertion failure: args.nCopiedVars == fun->u.i.nvars, at /Users/jruderman/tracemonkey/js/src/jsfun.cpp:2686
Comment 62 Gary Kwong [:gkw] [:nth10sd] 2009-02-16 18:37:26 PST
(function(){
    var x;
    this.init_by_array = function()
    x = 0;
})();


asserts at Assertion failure: ATOM_IS_STRING(atom), at ../jsinterp.cpp:5686 in jsfunfuzz's Mersenne Twister with patch in comment #60. This testcase is reduced from jsfunfuzz itself.
Comment 63 Brendan Eich [:brendan] 2009-02-17 11:32:57 PST
Fixes coming for reports from recent comments. Here's another test fixed in the forthcoming patch, which I reduced from nsSessionStore.js (which abuses var and let, and contains forward refs to deeper var -- hoisting saves these from being unintended global var assignments):

function f(that) {
    for (ix in this)
        print(ix);
    for (let ix in that)
        ;
}

I'll file a bug on nsSessionStore.js today.

/be
Comment 64 Brendan Eich [:brendan] 2009-02-17 22:42:06 PST
(In reply to comment #63)
> I'll file a bug on nsSessionStore.js today.

Bug 479005.

/be
Comment 65 Brendan Eich [:brendan] 2009-02-18 03:29:00 PST
Created attachment 362868 [details] [diff] [review]
better, but browser doesn't render and 39 tests beyond baseline fail

Not sure this is worth fuzzing. Known issue: named function expressions can't call themselves recursively yet.

/be
Comment 66 Gary Kwong [:gkw] [:nth10sd] 2009-02-18 06:21:00 PST
(In reply to comment #65)
> Created an attachment (id=362868) [details]
> better, but browser doesn't render and 39 tests beyond baseline fail
> 
> Not sure this is worth fuzzing. Known issue: named function expressions can't
> call themselves recursively yet.
> 
> /be

Yes, this is ok with jsfunfuzz, it runs properly now, testcases are now being generated correctly. More to follow.
Comment 67 Brendan Eich [:brendan] 2009-02-19 01:24:48 PST
Created attachment 363084 [details] [diff] [review]
getting there

24 tests above baseline to go...

The Function constructor implementation really ought to build a source string including formal parameters, and let the real parser do the parsing work. Instead it recapitulates part of the parser (ECMA-262 allows Function("a,b", "a+b") -- for ES4 with destructuring we were going to allow more like that, and for return type annotation we'd allow parens!). This is a FIXME to deal with later.

/be
Comment 68 Gary Kwong [:gkw] [:nth10sd] 2009-02-19 07:46:18 PST
eval("*;", (3/0 ? function(id) { return id } : <><x><y/></x></>));

=====

foo = "" + new Function("while(\u3056){let \u3056 = x}");

=====

function a(){ let c; eval("let c, y"); }
a();

=====

{x: 1e+81 ? c : arguments}

=====

(function(q){return q;} for each (\u3056 in [])) 

=====

for(let x = <{x}/> in <y/>) x2 

=====

for(
const NaN; 
this.__defineSetter__("x4", function(){}); 
(eval("", (p={})))) let ({} = (((x ))(function ([]) {})), x1) y

=====

function f(){ var c; eval("{var c = NaN, c;}"); }
f();

=====

feed this in as a .js file:


x
let(x) {
var x 

=====


These 9 testcases cause assertions for the patch in both comment #65 and comment #67. Sorry I haven't been able to catch anyone to help debug what's going wrong behind each testcase.
Comment 69 Brendan Eich [:brendan] 2009-02-20 00:05:40 PST
Created attachment 363272 [details] [diff] [review]
fixes everything from gary's last comment

I knew some of those were broken, but you can't hide from the fuzzer. Mainly Function constructor (again), for-in and C-style-for scoping, hoisting (again!), var vs. function etc. conflicts.

Down to 14 test failures, more after sleep.

/be
Comment 70 Brendan Eich [:brendan] 2009-02-20 00:11:02 PST
Created attachment 363275 [details] [diff] [review]
rebased

Will go through jorendorff's comments tomorrow too.

/be
Comment 71 Jesse Ruderman 2009-02-20 00:23:45 PST
js> x; var x
Assertion failure: pn->pn_op == JSOP_NOP, at ../jsparse.cpp:1118
Comment 72 Jesse Ruderman 2009-02-20 00:27:57 PST
js> v = function p() { delete p; };
Assertion failure: JOF_OPTYPE(op) == JOF_ATOM, at ../jsemit.cpp:1711
Comment 73 Jesse Ruderman 2009-02-20 00:28:52 PST
js> function() { var arguments }
Assertion failure: (uintN)i < ss->top, at ../jsopcode.cpp:2801

and lots of other problems with "const arguments" etc
Comment 74 Jesse Ruderman 2009-02-20 00:39:55 PST
const [d] = [1]; [d] = [2]; print(d);

without this patch: 1
with this patch: 2
Comment 75 Jesse Ruderman 2009-02-20 00:43:26 PST
js> (function p(){ p = 3; })
function p() {
    p;
}

js> (function p(){ p = 3; return p; })()
Assertion failure: regs.sp == StackBase(fp), at ../jsinterp.cpp:2980
Comment 76 Jesse Ruderman 2009-02-20 00:45:46 PST
echo "for (let d = 0; d < 4; ++d) { d; }" | ~/tracemonkey/js/src/dbg-upvar/js
1: ReferenceError: d is not defined
Comment 77 Brendan Eich [:brendan] 2009-02-20 02:14:14 PST
(In reply to comment #74)
> const [d] = [1]; [d] = [2]; print(d);
> 
> without this patch: 1
> with this patch: 2

True, but wrap it in a function and get 2 without the patch.

/be
Comment 78 Brendan Eich [:brendan] 2009-02-20 02:15:43 PST
Created attachment 363283 [details] [diff] [review]
fixes for Jesse (const [d] and the for(let...) scope prob remain)
Comment 79 Jason Orendorff [:jorendorff] 2009-02-20 09:03:22 PST
x; var x; function x() 0

Assertion failure: !(pn->pn_dflags & flag), at ../jsparse.h:635
Comment 80 Gary Kwong [:gkw] [:nth10sd] 2009-02-20 09:16:19 PST
Thanks to jorendorff for helping with my testcase above, I've found more testcases using the patch in comment #78 which will be disclosed later..
Comment 81 Brendan Eich [:brendan] 2009-02-20 10:55:05 PST
Created attachment 363341 [details] [diff] [review]
fixes comment 78 and comment 79 probs
Comment 82 Gary Kwong [:gkw] [:nth10sd] 2009-02-20 11:47:20 PST
There's more other than these below that cause assertions/crashes -- need zzz desperately though.


=====

(function(){function  x(){} {let x = [] }});

=====

var f = new Function("new function x(){ return x |= function(){} } ([], function(){})");
"" + f;

=====

var f = new Function("for(let [] = [0]; (y) = *; new (*::*)()) {}");
"" + f;

=====

uneval(function(){[y] = [x];});

=====

function g(code)
{
  var f = new Function(code);
  f();
}
g("for (var x = 0; x < 3; ++x)(new (function(){})());");

=====

(function(){new (function ({}, x) { yield (x(1e-81) for (x4 in undefined)) } )()})()

=====

(function(){[(function ([y]) { })() for each (x in [])];})();

=====

(function(){for(var x2 = [function(id) { return id } for each (x in []) if ([])] in functional) function(){};})()

=====

(function(){with(window){1e-81; }for(let [x, x3] = window -= x in []) function(){}})()

=====

for(let [
function  x () { M:if([1,,])  } 

=====

function foo(code)
{
  var c;
  eval("const c, x5 = c;");
}  
foo();

=====

var f = new Function("try { with({}) x = x; } catch(\u3056 if (function(){x = x2;})()) { let([] = [1.2e3.valueOf(\"number\")]) ((function(){})()); } ");
"" + f;

=====

var f = new Function("[] = [( '' )()];");
"" + f;

=====

var f = new Function("let ([] = [({ get x5 this (x) {}  })]) { for(let y in []) with({}) {} }");
"" + f;

=====

for(let x; 
([,,,]
.toExponential(new Function(), (function(){}))); [] = {}) 
for(var [x, x] = * in this.__defineSetter__("", function(){})) 

=====
Comment 83 Brendan Eich [:brendan] 2009-02-20 15:29:03 PST
Gary, Jesse: a bunch of those are variations on a group assignment theme, and I stupidly messed with codegen there and broke decompilation. Fixing in next patch.

A bunch more are variations on a generator expression theme, revealing the need to refactor the parser to reuse more FunctionDef code under GeneratorExpr, which is a bit of work. If it's easy to take out generator expressions from the fuzzer, and group assignment, that would help steer clear of these known bugs.

/be
Comment 84 Brendan Eich [:brendan] 2009-02-26 12:10:11 PST
Created attachment 364360 [details] [diff] [review]
passes js tests

Passes js/tests (-L lc2 lc3 spidermonkey-n.tests slow-n.tests, modulo big-O woes).

/be
Comment 85 Brendan Eich [:brendan] 2009-02-26 12:41:07 PST
Created attachment 364370 [details] [diff] [review]
rebased to tip of tm
Comment 86 Andreas Gal :gal 2009-02-26 13:13:43 PST
The patch still fails to run the sunspider test harness:

./sunspider-compare-results --shell=../tracemonkey/src/Darwin_OPT.OBJ/js
resources/sunspider-compare-results.js:97: TypeError: itemTotals1.total is undefined

It uses a lot of undeclared globals and other high quality code constructs.
Comment 87 Andreas Gal :gal 2009-02-26 13:14:42 PST
baseline vs upvar on sunspider:

TEST                   COMPARISON            FROM                 TO             DETAILS

=============================================================================

** TOTAL **:           *1.007x as slow*  1022.5ms +/- 0.1%   1030.2ms +/- 0.1%     significant

=============================================================================

  3d:                  *1.056x as slow*   152.6ms +/- 0.3%    161.1ms +/- 0.2%     significant
    cube:              *1.010x as slow*    39.8ms +/- 0.7%     40.2ms +/- 0.6%     significant
    morph:             1.008x as fast      29.0ms +/- 0.2%     28.8ms +/- 0.4%     significant
    raytrace:          *1.099x as slow*    83.8ms +/- 0.2%     92.1ms +/- 0.2%     significant

  access:              *1.023x as slow*   131.4ms +/- 0.2%    134.5ms +/- 0.2%     significant
    binary-trees:      1.005x as fast      39.2ms +/- 0.3%     39.0ms +/- 0.3%     significant
    fannkuch:          *1.051x as slow*    57.2ms +/- 0.2%     60.1ms +/- 0.2%     significant
    nbody:             *1.017x as slow*    23.8ms +/- 0.4%     24.2ms +/- 0.5%     significant
    nsieve:            -                   11.2ms +/- 1.0%     11.1ms +/- 0.9% 

  bitops:              *1.018x as slow*    34.8ms +/- 0.6%     35.5ms +/- 0.7%     significant
    3bit-bits-in-byte: *1.143x as slow*     1.5ms +/- 9.3%      1.8ms +/- 7.0%     significant
    bits-in-byte:      1.015x as fast       8.1ms +/- 1.1%      8.0ms +/- 0.5%     significant
    bitwise-and:       1.020x as fast       2.0ms +/- 0.0%      2.0ms +/- 5.0%     significant
    nsieve-bits:       *1.025x as slow*    23.2ms +/- 0.5%     23.8ms +/- 0.6%     significant

  controlflow:         1.024x as fast      32.4ms +/- 0.4%     31.6ms +/- 0.4%     significant
    recursive:         1.024x as fast      32.4ms +/- 0.4%     31.6ms +/- 0.4%     significant

  crypto:              -                   60.7ms +/- 0.4%     60.5ms +/- 0.3% 
    aes:               -                   34.6ms +/- 0.5%     34.7ms +/- 0.5% 
    md5:               1.019x as fast      19.6ms +/- 0.7%     19.3ms +/- 0.7%     significant
    sha1:              ??                   6.5ms +/- 2.2%      6.6ms +/- 2.2%     not conclusive: might be *1.015x as slow*

  date:                *1.013x as slow*   169.8ms +/- 0.2%    171.9ms +/- 0.2%     significant
    format-tofte:      1.008x as fast      66.8ms +/- 0.2%     66.3ms +/- 0.2%     significant
    format-xparb:      *1.026x as slow*   102.9ms +/- 0.2%    105.6ms +/- 0.2%     significant

  math:                ??                  38.3ms +/- 0.6%     38.4ms +/- 0.7%     not conclusive: might be *1.004x as slow*
    cordic:            -                   18.8ms +/- 0.6%     18.8ms +/- 0.6% 
    partial-sums:      ??                  13.5ms +/- 1.1%     13.6ms +/- 1.1%     not conclusive: might be *1.009x as slow*
    spectral-norm:     ??                   6.0ms +/- 0.9%      6.0ms +/- 1.0%     not conclusive: might be *1.007x as slow*

  regexp:              *1.008x as slow*    43.7ms +/- 0.3%     44.1ms +/- 0.4%     significant
    dna:               *1.008x as slow*    43.7ms +/- 0.3%     44.1ms +/- 0.4%     significant

  string:              1.018x as fast     358.8ms +/- 0.2%    352.5ms +/- 0.1%     significant
    base64:            *1.010x as slow*    15.8ms +/- 0.7%     16.0ms +/- 0.5%     significant
    fasta:             1.008x as fast      75.0ms +/- 0.3%     74.4ms +/- 0.2%     significant
    tagcloud:          -                   98.3ms +/- 0.2%     98.1ms +/- 0.2% 
    unpack-code:       1.046x as fast     138.7ms +/- 0.2%    132.7ms +/- 0.1%     significant
    validate-input:    *1.014x as slow*    31.0ms +/- 0.4%     31.5ms +/- 0.6%     significant
Comment 88 Brendan Eich [:brendan] 2009-02-26 13:51:29 PST
In the midst of work, wondering if you can try taking compile-time out of the measuremen, a la the v8 benchmarks. Prove it's the compiler slowing things down, not generated code (very few flat closures in SunSpider, possibly just the one in 3d-raytrace.js).

/be
Comment 89 Brendan Eich [:brendan] 2009-02-26 15:33:28 PST
Created attachment 364409 [details] [diff] [review]
Fix bug that bit sunspider-compare-results.js
Comment 90 Mike Shaver (:shaver -- probably not reading bugmail closely) 2009-02-26 15:47:54 PST
sunspider-in-browser doesn't count compilation, right?  What does it do there?
Comment 91 Jesse Ruderman 2009-02-26 16:50:34 PST
js> print(function eval() { eval("v"); })
function eval() {
    v("v");
}
Comment 92 Jesse Ruderman 2009-02-26 16:54:12 PST
(function (e) { var e; const e; })

Without patch: "TypeError: redeclaration of var e"
With patch:    "TypeError: redeclaration of formal parameter e:"
Comment 93 Brendan Eich [:brendan] 2009-02-26 17:31:43 PST
Created attachment 364435 [details] [diff] [review]
really fix bug that bit sunspider-compare-results.js

Yikes. Dumb bug.

/be
Comment 94 Brendan Eich [:brendan] 2009-02-26 18:01:20 PST
1. Last (and a -j one) test in comment 53 still busted:

for (var z = 0; z < 3; z++) { (new function(){}); }

2. First test from comment 54 also busted still:

function f() { var x; eval("let x, x;"); }
f();

3. Test 5 from comment 68:

(function(q){return q;} for each (\u3056 in []))

4. Test 6 from comment 68:

for(let x = <{x}/> in <y/>) x2

5. Test 9 from comment 82:

(function(){with(window){1e-81;}for(let[x,x3]=window-=x in[])function({}})()

6. Test 10 from comment 82:

for(let [function  x () { M:if([1,,])  } 

7. Test 11 from comment 82:

function foo(code)
{
  var c;
  eval("const c, x5 = c;");
}  
foo();

8. Test 15 from comment 82:

for(let x; 
([,,,]
.toExponential(new Function(), (function(){}))); [] = {}) 
for(var [x, x] = * in this.__defineSetter__("", function(){})) 

Some of these are the same bug. Will fix and get a new patch up ASAP.

SunSpider results are suspect -- they were not verified via correct.sh, but they might just be right if SS dodges all these bullets (it certainly won't get bitten by let or genexp bugs). Correctness first, then perf. Pretty sure I can get this patch to be a perf win, FWIW.

/be
Comment 95 Brendan Eich [:brendan] 2009-02-26 18:17:15 PST
(In reply to comment #92)
> (function (e) { var e; const e; })
> 
> Without patch: "TypeError: redeclaration of var e"
> With patch:    "TypeError: redeclaration of formal parameter e:"

The : at the end is correct, in case anyone is worrying -- part of the bona fide syntax error report:

js> (function (e) { var e; const e; })
typein:1: TypeError: redeclaration of formal parameter e:
typein:1: (function (e) { var e; const e; })
typein:1: .............................^

I call this a bug fix, since the previous error message made var e sound like it replaced or shadowed the formal parameter, but that's not how JS works. Anyone have a counter-argument, please lay it on me.

/be
Comment 96 Brendan Eich [:brendan] 2009-02-26 18:22:32 PST
(In reply to comment #94)
> 4. Test 6 from comment 68:
> 
> for(let x = <{x}/> in <y/>) x2
...
> 8. Test 15 from comment 82:
> 
> for(let x; 
> ([,,,]
> .toExponential(new Function(), (function(){}))); [] = {}) 
> for(var [x, x] = * in this.__defineSetter__("", function(){})) 
...
> SunSpider results are suspect -- they were not verified via correct.sh, but
> they might just be right if SS dodges all these bullets (it certainly won't get
> bitten by let or genexp bugs).

Or E4X, grrr! Why you little... [Homer throttling Bart noises]

At least these are easy, "goofy AST surprise-shapes" bugs to fix.

/be
Comment 97 Brendan Eich [:brendan] 2009-02-26 21:12:25 PST
Created attachment 364472 [details] [diff] [review]
fixes all comment 94 tests except the first (the -j) one
Comment 98 Gary Kwong [:gkw] [:nth10sd] 2009-02-26 22:12:01 PST
(In reply to comment #97)
> Created an attachment (id=364472) [details]
> fixes all comment 94 tests except the first (the -j) one

(The first one causes this assertion: Assertion failure: scope->object == ctor, at ../jsbuiltins.cpp:236 )

$ ./js-dbg-tm-intelmac -j
js> for(let [x] = (x) in []) {}
Assertion failure: !(pnu->pn_dflags & PND_BOUND), at ../jsemit.cpp:1818

$ ./js-dbg-tm-intelmac -j
js> uneval(function(){(Number(0) for each (NaN in []) for each (x4 in this))});
Assertion failure: pos == 0, at ../jsopcode.cpp:2963

There's still a bunch more opt crashes and these assertions above were from a quick non-comprehensive debug run)
Comment 99 Gary Kwong [:gkw] [:nth10sd] 2009-02-27 00:06:53 PST
(In reply to comment #97)
> Created an attachment (id=364472) [details]
> fixes all comment 94 tests except the first (the -j) one

More results:

=====

eval("(function x(){x.(this)} )();");

Assertion failure: (uint32)(index_) < atoms_->length, at ../jsinterp.cpp:327
Crash [@ js_FullTestPropertyCache] at null in opt, -j not required.
=====

(function(){try {x} finally {}; ([x in []] for each (x in x))})();

Assertion failure: lexdep->frameLevel() <= funbox->level, at
../jsparse.cpp:1735
Crash [@ BindNameToSlot] near null in opt, -j not required.
=====

while( getter = function() { return y } for (y in y) )( /x/g );

Assertion failure: lexdep->frameLevel() <= funbox->level, at
../jsparse.cpp:1771
Crash [@ JSCompiler::setFunctionKinds] near null in opt, -j not required.
=====
Comment 100 Brendan Eich [:brendan] 2009-02-27 00:29:44 PST
Created attachment 364488 [details] [diff] [review]
fix all but the -j one from comment 53, and the last two gary found (lexdep...)
Comment 101 Gary Kwong [:gkw] [:nth10sd] 2009-02-27 01:13:31 PST
(In reply to comment #100)
> Created an attachment (id=364488) [details]
> fix all but the -j one from comment 53, and the last two gary found (lexdep...)

$ ./js-dbg-tm-intelmac 
js> uneval(function(){with({functional: []}){x5, y = this;const y }});
Assertion failure: strcmp(rval, with_cookie) == 0, at ../jsopcode.cpp:2567

More later tonight.
Comment 102 Gary Kwong [:gkw] [:nth10sd] 2009-02-27 05:30:44 PST
(In reply to comment #100)
> Created an attachment (id=364488) [details]
> fix all but the -j one from comment 53, and the last two gary found (lexdep...)

Note, ignoring the -j one from comment #53, the 2 lexdep ones in comment #99, and the strcmp one in comment #101, (this batch came from Ubuntu 32-bit 8.10)

=====
(function(){function x(){} function x()y})();

Assertion failure: JOF_OPTYPE(op) == JOF_ATOM, at ../jsemit.cpp:1710
=====
function f() {
  "" + (function(){
    for( ; [function(){}] ; x = 0)
    with({x: ""})
    const x = []
  });
}
f();

Assertion failure: ss->top - saveTop <= 1U, at ../jsopcode.cpp:2156
=====
function f() {
  var x;
  eval("const x = [];");
}
f();

Assertion failure: regs.sp == StackBase(fp), at ../jsinterp.cpp:2984
=====
do {x} while([[] for (x in []) ])

Assertion failure: !(pnu->pn_dflags & PND_BOUND), at ../jsemit.cpp:1818
=====
{x} ((x=[] for (x in []))); x

Assertion failure: cg->staticLevel >= level, at ../jsemit.cpp:2014
Crash [@ BindNameToSlot] in opt without -j
=====
Comment 103 Jesse Ruderman 2009-02-27 18:14:55 PST
js> (function(a) { v = 5; let [v] = [3]; (function(){ v; })(); })();
Assertion failure: !(pn->pn_dflags & flag), at ../jsparse.h:638
Comment 104 Jesse Ruderman 2009-02-27 18:17:52 PST
js> (function(a) { function b() { a; } function a() { } })();
Assertion failure: pn_defn, at ../jsparse.h:655
Comment 105 Brendan Eich [:brendan] 2009-02-27 20:07:14 PST
Created attachment 364635 [details] [diff] [review]
fix all but the -j issue and comment 104 testcase

Thanks for the timely tests. Generator expressions should be good now, please release the fuzzing hounds.

/be
Comment 106 Brendan Eich [:brendan] 2009-02-27 20:08:07 PST
Latest patch passe the js testsuite and trace-test.js without -j. With -j I have not run the testsuite, and trace-test.js passes except for testThinLoopDemote. I will investigate that later tonight.

/be
Comment 107 Jesse Ruderman 2009-02-27 20:25:12 PST
js> print(function d() { const d; ++d; })
function d() {
    const d;
    + d + 1;
}
Comment 108 Jesse Ruderman 2009-02-27 20:29:34 PST
print(function p(){p})

With patch:
function p() {
}

Without patch:
function p() {
    p;
}
Comment 109 Jesse Ruderman 2009-02-27 20:35:14 PST
var v; const v = function d(1) { }

Without patch:
3: TypeError: redeclaration of var v:
3: var v; const v = function d(1) { }
3: .............^

With patch:
3: SyntaxError: missing formal parameter:
3: var v; const v = function d(1) { }
3: ............................^

var v; const v = function d(`) { }

Without patch:
3: TypeError: redeclaration of var v:
3: var v; const v = function d(`) { }
3: .............^

With patch:
3: missing formal parameter:
3: var v; const v = function d(`) { }
3: ............................^
3: SyntaxError: illegal character:
3: var v; const v = function d(`) { }
3: ............................^

Getting two error messages from one script is pretty neat.
Comment 110 Brendan Eich [:brendan] 2009-02-27 21:26:40 PST
(In reply to comment #107)
> js> print(function d() { const d; ++d; })
> function d() {
>     const d;
>     + d + 1;
> }

This is a bug fix, compare the following between Firefox 3 era js shell:

js> function f(a) { const b = a; print(++b); return b; }
js> f
function f(a) {
    const b = a;
    print(b);
    return b;
}js> f(1)
1
1
js> f
function f(a) {
    const b = a;
    print(b);
    return b;
}
js> const x = 1; print(++x); x
2
1

and shell built with this bug's patch:

js> function f(a) { const b = a; print(++b); return b; }
js> f
function f(a) {
    const b = a;
    print(+ b + 1);
    return b;
}
js> f(1)
2
1
js> const x = 1; print(++x); x
2
1

The unary + operator is necessary to convert b to a number (which conversion could have effects, so the whole ++b can't be optimized away):

js> f({valueOf:function(){print('effect!');return 42}})
effect!
43
[object Object]

That's with this bug's patch applied; without, you get:

js> f({valueOf:function(){print('effect!');return 42}})
[object Object]
[object Object]

There's still a bug for further work to resolve in your example, however:

print(function d() { const d; ++d; })

shows a named function expression that defines a const (which ES-Harmony is likely to make a block-scoped binding form that may occur per block at most once per declared identifier, and that must have an initializer) to undefined. So the ++d is guaranteed useless (without effects) and it could be removed by SpiderMonkey's useless expression eliminator.

This can wait for a more Harmonious const proposal and implementation.

Note that the function's name is in an outer lexical environment, so shadowed by a var or const at top level (even an uninitialized var or const).

(In reply to comment #108)
> print(function p(){p})
> 
> With patch:
> function p() {
> }

Here you see the useless expression eliminator in action.

> Without patch:
> function p() {
>     p;
> }

Previously we couldn't reason so easily about the outer environment object in which the function name was bound because we used to follow ES3 and use an Object instance for the environment frame, but that's both leaky and hijackable (note that the useless expression elimination optimization applies is only for named function expression case, not for a top-level definition of function p -- there the name may or may not persist in the global object, so we can't be sure what p; means).

(In reply to comment #109)
> var v; const v = function d(1) { }
> 
> Without patch:
> 3: TypeError: redeclaration of var v:
> 3: var v; const v = function d(1) { }
> 3: .............^
> 
> With patch:
> 3: SyntaxError: missing formal parameter:
> 3: var v; const v = function d(1) { }
> 3: ............................^

This is due to a reordering of semantic actions including error checks while parsing. We now process the initializer before the declared name and its kind (const vs. var, etc.). I'm not sure this is right, though.

> var v; const v = function d(`) { }
> 
> Without patch:
> 3: TypeError: redeclaration of var v:
> 3: var v; const v = function d(`) { }
> 3: .............^
> 
> With patch:
> 3: missing formal parameter:
> 3: var v; const v = function d(`) { }
> 3: ............................^
> 3: SyntaxError: illegal character:
> 3: var v; const v = function d(`) { }
> 3: ............................^
> 
> Getting two error messages from one script is pretty neat.

Just a latent bug. We don't do any error recovery, we should progagate failure on the first error (which is the illegal character one).

/be
Comment 111 Gary Kwong [:gkw] [:nth10sd] 2009-02-27 21:36:27 PST
options("anonfunfix");
new Function("{function x(){}}");

Assertion failure: pn->pn_defn || (fun->flags & JSFUN_LAMBDA), at ../jsemit.cpp:4149

I took a really long time to reduce this extremely-frequent testcase in jsfunfuzz.
Comment 112 Jesse Ruderman 2009-02-27 22:04:43 PST
q = new Function("(function() { q(3); })(); const q;"); q();

Without patch: TypeError: q is not a function
With patch: InternalError: too much recursion
Comment 113 Jesse Ruderman 2009-02-27 22:22:53 PST
This patch causes the "redeclaration of var v" to point to the wrong part of the line with this testcase:

var v; const v = function() { qqqqqqqqqqqqqqqqqq + qqqqqqqqqqqqqqqqqq + qqqqqqqqqqqqqqqqqq + qqqqqqqqqqqqqqqqqq + qqqqqqqqqqqqqqqqqq + qqqqqqqqqqqqqqqqqq + qqqqqqqqqqqqqqqqqq + qqqqqqqqqqqqqqqqqq + qqqqqqqqqqqqqqqqqq + qqqqqqqqqqqqqqqqqq + qqqqqqqqqqqqqqqqqq + qqqqqqqqqqqqqqqqqq + qqqqqqqqqqqqqqqqqq + qqqqqqqqqqqqqqqqqq };
Comment 114 Gary Kwong [:gkw] [:nth10sd] 2009-02-28 00:43:50 PST
(In reply to comment #105)
> Created an attachment (id=364635) [details]
> fix all but the -j issue and comment 104 testcase
> 
> Thanks for the timely tests. Generator expressions should be good now, please
> release the fuzzing hounds.
> 
> /be

Besides the -j issue, comment #104 and comment #111 to 113 testcases,

More results:

=====
for (var x = 0; x < 3; ++x){ y = function (){} }

glorp!
Assertion failed: "Constantly false guard detected": 0 (../nanojit/LIR.cpp:999)
(note, this is with -j; I don't know what the glorp! message is about.)
=====
while(x|=#3={}) with({}) const x;

Assertion failure: cg->stackDepth >= 0, at ../jsemit.cpp:185
=====
function y([{x: x, x}]){}

Assertion failure: UPVAR_FRAME_SKIP(pn->pn_cookie) == (pn->pn_defn ? cg->staticLevel : 0), at ../jsemit.cpp:3547
=====
eval("(1.3.__defineGetter__(\"\"));let (y, x) { var z = true, x = 0; }");

Assertion failure: ATOM_IS_STRING(atom), at ../jsinterp.cpp:5691
Comment 115 Brendan Eich [:brendan] 2009-02-28 01:26:20 PST
Created attachment 364650 [details] [diff] [review]
fix all reported bugs except for the -j ones

The only trace-test.js failure with -j is again:

testThinLoopDemote : FAILED: expected number ( 10000 )  [recorderStarted: 2 recorderAborted: 0 traceCompleted: 2 traceTriggered: 3 unstableLoopVariable: 1]  != actual number ( 10000 )  [recorderStarted: 2 recorderAborted: 0 traceCompleted: 2 traceTriggered: 2 unstableLoopVariable: 1] 

passed: 0

FAILED: testThinLoopDemote
recorder: started(38), aborted(6), completed(32), different header(0), trees trashed(0), slot promoted(0), unstable loop variable(14), breaks(0), returns(0), unstableInnerCalls(2)
monitor: triggered(879), exits(879), type mismatch(0), global mismatch(4)

Stats changed, will investigate after sleep.

/be
Comment 116 Gary Kwong [:gkw] [:nth10sd] 2009-02-28 02:12:04 PST
(In reply to comment #115)
> Created an attachment (id=364650) [details]
> fix all reported bugs except for the -j ones

I ran a quick run through -j and came up with a lot of such testcases:

(new Function("for (var x = 0; x < 3; ++x) { (function(){})() } "))();

Crash [@ js_IsActiveWithOrBlock]


That said, I have switched off -j on all machines for the moment, to pound for a solid interp first.
Comment 117 Gary Kwong [:gkw] [:nth10sd] 2009-02-28 12:12:00 PST
(In reply to comment #115)
> Created an attachment (id=364650) [details]
> fix all reported bugs except for the -j ones

The following all do not require -j.

=====
x; function  x(){}; const x;

Assertion failure: !pn->isPlaceholder(), at ../jsparse.cpp:4876
=====
(function(){ var x; eval("var x; x = null"); })()

Assertion failure: regs.sp == StackBase(fp), at ../jsinterp.cpp:2984
=====
function this ({x}) { function x(){} }

Assertion failure: cg->stackDepth == stackDepth, at ../jsemit.cpp:3664
=====
for(let x = [ "" for (y in /x/g ) if (x)] in (""));

Assertion failure: !(pnu->pn_dflags & PND_BOUND), at ../jsemit.cpp:1818
=====
(function(){const x = 0, y = delete x;})()

Assertion failure: JOF_OPTYPE(op) == JOF_ATOM, at ../jsemit.cpp:1710
=====
(function(){(yield []) (function(){with({}){x} }); const x;})()

Assertion failure: cg->upvars.lookup(atom), at ../jsemit.cpp:2022
=====
(function(){([]) ((function(q) { return q; })for (each in *))})();

Assertion failure: lexdep->frameLevel() <= funbox->level, at ../jsparse.cpp:1782
Opt crash [@ JSCompiler::setFunctionKinds] near null
=====
eval("((x1) > [(x)(function() { x;}) for each (x in x)])()");

Assertion failure: pnu->pn_lexdef == dn, at ../jsemit.cpp:1817
=====
uneval(function(){for(var [arguments] = ({ get y(){} }) in y ) (x);})

Assertion failure: slot < StackDepth(jp->script), at ../jsopcode.cpp:1318
=====
uneval(function(){([] for ([,,]in <><y/></>));})

Assertion failure: n != 0, at ../jsfun.cpp:2689
=====
(function(){{for(c in (function (){ for(x in (x1))window} )()) {const x;} }})()

Assertion failure: op == JSOP_GETLOCAL, at ../jsemit.cpp:4557
=====
(eval("(function(){let x , x =  (x for (x in null))});"))();

Assertion failure: (fun->u.i.script)->upvarsOffset != 0, at ../jsfun.cpp:1537
Opt crash [@ js_NewFlatClosure] near null
=====
"" + function(){for(var [x] in x1) ([]); function x(){}}

Assertion failure: cg->stackDepth == stackDepth, at ../jsemit.cpp:3664
Opt crash [@ JS_ArenaRealloc] near null
=====
for (a in (function(){
  for each (let x in ['']) { return new function x1 (\u3056) { yield x } ();
  }})())  
function(){}

Crash [@ js_Interpret] near null
=====


zzz now.....
Comment 118 Jesse Ruderman 2009-02-28 17:02:34 PST
(function() { (function() { e *= 4; })(); var e; })(); 

Without patch: no output
With patch:   ReferenceError: reference to undefined property "e"
Comment 119 Gary Kwong [:gkw] [:nth10sd] 2009-02-28 22:58:05 PST
(In reply to comment #115)
> Created an attachment (id=364650) [details]
> fix all reported bugs except for the -j ones

The following additional testcases also do not require -j.

=====
function f() {
  var x;
  eval("for(let y in [false]) var x, x = 0");
}
f();

Assertion failure: !JSVAL_IS_PRIMITIVE(regs.sp[-2]), at ../jsinterp.cpp:3243
Opt crash [@ JS_GetMethodById] near null
=====
new Function("for(x1 in ((function (){ yield x } )())){var c, x = []} function x(){} ");

Assertion failure: pn_used, at ../jsparse.h:401
Opt crash [@ FindFunArgs] at null
=====
uneval(new Function("[(x = x) for (c in []) if ([{} for (x in [])])]"))

Assertion failure: (uintN)i < ss->top, at ../jsopcode.cpp:2814
=====
function f() {
  var x;
  (function(){})();
  eval("if(x|=[]) {const x; }");
}
f();

Opt crash [@ js_ValueToNumber] at 0xc3510424
Dbg crash [@ js_ValueToNumber] at 0xdadadad8
=====
Comment 120 Brendan Eich [:brendan] 2009-03-01 00:48:15 PST
Created attachment 364750 [details] [diff] [review]
fixes all of comment 116&118, some of 117&119

This also makes the -j cases reported so far work. Release the -j hounds!

/be
Comment 121 Gary Kwong [:gkw] [:nth10sd] 2009-03-01 01:13:04 PST
(In reply to comment #120)
> Created an attachment (id=364750) [details]
> fixes all of comment 116&118, some of 117&119
> 
> This also makes the -j cases reported so far work. Release the -j hounds!
> 
> /be

virtually floods jsfunfuzz debug spew (-j not needed) with this:

x = function()x

Assertion failure: !(pn->pn_dflags & flag), at ../jsparse.h:651
Comment 122 Brendan Eich [:brendan] 2009-03-01 01:45:27 PST
Created attachment 364753 [details] [diff] [review]
fixes all of comment 116&118, some of 117&119, without bogo-assert
Comment 123 Gary Kwong [:gkw] [:nth10sd] 2009-03-01 10:23:49 PST
(In reply to comment #122)
> Created an attachment (id=364753) [details]
> fixes all of comment 116&118, some of 117&119, without bogo-assert

(Ignoring all testcases in 117 & 119,)

Does not require -j:
=====
y = (function (){y} for (x in [])

Assertion failure: !(pn->pn_dflags & flag), at ../jsparse.h:651
=====
(function(){for(var x = arguments in []){} function x(){}})()

Assertion failure: dn->pn_defn, at ../jsemit.cpp:1873
=====


Requires -j:
=====
(function(){ eval("for (x in ['', {}, '', {}]) { this; }" )})()

Assertion failure: fp->thisp == JSVAL_TO_OBJECT(fp->argv[-1]), at ../jstracer.cpp:4172
=====
for each (let x in ['', '', '']) { (new function(){} )}

Assertion failure: scope->object == ctor, at ../jsbuiltins.cpp:236
Opt crash [@ js_FastNewObject] near null
=====


Getting much better with the hounds now -- time for zzz though.
Comment 124 Gabriele Best [:Gabri] 2009-03-01 11:25:21 PST
Brendan, can you make a tryserver build?
Thanks.
Comment 125 Brendan Eich [:brendan] 2009-03-01 14:04:58 PST
(In reply to comment #123)
> Does not require -j:
> =====
> y = (function (){y} for (x in [])
> 
> Assertion failure: !(pn->pn_dflags & flag), at ../jsparse.h:651
> =====
> (function(){for(var x = arguments in []){} function x(){}})()
> 
> Assertion failure: dn->pn_defn, at ../jsemit.cpp:1873
> =====

I have fixes for these. I'll attach a new patch with more fixes beyond these, later today.

> Requires -j:
> =====
> (function(){ eval("for (x in ['', {}, '', {}]) { this; }" )})()
> 
> Assertion failure: fp->thisp == JSVAL_TO_OBJECT(fp->argv[-1]), at
> ../jstracer.cpp:4172

I think this is bug 479740.

> =====
> for each (let x in ['', '', '']) { (new function(){} )}
> 
> Assertion failure: scope->object == ctor, at ../jsbuiltins.cpp:236
> Opt crash [@ js_FastNewObject] near null

This one is on the agenda to fix here.

/be
Comment 126 Brendan Eich [:brendan] 2009-03-01 14:42:06 PST
(In reply to comment #124)
> Brendan, can you make a tryserver build?
> Thanks.

If someone wants to do that and has access, I would appreciate the help -- but not today, please wait a day to avoid reproducing and re-testing the same bugs.

/be
Comment 127 Gary Kwong [:gkw] [:nth10sd] 2009-03-01 19:43:44 PST
(In reply to comment #125)
> > Requires -j:
> > =====
> > (function(){ eval("for (x in ['', {}, '', {}]) { this; }" )})()
> > 
> > Assertion failure: fp->thisp == JSVAL_TO_OBJECT(fp->argv[-1]), at
> > ../jstracer.cpp:4172
> 
> I think this is bug 479740.

Btw, closest I found is bug 457065. Bug 479740 seems WFM.
Comment 128 Brendan Eich [:brendan] 2009-03-02 00:18:29 PST
Created attachment 364868 [details] [diff] [review]
the last two from comment 119 are still broken

All other tests seem to pass -- please double-check. I'm putting this up to get more testing mainly from Gary, and to show interdiff progress (and some hacking I need to clean up tomorrow).

/be
Comment 129 Gary Kwong [:gkw] [:nth10sd] 2009-03-02 01:54:45 PST
(In reply to comment #128)
> Created an attachment (id=364868) [details]
> the last two from comment 119 are still broken
> 
> All other tests seem to pass -- please double-check. I'm putting this up to get
> more testing mainly from Gary, and to show interdiff progress (and some hacking
> I need to clean up tomorrow).

Besides the last two from comment #119, _and_ the last one in comment #123 which seem to still be broken,


Does not require -j:
=====
({ set x x () { for(x in function(){}){}}  })

Assertion failure: JOF_OPTYPE(op) == JOF_ATOM, at ../jsemit.cpp:1710
=====
(function (){ eval("(function(){delete !function(){}});"); })();

Debug crash [@ JSParseNode::isFunArg] at null
Opt crash [@ FindFunArgs] near null
=====

More to follow hopefully in the next few hours.
Comment 130 Gary Kwong [:gkw] [:nth10sd] 2009-03-02 07:41:21 PST
(In reply to comment #129)
> (In reply to comment #128)
> > Created an attachment (id=364868) [details] [details]
> > the last two from comment 119 are still broken
> 
> Besides the last two from comment #119, _and_ the last one in comment #123
> which seem to still be broken,

_and_ besides the two testcases in comment #129,


Does not require -j:
=====
((function x()x in []) for (y in []))

Assertion failure: lexdep->frameLevel() <= funbox->level, at ../jsparse.cpp:1778
Opt crash [@ JSCompiler::setFunctionKinds]
=====
let(x=[]) function(){try {x} catch(x) {} }

Assertion failure: cg->upvars.lookup(atom), at ../jsemit.cpp:2034
=====
for(let [y] = (let (x) (y)) in []) function(){}

Assertion failure: !(pnu->pn_dflags & PND_BOUND), at ../jsemit.cpp:1818
=====


Requires -j:
=====
for (var x = 0; x < 3; ++x) { new function(){} }

Assertion failure: cx->bailExit, at ../jstracer.cpp:4672
Opt crash [@ LeaveTree] near null
(variants crash debug at js_SynthesizeFrame but their stacks below this line look similar to the opt crash ones)
(this assertion is similar to the last one in comment #123 but that one didn't crash opt or debug previously)


That's all for now, 4 (now) plus 5 from previous comments leave only 9 distinct assertions/crashes though some may have common causes; things are looking brighter than before. :)
Comment 131 Gary Kwong [:gkw] [:nth10sd] 2009-03-02 17:16:45 PST
In summary, besides the

last two from comment #119,
last one from comment #123,
two from comment #129,
four from comment #130,

here's the latest two from an overnight run bringing the total to 11:


Does not require -j:
=====
((__defineGetter__, function (x) { function x(){} }) for

Assertion failure: pn->pn_cookie == FREE_UPVAR_COOKIE, at ../jsparse.cpp:5698
=====
( ""  ? 1.3 : x); *::*; x::x;

Assertion failure: pn != dn->dn_uses, at ../jsparse.cpp:1171
=====
Comment 132 Brendan Eich [:brendan] 2009-03-13 19:13:51 PDT
Created attachment 367341 [details] [diff] [review]
fixes all but the three non-jit ones in comment 130

I'll work on this more tonight.

/be
Comment 133 Brendan Eich [:brendan] 2009-03-13 22:07:04 PDT
Created attachment 367351 [details] [diff] [review]
fix bug reported by Gary over IRC

The testcase Gary found:

eval("(function(){if(function(){}){}});");

js_FoldConstants does not maintain a stack (forming a tree if retained) of JSTreeContexts, so when descending into functions it must save and restore a few fields in tc.

/be
Comment 134 Brendan Eich [:brendan] 2009-03-14 01:26:39 PDT
Created attachment 367359 [details] [diff] [review]
fix comment 130 hard cases
Comment 135 Gary Kwong [:gkw] [:nth10sd] 2009-03-14 05:27:55 PDT
(In reply to comment #134)
> Created an attachment (id=367359) [details]
> fix comment 130 hard cases

In summary, all recent prior testcases have been fixed.

-j is not required:
===
Testcase in bug 473014 (this bug depends on upvar2 bug) still asserts at
for (let i = 0; i < 9; ++i) { 
  let m = i;
  if (i % 3 == 1) {
    print('' + (function() { return m; })());
  }
}

Assertion failure: fp2->fun && fp2->script, at ../jsinterp.cpp:5633
Opt crash [@ js_Interpret]
===
(run from the command line - e.g. `./js a.js` )
(x for each (c in []))
x 

Assertion failure: dn_kind == JSDefinition::VAR || dn_kind == JSDefinition::CONST, at ../jsemit.cpp:2187
===
eval("do ([]); while(y for each (x in []))");

Debug & opt crash [@ JSCompiler::setFunctionKinds]
===
"" + (function(){L:if (*::*){ var x } function x(){}})

Assertion failure: slot < StackDepth(jp->script), at ../jsopcode.cpp:1329
===
"" + (function(){if (*::*){ var x } function x(){}})

Assertion failure: (uintN)i < ss->top, at ../jsopcode.cpp:2825
===
"" + (function(){{<y/>; throw <z/>;for(var x = [] in false) return } function x(){}})

Assertion failure: ss->printer->pcstack, at ../jsopcode.cpp:909
===
(function(){for(; (this); ((window for (x in [])) for (y in []))) 0})

Assertion failure: level >= tc->staticLevel, at ../jsparse.cpp:5773
===
eval(uneval( function(){
  ((function()y)() for each (x in this))
} ))

Debug & opt crash [@ BindNameToSlot]

---------------------------------------

-j is required:
===
Testcase in bug 469237 (duped to a bug that upvar2 bug blocks) still asserts at
for (let a=0;a<3;++a) for (let b=0;b<3;++b) if ((g=a|(a%b))) with({}){}

Assertion failure: OBJ_IS_CLONED_BLOCK(obj), at ../jsobj.cpp:2392
Comment 136 Brendan Eich [:brendan] 2009-03-14 11:12:13 PDT
Created attachment 367407 [details] [diff] [review]
fix all reported fuzzer-generated tests

(In reply to comment #135)
> Testcase in bug 469237 (duped to a bug that upvar2 bug blocks) still asserts at
> for (let a=0;a<3;++a) for (let b=0;b<3;++b) if ((g=a|(a%b))) with({}){}
> 
> Assertion failure: OBJ_IS_CLONED_BLOCK(obj), at ../jsobj.cpp:2392

I reopened bug 469237, it is not a dup of the bug blocked by this bug. It has no function anywhere in its test, only for(let...;...;...) loops containing if and with. The 'with' is the issue, so upvar can't help.

/be
Comment 137 Brendan Eich [:brendan] 2009-03-14 11:32:00 PDT
Created attachment 367408 [details] [diff] [review]
refresh to match tm tip
Comment 138 Gary Kwong [:gkw] [:nth10sd] 2009-03-15 01:31:32 PDT
(In reply to comment #137)
> Created an attachment (id=367408) [details]
> refresh to match tm tip

Does not require -j:
===
((function x(){ yield (x = undefined) } ) for (y in /x/))

Assertion failure: lexdep->frameLevel() <= funbox->level, at ../jsparse.cpp:1820
===
for(let x in ( x for (y in x) for each (x in []) )) y

Assertion failure: cg->upvars.lookup(atom), at ../jsemit.cpp:2034
===

-------------------------------

Requires -j:
===
(function(){eval("([0 for each (x in [/x/, this, /x/])])")})()

(again this assertion is similar to bug 457065, but the exact wording is different, I don't know if they're the same issue.)
Assertion failure: fp->thisp == JSVAL_TO_OBJECT(fp->argv[-1]), at ../jstracer.cpp:4213
===
for each (let x in ['']) {
  for (var b = 0; b < 5; ++b) {
    if (b % 5 == 3) {
      with([]) this
    }
  }  
}

(Bug 472528, which this upvar2 bug blocks, and has the same assertion, no longer asserts with this patch.)
Assertion failure: !js_IsActiveWithOrBlock(cx, fp->scopeChain, 0), at ../jsinterp.cpp:7151
===
Comment 139 Gary Kwong [:gkw] [:nth10sd] 2009-03-15 05:37:22 PDT
(In reply to comment #137)
> Created an attachment (id=367408) [details]
> refresh to match tm tip

Besides the testcases in comment #138,

Does not require -j:
===
(function(){var x = x (x() for each (x in []))})()

Assertion failure: (fun->u.i.script)->upvarsOffset != 0, at ../jsfun.cpp:1541
Opt crash near null [@ js_NewFlatClosure]
===
Comment 140 Bob Clary [:bc:] 2009-03-15 11:27:16 PDT
Created attachment 367492 [details]
js1_8_1/trace/regress-452498-01.js

brendan, this test fails with your most recent patch.

FAILED! upvar2: jit heavyweight functions: time nonjit: 1328, time    jit: 2172 : Expected value 'true', Actual value 'false'
Comment 141 Brendan Eich [:brendan] 2009-03-15 20:17:01 PDT
(In reply to comment #140)
> Created an attachment (id=367492) [details]
> js1_8_1/trace/regress-452498-01.js
> 
> brendan, this test fails with your most recent patch.
> 
> FAILED! upvar2: jit heavyweight functions: time nonjit: 1328, time    jit: 2172
> : Expected value 'true', Actual value 'false'

recorder: started(14), aborted(13), completed(17), different header(0), trees trashed(0), slot promoted(0), unstable loop variable(0), breaks(5), returns(0), unstableInnerCalls(1)
monitor: triggered(50692), exits(50692), type mismatch(0), global mismatch(0)

Abort recording of tree ./js1_8_1/trace/regress-452498-01.js:80@72 at ./js1_8_1/trace/regress-452498-01.js:66@24: Inner tree is trying to grow, abort outer recording.
Abort recording of tree ./js1_8_1/trace/regress-452498-01.js:80@72 at ./js1_8_1/trace/regress-452498-01.js:66@24: Inner tree is trying to grow, abort outer recording.
Abort recording of tree ./js1_8_1/trace/regress-452498-01.js:80@72 at ./js1_8_1/trace/regress-452498-01.js:66@24: Inner tree is trying to grow, abort outer recording.
Abort recording of tree ./js1_8_1/trace/regress-452498-01.js:80@72 at ./js1_8_1/trace/regress-452498-01.js:66@24: Inner tree is trying to grow, abort outer recording.
Abort recording of tree ./js1_8_1/trace/regress-452498-01.js:80@72 at ./js1_8_1/trace/regress-452498-01.js:66@24: Inner tree is trying to grow, abort outer recording.
Abort recording of tree ./js1_8_1/trace/regress-452498-01.js:80@72 at ./js1_8_1/trace/regress-452498-01.js:66@24: Inner tree is trying to grow, abort outer recording.
Abort recording of tree ./js1_8_1/trace/regress-452498-01.js:80@72 at ./js1_8_1/trace/regress-452498-01.js:66@24: Inner tree is trying to grow, abort outer recording.
Abort recording of tree ./js1_8_1/trace/regress-452498-01.js:80@72 at ./js1_8_1/trace/regress-452498-01.js:66@24: Inner tree is trying to grow, abort outer recording.
Abort recording of tree ./js1_8_1/trace/regress-452498-01.js:80@72 at ./js1_8_1/trace/regress-452498-01.js:66@24: Inner tree is trying to grow, abort outer recording.
Abort recording of tree ./js1_8_1/trace/regress-452498-01.js:80@72 at ./js1_8_1/trace/regress-452498-01.js:66@24: Inner tree is trying to grow, abort outer recording.
Abort recording of tree ./js1_8_1/trace/regress-452498-01.js:79@60 at ./js1_8_1/trace/regress-452498-01.js:80@72: No compatible inner tree.
Abort recording of tree ./js1_8_1/trace/regress-452498-01.js:80@72 at ./js1_8_1/trace/regress-452498-01.js:66@24: Inner tree is trying to grow, abort outer recording.
Abort recording of tree ./js1_8_1/trace/regress-452498-01.js:79@60 at ./js1_8_1/trace/regress-452498-01.js:66@24: Inner tree is trying to grow, abort outer recording.

Andreas, what's this mean?

/be
Comment 142 Brendan Eich [:brendan] 2009-03-15 22:46:23 PDT
(In reply to comment #138)
> Requires -j:
> ===
> (function(){eval("([0 for each (x in [/x/, this, /x/])])")})()
> 
> (again this assertion is similar to bug 457065, but the exact wording is
> different, I don't know if they're the same issue.)
> Assertion failure: fp->thisp == JSVAL_TO_OBJECT(fp->argv[-1]), at
> ../jstracer.cpp:4213

This is testing bug 457065, not this bug.

> ===
> for each (let x in ['']) {
>   for (var b = 0; b < 5; ++b) {
>     if (b % 5 == 3) {
>       with([]) this
>     }
>   }  
> }
> 
> (Bug 472528, which this upvar2 bug blocks, and has the same assertion, no
> longer asserts with this patch.)
> Assertion failure: !js_IsActiveWithOrBlock(cx, fp->scopeChain, 0), at
> ../jsinterp.cpp:7151

See bug 469237 comment 2. This testcase has no functions in it, so nothing to do with this bug or its patch.

/be
Comment 143 Andreas Gal :gal 2009-03-15 23:02:41 PDT
Here we are trying to complete an outer tree, call the inner tree, but instead of returning on a loop exit, it returns on a branch exit. We abort recording the outer tree and instead try to extend the inner tree so next time around we call the inner tree and hopefully this missing path will be covered and it will return on a loop exit. 

Causes:
1) path couldn't be compiled (abort along that path)
2) bug in loop_exit for some weird case (i.e. constant loop condition in a secondary path, see review in your queue for a fix for that)
Comment 144 Gary Kwong [:gkw] [:nth10sd] 2009-03-17 00:26:05 PDT
(In reply to comment #142)
> > for each (let x in ['']) {
> >   for (var b = 0; b < 5; ++b) {
> >     if (b % 5 == 3) {
> >       with([]) this
> >     }
> >   }  
> > }
> > 
> > (Bug 472528, which this upvar2 bug blocks, and has the same assertion, no
> > longer asserts with this patch.)
> > Assertion failure: !js_IsActiveWithOrBlock(cx, fp->scopeChain, 0), at
> > ../jsinterp.cpp:7151
> 
> See bug 469237 comment 2. This testcase has no functions in it, so nothing to
> do with this bug or its patch.
> 
> /be

Spun off as bug 483749.
Comment 145 Robert Sayre 2009-03-17 10:00:07 PDT
This needs to be updated to tm tip again. I'd like to go through our blockers with this patch applied and see which are fixed.
Comment 146 Brendan Eich [:brendan] 2009-03-17 10:57:01 PDT
Created attachment 367808 [details] [diff] [review]
refresh to match tm tip

I'll update this later today to fix the remaining problems Gary reported, and possibly also the tracing issues Bob's test js1_8_1/trace/regress-452498-01.js demonstrates.

/be
Comment 147 Robert Sayre 2009-03-17 13:08:13 PDT
Created attachment 367836 [details]
assertion on browser startup
Comment 148 Robert Sayre 2009-03-17 14:14:33 PDT
Comment on attachment 367836 [details]
assertion on browser startup

This is from nsBrowserContentHandler registerSelf(). It does have an inline function.
Comment 149 Bob Clary [:bc:] 2009-03-17 16:45:51 PDT
Created attachment 367910 [details]
preliminary tests and results

untar in js1_8_1/regress/

regress-452498.out contains results with patch and -j.

Assertion failure: regs.sp == StackBase(fp), at ../jsinterp.cpp:2959
Assertion failure: regs.sp == StackBase(fp), at ../jsinterp.cpp:2959
Assertion failure: cg->upvars.lookup(atom), at ../jsemit.cpp:2034
Assertion failure: (fun->u.i.script)->upvarsOffset != 0, at ../jsfun.cpp:1541

there are several other fails. test review appreciated.
Comment 150 Bob Clary [:bc:] 2009-03-17 19:28:00 PDT
(In reply to comment #149)

> Assertion failure: regs.sp == StackBase(fp), at ../jsinterp.cpp:2959
> Assertion failure: regs.sp == StackBase(fp), at ../jsinterp.cpp:2959
> Assertion failure: cg->upvars.lookup(atom), at ../jsemit.cpp:2034
> Assertion failure: (fun->u.i.script)->upvarsOffset != 0, at ../jsfun.cpp:1541

these were with an obsolete patch/build. redoing a complete run now. news later.
Comment 151 Brendan Eich [:brendan] 2009-03-17 22:57:08 PDT
Created attachment 367963 [details] [diff] [review]
fix XDR of function script to not treat upvars as vars
Comment 152 Bob Clary [:bc:] 2009-03-18 06:09:39 PDT
with attachment 367963 [details] [diff] [review],
js1_5/decompilation/regress-352073.js fails:

Expected value ' function ( ) { ( function x ( ) { } ) ; return x ; } ', Actual value ' function ( ) { return x ; } '
Comment 153 Bob Clary [:bc:] 2009-03-18 07:05:42 PDT
failures with jit:

js1_5/decompilation/regress-352073.js 
decompilation of function expressions reason: Expected value ' function ( ) { ( function x ( ) { } ) ; return x ; } ', Actual value ' function ( ) { return x ; } '

js1_8_1/regress/regress-452498-062.js .
js1_8_1/regress/regress-452498-062.js:1: ReferenceError: x is not defined

js1_8_1/regress/regress-452498-074.js 
Expected value '2', Actual value '1'

js1_8_1/regress/regress-452498-077.js 
Expected value '2', Actual value '1'

js1_8_1/regress/regress-452498-102.js CRASHED signal 5 SIGTRAP
Assertion failure: regs.sp == StackBase(fp), at ../jsinterp.cpp:3002

js1_8_1/regress/regress-452498-110.js 
function g(a) { const b = a; print(actual = ++b); return b; } reason: Expected value '21', Actual value '01'

js1_8_1/regress/regress-452498-117.js 
js1_8_1/regress/regress-452498-117.js:129: SyntaxError: invalid for/in left-hand side

js1_8_1/regress/regress-452498-119.js CRASHED signal 5 SIGTRAP
Assertion failure: regs.sp == StackBase(fp), at ../jsinterp.cpp:3002

js1_8_1/regress/regress-452498-135.js 
BUGNUMBER: 452498; 1; 4; 7; uncaught exception:

js1_8_1/regress/regress-452498-138.js CRASHED signal 5 SIGTRAP
Assertion failure: cg->upvars.lookup(atom), at ../jsemit.cpp:2034

js1_8_1/regress/regress-452498-139.js CRASHED signal 5 SIGTRAP
Assertion failure: (fun->u.i.script)->upvarsOffset != 0, at ../jsfun.cpp:1543

js1_8_1/trace/regress-452498-01.js NORMAL, 
time nonjit: 945, time    jit: 1621 reason: Expected value 'true', Actual value 'false'
Comment 154 Bob Clary [:bc:] 2009-03-18 09:20:26 PDT
Created attachment 368032 [details]
tests v2 w/ results log.
Comment 155 Gary Kwong [:gkw] [:nth10sd] 2009-03-18 20:15:19 PDT
(In reply to comment #151)
> Created an attachment (id=367963) [details]
> fix XDR of function script to not treat upvars as vars

Besides the first 2 testcases in comment #138,
and the testcase in comment #139,

Does not require -j:
===
delete (1 ? window : function(){})

Assertion failure: pn_arity == PN_FUNC || pn_arity == PN_NAME, at ../jsparse.h:449
Opt crash [@ FindFunArgs]
Comment 156 Bob Clary [:bc:] 2009-03-19 22:33:50 PDT
suffix numbers are comment numbers where the tests appeared.

http://hg.mozilla.org/tracemonkey/rev/a75df88d280b

/cvsroot/mozilla/js/tests/js1_8_1/regress/regress-452498-006.js,v  <--  regress-452498-006.js initial  revision: 1.1
/cvsroot/mozilla/js/tests/js1_8_1/regress/regress-452498-027.js,v  <--  regress-452498-027.js initial  revision: 1.1
/cvsroot/mozilla/js/tests/js1_8_1/regress/regress-452498-030.js,v  <--  regress-452498-030.js initial  revision: 1.1
/cvsroot/mozilla/js/tests/js1_8_1/regress/regress-452498-038.js,v  <--  regress-452498-038.js initial  revision: 1.1
/cvsroot/mozilla/js/tests/js1_8_1/regress/regress-452498-039.js,v  <--  regress-452498-039.js initial  revision: 1.1
/cvsroot/mozilla/js/tests/js1_8_1/regress/regress-452498-040.js,v  <--  regress-452498-040.js initial  revision: 1.1
/cvsroot/mozilla/js/tests/js1_8_1/regress/regress-452498-050.js,v  <--  regress-452498-050.js initial  revision: 1.1
/cvsroot/mozilla/js/tests/js1_8_1/regress/regress-452498-051.js,v  <--  regress-452498-051.js initial  revision: 1.1
/cvsroot/mozilla/js/tests/js1_8_1/regress/regress-452498-052-a.js,v  <--  regress-452498-052-a.js initial  revision: 1.1
/cvsroot/mozilla/js/tests/js1_8_1/regress/regress-452498-052.js,v  <--  regress-452498-052.js initial  revision: 1.1
/cvsroot/mozilla/js/tests/js1_8_1/regress/regress-452498-053.js,v  <--  regress-452498-053.js initial  revision: 1.1
/cvsroot/mozilla/js/tests/js1_8_1/regress/regress-452498-054.js,v  <--  regress-452498-054.js initial  revision: 1.1
/cvsroot/mozilla/js/tests/js1_8_1/regress/regress-452498-058.js,v  <--  regress-452498-058.js initial  revision: 1.1
/cvsroot/mozilla/js/tests/js1_8_1/regress/regress-452498-062.js,v  <--  regress-452498-062.js initial  revision: 1.1
/cvsroot/mozilla/js/tests/js1_8_1/regress/regress-452498-063.js,v  <--  regress-452498-063.js initial  revision: 1.1
/cvsroot/mozilla/js/tests/js1_8_1/regress/regress-452498-068.js,v  <--  regress-452498-068.js initial  revision: 1.1
/cvsroot/mozilla/js/tests/js1_8_1/regress/regress-452498-071.js,v  <--  regress-452498-071.js initial  revision: 1.1
/cvsroot/mozilla/js/tests/js1_8_1/regress/regress-452498-072.js,v  <--  regress-452498-072.js initial  revision: 1.1
/cvsroot/mozilla/js/tests/js1_8_1/regress/regress-452498-073.js,v  <--  regress-452498-073.js initial  revision: 1.1
/cvsroot/mozilla/js/tests/js1_8_1/regress/regress-452498-074.js,v  <--  regress-452498-074.js initial  revision: 1.1
/cvsroot/mozilla/js/tests/js1_8_1/regress/regress-452498-075.js,v  <--  regress-452498-075.js initial  revision: 1.1
/cvsroot/mozilla/js/tests/js1_8_1/regress/regress-452498-076.js,v  <--  regress-452498-076.js initial  revision: 1.1
/cvsroot/mozilla/js/tests/js1_8_1/regress/regress-452498-077.js,v  <--  regress-452498-077.js initial  revision: 1.1
/cvsroot/mozilla/js/tests/js1_8_1/regress/regress-452498-079.js,v  <--  regress-452498-079.js initial  revision: 1.1
/cvsroot/mozilla/js/tests/js1_8_1/regress/regress-452498-082.js,v  <--  regress-452498-082.js initial  revision: 1.1
/cvsroot/mozilla/js/tests/js1_8_1/regress/regress-452498-091.js,v  <--  regress-452498-091.js initial  revision: 1.1
/cvsroot/mozilla/js/tests/js1_8_1/regress/regress-452498-092.js,v  <--  regress-452498-092.js initial  revision: 1.1
/cvsroot/mozilla/js/tests/js1_8_1/regress/regress-452498-098.js,v  <--  regress-452498-098.js initial  revision: 1.1
/cvsroot/mozilla/js/tests/js1_8_1/regress/regress-452498-099-a.js,v  <--  regress-452498-099-a.js initial  revision: 1.1
/cvsroot/mozilla/js/tests/js1_8_1/regress/regress-452498-099.js,v  <--  regress-452498-099.js initial  revision: 1.1
/cvsroot/mozilla/js/tests/js1_8_1/regress/regress-452498-101.js,v  <--  regress-452498-101.js initial  revision: 1.1
/cvsroot/mozilla/js/tests/js1_8_1/regress/regress-452498-102.js,v  <--  regress-452498-102.js initial  revision: 1.1
/cvsroot/mozilla/js/tests/js1_8_1/regress/regress-452498-103.js,v  <--  regress-452498-103.js initial  revision: 1.1
/cvsroot/mozilla/js/tests/js1_8_1/regress/regress-452498-104.js,v  <--  regress-452498-104.js initial  revision: 1.1
/cvsroot/mozilla/js/tests/js1_8_1/regress/regress-452498-107.js,v  <--  regress-452498-107.js initial  revision: 1.1
/cvsroot/mozilla/js/tests/js1_8_1/regress/regress-452498-108.js,v  <--  regress-452498-108.js initial  revision: 1.1
/cvsroot/mozilla/js/tests/js1_8_1/regress/regress-452498-110.js,v  <--  regress-452498-110.js initial  revision: 1.1
/cvsroot/mozilla/js/tests/js1_8_1/regress/regress-452498-111.js,v  <--  regress-452498-111.js initial  revision: 1.1
/cvsroot/mozilla/js/tests/js1_8_1/regress/regress-452498-112.js,v  <--  regress-452498-112.js initial  revision: 1.1
/cvsroot/mozilla/js/tests/js1_8_1/regress/regress-452498-114-a.js,v  <--  regress-452498-114-a.js initial  revision: 1.1
/cvsroot/mozilla/js/tests/js1_8_1/regress/regress-452498-114.js,v  <--  regress-452498-114.js initial  revision: 1.1
/cvsroot/mozilla/js/tests/js1_8_1/regress/regress-452498-116.js,v  <--  regress-452498-116.js initial  revision: 1.1
/cvsroot/mozilla/js/tests/js1_8_1/regress/regress-452498-117.js,v  <--  regress-452498-117.js initial  revision: 1.1
/cvsroot/mozilla/js/tests/js1_8_1/regress/regress-452498-118.js,v  <--  regress-452498-118.js initial  revision: 1.1
/cvsroot/mozilla/js/tests/js1_8_1/regress/regress-452498-119.js,v  <--  regress-452498-119.js initial  revision: 1.1
/cvsroot/mozilla/js/tests/js1_8_1/regress/regress-452498-121.js,v  <--  regress-452498-121.js initial  revision: 1.1
/cvsroot/mozilla/js/tests/js1_8_1/regress/regress-452498-123.js,v  <--  regress-452498-123.js initial  revision: 1.1
/cvsroot/mozilla/js/tests/js1_8_1/regress/regress-452498-129.js,v  <--  regress-452498-129.js initial  revision: 1.1
/cvsroot/mozilla/js/tests/js1_8_1/regress/regress-452498-130.js,v  <--  regress-452498-130.js initial  revision: 1.1
/cvsroot/mozilla/js/tests/js1_8_1/regress/regress-452498-131.js,v  <--  regress-452498-131.js initial  revision: 1.1
/cvsroot/mozilla/js/tests/js1_8_1/regress/regress-452498-135-a.js,v  <--  regress-452498-135-a.js initial  revision: 1.1
/cvsroot/mozilla/js/tests/js1_8_1/regress/regress-452498-135.js,v  <--  regress-452498-135.js initial  revision: 1.1
/cvsroot/mozilla/js/tests/js1_8_1/regress/regress-452498-138.js,v  <--  regress-452498-138.js initial  revision: 1.1
/cvsroot/mozilla/js/tests/js1_8_1/regress/regress-452498-139.js,v  <--  regress-452498-139.js initial  revision: 1.1
/cvsroot/mozilla/js/tests/js1_8_1/regress/regress-452498-155.js,v  <--  regress-452498-155.js initial  revision: 1.1
/cvsroot/mozilla/js/tests/js1_8_1/trace/regress-452498-01.js,v  <--  regress-452498-01.js initial  revision: 1.1

w/latest patch

js1_8_1/regress/regress-452498-074.js
Expected value '2', Actual value '1'

js1_8_1/regress/regress-452498-102.js 
Assertion failure: regs.sp == StackBase(fp)

js1_8_1/regress/regress-452498-117.js
Assertion failure: (fun->u.i.script)->upvarsOffset != 0
also crashes in opt.

js1_8_1/regress/regress-452498-119.js
Assertion failure: regs.sp == StackBase(fp)

js1_8_1/regress/regress-452498-138.js
Assertion failure: lexdep->frameLevel() <= funbox->level

js1_8_1/regress/regress-452498-139.js
Assertion failure: (fun->u.i.script)->upvarsOffset != 0
also crashes in opt

js1_8_1/regress/regress-452498-155.js
Assertion failure: pn_arity == PN_FUNC || pn_arity == PN_NAME
also crashes in opt
Comment 157 Brendan Eich [:brendan] 2009-03-21 00:41:07 PDT
(In reply to comment #152)
> with attachment 367963 [details] [diff] [review],
> js1_5/decompilation/regress-352073.js fails:
> 
> Expected value ' function ( ) { ( function x ( ) { } ) ; return x ; } ', Actual
> value ' function ( ) { return x ; } '

This is expected. The named function expression has no effects (ES3.1 fix to avoid creating a new Object as if by that expression, binding x in it) so is eliminated as a useless expression.

/be
Comment 158 Brendan Eich [:brendan] 2009-03-21 22:31:12 PDT
(In reply to comment #156)
> js1_8_1/regress/regress-452498-074.js
> Expected value '2', Actual value '1'

This test is expecting the wrong value -- it should expect 1, not 2. The bug Jesse was reporting in comment 74 was that the const could be updated from 1 to 2, due to a bug fixed many patchs ago here.

The other failures are legit -- thanks! -- and fixed by the new patch I'm going to attach after some more testing.

/be
Comment 159 Brendan Eich [:brendan] 2009-03-22 01:20:09 PDT
Created attachment 368765 [details] [diff] [review]
latest and greatest

Passes (patched to correct test bug noted in comment 158) the testsuite, fixes these tests compared to tm tip:

js1_8/genexps/regress-380237-03.js
js1_8_1/regress/regress-452498-039.js
js1_8_1/regress/regress-452498-092.js
js1_8_1/regress/regress-452498-107.js

Fixes stragglers noted in comment 155.

/be
Comment 160 Gary Kwong [:gkw] [:nth10sd] 2009-03-22 09:10:23 PDT
(In reply to comment #159)
> Created an attachment (id=368765) [details]
> latest and greatest

Does not require -j :
=====
(function(){for(var x in (x::window = x for (x in []))[[]]){}})()

Assertion failure: cg->upvars.lookup(atom), at ../jsemit.cpp:2034
=====
(eval("(function(){\
  watch(\"x\", function () { \
    new function ()y\
  } );\
  const y \
});"))();
x = NaN

Debug & opt crash [@ js_Interpret] near null
=====
({ set z(){},  set y x()--x })

Assertion failure: JOF_OPTYPE(op) == JOF_ATOM, at ../jsemit.cpp:5916
=====
Comment 161 Brendan Eich [:brendan] 2009-03-22 16:42:04 PDT
Created attachment 368812 [details] [diff] [review]
fixes for bugs Gary found, and rebased

Still tracking a bug running mochitests. More soon.

/be
Comment 162 Gary Kwong [:gkw] [:nth10sd] 2009-03-22 22:34:30 PDT
(In reply to comment #161)
> Created an attachment (id=368812) [details]
> fixes for bugs Gary found, and rebased
> 
> Still tracking a bug running mochitests. More soon.
> 
> /be

All previous recent bugs should have been fixed.

Requires -j :
=====
__defineGetter__("x3", Function)
undefined = x3;
undefined.prototype = []
for (var z = 0; z < 4; ++z) { new undefined() }

Assertion failure: !OBJ_GET_CLASS(cx, proto)->getObjectOps, at ../jsobj.cpp:2030
=====
Comment 163 Brendan Eich [:brendan] 2009-03-22 23:02:46 PDT
(In reply to comment #162)
> Requires -j :
> =====
> __defineGetter__("x3", Function)
> undefined = x3;
> undefined.prototype = []
> for (var z = 0; z < 4; ++z) { new undefined() }
> 
> Assertion failure: !OBJ_GET_CLASS(cx, proto)->getObjectOps, at
> ../jsobj.cpp:2030
> =====

This fails without this bug's patch, on tm tip. Please test tm tip shell too to avoid losing a bug that should be filed separately.

I'll leave filing this one to you, although it may have gal and my hg blame all over it. js_NewInstance has a dense Array proto here.

/be
Comment 164 Gary Kwong [:gkw] [:nth10sd] 2009-03-23 01:15:12 PDT
(In reply to comment #163)
> This fails without this bug's patch, on tm tip. Please test tm tip shell too to
> avoid losing a bug that should be filed separately.

You're right, apologies for that, I should have checked beforehand.

> I'll leave filing this one to you, although it may have gal and my hg blame all
> over it. js_NewInstance has a dense Array proto here.

Spun off as bug 484751, and on further investigation seems to be related to bug 480759.
Comment 165 Brendan Eich [:brendan] 2009-03-23 08:31:17 PDT
Created attachment 368896 [details] [diff] [review]
fix TraceRecorder::record_JSOP_DSLOT, passing MochiKit tests now

While inlining with callee identity guarding means we can burn in the callee at callDepth != 0, at callDepth 0 we must get(&cx->fp->argv[-2]).

/be
Comment 166 Brendan Eich [:brendan] 2009-03-23 10:13:48 PDT
Comment on attachment 368896 [details] [diff] [review]
fix TraceRecorder::record_JSOP_DSLOT, passing MochiKit tests now

Interdiff shows this rev also fixed a bad latent but in js_AddProperty, where a slot mapped by the property just added to the scope would be freed before false return (fake OOM to cause retry in the interpreter to handle add-property hard cases).

/be
Comment 167 Brendan Eich [:brendan] 2009-03-23 19:30:56 PDT
Created attachment 369011 [details] [diff] [review]
more fixes, now at jQuery tests, know what's wrong

Will fix shortly.

/be
Comment 168 Gary Kwong [:gkw] [:nth10sd] 2009-03-23 21:20:38 PDT
(In reply to comment #167)
> Created an attachment (id=369011) [details]
> more fixes, now at jQuery tests, know what's wrong

Does not require -j :
=====
(
  new Function("const x = (function () { if (1e+81){} else{x} } )"
))();

Opt crash [@ js_NewFlatClosure]
Assertion failure: (fun->u.i.script)->upvarsOffset != 0, at ../jsfun.cpp:1543
=====
for(let x; __defineSetter__; (<{x}></{x}> for (x in x))) {}

Assertion failure: cg->upvars.lookup(atom), at ../jsemit.cpp:2047
=====
Comment 169 Brendan Eich [:brendan] 2009-03-24 00:46:12 PDT
Created attachment 369038 [details] [diff] [review]
comment 168 fixes, plus deferred space policing in js_Interpret
Comment 170 Brendan Eich [:brendan] 2009-03-24 00:50:00 PDT
There may be a few more of the

for(let x; __defineSetter__; (<{x}></{x}> for (x in x))) {}

form, which involves E4X parse trees not having the correct beginning source coordinates propagated from child to parent on construction (during recursive descent parsing we sometimes make the child node first, even though the parse is top-down). The generator expression syntax has to recast the left operand of 'for' in a new static scope (an implicit generator function). This requires exact source coords.

/be
Comment 171 IU 2009-03-24 09:45:39 PDT
Brendan: based on your comment #167, does your patch possibly fix Bug 479553?  Of course, Bug 479553 is strictly SpiderMonkey and does not require TM.
Comment 172 Brendan Eich [:brendan] 2009-03-24 11:24:40 PDT
(In reply to comment #171)
> Brendan: based on your comment #167, does your patch possibly fix Bug 479553? 
> Of course, Bug 479553 is strictly SpiderMonkey and does not require TM.

I don't think so, but if you could try the patch (or if you don't build from source, perhaps there are tryserver builds -- sayrer was gonna fire some up). We should fix bug 479553, but we need a reproducible and ideally reduced testcase to study back here in the lab. Thanks,

/be
Comment 173 Brendan Eich [:brendan] 2009-03-24 19:37:25 PDT
Created attachment 369208 [details] [diff] [review]
more fixes, still some issues mochitesting

I hope bugzilla interdiff works.

/be
Comment 174 Brendan Eich [:brendan] 2009-03-24 19:39:48 PDT
No, bugzilla interdiff fails. Is there a bug on its lameness?

/be
Comment 175 Gary Kwong [:gkw] [:nth10sd] 2009-03-25 04:15:39 PDT
(In reply to comment #174)
> No, bugzilla interdiff fails. Is there a bug on its lameness?
> 
> /be

This might be the bug, though I could be wrong:

Bug 210407 - [PatchReader] interdiff does not work on files diffed against two different versions
Comment 176 Gary Kwong [:gkw] [:nth10sd] 2009-03-25 09:24:03 PDT
(In reply to comment #173)
> Created an attachment (id=369208) [details]
> more fixes, still some issues mochitesting

Does not require -j :
=====
if(delete( 5 ? [] : (function(){})() )) []

Assertion failure: pn_arity == PN_FUNC || pn_arity == PN_NAME, at ../jsparse.h:449
Opt crash [@ FindFunArgs] at null
=====
Comment 177 Brendan Eich [:brendan] 2009-03-25 19:12:50 PDT
Created attachment 369410 [details] [diff] [review]
fix for good the constant-folding-leaves-dead-funbox bug

from comment 176 (and earlier -- I should have put in a general fix on the first fuzzer-generated testcase).

/be
Comment 178 Gary Kwong [:gkw] [:nth10sd] 2009-03-26 07:27:12 PDT
(In reply to comment #177)
> Created an attachment (id=369410) [details]
> fix for good the constant-folding-leaves-dead-funbox bug
> 
> from comment 176 (and earlier -- I should have put in a general fix on the
> first fuzzer-generated testcase).
> 
> /be

This is occurring often, (does not require -j):
=====
eval("with({}) let(x=[])(function(){#2=x})()");

Assertion failure: slot < fp2->script->nfixed, at ../jsinterp.cpp:5610
=====
Comment 179 Brendan Eich [:brendan] 2009-03-26 08:01:21 PDT
Created attachment 369503 [details] [diff] [review]
fix bogus assertion in last patch

Sorry, I forgot about let at top level in trying to tighten that assertion. You can always check opt builds to see if anything goes south (not proof of absence of a bug, of course, but suggestive [at best] of bogus assertion).

/be
Comment 180 Brendan Eich [:brendan] 2009-03-26 16:00:42 PDT
Created attachment 369590 [details] [diff] [review]
refreshed and tweaked a bit

Still a bug (syndrome) to do with closures generated on trace, will fix shortly.

/be
Comment 181 Jesse Ruderman 2009-03-26 16:13:24 PDT
(function(print) { delete print; })(); print(3);

without patch: 3
with patch: "print is not defined"
Comment 182 Brendan Eich [:brendan] 2009-03-26 16:38:00 PDT
Created attachment 369594 [details] [diff] [review]
fix the bug Jesse found

Basic principle of compiler is to record what it knows ASAP. Failure to do so can result in loss of context (in this case, delete on a formal parameter).

/be
Comment 183 Jesse Ruderman 2009-03-26 17:07:17 PDT
I'm hitting the ++const issue in comment 39 (still? again?).
Comment 184 Jesse Ruderman 2009-03-26 17:12:16 PDT
const e = 8; print('' + ((e += 3)));

without patch: 11
with patch: function print() { [native code] }3
Comment 185 Jesse Ruderman 2009-03-26 17:18:39 PDT
{ var e = 3; let e = ""; } print(typeof e);

without patch: undefined
with patch: number
Comment 186 Gary Kwong [:gkw] [:nth10sd] 2009-03-26 17:56:02 PDT
(In reply to comment #182)
> Created an attachment (id=369594) [details]
> fix the bug Jesse found
> 
> Basic principle of compiler is to record what it knows ASAP. Failure to do so
> can result in loss of context (in this case, delete on a formal parameter).
> 
> /be

(I am quite sure that this issue is due to upvar2)

Requires -j:
=====
while((window = /x/) && 0)
gczeal(2)
for (var a = 0; a < 2; ++a) { let(x) ((function(){window = x})())}

Assertion failure: i != NULL, at ../jstracer.cpp:1896
Opt crash at:
(gdb) bt
#0  0x080c98d7 in isi2f ()
#1  0x080c9948 in isPromoteInt ()
#2  0x080c9c1c in TraceRecorder::writeBack ()
#3  0x080cf6fb in TraceRecorder::set ()
#4  0x080d2118 in TraceRecorder::stack ()
#5  0x080d32ca in TraceRecorder::record_JSOP_GETUPVAR ()
#6  0x080d703a in TraceRecorder::monitorRecording ()
#7  0x080eb73c in js_Interpret ()
#8  0x0807b430 in js_Execute ()
#9  0x0805226b in JS_ExecuteScript ()
#10 0x0804d6a7 in Process ()
#11 0x0804dee5 in main ()
Comment 187 Jesse Ruderman 2009-03-26 18:09:42 PDT
const x; for (x in []);

without patch: no output
with patch: "SyntaxError: invalid for/in left-hand side: ..."
Comment 188 Brendan Eich [:brendan] 2009-03-26 22:35:31 PDT
Created attachment 369629 [details] [diff] [review]
Fix bug from comment 184

(In reply to comment #183)
> I'm hitting the ++const issue in comment 39 (still? again?).

That's covered by js1_8_1/regress/regress-452498-039.js and it is fixed AFAICT. What is your exact testcase?

(In reply to comment #184)
> const e = 8; print('' + ((e += 3)));
> 
> without patch: 11
> with patch: function print() { [native code] }3

Fixed.

(In reply to comment #185)
> { var e = 3; let e = ""; } print(typeof e);
> 
> without patch: undefined
> with patch: number

New in JS1.8.1:

js> { var e = 3; let e = ""; } print(typeof e);
typein:1: TypeError: redeclaration of variable e:
typein:1: { var e = 3; let e = ""; } print(typeof e);
typein:1: .................^

Can you dig it?

(In reply to comment #187)
> const x; for (x in []);
> 
> without patch: no output
> with patch: "SyntaxError: invalid for/in left-hand side: ..."

Ditto.

/be
Comment 189 Jesse Ruderman 2009-03-26 23:13:49 PDT
> > I'm hitting the ++const issue in comment 39 (still? again?).
> 
> That's covered by js1_8_1/regress/regress-452498-039.js and it is fixed AFAICT.
> What is your exact testcase?

Same testcase!  If the regression test is passing for you, it's probably not testing the same thing as comment 39.

> New in JS1.8.1 ... Can you dig it?

Sure, that's reasonable.  I can tell my comparison-fuzzers to ignore this precise change, as well as the const/for..in change.
Comment 190 Brendan Eich [:brendan] 2009-03-27 00:59:36 PDT
(In reply to comment #186)
> (In reply to comment #182)
> (I am quite sure that this issue is due to upvar2)
> 
> Requires -j:
> =====
> while((window = /x/) && 0)
> gczeal(2)
> for (var a = 0; a < 2; ++a) { let(x) ((function(){window = x})())}

Change the var to a let and it works. I forgot that Igor patched global let to require a patch-up pass after code generation to adjust block depths based on the total number of gvars (and regexps too). This will require fixing up every upvar array in a function that references such a let binding. Ugh.

/be
Comment 191 Brendan Eich [:brendan] 2009-03-27 01:45:12 PDT
Created attachment 369634 [details] [diff] [review]
fix comment 186 bug, improve var vs. let, etc., error checking

Whew, it was easy to adjust JSOP_{GET,CALL}UPVAR to bias let slots by the number of fixed slots in the global frame containing the let slot.

I also fixed some var vs. let case analysis:

js> { var x; {let x;} }
js> { let x; {var x;} }
typein:2: TypeError: redeclaration of let x:
typein:2: { let x; {var x;} }
typein:2: ..............^

let can shadow var, and let can shadow itself (oops, broke that in haste in the previous patch), but once you use let, var (or const) can't be used in an inner block for the same variable name. The hoisting is weird enough to make this a "don't do that". It would be too extreme to ban all var once someone started using let, but redeclaring with hoisting should be an error.

/be
Comment 192 Brendan Eich [:brendan] 2009-03-27 10:22:48 PDT
Created attachment 369691 [details] [diff] [review]
latest and greatest

Fix a bug Gary found and reported over IRC:

with({x: (x -= 0)}){([]); const x }

/be
Comment 193 Gary Kwong [:gkw] [:nth10sd] 2009-03-27 21:46:54 PDT
(In reply to comment #192)
> Created an attachment (id=369691) [details]
> latest and greatest
> 
> Fix a bug Gary found and reported over IRC:
> 
> with({x: (x -= 0)}){([]); const x }
> 
> /be

Does not require -j :
=====
watch("x", Function)
NaN = uneval({ get \u3056 (){ return undefined } })
x+=NaN

Assertion failure: afunbox->parent, at ../jsparse.cpp:1912
Opt crash near null [@ JSCompiler::setFunctionKinds]
=====
Comment 194 Brendan Eich [:brendan] 2009-03-28 14:05:18 PDT
Created attachment 369840 [details] [diff] [review]
fix bug reported in comment 193
Comment 195 Brendan Eich [:brendan] 2009-03-30 00:47:42 PDT
Created attachment 369973 [details] [diff] [review]
fix mochitest "FRAME_LENGTH is not defined" bug
Comment 196 Gary Kwong [:gkw] [:nth10sd] 2009-03-30 02:27:53 PDT
(In reply to comment #195)
> Created an attachment (id=369973) [details]
> fix mochitest "FRAME_LENGTH is not defined" bug

Does not require -j:
=====
__defineSetter__("x", eval)
let(x = []) 
((function(){ let(x4) 
  ((function(){ [] 
    ((function(){ 
    for(let y in [x, '']) x = y
    }
    )())
  }
  )())
}
)())

Assertion failure: localKind == JSLOCAL_VAR || localKind == JSLOCAL_CONST, at ../jsfun.cpp:916
=====
(function (){
  var x;
  eval("const x; (function ()x)");
}
)();

Assertion failure: lexdep->isLet(), at ../jsparse.cpp:1900
=====
Comment 197 Brendan Eich [:brendan] 2009-03-30 05:04:14 PDT
Created attachment 369992 [details] [diff] [review]
fix bug and bogo-assert reported in comment 196

I also had to fix this assertion in js_Interpret:

@@ -6955,7 +6991,7 @@ js_Interpret(JSContext *cx)
         // To keep things simple, we hard-code imacro exception handlers here.
         if (*fp->imacpc == JSOP_NEXTITER) {
             // pc may point to JSOP_DUP here due to bug 474854.
-            JS_ASSERT(*regs.pc == JSOP_CALL || *regs.pc == JSOP_DUP);
+            JS_ASSERT(*regs.pc == JSOP_CALL || *regs.pc == JSOP_DUP || *regs.pc == JSOP_TRUE);
             if (js_ValueIsStopIteration(cx->exception)) {
                 cx->throwing = JS_FALSE;
                 cx->exception = JSVAL_VOID;

IIRC gal has a patch for this in flight. It bites during mochitests.

/be
Comment 198 Brendan Eich [:brendan] 2009-03-30 17:48:47 PDT
Created attachment 370117 [details] [diff] [review]
refreshed patch
Comment 199 Robert Sayre 2009-03-31 00:28:10 PDT
Comment on attachment 370117 [details] [diff] [review]
refreshed patch

This patch didn't build on Windows, but it did on Linux and Mac. There, it passed mochitest, mochichrome, reftest, and crashtest. It failed in TUnit and browser-chrome with identical failures.

http://pastebin.mozilla.org/638612
Comment 200 Robert Sayre 2009-03-31 00:30:44 PDT
That single TUnit test has some functions in .forEach(function(){...})

run like so, with your $OBJDIR

SOLO_FILE="test_405938_restore_queries.js" make -C $OBJDIR/toolkit/components/places/tests/ check-one

The performance tests looked fine.
Comment 201 Brendan Eich [:brendan] 2009-03-31 17:55:33 PDT
Created attachment 370330 [details] [diff] [review]
fix unit test bug sayrer reported
Comment 202 Robert Sayre 2009-03-31 18:35:16 PDT
msvc fixes:

enum KIND {} can't have the name "CONST" in it

JSVariablesParser can't take a default value for bool inLetHead
Comment 203 Robert Sayre 2009-03-31 18:40:33 PDT
Created attachment 370333 [details] [diff] [review]
upvar with windows fixes
Comment 204 Brendan Eich [:brendan] 2009-03-31 23:35:12 PDT
Created attachment 370361 [details] [diff] [review]
KONST in honor of MSFT; VariablesParser will be better in a cleanup pass later
Comment 205 Andreas Gal :gal 2009-03-31 23:40:40 PDT
How about Const? _CONST? _CONST_? CONSTANT? CNST? KONST and KLASS are really ... lame.
Comment 206 Brendan Eich [:brendan] 2009-03-31 23:49:21 PDT
KONST rawks, c'mon! Old hacker tradition, cf. klazz. Plus, I'm in Germany this week :-P.

The Mozilla (Gecko) way would be kCONST, kLET, etc. I may do that but not right now.

/be
Comment 207 Jeff Walden [:Waldo] (remove +bmo to email) 2009-04-01 00:00:30 PDT
I would prefer an #ifdef CONST / #undef CONST / #endif assuming it's only used in internal headers, otherwise expand to CONSTANT.
Comment 208 Boris Zbarsky [:bz] 2009-04-01 05:45:47 PDT
By clear analogy with "clazz", it should be "CONZZT".
Comment 209 Igor Bukanov 2009-04-01 11:04:49 PDT
CONST should be NOT_LET_VAR.
Comment 210 Robert Sayre 2009-04-01 11:11:35 PDT
current status:

failure in TUnit, but different from last time
7 failures in browser-chrome, down from 18
compiles on Windows, exhibits identical behavior to linux and mac
performance tests continue to look ok

The failing TUnit test is a private browsing test that closes over a variable declared with let. Here's the command line, substitute your OBJDIR for "/Users/sayrer/dev/clean-debug-tracemonkey/".

/Users/sayrer/dev/clean-debug-tracemonkey/dist/bin/xpcshell -g /Users/sayrer/dev/clean-debug-tracemonkey/dist/bin -j -s -f /Users/sayrer/dev/clean-tm/testing/xpcshell/head.js -e "function do_load_httpd_js() {load('/Users/sayrer/dev/clean-debug-tracemonkey/dist/bin/components/httpd.js');}" -f /Users/sayrer/dev/clean-debug-tracemonkey/_tests/xpcshell/test_dm/unit/head_download_manager.js -f /Users/sayrer/dev/clean-debug-tracemonkey/_tests/xpcshell/test_dm/unit/test_privatebrowsing.js -f /Users/sayrer/dev/clean-tm/testing/xpcshell/tail.js -e "_execute_test();"
Comment 211 Robert Sayre 2009-04-01 11:36:03 PDT
The difference is here:

   // Create Download-B
   let dlB = addDownload({
     targetFile: fileB,
     sourceURI: downloadBSource,
     downloadName: downloadBName,
     runBeforeStart: function (aDownload) {
       // Check that Download-B is retrievable
       do_check_eq(dm.activeDownloadCount, 1);
       do_check_true(is_active_download_available(aDownload.id,
         downloadBSource, fileB, downloadBName));
       do_check_true(is_download_available(aDownload.id,
         downloadBSource, fileB, downloadBName));
     }
   });
   
The values for targetFile: fileB, sourceURI: downloadBSource, and downloadName: downloadBName are declared below this code with let. If I move their decalarations above this function, everything works.
Comment 212 Brendan Eich [:brendan] 2009-04-01 11:51:14 PDT
There's a closure around the switch that surrounds that let dlB = <object init that uses outer let bindings), so the fileB, etc., refs are upvars.

(Bikeshedders, stay your hands! Waldo's #undef CONST won, as sayrer says because it lets us rant in comments against windows.h namespace pollution.)

/be
Comment 213 Brendan Eich [:brendan] 2009-04-01 17:41:38 PDT
Created attachment 370547 [details] [diff] [review]
fix lexdep redirection bug + better msvc/windows.h portability
Comment 214 Brendan Eich [:brendan] 2009-04-02 08:42:02 PDT
Created attachment 370666 [details] [diff] [review]
rebased
Comment 215 Robert Sayre 2009-04-02 08:49:49 PDT
only two browser chrome failures:

TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/browser/base/content/test/browser_feed_tab.js | Timed out
TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/browser/components/privatebrowsing/test/browser/browser_privatebrowsing_transition.js | There should only be one tab - Got 2, expected 1
Comment 216 Dão Gottwald [:dao] 2009-04-02 08:53:18 PDT
... and the second failure is likely a side effect of the first one.
Comment 217 Robert Sayre 2009-04-02 10:39:00 PDT
The failure in this test case is that the "$" doesn't reflect the correct value of pageInfo when executed, although the dump call I've inserted does.

  var pageInfo;
  ...
  function handlePageInfo() {
    dump("pageInfo: " + pageInfo.document + "\n");
    function $(aId) { return pageInfo.document.getElementById(aId) };
    var feedTab = $("feedTab");
    var feedListbox = $("feedListbox");
  ...
Comment 218 Robert Sayre 2009-04-02 10:41:20 PDT
I've tried creating a simple shell test case for this situation, but everything I write passes.
Comment 219 Nochum Sossonko [:Natch] 2009-04-02 10:52:32 PDT
It's a function (a) in a function (b) where the variable is declared in the outer function and initialized in a different inner (c) function, then used in yet another inner function _within_ b (so that's three levels down). Hope that helps.
Comment 220 Brendan Eich [:brendan] 2009-04-02 17:03:24 PDT
Created attachment 370753 [details] [diff] [review]
fix bug reported in comment 215

Rebased to tip just now, too.

/be
Comment 221 Brendan Eich [:brendan] 2009-04-02 19:03:45 PDT
Created attachment 370767 [details] [diff] [review]
followup fix to remove bogus off-by-1-level code in FindFunArgs

This fixes the testIntOverflow case in trace-test.js that mysteriously failed only when run in sequence, not when run by itself.

/be
Comment 222 Brendan Eich [:brendan] 2009-04-03 02:35:51 PDT
Created attachment 370824 [details] [diff] [review]
fix hole in funarg escape analysis, and refresh to tm tip

mrbkap should be looking at interdiffs (which may not work -- probably won't, the last few changes Igor landed caused major .rej file pain :-(), as he and I worked through some of the analysis originally. It's more complex (algorithmically) than we thought.

/be
Comment 223 Brendan Eich [:brendan] 2009-04-03 02:49:54 PDT
Interdiffs are hopeless, sorry. Read the latest patch.

/be
Comment 224 Gary Kwong [:gkw] [:nth10sd] 2009-04-03 08:53:32 PDT
(In reply to comment #222)
> Created an attachment (id=370824) [details]
> fix hole in funarg escape analysis, and refresh to tm tip
>

Does not require -j:
=====
((#0={}) for(x in null))

Assertion failure: cg->flags & TCF_HAS_SHARPS, at ../jsemit.cpp:6439
=====
Comment 225 Robert Sayre 2009-04-03 12:01:27 PDT
This passed the tryserver
Comment 226 Mike Shaver (:shaver -- probably not reading bugmail closely) 2009-04-03 12:21:31 PDT
(In reply to comment #225)
> This passed the tryserver

SHIP. IT.
Comment 227 Brendan Eich [:brendan] 2009-04-03 12:40:39 PDT
Created attachment 370933 [details] [diff] [review]
fix bug from comment 224

TCF_HAS_SHARPS deoptimizes to JSOP_NEWINIT, generator expressions involve rewriting the AST, ergo any sharp to the left of a genexp in a tree context (fun or prog) will deoptimize array initialisers in the genexp (which is desugared into a generator lambda that's called immediately). Boo hoo!

/be
Comment 230 Blake Kaplan (:mrbkap) (please use needinfo!) 2009-04-03 16:54:53 PDT
Comment on attachment 370933 [details] [diff] [review]
fix bug from comment 224

*stamp*
Comment 231 Gary Kwong [:gkw] [:nth10sd] 2009-04-04 02:26:56 PDT
Landed on TM.

http://hg.mozilla.org/tracemonkey/rev/972c44aa9d1f

"upvar2, aka the big one (452598, r=mrbkap)."
Comment 232 Gary Kwong [:gkw] [:nth10sd] 2009-04-04 02:30:04 PDT
(Checkin was landed by Brendan)

Re-nominating in-testsuite? to pick up some more testcases after the first round of in-testsuite.
Comment 233 Brendan Eich [:brendan] 2009-04-04 15:13:43 PDT
Created attachment 371081 [details] [diff] [review]
attempt to recover and refresh patch that was backed out

If you back someone out, please update the bug...

/be
Comment 234 Brendan Eich [:brendan] 2009-04-04 15:20:03 PDT
Created attachment 371082 [details] [diff] [review]
plain diff of last two patches

For reference, since interdiff fails.

/be
Comment 235 Andreas Gal :gal 2009-04-04 15:23:58 PDT
Sorry, forgot to update the bug. I usually do hg export from the history to recover the version that was checked in/backed out.
Comment 236 Andreas Gal :gal 2009-04-04 15:24:55 PDT
You will need this patch to land on top of my mismatch patch:

--- jstracer.cpp
+++ jstracer.cpp
@@ -7698,7 +7788,7 @@ TraceRecorder::record_JSOP_GETDSLOT()
     LIns* dslots_ins = NULL;
     LIns* v_ins = stobj_get_dslot(callee_ins, index, dslots_ins);
 
-    unbox_jsval(callee->dslots[index], v_ins);
+    unbox_jsval(callee->dslots[index], v_ins, snapshot(BRANCH_EXIT));
     stack(0, v_ins);
     return true;
 }
Comment 237 Brendan Eich [:brendan] 2009-04-04 15:31:10 PDT
The latest patch is up to date, already!

/be
Comment 238 Gary Kwong [:gkw] [:nth10sd] 2009-04-04 21:06:38 PDT
(In reply to comment #227)
> Created an attachment (id=370933) [details]
> fix bug from comment 224
> 
> TCF_HAS_SHARPS deoptimizes to JSOP_NEWINIT, generator expressions involve
> rewriting the AST, ergo any sharp to the left of a genexp in a tree context
> (fun or prog) will deoptimize array initialisers in the genexp (which is
> desugared into a generator lambda that's called immediately). Boo hoo!
> 
> /be

This previous patch was backed out.

http://hg.mozilla.org/tracemonkey/rev/c33d9b115c6d
Comment 239 Robert Sayre 2009-04-05 10:33:34 PDT
(In reply to comment #237)
> The latest patch is up to date, already!

This is crashing in

layout/forms/test/test_bug348236.html

on all 3 platforms.
Comment 240 Robert Sayre 2009-04-05 11:30:22 PDT
It's crashing in keypressOnSelect(), at the line

> sec.PrivilegeManager.enablePrivilege("UniversalXPConnect")
Comment 241 Brendan Eich [:brendan] 2009-04-05 14:57:03 PDT
Created attachment 371168 [details] [diff] [review]
fix dumb bugs in analysis

The work queue needs to be a set (add at most once), and it needs to not confuse full for empty state.

/be
Comment 242 Brendan Eich [:brendan] 2009-04-05 15:03:57 PDT
Created attachment 371169 [details] [diff] [review]
don't forget to check analyzeFunction's new bool return value
Comment 243 Robert Sayre 2009-04-05 20:38:40 PDT
(In reply to comment #242)
> Created an attachment (id=371169) [details]
> don't forget to check analyzeFunction's new bool return value

This patch passed tryserver on all platforms.
Comment 245 Brendan Eich [:brendan] 2009-04-06 11:19:10 PDT
*** Bug 473271 has been marked as a duplicate of this bug. ***
Comment 247 Bob Clary [:bc:] 2009-04-07 06:11:42 PDT
ecma_3/ExecutionContexts/regress-448595-01.js
js1_5/Regress/regress-146596.js
js1_7/regress/regress-350387.js
js1_8/regress/regress-465567-02.js
js1_8_1/regress/regress-452498-052.js
js1_8_1/regress/regress-452498-082.js

all fail now on tracemonkey with variations of "redeclaration of let..."
Comment 248 Bob Clary [:bc:] 2009-04-07 06:29:12 PDT
ecma_3/ExecutionContexts/regress-448595-01.js
js1_5/Regress/regress-146596.js

do not use let. It appears to happen when redeclaring an exception variable using |var| in a catch clause.

js1_7/regress/regress-350387.js
js1_8/regress/regress-465567-02.js
js1_8_1/regress/regress-452498-052.js
js1_8_1/regress/regress-452498-082.js

are redeclarations.
Comment 250 Bob Clary [:bc:] 2009-05-19 06:34:31 PDT
(In reply to comment #153)

> 
> js1_8_1/regress/regress-452498-110.js 
> function g(a) { const b = a; print(actual = ++b); return b; } reason: Expected
> value '21', Actual value '01'

This still fails. Bad test?
Comment 251 Brendan Eich [:brendan] 2009-05-19 10:21:07 PDT
(In reply to comment #250)
> (In reply to comment #153)
> 
> > 
> > js1_8_1/regress/regress-452498-110.js 
> > function g(a) { const b = a; print(actual = ++b); return b; } reason: Expected
> > value '21', Actual value '01'
> 
> This still fails. Bad test?

Definitely -- the relevant code is:

  expect = '21';
  actual = 0;

  function g(a) { const b = a; print(actual = ++b); return b; }
  actual = String(actual) + g(1);
  reportCompare(expect, actual, 'function g(a) { const b = a; print(actual = ++b); return b; }');

The definition of function g does not alter actual, so the assignment

  actual = String(actual) + g(1);

is emphatically going to convert 0 to '0' and then append to it. So the exepcted result can't start with '2' and must start with '0'.

And g(1) returns the constant b assigned from a=1, or 1.

As noted in comment 110, 1.9 and older will print the wrong result of ++b.

/be
Comment 252 TheRave 2009-05-21 08:06:03 PDT
Why break this bugfix the extension stockicker? 

When Stockticker is installed and Fx was compiled with changeset 24809:b2d7dbeb0f1b its ok, but with changeset 24812:79606200f871 the extension breaks the entire firefox.

You can't open or close tabs, addonmanager has the scrollbar on left side, scroll with mouse in addon manager doesnt work, options window is empty...

1. Create a clean profile.
2. Install Stockticker 1.04 or 1.05 and restart
3. go to js console - no error
4. go to addon manager and then you get the following messages (possible you need 2 or 3 trys before fx break)

Error: formatURLPref: Couldn't get pref: extensions.getMoreExtensionsURL
Source File: file:///C:/Programme/Mozilla_Firefox31/components/nsURLFormatter.js
Line: 68

Error: uncaught exception: [Exception... "Component returned failure code: 0xc1f30001 (NS_ERROR_NOT_INITIALIZED) [nsIPrefBranch2.getBoolPref]"  nsresult: "0xc1f30001 (NS_ERROR_NOT_INITIALIZED)"  location: "JS frame :: chrome://mozapps/content/extensions/extensions.js :: updateGlobalCommands :: line 2187"  data: no]

5. go to options window - its empty

Error: uncaught exception: [Exception... "Component returned failure code: 0xc1f30001 (NS_ERROR_NOT_INITIALIZED) [nsIPrefBranch.getBoolPref]"  nsresult: "0xc1f30001 (NS_ERROR_NOT_INITIALIZED)"  location: "JS frame :: chrome://global/content/bindings/preferences.xml ::  :: line 576"  data: no]
Comment 253 Mike Shaver (:shaver -- probably not reading bugmail closely) 2009-05-21 08:08:51 PDT
Do you have chrome JIT enabled?  Please file a bug with your problem and that detail, and mention it here.
Comment 254 TheRave 2009-05-21 08:38:59 PDT
JIT on/off its equal. Bug 494186 was created.
Comment 255 Bob Clary [:bc:] 2009-06-01 17:17:35 PDT
js1_8_1/trace/trace-test.js
http://hg.mozilla.org/tracemonkey/rev/61892f57b46a

js/tests/js1_8_1/extensions/regress-452498-162.js
js/tests/js1_8_1/extensions/regress-452498-193.js
js/tests/js1_8_1/extensions/regress-452498-196.js
js/tests/js1_8_1/extensions/regress-452498-224.js
js/tests/js1_8_1/regress/regress-452498-052.js
js/tests/js1_8_1/regress/regress-452498-082.js
js/tests/js1_8_1/regress/regress-452498-110.js
js/tests/js1_8_1/regress/regress-452498-160.js
js/tests/js1_8_1/regress/regress-452498-168-1.js
js/tests/js1_8_1/regress/regress-452498-168-2.js
js/tests/js1_8_1/regress/regress-452498-176.js
js/tests/js1_8_1/regress/regress-452498-178.js
js/tests/js1_8_1/regress/regress-452498-181.js
js/tests/js1_8_1/regress/regress-452498-184.js
js/tests/js1_8_1/regress/regress-452498-185.js
js/tests/js1_8_1/regress/regress-452498-187.js
js/tests/js1_8_1/regress/regress-452498-191.js
js/tests/js1_8_1/regress/regress-452498-192.js
http://hg.mozilla.org/tracemonkey/rev/46b6e8f3801d
Comment 256 Bob Clary [:bc:] 2009-06-04 10:43:05 PDT
v 1.9.1, 1.9.2 modulo:

bug 496127 - js1_8_1/trace/regress-452498-01.js

and js1_8_1/regress/regress-452498-168-2.js time out/slow.
Comment 257 Bob Clary [:bc:] 2009-08-07 14:14:21 PDT
/cvsroot/mozilla/js/tests/js1_8/regress/regress-465567-02.js,v  <--  regress-465567-02.js
new revision: 1.2; previous revision: 1.1

/cvsroot/mozilla/js/tests/js1_8_1/extensions/regress-452498-162.js,v  <--  regress-452498-162.js
initial revision: 1.1

/cvsroot/mozilla/js/tests/js1_8_1/extensions/regress-452498-193.js,v  <--  regress-452498-193.js
initial revision: 1.1

/cvsroot/mozilla/js/tests/js1_8_1/extensions/regress-452498-196.js,v  <--  regress-452498-196.js
initial revision: 1.1

/cvsroot/mozilla/js/tests/js1_8_1/extensions/regress-452498-224.js,v  <--  regress-452498-224.js
initial revision: 1.1

/cvsroot/mozilla/js/tests/js1_8_1/regress/regress-452498-052.js,v  <--  regress-452498-052.js
new revision: 1.2; previous revision: 1.1

/cvsroot/mozilla/js/tests/js1_8_1/regress/regress-452498-082.js,v  <--  regress-452498-082.js
new revision: 1.2; previous revision: 1.1

/cvsroot/mozilla/js/tests/js1_8_1/regress/regress-452498-110.js,v  <--  regress-452498-110.js
new revision: 1.2; previous revision: 1.1

/cvsroot/mozilla/js/tests/js1_8_1/regress/regress-452498-160.js,v  <--  regress-452498-160.js
initial revision: 1.1

/cvsroot/mozilla/js/tests/js1_8_1/regress/regress-452498-168-1.js,v  <--  regress-452498-168-1.js
initial revision: 1.1

/cvsroot/mozilla/js/tests/js1_8_1/regress/regress-452498-168-2.js,v  <--  regress-452498-168-2.js
initial revision: 1.1

/cvsroot/mozilla/js/tests/js1_8_1/regress/regress-452498-176.js,v  <--  regress-452498-176.js
initial revision: 1.1

/cvsroot/mozilla/js/tests/js1_8_1/regress/regress-452498-178.js,v  <--  regress-452498-178.js
initial revision: 1.1

/cvsroot/mozilla/js/tests/js1_8_1/regress/regress-452498-181.js,v  <--  regress-452498-181.js
initial revision: 1.1

/cvsroot/mozilla/js/tests/js1_8_1/regress/regress-452498-184.js,v  <--  regress-452498-184.js
initial revision: 1.1

/cvsroot/mozilla/js/tests/js1_8_1/regress/regress-452498-185.js,v  <--  regress-452498-185.js
initial revision: 1.1

/cvsroot/mozilla/js/tests/js1_8_1/regress/regress-452498-187.js,v  <--  regress-452498-187.js
initial revision: 1.1

/cvsroot/mozilla/js/tests/js1_8_1/regress/regress-452498-191.js,v  <--  regress-452498-191.js
initial revision: 1.1

/cvsroot/mozilla/js/tests/js1_8_1/regress/regress-452498-192.js,v  <--  regress-452498-192.js
initial revision: 1.1

/cvsroot/mozilla/js/tests/slow-n-1.9.1.tests,v  <--  slow-n-1.9.1.tests
new revision: 1.5; previous revision: 1.4

/cvsroot/mozilla/js/tests/slow-n-1.9.2.tests,v  <--  slow-n-1.9.2.tests
new revision: 1.4; previous revision: 1.3

/cvsroot/mozilla/js/tests/slow-n.tests,v  <--  slow-n.tests
new revision: 1.15; previous revision: 1.14

/cvsroot/mozilla/js/tests/spidermonkey-n-1.9.1.tests,v  <--  spidermonkey-n-1.9.1.tests
new revision: 1.11; previous revision: 1.10

/cvsroot/mozilla/js/tests/spidermonkey-n-1.9.2.tests,v  <--  spidermonkey-n-1.9.2.tests
new revision: 1.4; previous revision: 1.3
Comment 258 Bob Clary [:bc:] 2009-08-07 14:42:23 PDT
cvsroot/mozilla/js/tests/js1_8_1/trace/trace-test.js,v  <--  trace-test.js
new revision: 1.14; previous revision: 1.13

/cvsroot/mozilla/js/tests/shell.js,v  <--  shell.js
Comment 259 Bob Clary [:bc:] 2009-08-18 00:57:29 PDT
disable js1_5/decompilation/regress-352073.js for 1.8.x, 1.9.0 due to changes in test.

http://hg.mozilla.org/tracemonkey/rev/59eb7304b290

/cvsroot/mozilla/js/tests/spidermonkey-n-1.8.0.tests,v  <--  spidermonkey-n-1.8.0.tests
new revision: 1.3; previous revision: 1.2

/cvsroot/mozilla/js/tests/spidermonkey-n-1.8.1.tests,v  <--  spidermonkey-n-1.8.1.tests
new revision: 1.3; previous revision: 1.2

/cvsroot/mozilla/js/tests/spidermonkey-n-1.9.0.tests,v  <--  spidermonkey-n-1.9.0.tests
new revision: 1.5; previous revision: 1.4

Note You need to log in before you can comment on or make changes to this bug.