352092 - "Assertion failure: !let || (tc->topScopeStmt && (tc->topScopeStmt->flags & SIF_SCOPE))" involving "with" and "let"

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P2)

Description

[Jesse Ruderman]

js> version(170)
0
js> let(z) { with({}) let y = 3; }
Assertion failure: !let || (tc->topScopeStmt && (tc->topScopeStmt->flags & SIF_SCOPE)), at jsparse.c:3553

#0  0x00009558 in JS_Assert
#1  0x000dc144 in Variables (cx=0x500180, ts=0x1814c10, tc=0xbfffe6dc) at jsparse.c:3552
#2  0x000db900 in Statement (cx=0x500180, ts=0x1814c10, tc=0xbfffe6dc) at jsparse.c:3370
#3  0x000db484 in Statement (cx=0x500180, ts=0x1814c10, tc=0xbfffe6dc) at jsparse.c:3261
#4  0x000d5bf8 in Statements (cx=0x500180, ts=0x1814c10, tc=0xbfffe6dc) at jsparse.c:1440
#5  0x000d8cc0 in LetBlock (cx=0x500180, ts=0x1814c10, tc=0xbfffe6dc, statement=1) at jsparse.c:2434
#6  0x000db564 in Statement (cx=0x500180, ts=0x1814c10, tc=0xbfffe6dc) at jsparse.c:3288
#7  0x000d5bf8 in Statements (cx=0x500180, ts=0x1814c10, tc=0xbfffe6dc) at jsparse.c:1440
#8  0x000d3120 in js_ParseTokenStream (cx=0x500180, chain=0x1804ec0, ts=0x1814c10) at jsparse.c:451
#9  0x0001fb54 in JS_BufferIsCompilableUnit (cx=0x500180, obj=0x1804ec0, bytes=0xbfffe804 "let(z) { with({}) let y = 3; }\n", length=31) at jsapi.c:3968
#10 0x000024dc in Process (cx=0x500180, obj=0x1804ec0, filename=0x0, forceTTY=0) at js.c:257
#11 0x00003134 in ProcessArgs (cx=0x500180, obj=0x1804ec0, argv=0xbffff9e0, argc=0) at js.c:486
#12 0x000094c4 in main (argc=0, argv=0xbffff9e0, envp=0xbffff9e4) at js.c:3086

Comment 1

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Skip the with

I ran this through GCC optimized and it didn't bogo-warn me about my use of scopeStmt. Basically, we need to skip scope statements that aren't MAYBE_SCOPE that have been pushed onto tc->topScopeStmt.

Comment 2

[Brendan Eich [:brendan]]

Comment on attachment 237699 [details] [diff] [review]
Skip the with

Would gcc prefer it if you set scopeStmt unconditionally, just before the first let test?  r=me with that.

/be

Comment 3

[Brendan Eich [:brendan]]

Sorry, read the patch but not comment 1.  So no need to unconditionally set scopeStmt.  We should get this into 1.8 branch pronto.

/be

Comment 4

[Blake Kaplan (:mrbkap) (inactive)]

Fix checked into trunk.

Comment 5

[Brendan Eich [:brendan]]

My Macbook's gcc wants you to apply this patch after your patch:

Index: jsparse.c
===================================================================
RCS file: /cvsroot/mozilla/js/src/jsparse.c,v
retrieving revision 3.241
diff -p -u -8 -r3.241 jsparse.c
--- jsparse.c   12 Sep 2006 04:00:31 -0000      3.241
+++ jsparse.c   12 Sep 2006 06:07:06 -0000
@@ -3547,18 +3547,18 @@ Variables(JSContext *cx, JSTokenStream *
      * - TOK_LP: We are parsing the head of a let block.
      * - Otherwise, we're parsing var declarations.
      */
     tt = CURRENT_TOKEN(ts).type;
     let = (tt == TOK_LET || tt == TOK_LP);
     JS_ASSERT(let || tt == TOK_VAR);
 
     /* Make sure that Statement set the tree context up correctly. */
+    scopeStmt = tc->topScopeStmt;
     if (let) {
-        scopeStmt = tc->topScopeStmt;
         while (scopeStmt && !(scopeStmt->flags & SIF_SCOPE)) {
             JS_ASSERT(!STMT_MAYBE_SCOPE(scopeStmt));
             scopeStmt = scopeStmt->downScope;
         }
         JS_ASSERT(scopeStmt);
     }
 
     data.pn = NULL;

to avoid a warning.

/be

Comment 6

[Brendan Eich [:brendan]]

Comment on attachment 237699 [details] [diff] [review]
Skip the with

Please nominate patches like this -- we need 'em now or never.  Any more hiding in your resolved bug list? :-P

/be

Comment 7

[Mike Schroepfer]

Comment on attachment 237699 [details] [diff] [review]
Skip the with

a=schrep for 181drivers for JS crash.

Comment 8

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Although I wonder if compilers will actually optimize all that away (or if they even can)?

Comment 9

[Brendan Eich [:brendan]]

Sure, with strict aliasing assumed (different type pointers cannot alias) and any modern SSA-based back end, the load can be moved into the consequent.  But should it have to be?  And does GCC do this optimization?

/be

Comment 10

[Brendan Eich [:brendan]]

Fixed on the 1.8 branch.

/be

Comment 11

[Bob Clary [:bc] (inactive)]

Checking in regress-352092.js;
/cvsroot/mozilla/js/tests/js1_7/block/regress-352092.js,v  <--  regress-352092.js
initial revision: 1.1
done

Comment 12

[Bob Clary [:bc] (inactive)]

verified fixed 1.8 20060914 windows/linux 1.9 20060914 windows/mac*/linux