473096 - js1_5/Regress/regress-366601.js - Internal Error: script too large

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P2)

Description

[Bob Clary [:bc] (inactive)]

js1_5/Regress/regress-366601.js - Internal Error: script too large

regressed by http://hg.mozilla.org/tracemonkey/rev/6475993319c4 bug 466905

Comment 1

[Bob Clary [:bc] (inactive)]

also js1_8_1/decompilation/regress-352026.js

Comment 2

[Jeff Walden [:Waldo]]

Hm, maybe we need to bail to JSOP_NEWINIT if the array's sufficiently large...

Comment 3

[Brendan Eich [:brendan]]

The test makes an array initialiser with 100000 entries, and 1e5 fits easily in 24 bits. What is the stack for the internal error report?

/be

Comment 4

[Brendan Eich [:brendan]]

Array comprehensions and initialisers are limited to 2^24 elements, no need to switch over to JSOP_NEWINIT for more entries.

/be

Comment 5

[Brendan Eich [:brendan]]

Duh, 2^24 > 2^16, and JSScript.nfixed + cg->maxStackDepth <= 2^16. Silly of me to forget this.

Falling back on JSOP_NEWINIT is the ticket. This doesn't affect comprehensions, but we should keep ARRAY_INIT_LIMIT around for sanity and enforce it even though we fall back on JSOP_NEWINIT.

/be

Comment 6

[Brendan Eich [:brendan]]

The emitter can't know the full stack depth including what ends up in nfixed, for global code, since we emit each top level statement as we go to avoid using a ton of space for giant top level scripts (on the web, code generators spew scripts with 1e5 or more lines that just assign successive array elements using a long column of assignment statement expressions).

So we could still end up getting spurious "statement too large" internal error from js_NewScriptFromCG, for global code. For eval and function code we parse the entire program or function body and then emit.

The code that finalizes what ends up in script->nfixed for global programs is in jsparse.cpp:js_CompileScript after this comment:

    /*
     * Global variables and regexps shares the index space with locals. Due to
     * incremental code generation we need to patch the bytecode to adjust the
     * local references to skip the globals.
     */

The loop after this rebases local slot immediates during its scan over generated bytecode. Unfortunately it's hard to rewrite JSOP_NEWARRAY as the needed opcodes starting with JSOP_NEWINIT.

To play it safe, we could avoid JSOP_NEWARARY when compiling global code. Test (tc->flags & TCF_IN_FUNCTION) to decide that we're emitting function code. The eval case isn't so easy to identify, AFAICT (for too long it was because we abused the eval frame to pass scope chain, etc. to the compiler, so you could test the JSFRAME_EVAL flag, but that's been fixed). Cc'ing igor for ideas.

/be

Comment 7

[Brendan Eich [:brendan]]

Forgot to add: needless to say, JSOP_NEWARRAY should have JOF_UINT16 format-type. This simplifies things all around.

/be

Comment 8

[Jeff Walden [:Waldo]]

The change in comment 1 is expected, as a byproduct of preserving correct parenthesization of a function call whose single argument is a parenthesized comma expression (this was a bug in the previous code):

function foo(a, b)
{
  return bar((a, b));
}

This also caused a redundant set of parentheses to be preserved around:

function() { z(yield 3); }

..to convert it to:

function ( ) { z ( ( yield 3 ) ) ; }

It's perhaps not ideal, but given how precedence is used to determine parenthesization in decompilation, and given that yield must have lower precedence than comma expressions for |fun((yield 3), "b")| to decompile correctly, I'm not actually sure how this could easily be fixed, and I don't think it's all that useful to try to fix it.

Too late tonight, but I'll deal with the length-limiting and above comments tomorrow...

Comment 9

[Brendan Eich [:brendan]]

yield does not have lower precedence than comma, we simply mandate parens as Python does to ward off confusion. For ES4/Harmony this was going to be relaxed, maybe.

The decompiler tries to avoid excessive parens, and the single param case fits that nit-picky bill. How hard can it be, to borrow from Homer Simpson?

/be

Comment 10

[Brendan Eich [:brendan]]

(In reply to comment #6)
> To play it safe, we could avoid JSOP_NEWARARY when compiling global code. Test
> (tc->flags & TCF_IN_FUNCTION) to decide that we're emitting function code.

This is sound.

> The
> eval case isn't so easy to identify, AFAICT (for too long it was because we
> abused the eval frame to pass scope chain, etc. to the compiler, so you could
> test the JSFRAME_EVAL flag, but that's been fixed). Cc'ing igor for ideas.

This is easy to test too: tc->parseContext->callerFrame will be non-null for eval and debugger equivalent compiler activations. As with functions, such eval code compiler activations parse an entire AST and then walk it to emit code, so they have a complete stack budget model -- no fixup post-pass required.

So test

    if ((tc->flags & TCF_IN_FUNCTION) || tc->parseContext->callerFrame)

to try to select JSOP_NEWARRAY, provided there is enough room on the stack for the fixed slots plus all the operands including the ones pushed for the array elements.

/be

Comment 11

[Jeff Walden [:Waldo]]

I'm worried about code generators that might possibly generate really huge arrays that could bite us here; I also don't think it's a good idea to let this regress in any case.

Comment 12

[Jeff Walden [:Waldo]]

Attachment: v1

This needs bug 396512 to be fully testable wrt op selection at top-level and in functions, but this does fix the original reported testsuite failure (as ordered, I think).

Comment 13

[Jeff Walden [:Waldo]]

Attachment: bump

Comment 14

[Brendan Eich [:brendan]]

Comment on attachment 370362 [details] [diff] [review]
bump

>+         * If no sharp variable is defined, the initializer is not for an array
>+         * comprehension, the initializer is not overlarge, and the initializer
>+         * is not in global code (which is more likely to use a large array that
>+         * might contain a stack-overflowing expression)

That's not the reason, exactly. Isn't the problem precisely that for global code, the emitter cannot judge stack depth exceeding 2^16 because of deferred gvar and regexp indexes below the operand stack? Say so directly.

>...         , use JSOP_NEWARRAY to
>+         * minimize opcodes and to more efficiently use the stack (with respect
>+         * to memory churn, not growth).

Churn has nothing to do with this change, AFAIK. It's all about avoiding the 2^16 limit and the unknown-as-yet script->nfixed bias to operand stack slots.

>          */
>         pn2 = pn->pn_head;
>-        op = JSOP_NEWARRAY;
>+        op = (JS_LIKELY(pn->pn_count < JS_BIT(16)) &&
>+              (cg->treeContext.parseContext->callerFrame ||

Global eval passes non-null callerFrame -- you need to test callerFrame->fun in addition to be sure you are in function code, i.e., not in the COMPILE_N_GO world where the nfixed "basement" to the operand stack can't be known before code gen.

>@@ -132,6 +132,16 @@ struct JSStackFrame {
> #ifdef DEBUG
>     jsrefcount      pcDisabledSave; /* for balanced property cache control */
> #endif
>+
>+    jsval*
>+    stackBase() {

C++ good style wants the type+declarator-mode and the declarator name and params and opening brace on the same line. Short methods can be one-liners:

    jsval* stackBase() { return slots + script->nfixed; }

But wait, you're breaking LiveConnect. See #ifdef __cpluscplus in context below.

>+        return slots + script->nfixed;
>+    }
>+
>+    void
>+    assertStackDepth(uintN depth) {
>+        JS_ASSERT(regs->sp >= stackBase() && depth <= JSUword(regs->sp - stackBase()));
>+    }
> };
> 
> #ifdef __cplusplus

This is why we use static helpers, not methods, like this one:

>@@ -145,7 +155,7 @@ FramePCOffset(JSStackFrame* fp)
> static JS_INLINE jsval *
> StackBase(JSStackFrame *fp)
> {
>-    return fp->slots + fp->script->nfixed;
>+    return fp->stackBase();
> }

Which suffices, so don't add stackBase (for now; were you to add it we'd want to lose StackBase).

/be

Comment 15

[Jeff Walden [:Waldo]]

Attachment: Fix comment, nix stackBase for now

(In reply to comment #14)
> That's not the reason, exactly.

Oh, I was misunderstanding your earlier comments, fixed.


> Churn has nothing to do with this change, AFAIK. It's all about avoiding the
> 2^16 limit and the unknown-as-yet script->nfixed bias to operand stack slots.

Well, it does somewhat -- no pushing a value to pop it immediately.  But I guess that's no different from pushing everything and then popping everything, overall, in some ways, except that pointers don't have to be decremented singly to do the popping.


> Global eval passes non-null callerFrame -- you need to test callerFrame->fun
> in addition to be sure you are in function code, i.e., not in the COMPILE_N_GO
> world where the nfixed "basement" to the operand stack can't be known before
> code gen.

Okay, I'm just following your lead here verbatim.  Frankly, I'm not particularly worried that the previous changes will actually break all that many sites; we haven't yet had a real-world bug report that could be tracked to this change.


> >+    jsval*
> >+    stackBase() {
> 
> C++ good style wants the type+declarator-mode and the declarator name and
> params and opening brace on the same line. Short methods can be one-liners:
> 
>     jsval* stackBase() { return slots + script->nfixed; }

I prefer this style better anyway, but I'm not sure you're right to say this is good (better) C++ style.  Mozilla (no paradigm to be sure, but still) doesn't use this style, and I don't remember seeing it in the occasional style guide I've read.


> But wait, you're breaking LiveConnect. See #ifdef __cpluscplus in context
> below.

I added a hackaround ifdef; I shall not crucify SpiderMonkey on the cross of LiveConnect.


> This is why we use static helpers, not methods, like this one:

This is part of it, but frankly, I think it's because we've inculcated more C standard practices than C++ standard practices, even if the latter are better.


> Which suffices, so don't add stackBase (for now; were you to add it we'd want
> to lose StackBase).

Would have, didn't want to make too many unrelated changes in one patch -- perhaps that was a little too much to do in this bug.

Comment 16

[Jeff Walden [:Waldo]]

Attachment: hg qref

Comment 17

[Jeff Walden [:Waldo]]

Attachment: Post-upvar2

Comment 18

[Brendan Eich [:brendan]]

Comment on attachment 371306 [details] [diff] [review]
Post-upvar2

>From: Jeff Walden <[e-mail]>
>
>Bug 473096 - js1_5/Regress/regress-366601.js - Internal Error: script too large.  NOT REVIEWED YET
>
>diff --git a/js/src/jsemit.cpp b/js/src/jsemit.cpp
>--- a/js/src/jsemit.cpp
>+++ b/js/src/jsemit.cpp
>@@ -6265,32 +6265,43 @@ js_EmitTree(JSContext *cx, JSCodeGenerat
>       case TOK_ARRAYCOMP:
> #endif
>         /*
>-         * Emit code for [a, b, c] of the form:
>+         * Emit code for [a, b, c] of roughly (save for not invoking setters)

The parenthetical is ambiguous: does it mean "roughly, but invoking setters is not shown"? Obviously (to you and me) not, but it could be read that way. How about "Emit code for [a, b, c] that is equivalent except that setters are never called to the following:"

>+         * the form:
>          *
>          *   t = new Array; t[0] = a; t[1] = b; t[2] = c; t;

Or perhaps it's time to lose the misleading desugaring and use meta-notation.

>          *
>          * but use a stack slot for t and avoid dup'ing and popping it using
>          * the JSOP_NEWINIT and JSOP_INITELEM bytecodes.
>          *
>-         * If no sharp variable is defined and the initialiser is not for an
>-         * array comprehension, use JSOP_NEWARRAY.
>+         * If no sharp variable is defined, the initializer is not for an array
>+         * comprehension, the initializer is not overlarge, and the initializer
>+         * is not in global code (whose stack growth cannot be precisely modeled
>+         * due to the need to reserve space for global variables and regular
>+         * expressions), use JSOP_NEWARRAY to minimize opcodes and to create the
>+         * array using a fast, all-at-once process rather than a slow,
>+         * element-by-element process.

Uber-nit: you can wrap after - in house style.

>+        op = (JS_LIKELY(pn->pn_count < JS_BIT(16)) &&
>+              ((cg->flags & TCF_IN_FUNCTION) ||
>+               (cg->compiler->callerFrame && cg->compiler->callerFrame->fun)))

Now that I think about it and read the full condition, isn't this wrong? When eval calls JSCompiler::compileScript, the latter is going to compile a top-level statement at a time, which means the emitter cannot finalize fp->slots indexes -- that finalization is done with a separate fixup pass. Therefore the only condition to test here for "compiling whole ASTs" is (cg->flags & TCF_IN_FUNCTION).

>+
>+        off = js_EmitN(cx, cg, JSOP_NEWARRAY, 2);
>         if (off < 0)
>             return JS_FALSE;
>         pc = CG_CODE(cg, off);
>-        SET_UINT24(pc, atomIndex);
>+        JS_ASSERT(atomIndex < JS_BIT(16));
>+        SET_UINT16(pc, atomIndex);
>         UpdateDepth(cx, cg, off);

Don't use js_EmitN, ..., UpdateDepth for JOF_UINT16, do use EMIT_UINT16_IMM_OP. Should be able to compute the stack budget change from inside the js_Emit3 call when it calls UpdateDepth, eh?

>+#ifdef __cplusplus /* Aargh, LiveConnect, bug 442399. */
>+    void assertStackDepth(uintN depth) {

The method and param names are misleading -- how about assertValidStackIndex(uintN index)? "depth" is used for total budget, and for starting index of a block's slots, but this method is more general.

>+        extern jsval * StackBase(JSStackFrame *fp);

No space after * (in the prevailing style in this file).

>+        JS_ASSERT(regs->sp >= StackBase(this) &&
>+                  depth <= JSUword(regs->sp - StackBase(this)));

Don't use JSUWord. Old school: jsuword; new school: uintptr_t but that may not work in this header while it is over-included outside js/src.

Also, tests like this should use number line order and be split into two assertions if the conditions are separate and have a dependency:

          JS_ASSERT(0 <= regs->sp - StackBase(this));
          JS_ASSERT(depth <= uintptr_t(regs->sp - StackBase(this)));

The regs->sp - StackBase(this) condition could be manually CSE'd into a ptrdiff_t (old school) or intptr_t (new) local.

>@@ -7922,7 +7926,7 @@ TraceRecorder::record_JSOP_APPLY()
>     jsbytecode *pc = fp->regs->pc;
>     uintN argc = GET_ARGC(pc);
>     jsval* vp = fp->regs->sp - (argc + 2);
>-    JS_ASSERT(vp >= StackBase(fp));
>+    cx->fp->assertStackDepth(argc + 2);

One blank line before the method call, and shouldn't it be done before vp is computed? Not that that computation could do harm, but just on assert-earliest grounds.

>     jsuint length = 0;
>     JSObject* aobj = NULL;
>     LIns* aobj_ins = NULL;
>@@ -9915,7 +9919,8 @@ TraceRecorder::record_JSOP_NEWARRAY()
>     if (!getClassPrototype(JSProto_Array, proto_ins))
>         return false;
> 
>-    uint32 len = GET_UINT24(cx->fp->regs->pc);
>+    uint32 len = GET_UINT16(cx->fp->regs->pc);
>+    cx->fp->assertStackDepth(len);
>     LIns* args[] = { lir->insImm(len), proto_ins, cx_ins };

Ditto about blank line, maybe -- your call.

/be

Comment 19

[Jeff Walden [:Waldo]]

Attachment: Updated

(In reply to comment #18)
> The parenthetical is ambiguous: does it mean "roughly, but invoking setters
> is not shown"? Obviously (to you and me) not, but it could be read that way.
> How about "Emit code for [a, b, c] that is equivalent except that setters
> are never called to the following:"

"except that setters are never called to the following", ugh.


> >+         * the form:
> >          *
> >          *   t = new Array; t[0] = a; t[1] = b; t[2] = c; t;
> 
> Or perhaps it's time to lose the misleading desugaring and use
> meta-notation.

Maybe it makes sense to just give up on pretending that initializers are syntactic sugar, since they really do differ these days.  I rewrote the prose for [] and {} to be more precise about exactly what happens and why; how does the revised text strike you?


> Now that I think about it and read the full condition, isn't this wrong?
> When eval calls JSCompiler::compileScript, the latter is going to compile a
> top-level statement at a time, which means the emitter cannot finalize
> fp->slots indexes -- that finalization is done with a separate fixup pass.
> Therefore the only condition to test here for "compiling whole ASTs" is
> (cg->flags & TCF_IN_FUNCTION).

Yeah, I think you're right about this; I tried a few different eval uses and poked in the debugger and never saw the bit set in those cases, so I think this is right.


> Don't use js_EmitN, ..., UpdateDepth for JOF_UINT16, do use
> EMIT_UINT16_IMM_OP.  Should be able to compute the stack budget change from
> inside the js_Emit3 call when it calls UpdateDepth, eh?

True, wonder why this wasn't previously done here...


> >+#ifdef __cplusplus /* Aargh, LiveConnect, bug 442399. */
> >+    void assertStackDepth(uintN depth) {
> 
> The method and param names are misleading -- how about
> assertValidStackIndex(uintN index)? "depth" is used for total budget, and
> for starting index of a block's slots, but this method is more general.

Hmm.  "Index" is typically 0-based, tho, so this name's sort of off by one -- not to mention there aren't any indexes in sight.  I minimally prefer the previous name to what you've suggested (but still implemented yours in the patch), but given your comments it's still a little wrong.  How does assertStackHeight (or Size) strike you?


> new school: uintptr_t but that may not work in this header while it is
> over-included outside js/src.

I wasn't using this for the reason you note, but this is an internal or friend-at-best header -- use of the name doesn't cause any more damage than the typedef's presence in the header already causes -- changed to uintptr_t.


>           JS_ASSERT(0 <= regs->sp - StackBase(this));
>           JS_ASSERT(depth <= uintptr_t(regs->sp - StackBase(this)));
> 
> The regs->sp - StackBase(this) condition could be manually CSE'd into a
> ptrdiff_t (old school) or intptr_t (new) local.

I don't want to include any non-debug code, and sticking to JS_ASSERTs nicely avoids the need for #ifdef DEBUG / #endif; ordinarily I would have, tho.


> One blank line before the method call, and shouldn't it be done before vp is
> computed? Not that that computation could do harm, but just on assert-earliest
> grounds.

> Ditto about blank line, maybe -- your call.

Sensible.

Comment 20

[Brendan Eich [:brendan]]

Comment on attachment 371529 [details] [diff] [review]
Updated

Looks good. I'm wrong, you are right -- checkValidStackDepth(depth) is best since we are talking about a closed end range test [0, script->depth] worst case.

Looks like my comment 18 words cited here:

>+        op = (JS_LIKELY(pn->pn_count < JS_BIT(16)) &&
>+              ((cg->flags & TCF_IN_FUNCTION) ||
>+               (cg->compiler->callerFrame && cg->compiler->callerFrame->fun)))

Now that I think about it and read the full condition, isn't this wrong? When
eval calls JSCompiler::compileScript, the latter is going to compile a
top-level statement at a time, which means the emitter cannot finalize
fp->slots indexes -- that finalization is done with a separate fixup pass.
Therefore the only condition to test here for "compiling whole ASTs" is
(cg->flags & TCF_IN_FUNCTION).

were missed.

/be

Comment 21

[Jeff Walden [:Waldo]]

Attachment: Final patch

Hmm, I have no idea how the TCF_IN_FUNCTION change didn't make it into the patch, maybe it happened when I was changing the comments for the initializer bytecode.

Comment 22

[Brendan Eich [:brendan]]

Comment on attachment 372155 [details] [diff] [review]
Final patch

Nice!

/be

Comment 23

[Jeff Walden [:Waldo]]

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

Comment 24

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/b47a618c5949

Comment 25

[Mark Banner (:standard8)]

(In reply to comment #24)
> http://hg.mozilla.org/mozilla-central/rev/b47a618c5949

I think landing of this patch on mozilla-central has caused bustage on the WINNT 5.2 mozilla-central leak test build:

link -NOLOGO -DLL -OUT:js3250.dll -PDB:js3250.pdb -SUBSYSTEM:WINDOWS  jsapi.obj jsarena.obj jsarray.obj jsatom.obj jsbool.obj jscntxt.obj jsdate.obj jsdbgapi.obj jsdhash.obj jsdtoa.obj jsemit.obj jsexn.obj jsfun.obj jsgc.obj jshash.obj jsinterp.obj jsinvoke.obj jsiter.obj jslock.obj jslog2.obj jsmath.obj jsnum.obj jsobj.obj json.obj jsopcode.obj jsparse.obj jsprf.obj jsregexp.obj jsscan.obj jsscope.obj jsscript.obj jsstr.obj jsutil.obj jsxdrapi.obj jsxml.obj prmjtime.obj jstracer.obj Assembler.obj Fragmento.obj LIR.obj RegAlloc.obj avmplus.obj Nativei386.obj jsbuiltins.obj     -NXCOMPAT -SAFESEH -DYNAMICBASE  -DEBUG -DEBUGTYPE:CV        e:/builds/moz2_slave/mozilla-central-win32-debug/build/obj-firefox/dist/lib/nspr4.lib e:/builds/moz2_slave/mozilla-central-win32-debug/build/obj-firefox/dist/lib/plc4.lib e:/builds/moz2_slave/mozilla-central-win32-debug/build/obj-firefox/dist/lib/plds4.lib  kernel32.lib user32.lib gdi32.lib winmm.lib wsock32.lib advapi32.lib   
jsinterp.obj : error LNK2001: unresolved external symbol _StackBase
jstracer.obj : error LNK2001: unresolved external symbol _StackBase
js3250.dll : fatal error LNK1120: 1 unresolved externals
make[4]: Leaving directory `/e/builds/moz2_slave/mozilla-central-win32-debug/build/obj-firefox/js/src'

If not, it certainly looks like one of the TM patches that landed today.

Comment 26

[Jeff Walden [:Waldo]]

http://hg.mozilla.org/mozilla-central/rev/a94142e82a0d fixed the problem in m-c, and I figure it shouldn't be a problem to pick it up in TM when the next merge happens.

Comment 27

[Robert Sayre]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/4948d39190a9

Comment 28

[Bob Clary [:bc] (inactive)]

v 1.9.1, 1.9.2

Comment 29

[Bob Clary [:bc] (inactive)]

updated test js1_8_1/decompilation/regress-352026.js	
http://hg.mozilla.org/tracemonkey/rev/2cfdb980c79d

Comment 30

[Bob Clary [:bc] (inactive)]

/cvsroot/mozilla/js/tests/js1_8_1/decompilation/regress-352026.js,v  <--  regress-352026.js
new revision: 1.4; previous revision: 1.3