Bug 452498 (upvar2)

TM: Can we jit heavyweight functions? (upvar, part deux)

VERIFIED FIXED in mozilla1.9.1

Status

()

defect
P1
normal
VERIFIED FIXED
11 years ago
8 years ago

People

(Reporter: bzbarsky, Assigned: brendan)

Tracking

(Blocks 1 bug, {verified1.9.1})

Trunk
mozilla1.9.1
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9.1 +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(6 attachments, 75 obsolete attachments)

1.18 KB, text/plain
Details
8.54 KB, text/plain
Details
10.86 KB, text/plain
Details
3.46 KB, text/plain
Details
4.60 KB, patch
Details | Diff | Splinter Review
560.50 KB, patch
mrbkap
: review+
Details | Diff | Splinter Review
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.
Assignee

Comment 2

11 years ago
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
Assignee: general → brendan
OS: Mac OS X → All
Priority: -- → P1
Hardware: PC → All
Target Milestone: --- → mozilla1.9.1
Assignee

Comment 3

11 years ago
(In reply to comment #2)
> Ah, here is the "upvar, part deux" bug I want. See 

Oops, should have been "See bug 445262."

/be
Status: NEW → ASSIGNED

Updated

11 years ago
Depends on: upvar1
Summary: TM: Can we jit heavyweight functions? → TM: Can we jit heavyweight functions? (upvar, part deux)

Comment 4

11 years ago
A note about display optimization: why the display is per context and not per frame? With the former it is hard to optimize escaping closures that overlived the caller frames, while with the latter this is pretty straightforward. 

The idea is to store in the display pointers to arg and slots arrays of the caller's frames. When the parent frame terminates, the pointers will be replaced by the pointers to the corresponding arrays from the call object.

Comment 5

11 years ago
Here is details of the display optimization that should work with escaping closures:

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

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

3. When the function starts, the reference to the array of upper frames or call objects is stored in the frame itself for faster access. Similarly, when an eval script starts, the array of the upper frames is constructed and stored.

Comment 6

11 years ago
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?
Blocks: 460050
Assignee

Comment 7

11 years ago
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
Assignee

Comment 8

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

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

> SETUPVAR up, slot, value

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

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

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

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

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

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

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

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

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

SETLOCAL?

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

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

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

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

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

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

This machinery already exists.

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

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

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

/be

Comment 9

11 years ago
(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.
Assignee

Comment 10

11 years ago
(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
Assignee

Comment 11

11 years ago
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
Assignee

Comment 12

11 years ago
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
Assignee

Comment 13

11 years ago
Better is coming, ho ho ho.

/be
Some kind of JavaScript Christmas miracle?
Assignee

Comment 15

11 years ago
Posted patch backup of work in progress (obsolete) — Splinter Review
Assignee

Comment 16

11 years ago
Posted patch backup, v2 (obsolete) — Splinter Review
Attachment #355512 - Attachment is obsolete: true
Assignee

Updated

11 years ago
Blocks: 473014
Assignee

Comment 17

11 years ago
Posted patch backup, v3 (obsolete) — Splinter Review
Attachment #355527 - Attachment is obsolete: true
Assignee

Comment 18

11 years ago
Posted patch backup, v4 (obsolete) — Splinter Review
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
Attachment #356403 - Attachment is obsolete: true
Assignee

Comment 19

11 years ago
Posted patch backup, v5 (obsolete) — Splinter Review
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
Attachment #356611 - Attachment is obsolete: true

Updated

11 years ago
Blocks: 473271
Assignee

Updated

11 years ago
Blocks: 472528
Assignee

Updated

11 years ago
Duplicate of this bug: 458838
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. :-/
Assignee

Comment 22

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

/be
Awesome, operators are standing by.

Updated

11 years ago
Flags: blocking1.9.1+
Assignee

Comment 24

10 years ago
Posted patch backup, v6 (obsolete) — Splinter Review
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
Attachment #356637 - Attachment is obsolete: true
Assignee

Comment 25

10 years ago
Posted patch backup, v7 (obsolete) — Splinter Review
Very close now, will have testable patch tomorrow. Need sleep, sorry for delays -- whole-family flu finally going away.

/be
Attachment #361019 - Attachment is obsolete: true
Assignee

Comment 26

10 years ago
Posted patch backup, v7a (obsolete) — Splinter Review
Attachment #361232 - Attachment is obsolete: true
Assignee

Comment 27

10 years ago
Posted patch v8 (obsolete) — Splinter Review
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
Attachment #361236 - Attachment is obsolete: true
Assignee

Comment 28

10 years ago
Posted patch traces bz's testcase (obsolete) — Splinter Review
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
Attachment #361452 - Attachment is obsolete: true
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
Assignee

Comment 32

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

/be
Attachment #361483 - Attachment is obsolete: true
(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
Assignee

Comment 34

10 years ago
Attachment #361696 - Attachment is obsolete: true

Comment 35

10 years ago
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.
Assignee

Comment 36

10 years ago
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
Attachment #361810 - Attachment is obsolete: true

Comment 38

10 years ago
[0 for (a in [])]

Assertion failure: pn->pn_type == TOK_LET, at ../jsemit.cpp:1838

Comment 39

10 years ago
const e = 0; print(++e);

Without patch: 0
With patch: 1

Comment 40

10 years ago
This crashes in js_Interpret:

function m()
{
	function a() { }
	function b() { a(); }
	this.c = function () { b(); }
}
(new m).c();

Comment 41

10 years ago
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|.
Assignee

Comment 44

10 years ago
Right, the analysis is buggy but the NULL_CLOSURE judgment is correct -- no need for fat scope chain if the function does not escape but is only called with the frame stack holding its upvars.

/be

Comment 45

10 years ago
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;
Assignee

Comment 46

10 years ago
Attachment #361834 - Attachment is obsolete: true
Assignee

Comment 47

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

/be
Attachment #361939 - Attachment is obsolete: true

Updated

10 years ago
Blocks: 477733
Assignee

Comment 48

10 years ago
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
Attachment #361971 - Attachment is obsolete: true
Assignee

Comment 49

10 years ago
(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

Updated

10 years ago
Alias: upvar2
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; }
Alias: upvar2
No longer blocks: 477733
Alias: upvar2
Blocks: 477733
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; }()");
Assignee

Comment 55

10 years ago
More in a bit.

/be
Attachment #362191 - Attachment is obsolete: true
Assignee

Comment 56

10 years ago
Hoisting!!! Grrrr.

/be
Attachment #362358 - Attachment is obsolete: true
Assignee

Comment 57

10 years ago
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.
Assignee

Comment 59

10 years ago
Posted patch fix bogus assertion (obsolete) — Splinter Review
Gary, sorry about that -- try this and find me on IRC if there are other such bogo-asserts.

/be
Attachment #362550 - Attachment is obsolete: true
Assignee

Comment 60

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

/be
Attachment #362598 - Attachment is obsolete: true

Comment 61

10 years ago
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.
Assignee

Comment 63

10 years ago
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
Assignee

Comment 64

10 years ago
(In reply to comment #63)
> I'll file a bug on nsSessionStore.js today.

Bug 479005.

/be
Assignee

Comment 65

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

/be
Attachment #362610 - Attachment is obsolete: true
(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.
Assignee

Comment 67

10 years ago
Posted patch getting there (obsolete) — Splinter Review
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
Attachment #362868 - Attachment is obsolete: true
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.
Assignee

Comment 69

10 years ago
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
Attachment #363084 - Attachment is obsolete: true
Assignee

Comment 70

10 years ago
Posted patch rebased (obsolete) — Splinter Review
Will go through jorendorff's comments tomorrow too.

/be
Attachment #363272 - Attachment is obsolete: true

Comment 71

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

Comment 72

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

Comment 73

10 years ago
js> function() { var arguments }
Assertion failure: (uintN)i < ss->top, at ../jsopcode.cpp:2801

and lots of other problems with "const arguments" etc

Comment 74

10 years ago
const [d] = [1]; [d] = [2]; print(d);

without this patch: 1
with this patch: 2

Comment 75

10 years ago
js> (function p(){ p = 3; })
function p() {
    p;
}

js> (function p(){ p = 3; return p; })()
Assertion failure: regs.sp == StackBase(fp), at ../jsinterp.cpp:2980

Comment 76

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

Comment 77

10 years ago
(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..
Assignee

Comment 81

10 years ago
Attachment #363283 - Attachment is obsolete: true
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(){})) 

=====
Assignee

Comment 83

10 years ago
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
Version: unspecified → Trunk
Assignee

Comment 84

10 years ago
Posted patch passes js tests (obsolete) — Splinter Review
Passes js/tests (-L lc2 lc3 spidermonkey-n.tests slow-n.tests, modulo big-O woes).

/be
Attachment #363341 - Attachment is obsolete: true
Assignee

Comment 85

10 years ago
Posted patch rebased to tip of tm (obsolete) — Splinter Review
Attachment #362606 - Attachment is obsolete: true
Attachment #364360 - Attachment is obsolete: true

Comment 86

10 years ago
The patch still fails to run the sunspider test harness:

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

It uses a lot of undeclared globals and other high quality code constructs.

Comment 87

10 years ago
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
Assignee

Comment 88

10 years ago
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
Assignee

Comment 89

10 years ago
Attachment #364370 - Attachment is obsolete: true
sunspider-in-browser doesn't count compilation, right?  What does it do there?

Comment 91

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

Comment 92

10 years ago
(function (e) { var e; const e; })

Without patch: "TypeError: redeclaration of var e"
With patch:    "TypeError: redeclaration of formal parameter e:"
Assignee

Comment 93

10 years ago
Yikes. Dumb bug.

/be
Attachment #364409 - Attachment is obsolete: true
Assignee

Comment 94

10 years ago
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
Assignee

Comment 95

10 years ago
(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
Assignee

Comment 96

10 years ago
(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
Assignee

Comment 97

10 years ago
Attachment #364435 - Attachment is obsolete: true
(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
=====

Comment 103

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

Comment 104

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

Comment 105

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

/be
Attachment #364488 - Attachment is obsolete: true
Assignee

Comment 106

10 years ago
Latest patch passe the js testsuite and trace-test.js without -j. With -j I have not run the testsuite, and trace-test.js passes except for testThinLoopDemote. I will investigate that later tonight.

/be

Comment 107

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

Comment 108

10 years ago
print(function p(){p})

With patch:
function p() {
}

Without patch:
function p() {
    p;
}

Comment 109

10 years ago
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.
Assignee

Comment 110

10 years ago
(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.

Comment 112

10 years ago
q = new Function("(function() { q(3); })(); const q;"); q();

Without patch: TypeError: q is not a function
With patch: InternalError: too much recursion

Comment 113

10 years ago
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
Assignee

Comment 115

10 years ago
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
Attachment #364635 - Attachment is obsolete: true
(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.....

Comment 118

10 years ago
(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
=====
Assignee

Comment 120

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

/be
Attachment #364650 - Attachment is obsolete: true
(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.
Assignee

Comment 125

10 years ago
(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
Assignee

Comment 126

10 years ago
(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.
Assignee

Comment 128

10 years ago
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
Assignee

Updated

10 years ago
Attachment #364753 - Attachment is obsolete: true
Assignee

Updated

10 years ago
Attachment #364868 - Attachment is patch: true
Attachment #364868 - Attachment mime type: application/octet-stream → text/plain
(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
=====

Updated

10 years ago
Priority: P1 → P2
Assignee

Updated

10 years ago
Blocks: 456588
Assignee

Updated

10 years ago
Blocks: 448605
Assignee

Updated

10 years ago
Blocks: 483179
Assignee

Comment 132

10 years ago
I'll work on this more tonight.

/be
Attachment #364868 - Attachment is obsolete: true
Assignee

Comment 133

10 years ago
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
Attachment #367341 - Attachment is obsolete: true
Assignee

Comment 134

10 years ago
Posted patch fix comment 130 hard cases (obsolete) — Splinter Review
Attachment #367351 - Attachment is obsolete: true
(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
Assignee

Comment 136

10 years ago
(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
Attachment #367359 - Attachment is obsolete: true
Assignee

Comment 137

10 years ago
Posted patch refresh to match tm tip (obsolete) — Splinter Review
Attachment #367407 - Attachment is obsolete: true
(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]
===

Comment 140

10 years ago
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'
Assignee

Comment 141

10 years ago
(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
Assignee

Comment 142

10 years ago
(In reply to comment #138)
> Requires -j:
> ===
> (function(){eval("([0 for each (x in [/x/, this, /x/])])")})()
> 
> (again this assertion is similar to bug 457065, but the exact wording is
> different, I don't know if they're the same issue.)
> Assertion failure: fp->thisp == JSVAL_TO_OBJECT(fp->argv[-1]), at
> ../jstracer.cpp:4213

This is testing bug 457065, not this bug.

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

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

/be

Comment 143

10 years ago
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.

Comment 145

10 years ago
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.
Assignee

Comment 146

10 years ago
Posted patch refresh to match tm tip (obsolete) — Splinter Review
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
Attachment #367408 - Attachment is obsolete: true

Comment 148

10 years ago
Comment on attachment 367836 [details]
assertion on browser startup

This is from nsBrowserContentHandler registerSelf(). It does have an inline function.

Comment 149

10 years ago
Posted file preliminary tests and results (obsolete) —
untar in js1_8_1/regress/

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

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

there are several other fails. test review appreciated.

Comment 150

10 years ago
(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.
Assignee

Comment 151

10 years ago
Attachment #367808 - Attachment is obsolete: true

Comment 152

10 years ago
with attachment 367963 [details] [diff] [review],
js1_5/decompilation/regress-352073.js fails:

Expected value ' function ( ) { ( function x ( ) { } ) ; return x ; } ', Actual value ' function ( ) { return x ; } '

Comment 153

10 years ago
failures with jit:

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

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

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

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

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

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

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

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

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

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

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

js1_8_1/trace/regress-452498-01.js NORMAL, 
time nonjit: 945, time    jit: 1621 reason: Expected value 'true', Actual value 'false'

Comment 154

10 years ago
Posted file tests v2 w/ results log. (obsolete) —
Attachment #367910 - Attachment is obsolete: true
(In reply to comment #151)
> Created an attachment (id=367963) [details]
> fix XDR of function script to not treat upvars as vars

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

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

Assertion failure: pn_arity == PN_FUNC || pn_arity == PN_NAME, at ../jsparse.h:449
Opt crash [@ FindFunArgs]

Comment 156

10 years ago
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
Flags: in-testsuite+

Updated

10 years ago
Attachment #368032 - Attachment is obsolete: true
Assignee

Comment 157

10 years ago
(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
Assignee

Comment 158

10 years ago
(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
Assignee

Comment 159

10 years ago
Posted patch latest and greatest (obsolete) — Splinter Review
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
Attachment #367963 - Attachment is obsolete: true
(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
=====
Assignee

Comment 161

10 years ago
Still tracking a bug running mochitests. More soon.

/be
Attachment #368765 - Attachment is obsolete: true
(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
=====
Assignee

Comment 163

10 years ago
(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.
Assignee

Comment 165

10 years ago
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
Attachment #368812 - Attachment is obsolete: true
Assignee

Comment 166

10 years ago
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
Assignee

Comment 167

10 years ago
Will fix shortly.

/be
Attachment #368896 - Attachment is obsolete: true
(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
=====
Assignee

Comment 170

10 years ago
There may be a few more of the

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

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

/be

Comment 171

10 years ago
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.
Assignee

Comment 172

10 years ago
(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
Assignee

Comment 173

10 years ago
I hope bugzilla interdiff works.

/be
Attachment #369038 - Attachment is obsolete: true
Assignee

Comment 174

10 years ago
No, bugzilla interdiff fails. Is there a bug on its lameness?

/be
(In reply to comment #174)
> No, bugzilla interdiff fails. Is there a bug on its lameness?
> 
> /be

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

Bug 210407 - [PatchReader] interdiff does not work on files diffed against two different versions
(In reply to comment #173)
> Created an attachment (id=369208) [details]
> more fixes, still some issues mochitesting

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

Assertion failure: pn_arity == PN_FUNC || pn_arity == PN_NAME, at ../jsparse.h:449
Opt crash [@ FindFunArgs] at null
=====
Assignee

Comment 177

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

/be
Attachment #369208 - Attachment is obsolete: true
(In reply to comment #177)
> Created an attachment (id=369410) [details]
> fix for good the constant-folding-leaves-dead-funbox bug
> 
> from comment 176 (and earlier -- I should have put in a general fix on the
> first fuzzer-generated testcase).
> 
> /be

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

Assertion failure: slot < fp2->script->nfixed, at ../jsinterp.cpp:5610
=====
Assignee

Comment 179

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

/be
Attachment #369410 - Attachment is obsolete: true
Assignee

Comment 180

10 years ago
Posted patch refreshed and tweaked a bit (obsolete) — Splinter Review
Still a bug (syndrome) to do with closures generated on trace, will fix shortly.

/be
Attachment #369503 - Attachment is obsolete: true

Comment 181

10 years ago
(function(print) { delete print; })(); print(3);

without patch: 3
with patch: "print is not defined"
Assignee

Comment 182

10 years ago
Posted patch fix the bug Jesse found (obsolete) — Splinter Review
Basic principle of compiler is to record what it knows ASAP. Failure to do so can result in loss of context (in this case, delete on a formal parameter).

/be
Attachment #369590 - Attachment is obsolete: true

Comment 183

10 years ago
I'm hitting the ++const issue in comment 39 (still? again?).

Comment 184

10 years ago
const e = 8; print('' + ((e += 3)));

without patch: 11
with patch: function print() { [native code] }3

Comment 185

10 years ago
{ var e = 3; let e = ""; } print(typeof e);

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

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

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

Assertion failure: i != NULL, at ../jstracer.cpp:1896
Opt crash at:
(gdb) bt
#0  0x080c98d7 in isi2f ()
#1  0x080c9948 in isPromoteInt ()
#2  0x080c9c1c in TraceRecorder::writeBack ()
#3  0x080cf6fb in TraceRecorder::set ()
#4  0x080d2118 in TraceRecorder::stack ()
#5  0x080d32ca in TraceRecorder::record_JSOP_GETUPVAR ()
#6  0x080d703a in TraceRecorder::monitorRecording ()
#7  0x080eb73c in js_Interpret ()
#8  0x0807b430 in js_Execute ()
#9  0x0805226b in JS_ExecuteScript ()
#10 0x0804d6a7 in Process ()
#11 0x0804dee5 in main ()

Comment 187

10 years ago
const x; for (x in []);

without patch: no output
with patch: "SyntaxError: invalid for/in left-hand side: ..."
Assignee

Comment 188

10 years ago
Posted patch Fix bug from comment 184 (obsolete) — Splinter Review
(In reply to comment #183)
> I'm hitting the ++const issue in comment 39 (still? again?).

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

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

Fixed.

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

New in JS1.8.1:

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

Can you dig it?

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

Ditto.

/be
Attachment #369594 - Attachment is obsolete: true

Comment 189

10 years ago
> > I'm hitting the ++const issue in comment 39 (still? again?).
> 
> That's covered by js1_8_1/regress/regress-452498-039.js and it is fixed AFAICT.
> What is your exact testcase?

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

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

Sure, that's reasonable.  I can tell my comparison-fuzzers to ignore this precise change, as well as the const/for..in change.
Assignee

Comment 190

10 years ago
(In reply to comment #186)
> (In reply to comment #182)
> (I am quite sure that this issue is due to upvar2)
> 
> Requires -j:
> =====
> while((window = /x/) && 0)
> gczeal(2)
> for (var a = 0; a < 2; ++a) { let(x) ((function(){window = x})())}

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

/be
Assignee

Comment 191

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

I also fixed some var vs. let case analysis:

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

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

/be
Attachment #369629 - Attachment is obsolete: true
Assignee

Comment 192

10 years ago
Posted patch latest and greatest (obsolete) — Splinter Review
Fix a bug Gary found and reported over IRC:

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

/be
Attachment #369634 - Attachment is obsolete: true
(In reply to comment #192)
> Created an attachment (id=369691) [details]
> latest and greatest
> 
> Fix a bug Gary found and reported over IRC:
> 
> with({x: (x -= 0)}){([]); const x }
> 
> /be

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

Assertion failure: afunbox->parent, at ../jsparse.cpp:1912
Opt crash near null [@ JSCompiler::setFunctionKinds]
=====
Assignee

Comment 194

10 years ago
Attachment #369691 - Attachment is obsolete: true
Assignee

Comment 195

10 years ago
Attachment #369840 - Attachment is obsolete: true
(In reply to comment #195)
> Created an attachment (id=369973) [details]
> fix mochitest "FRAME_LENGTH is not defined" bug

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

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

Assertion failure: lexdep->isLet(), at ../jsparse.cpp:1900
=====
Assignee

Comment 197

10 years ago
I also had to fix this assertion in js_Interpret:

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

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

/be
Attachment #369973 - Attachment is obsolete: true

Updated

10 years ago
Priority: P2 → P1
Assignee

Comment 198

10 years ago
Posted patch refreshed patch (obsolete) — Splinter Review
Attachment #369992 - Attachment is obsolete: true

Comment 199

10 years ago
Comment on attachment 370117 [details] [diff] [review]
refreshed patch

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

http://pastebin.mozilla.org/638612

Comment 200

10 years ago
That single TUnit test has some functions in .forEach(function(){...})

run like so, with your $OBJDIR

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

The performance tests looked fine.
Assignee

Comment 201

10 years ago
Attachment #370117 - Attachment is obsolete: true

Comment 202

10 years ago
msvc fixes:

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

JSVariablesParser can't take a default value for bool inLetHead

Comment 203

10 years ago
Posted patch upvar with windows fixes (obsolete) — Splinter Review

Comment 205

10 years ago
How about Const? _CONST? _CONST_? CONSTANT? CNST? KONST and KLASS are really ... lame.
Assignee

Comment 206

10 years ago
KONST rawks, c'mon! Old hacker tradition, cf. klazz. Plus, I'm in Germany this week :-P.

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

/be
I would prefer an #ifdef CONST / #undef CONST / #endif assuming it's only used in internal headers, otherwise expand to CONSTANT.
By clear analogy with "clazz", it should be "CONZZT".

Comment 209

10 years ago
CONST should be NOT_LET_VAR.

Comment 210

10 years ago
current status:

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

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

/Users/sayrer/dev/clean-debug-tracemonkey/dist/bin/xpcshell -g /Users/sayrer/dev/clean-debug-tracemonkey/dist/bin -j -s -f /Users/sayrer/dev/clean-tm/testing/xpcshell/head.js -e "function do_load_httpd_js() {load('/Users/sayrer/dev/clean-debug-tracemonkey/dist/bin/components/httpd.js');}" -f /Users/sayrer/dev/clean-debug-tracemonkey/_tests/xpcshell/test_dm/unit/head_download_manager.js -f /Users/sayrer/dev/clean-debug-tracemonkey/_tests/xpcshell/test_dm/unit/test_privatebrowsing.js -f /Users/sayrer/dev/clean-tm/testing/xpcshell/tail.js -e "_execute_test();"

Comment 211

10 years ago
The difference is here:

   // Create Download-B
   let dlB = addDownload({
     targetFile: fileB,
     sourceURI: downloadBSource,
     downloadName: downloadBName,
     runBeforeStart: function (aDownload) {
       // Check that Download-B is retrievable
       do_check_eq(dm.activeDownloadCount, 1);
       do_check_true(is_active_download_available(aDownload.id,
         downloadBSource, fileB, downloadBName));
       do_check_true(is_download_available(aDownload.id,
         downloadBSource, fileB, downloadBName));
     }
   });
   
The values for targetFile: fileB, sourceURI: downloadBSource, and downloadName: downloadBName are declared below this code with let. If I move their decalarations above this function, everything works.
Assignee

Comment 212

10 years ago
There's a closure around the switch that surrounds that let dlB = <object init that uses outer let bindings), so the fileB, etc., refs are upvars.

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

/be
Assignee

Comment 213

10 years ago
Attachment #370361 - Attachment is obsolete: true
Assignee

Comment 214

10 years ago
Posted patch rebased (obsolete) — Splinter Review
Attachment #370547 - Attachment is obsolete: true

Comment 215

10 years ago
only two browser chrome failures:

TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/browser/base/content/test/browser_feed_tab.js | Timed out
TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/browser/components/privatebrowsing/test/browser/browser_privatebrowsing_transition.js | There should only be one tab - Got 2, expected 1
... and the second failure is likely a side effect of the first one.

Comment 217

10 years ago
The failure in this test case is that the "$" doesn't reflect the correct value of pageInfo when executed, although the dump call I've inserted does.

  var pageInfo;
  ...
  function handlePageInfo() {
    dump("pageInfo: " + pageInfo.document + "\n");
    function $(aId) { return pageInfo.document.getElementById(aId) };
    var feedTab = $("feedTab");
    var feedListbox = $("feedListbox");
  ...

Comment 218

10 years ago
I've tried creating a simple shell test case for this situation, but everything I write passes.
It's a function (a) in a function (b) where the variable is declared in the outer function and initialized in a different inner (c) function, then used in yet another inner function _within_ b (so that's three levels down). Hope that helps.

Updated

10 years ago
Blocks: 485177
Assignee

Comment 220

10 years ago
Rebased to tip just now, too.

/be
Attachment #370666 - Attachment is obsolete: true
Assignee

Comment 221

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

/be
Attachment #370753 - Attachment is obsolete: true
Assignee

Updated

10 years ago
Attachment #370330 - Attachment is obsolete: true
Assignee

Comment 222

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

/be
Attachment #370767 - Attachment is obsolete: true
Assignee

Comment 223

10 years ago
Interdiffs are hopeless, sorry. Read the latest patch.

/be
(In reply to comment #222)
> Created an attachment (id=370824) [details]
> fix hole in funarg escape analysis, and refresh to tm tip
>

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

Assertion failure: cg->flags & TCF_HAS_SHARPS, at ../jsemit.cpp:6439
=====

Comment 225

10 years ago
This passed the tryserver
(In reply to comment #225)
> This passed the tryserver

SHIP. IT.
Assignee

Comment 227

10 years ago
Posted patch fix bug from comment 224 (obsolete) — Splinter Review
TCF_HAS_SHARPS deoptimizes to JSOP_NEWINIT, generator expressions involve rewriting the AST, ergo any sharp to the left of a genexp in a tree context (fun or prog) will deoptimize array initialisers in the genexp (which is desugared into a generator lambda that's called immediately). Boo hoo!

/be
Attachment #370824 - Attachment is obsolete: true
Landed on TM.

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

"upvar2, aka the big one (452598, r=mrbkap)."
Whiteboard: fixed-in-tracemonkey
(Checkin was landed by Brendan)

Re-nominating in-testsuite? to pick up some more testcases after the first round of in-testsuite.
Flags: in-testsuite+ → in-testsuite?

Updated

10 years ago
Depends on: 486820
Assignee

Comment 233

10 years ago
If you back someone out, please update the bug...

/be
Attachment #370933 - Attachment is obsolete: true
Assignee

Comment 234

10 years ago
For reference, since interdiff fails.

/be

Comment 235

10 years ago
Sorry, forgot to update the bug. I usually do hg export from the history to recover the version that was checked in/backed out.

Comment 236

10 years ago
You will need this patch to land on top of my mismatch patch:

--- jstracer.cpp
+++ jstracer.cpp
@@ -7698,7 +7788,7 @@ TraceRecorder::record_JSOP_GETDSLOT()
     LIns* dslots_ins = NULL;
     LIns* v_ins = stobj_get_dslot(callee_ins, index, dslots_ins);
 
-    unbox_jsval(callee->dslots[index], v_ins);
+    unbox_jsval(callee->dslots[index], v_ins, snapshot(BRANCH_EXIT));
     stack(0, v_ins);
     return true;
 }
Assignee

Comment 237

10 years ago
The latest patch is up to date, already!

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

This previous patch was backed out.

http://hg.mozilla.org/tracemonkey/rev/c33d9b115c6d
Whiteboard: fixed-in-tracemonkey

Comment 239

10 years ago
(In reply to comment #237)
> The latest patch is up to date, already!

This is crashing in

layout/forms/test/test_bug348236.html

on all 3 platforms.

Comment 240

10 years ago
It's crashing in keypressOnSelect(), at the line

> sec.PrivilegeManager.enablePrivilege("UniversalXPConnect")
Assignee

Comment 241

10 years ago
Posted patch fix dumb bugs in analysis (obsolete) — Splinter Review
The work queue needs to be a set (add at most once), and it needs to not confuse full for empty state.

/be
Attachment #371081 - Attachment is obsolete: true
Attachment #371168 - Flags: review?(mrbkap)
Assignee

Comment 242

10 years ago
Attachment #371168 - Attachment is obsolete: true
Attachment #371169 - Flags: review?(mrbkap)
Attachment #371168 - Flags: review?(mrbkap)

Comment 243

10 years ago
(In reply to comment #242)
> Created an attachment (id=371169) [details]
> don't forget to check analyzeFunction's new bool return value

This patch passed tryserver on all platforms.
Attachment #371169 - Flags: review?(mrbkap) → review+
Assignee

Updated

10 years ago
Duplicate of this bug: 473271

Comment 247

10 years ago
ecma_3/ExecutionContexts/regress-448595-01.js
js1_5/Regress/regress-146596.js
js1_7/regress/regress-350387.js
js1_8/regress/regress-465567-02.js
js1_8_1/regress/regress-452498-052.js
js1_8_1/regress/regress-452498-082.js

all fail now on tracemonkey with variations of "redeclaration of let..."

Comment 248

10 years ago
ecma_3/ExecutionContexts/regress-448595-01.js
js1_5/Regress/regress-146596.js

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

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

are redeclarations.

Updated

10 years ago
Depends on: 487209

Updated

10 years ago
Depends on: 487215

Updated

10 years ago
Depends on: 487251
Depends on: 487563
Depends on: 487957
Assignee

Updated

10 years ago
Depends on: 487967

Updated

10 years ago
Depends on: 488015
Depends on: 488034

Updated

10 years ago
Depends on: 488029

Updated

10 years ago
Depends on: 488050
Depends on: 488272
Assignee

Updated

10 years ago
Depends on: 487430
Assignee

Updated

10 years ago
Depends on: 490339
Depends on: 490568
Blocks: 458838
Depends on: 491806
Assignee

Updated

10 years ago
Depends on: 490818
Depends on: 492714

Updated

10 years ago
Depends on: 493177
Assignee

Updated

10 years ago
No longer depends on: 493177
Assignee

Updated

10 years ago
Depends on: 493466

Comment 250

10 years ago
(In reply to comment #153)

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

This still fails. Bad test?
Assignee

Comment 251

10 years ago
(In reply to comment #250)
> (In reply to comment #153)
> 
> > 
> > js1_8_1/regress/regress-452498-110.js 
> > function g(a) { const b = a; print(actual = ++b); return b; } reason: Expected
> > value '21', Actual value '01'
> 
> This still fails. Bad test?

Definitely -- the relevant code is:

  expect = '21';
  actual = 0;

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

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

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

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

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

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

/be

Comment 252

10 years ago
Why break this bugfix the extension stockicker? 

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

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

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

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

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

5. go to options window - its empty

Error: uncaught exception: [Exception... "Component returned failure code: 0xc1f30001 (NS_ERROR_NOT_INITIALIZED) [nsIPrefBranch.getBoolPref]"  nsresult: "0xc1f30001 (NS_ERROR_NOT_INITIALIZED)"  location: "JS frame :: chrome://global/content/bindings/preferences.xml ::  :: line 576"  data: no]
Do you have chrome JIT enabled?  Please file a bug with your problem and that detail, and mention it here.

Comment 254

10 years ago
JIT on/off its equal. Bug 494186 was created.
Assignee

Updated

10 years ago
No longer depends on: 494186

Comment 255

10 years ago
js1_8_1/trace/trace-test.js
http://hg.mozilla.org/tracemonkey/rev/61892f57b46a

js/tests/js1_8_1/extensions/regress-452498-162.js
js/tests/js1_8_1/extensions/regress-452498-193.js
js/tests/js1_8_1/extensions/regress-452498-196.js
js/tests/js1_8_1/extensions/regress-452498-224.js
js/tests/js1_8_1/regress/regress-452498-052.js
js/tests/js1_8_1/regress/regress-452498-082.js
js/tests/js1_8_1/regress/regress-452498-110.js
js/tests/js1_8_1/regress/regress-452498-160.js
js/tests/js1_8_1/regress/regress-452498-168-1.js
js/tests/js1_8_1/regress/regress-452498-168-2.js
js/tests/js1_8_1/regress/regress-452498-176.js
js/tests/js1_8_1/regress/regress-452498-178.js
js/tests/js1_8_1/regress/regress-452498-181.js
js/tests/js1_8_1/regress/regress-452498-184.js
js/tests/js1_8_1/regress/regress-452498-185.js
js/tests/js1_8_1/regress/regress-452498-187.js
js/tests/js1_8_1/regress/regress-452498-191.js
js/tests/js1_8_1/regress/regress-452498-192.js
http://hg.mozilla.org/tracemonkey/rev/46b6e8f3801d
Flags: in-testsuite? → in-testsuite+

Updated

10 years ago
Depends on: 496134

Updated

10 years ago
Depends on: 496316

Comment 256

10 years ago
v 1.9.1, 1.9.2 modulo:

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

and js1_8_1/regress/regress-452498-168-2.js time out/slow.
Status: RESOLVED → VERIFIED
Depends on: 496532
Assignee

Updated

10 years ago
Depends on: 496422
Depends on: 498395
Depends on: 502714
Depends on: 502604

Updated

10 years ago
Depends on: 505220
Depends on: 499524

Comment 257

10 years ago
/cvsroot/mozilla/js/tests/js1_8/regress/regress-465567-02.js,v  <--  regress-465567-02.js
new revision: 1.2; previous revision: 1.1

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

Comment 258

10 years ago
cvsroot/mozilla/js/tests/js1_8_1/trace/trace-test.js,v  <--  trace-test.js
new revision: 1.14; previous revision: 1.13

/cvsroot/mozilla/js/tests/shell.js,v  <--  shell.js

Comment 259

10 years ago
disable js1_5/decompilation/regress-352073.js for 1.8.x, 1.9.0 due to changes in test.

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

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

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

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

Updated

10 years ago
Depends on: 530650
Depends on: 531516
Depends on: 527032
Depends on: 540243
Depends on: 541255
Depends on: 546615
Depends on: 633828
Depends on: 646820
No longer depends on: 530650
You need to log in before you can comment on or make changes to this bug.