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.

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

(In reply to comment #2) > Ah, here is the "upvar, part deux" bug I want. See Oops, should have been "See bug 445262." /be

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.

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.

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?

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

(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

(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.

(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

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

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

Better is coming, ho ho ho. /be

Some kind of JavaScript Christmas miracle?

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

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

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. :-/

New patch coming soon, later tonight I hope. Testing help when it arrives would be great. /be

Awesome, operators are standing by.

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

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

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

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

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. :)

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.)

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

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

(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

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.

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

[0 for (a in [])] Assertion failure: pn->pn_type == TOK_LET, at ../jsemit.cpp:1838

const e = 0; print(++e); Without patch: 0 With patch: 1

This crashes in js_Interpret: function m() { function a() { } function b() { a(); } this.c = function () { b(); } } (new m).c();

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 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.

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|.

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

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;

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

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

(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

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; }

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();

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(){});

// 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(){}); }

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; }()");

More in a bit. /be

Hoisting!!! Grrrr. /be

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

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.

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

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

Assertion failure: args.nCopiedVars == fun->u.i.nvars, at /Users/jruderman/tracemonkey/js/src/jsfun.cpp:2686

(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.

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

(In reply to comment #63) > I'll file a bug on nsSessionStore.js today. Bug 479005. /be

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

(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.

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

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.

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

Will go through jorendorff's comments tomorrow too. /be

js> x; var x Assertion failure: pn->pn_op == JSOP_NOP, at ../jsparse.cpp:1118

js> v = function p() { delete p; }; Assertion failure: JOF_OPTYPE(op) == JOF_ATOM, at ../jsemit.cpp:1711

js> function() { var arguments } Assertion failure: (uintN)i < ss->top, at ../jsopcode.cpp:2801 and lots of other problems with "const arguments" etc

const [d] = [1]; [d] = [2]; print(d); without this patch: 1 with this patch: 2

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

echo "for (let d = 0; d < 4; ++d) { d; }" | ~/tracemonkey/js/src/dbg-upvar/js 1: ReferenceError: d is not defined

(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

x; var x; function x() 0 Assertion failure: !(pn->pn_dflags & flag), at ../jsparse.h:635

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..

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(){})) =====

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

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

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.

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

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

sunspider-in-browser doesn't count compilation, right? What does it do there?

js> print(function eval() { eval("v"); }) function eval() { v("v"); }

(function (e) { var e; const e; }) Without patch: "TypeError: redeclaration of var e" With patch: "TypeError: redeclaration of formal parameter e:"

Yikes. Dumb bug. /be

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

(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

(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

(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)

(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. =====

(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.

(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 =====

js> (function(a) { v = 5; let [v] = [3]; (function(){ v; })(); })(); Assertion failure: !(pn->pn_dflags & flag), at ../jsparse.h:638

js> (function(a) { function b() { a; } function a() { } })(); Assertion failure: pn_defn, at ../jsparse.h:655

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

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

js> print(function d() { const d; ++d; }) function d() { const d; + d + 1; }

print(function p(){p}) With patch: function p() { } Without patch: function p() { p; }

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.

(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

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.

q = new Function("(function() { q(3); })(); const q;"); q(); Without patch: TypeError: q is not a function With patch: InternalError: too much recursion

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 };

(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

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

(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.

(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.....

(function() { (function() { e *= 4; })(); var e; })(); Without patch: no output With patch: ReferenceError: reference to undefined property "e"

(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 =====

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

(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

(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.

Brendan, can you make a tryserver build? Thanks.

(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

(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

(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.

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

(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.

(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. :)

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 =====

I'll work on this more tonight. /be

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

(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

(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

(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 ===

(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] ===

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'

(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

(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

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)

(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.

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.

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 on attachment 367836 [details] assertion on browser startup This is from nsBrowserContentHandler registerSelf(). It does have an inline function.

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.

(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.

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 ; } '

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'

(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]

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

(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

(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

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

(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 =====

Still tracking a bug running mochitests. More soon. /be

(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 =====

(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

(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.

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 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

Will fix shortly. /be

(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 =====

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

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.

(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

I hope bugzilla interdiff works. /be

No, bugzilla interdiff fails. Is there a bug on its lameness? /be