309894 - Avoid nested js_Interpret for XML filtering predicate expression

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P2)

Description

[Brendan Eich [:brendan]]

See bug 309852 comment 5.  This will mean a bytecode incompatibility, so the XDR
script version magic needs to change.  Adding js1.6 keyword so we can consider
whether that implies getting it fixed in the 1.8 branch.

/be

Comment 1

[Brendan Eich [:brendan]]

Proposed bytecode scheme:

x.(e) => x; PUSH; L1: FILTER<L2>; e; ENDFILTER<L1>; L2: POP

ENDFILTER could be GOTO, but then it would need a srcnote to help decompilation.
 With ENDFILTER, the decompiler can do nothing for FILTER, letting e stack on
top of x, then pop both and print x.(e) as it does today.

/be

Comment 2

[Brendan Eich [:brendan]]

See bug 350809.

/be

Comment 3

[Igor Bukanov]

I am not working on the bug right now.

Comment 4

[Igor Bukanov]

Attachment: v1

Here is a backup of something that compiles and even passes trivial tests.

Comment 5

[Igor Bukanov]

Asking for a blocking flag as fixing this bug simplifies fixing bug 416601.

Comment 6

[Igor Bukanov]

Attachment: v2

Here is a version that passes the test suite except with 2 extra failures caused by bug 416601. They are caused by the fact that yield from a filter now works and test cases that previously has thrown a known exception now runs until hitting the assert.

The main idea behind the patch is to add a new xml filter object to hold filter's state and to ensure that the state is properly traced/finalized. To reuse the code the patch uses js_EnterWith/js_LeaveWith so all the work to push/pop with objects is done in one place.

Comment 7

[Brendan Eich [:brendan]]

Comment on attachment 302739 [details] [diff] [review]
v2

>+MSG_DEF(JSMSG_NON_XML_FILTER,         217, 1, JSEXN_TYPEERR, "XML filter is applied to non-xml value {0}")

s/non-xml/non-XML/

>+    "destructuring body",    /* BODY */
[...]
>+JS_STATIC_ASSERT(JS_ARRAY_LENGTH(statementName) == STMT_LIMIT);

Oops -- thanks!

>+
> static const char *
> StatementName(JSCodeGenerator *cg)
> {
>     if (!cg->treeContext.topStmt)
>         return js_script_str;
>     return statementName[cg->treeContext.topStmt->type];
> }
> 
>@@ -5853,28 +5856,32 @@ js_EmitTree(JSContext *cx, JSCodeGenerat
>                     return JS_FALSE;
>             }
>             if (js_Emit1(cx, cg, JSOP_GROUP) < 0)
>                 return JS_FALSE;
>         }
>         break;
> 
> #if JS_HAS_XML_SUPPORT
>-      case TOK_FILTER:
>+      case TOK_FILTER: {
>+        ptrdiff_t afterStart;
>+

Just use top, no need for another block local here.

> #endif /* JS_OPSMETER */
> 
>+JSBool
>+js_EnterWith(JSContext *cx, jsval *vp, jsint stackOffset)

Nit: please put these js_*With common subroutines up above the JS_THREADED_INTERP and JS_OPSMETER stuff, just below the js_StrictlyEqual, js_InvokeConstructor, and InternNonIntElementId functions.

Nit: out params after inout after in, so vp last.

Nit: stackOffset is long-ish yet vague (offset often connotes "byte offset"). How about canonical name such as spindex or just index? Or to be longer and more precise, withIndex or even withObjIndex?

>+{
>+    JSObject *obj, *parent, *withobj;
>+    JSStackFrame *fp;
>+
>+    fp = cx->fp;

Nit: order fp declaration by first assignment, so before the object vars.

>+            sp[-1] = JSVAL_NULL;
>           END_CASE(JSOP_ENTERWITH)

Is the JSVAL_NULL stored once the scope chain has been set strictly necessary?

>           BEGIN_CASE(JSOP_LEAVEWITH)
>+            JS_ASSERT(sp[-1] == JSVAL_NULL);

It reduces uniqueness -- asserting there's a With object is stronger.

>+            --sp;

(Note for later nit: note predecrement.)

>@@ -6068,29 +6105,39 @@ interrupt:
>                 rval = JSVAL_TRUE;      /* always succeed */
>             }
> 
>             sp--;

Ugh, prefer predecrement (pre-existing code I know).

>           BEGIN_CASE(JSOP_ENDFILTER)
>+            SAVE_SP_AND_PC(fp);
>+            ok = js_StepXMLListFilter(cx, sp);
>+            if (!ok)
>+                goto out;
>+            sp--;

Nit: pre-decrement here.

>+            if (sp[0] != JSVAL_NULL) {

Nit: *sp here -- I could even stand *--sp in the if condition, but YMMV.

>       case JSXML_CLASS_PROCESSING_INSTRUCTION:
>         /* Step 7. */
>+
>         return MakeXMLPIString(cx, &sb, xml->name->localName, xml->xml_value);

Nit: don't add blank line here.

>+static void
>+xmlfilter_trace(JSTracer *trc, JSObject *obj)
> {
>+    JSXMLFilter *filter;
> 
>+    filter = (JSXMLFilter *) JS_GetPrivate(trc->context, obj);
>+    if (filter) {

Nit here and in xmlfilter_finalize: slight preference for if (!filter) return to cast out uncommon prototype case and unindent common case code.

>+JSClass js_XMLFilterClass = {
>+    "XmlFilter",

Nit: "XMLFilter" (it will be bound by name even though JSCLASS_IS_ANONYMOUS if the embedding neglects to set JSCLASS_GLOBAL_FLAGS).

>+    if (sp[-1] == JSVAL_HOLE) {
>+        /*
>+         * We have not done any iterations yet, initialize the filter based

Suggest "We haven't iterated yet, so initialize ...."

>+         * on the value stored in sp[-2].
>+         */
>+        if (JSVAL_IS_PRIMITIVE(sp[-2]))
>+            goto bad_filter_target;

Nit: "target" would seem to connote the result list, while source would be what is meant here.

>+            /*
>+             * Root just created listobj. sp[-2] can not be used yet for

s/just created/just-created/

s/can not/cannot/

>+        /* This also roots resobj. */
>+        filter->result = (JSXML *) JS_GetPrivate(cx, resobj);
>+

Nit: superfluous blank line per prevailing style.

>+    } else {
>+        /* We have iterated at least once. */
>+        JS_ASSERT(!JSVAL_IS_PRIMITIVE(sp[-2]));
>+        filter = (JSXMLFilter *) JS_GetPrivate(cx, JSVAL_TO_OBJECT(sp[-2]));

Assert expected class here before unsafe cast?

>+        /* Null at sp[-1] signals the filter termination. */

Nit: no need for "the" here.

>+        /*
>+         * Use sp[-1] as a temporary rooted location for js_EnterWith and
>+         * pass -2  as stack's offset to indicate that the with object is
>+         * associated with stack slot for filterobj.
>+         */
>+        sp[-1] = OBJECT_TO_JSVAL(kidobj);
>+        if (!js_EnterWith(cx, &sp[-1], -2))
>+            return JS_FALSE;
>+        sp[-1] = JSVAL_TRUE;

Is this JSVAL_TRUE strictly necessary? kidobj is non-null so all you need is a non-JSVAL_NULL value. Why not leave OBJECT_TO_JSVAL(kidobj) there?

Great to see this finally getting fixed.

/be

Comment 8

[Igor Bukanov]

(In reply to comment #7)
> >+JSBool
> >+js_EnterWith(JSContext *cx, jsval *vp, jsint stackOffset)

> Nit: stackOffset is long-ish yet vague (offset often connotes "byte offset").
> How about canonical name such as spindex or just index? Or to be longer and
> more precise, withIndex or even withObjIndex?

stackOffset means here an offset or shift from sp to get the depth to associate with withobj, it is not a depth of withobj itself. JSOP_FILTER does not even push one to the stack. With the patch it has to be -2, not -1 as with the ordinary js_EnterWith. Otherwise at the start of the filter expression the depth stored in fp->scopeChain would be equal to the depth of stack. 

If it would be possible to put inside filters a try/catch or iterator, then this would lead to a bug where the try handler would execute js_LeaveWith before transferring the control into the filter. 

> >+            sp[-1] = JSVAL_NULL;
> >           END_CASE(JSOP_ENTERWITH)
> 
> Is the JSVAL_NULL stored once the scope chain has been set strictly necessary?

Yes, it is a must to increase the stack here. Otherwise given

    with (a) {
        try {
            with(b) { throw 1; }
        } finally { ... }
    }         

the try handler would not be able to distinguish the with blocks during scope chain unwind. 

This null-pushing can be avoided if the try note together with the depth of the stack would also record the depth of the scope chain. It should simplify the error recovery for little bit of extra bookkeeping in the emitter, but this, I guess, for another bug.      


> >             sp--;
> 
> Ugh, prefer predecrement (pre-existing code I know). 

Almost all the existing code in the interpreter uses sp--. On the trunk:

~/m/ff/mozilla/js/src $ grep 'sp--' jsinterp.c | wc --lines
24
~/m/ff/mozilla/js/src $ grep '--sp' jsinterp.c | wc --lines
3

where one of --sp comes from the POP() macro. So for consistency I will use sp-- in an updated patch everywhere.

Comment 9

[Igor Bukanov]

Attachment: v3

The new version, besides addressing the nits, moves all enter/leave logic for with blocks back to the interpreter not to expose too much interpreter's internals to jsxml.c.

Comment 10

[Igor Bukanov]

Attachment: v3b

The new version fixes few comments in v3.

Comment 11

[Brendan Eich [:brendan]]

Comment on attachment 302790 [details] [diff] [review]
v3b

>+MSG_DEF(JSMSG_NON_XML_FILTER,         217, 1, JSEXN_TYPEERR, "XML filter is applied to non-xml value {0}")

s/non-xml/non-XML/ again

>+static JSBool
>+EnterWith(JSContext *cx, jsint depthShift)

"Shift" seems wrong -- it suggests a logarithm base 2 shift-amount. "depth" by itself suggests script->depth. How about index or stackIndex?

>+            /*
>+             * We must ensure that different with blocks have different stack

Quote "with" here to make it stand out as jargon, not a preposition.

>+             * depth associated with them. It allows the try handler search to

s/It allows/This allows/

>+             * properly recover the scope chain. Thus we must keep the stack
>+             * at least at the current level.
>+             *
>+             * We set s[-1] to the current with object to help asserting the

sp[-1] misspelled

"with" again

>+            sp--;

I'll save my --sp; fetish for anothe bug -- obviously I've changed my tastes over the years ;-).

>+                /* Exit the with block left from the previous iteration. */

"with" again

>     /*
>      * At this point we are inevitably leaving an interpreted function or a
>      * top-level script, and returning to one of:
>      * (a) an inline call in the same js_Interpret;
>      * (b) an "out of line" call made through js_Invoke;
>      * (c) a js_Execute activation;
>      * (d) a generator (SendToGenerator, jsiter.c);
>-     * (e) js_FilterXMLList.

Period needs to be at end of (d) now, not semicolon.

>+#define JSFRAME_ITERATOR      0x80 /* trying to get an iterator for for-in */
>+#define JSFRAME_POP_BLOCKS    0x100 /* scope chain contains blocks to pop */

Repeated nit from last review on comment alignment in JSFRAME_ITERATOR line.

r=me again -- with all nits picked.

/be

Comment 12

[Igor Bukanov]

(In reply to comment #11)
> (From update of attachment 302790 [details] [diff] [review])
> >+MSG_DEF(JSMSG_NON_XML_FILTER,         217, 1, JSEXN_TYPEERR, "XML filter is applied to non-xml value {0}")

Sorry I have missed it.

> Repeated nit from last review on comment alignment in JSFRAME_ITERATOR line.

I do not see this nit in comment 7.

Comment 13

[Igor Bukanov]

Attachment: v3c

The new version should address all the nits. Here is a delta:

--- /home/igor/m/ff/mozilla/quilt.p22577/js/src/js.msg	2008-02-13 15:21:34.000000000 +0100
+++ js/src/js.msg	2008-02-13 15:14:20.000000000 +0100
@@ -297,5 +297,5 @@ MSG_DEF(JSMSG_BAD_GENERATOR_YIELD,    21
 MSG_DEF(JSMSG_BAD_GENERATOR_SYNTAX,   215, 1, JSEXN_SYNTAXERR, "{0} expression must be parenthesized")
 MSG_DEF(JSMSG_ARRAY_COMP_LEFTSIDE,    216, 0, JSEXN_SYNTAXERR, "invalid array comprehension left-hand side")
-MSG_DEF(JSMSG_NON_XML_FILTER,         217, 1, JSEXN_TYPEERR, "XML filter is applied to non-xml value {0}")
+MSG_DEF(JSMSG_NON_XML_FILTER,         217, 1, JSEXN_TYPEERR, "XML filter is applied to non-XML value {0}")
 MSG_DEF(JSMSG_EMPTY_ARRAY_REDUCE,     218, 0, JSEXN_TYPEERR, "reduce of empty array with no initial value")
 MSG_DEF(JSMSG_NON_LIST_XML_METHOD,    219, 2, JSEXN_TYPEERR, "cannot call {0} method on an XML list with {1} elements")
--- /home/igor/m/ff/mozilla/quilt.p22577/js/src/jsinterp.c	2008-02-13 15:21:34.000000000 +0100
+++ js/src/jsinterp.c	2008-02-13 15:14:20.000000000 +0100
@@ -2031,8 +2031,8 @@ InternNonIntElementId(JSContext *cx, JSO
 /*
  * Enter the new with scope using an object at sp[-1] and associate the depth
- * of the with block with sp + depthShift.
+ * of the with block with sp + stackIndex.
  */
 static JSBool
-EnterWith(JSContext *cx, jsint depthShift)
+EnterWith(JSContext *cx, jsint stackIndex)
 {
     JSStackFrame *fp;
@@ -2042,6 +2042,6 @@ EnterWith(JSContext *cx, jsint depthShif
     fp = cx->fp;
     sp = fp->sp;
-    JS_ASSERT(depthShift < 0);
-    JS_ASSERT(fp->spbase <= sp + depthShift);
+    JS_ASSERT(stackIndex < 0);
+    JS_ASSERT(fp->spbase <= sp + stackIndex);
 
     if (!JSVAL_IS_PRIMITIVE(sp[-1])) {
@@ -2063,5 +2063,5 @@ EnterWith(JSContext *cx, jsint depthShif
 
     withobj = js_NewWithObject(cx, obj, parent,
-                               sp + depthShift - fp->spbase);
+                               sp + stackIndex - fp->spbase);
     if (!withobj)
         return JS_FALSE;
@@ -2655,11 +2655,11 @@ interrupt:
 
             /*
-             * We must ensure that different with blocks have different stack
-             * depth associated with them. It allows the try handler search to
-             * properly recover the scope chain. Thus we must keep the stack
-             * at least at the current level.
+             * We must ensure that different "with" blocks have different
+             * stack depth associated with them. This allows the try handler
+             * search to properly recover the scope chain. Thus we must keep
+             * the stack at least at the current level.
              *
-             * We set s[-1] to the current with object to help asserting the
-             * enter/leave balance in [leavewith].
+             * We set sp[-1] to the current "with" object to help asserting
+             * the enter/leave balance in [leavewith].
              */
             sp[-1] = OBJECT_TO_JSVAL(fp->scopeChain);
@@ -6147,5 +6147,5 @@ interrupt:
             cond = (sp[-1] != JSVAL_HOLE);
             if (cond) {
-                /* Exit the with block left from the previous iteration. */
+                /* Exit the "with" block left from the previous iteration. */
                 LeaveWith(cx);
             }
@@ -6777,5 +6777,5 @@ out:
      * (b) an "out of line" call made through js_Invoke;
      * (c) a js_Execute activation;
-     * (d) a generator (SendToGenerator, jsiter.c);
+     * (d) a generator (SendToGenerator, jsiter.c).
      */
     if (!ok) {
--- /home/igor/m/ff/mozilla/quilt.p22577/js/src/jsinterp.h	2008-02-13 15:21:34.000000000 +0100
+++ js/src/jsinterp.h	2008-02-13 15:14:20.000000000 +0100
@@ -107,8 +107,8 @@ typedef struct JSInlineFrame {
 #define JSFRAME_SCRIPT_OBJECT 0x20  /* compiling source for a Script object */
 #define JSFRAME_YIELDING      0x40  /* js_Interpret dispatched JSOP_YIELD */
-#define JSFRAME_ITERATOR      0x80 /* trying to get an iterator for for-in */
-#define JSFRAME_POP_BLOCKS    0x100 /* scope chain contains blocks to pop */
-#define JSFRAME_GENERATOR     0x200 /* frame belongs to generator-iterator */
-#define JSFRAME_ROOTED_ARGV   0x400 /* frame.argv is rooted by the caller */
+#define JSFRAME_ITERATOR      0x80  /* trying to get an iterator for for-in */
+#define JSFRAME_POP_BLOCKS   0x100  /* scope chain contains blocks to pop */
+#define JSFRAME_GENERATOR    0x200  /* frame belongs to generator-iterator */
+#define JSFRAME_ROOTED_ARGV  0x400  /* frame.argv is rooted by the caller */
 
 #define JSFRAME_OVERRIDE_SHIFT 24   /* override bit-set params; see jsfun.c */

Comment 14

[Igor Bukanov]

I checked in the patch from comment 13 to the trunk:

http://bonsai.mozilla.org/cvsquery.cgi?module=PhoenixTinderbox&branch=HEAD&cvsroot=%2Fcvsroot&date=explicit&mindate=1202912823&maxdate=1202913224&who=igor%25mir2.org

Comment 15

[Bob Clary [:bc] (inactive)]

I believe this regressed:

js1_7/geniter/regress-350809.js

Assertion failure: JS_PROPERTY_CACHE(cx).disabled == fp->pcDisabledSave, at jsinterp.c:6792 BUGNUMBER: 350809 STATUS: Do not assertion: if yield in xml filtering predicate

js1_7/geniter/regress-352605.js

Assertion failure: JS_PROPERTY_CACHE(cx).disabled == fp->pcDisabledSave, at jsinterp.c:6792 BUGNUMBER

js1_7/geniter/regress-352605.js

Expected value 'InternalError: yield not yet supported from filtering predicate', Actual value 'No Crash'

Comment 16

[Igor Bukanov]

(In reply to comment #15)
> I believe this regressed:
> 
> js1_7/geniter/regress-350809.js
> js1_7/geniter/regress-352605.js

With this bug fixed the 2 test cases exposed the problem reported in the bug  	416601. The last patch there fixes the problem.

> 
> Assertion failure: JS_PROPERTY_CACHE(cx).disabled == fp->pcDisabledSave, at
> jsinterp.c:6792 BUGNUMBER
> 
> js1_7/geniter/regress-352605.js
> 
> Expected value 'InternalError: yield not yet supported from filtering
> predicate', Actual value 'No Crash'

The test case should be updated: with this bug resolved "yield" from xml filter like in <><x/></>.((yield 1)) works when currently the test case expects that this is not supported feature.

Comment 17

[Bob Clary [:bc] (inactive)]

Checking in regress-352605.js;
/cvsroot/mozilla/js/tests/js1_7/geniter/regress-352605.js,v  <--  regress-352605.js
new revision: 1.4; previous revision: 1.3