441362 - LOCAL_ASSERT causes out: label to be bypassed

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[(dormant account)]

js/src/jsopcode.cpp:1624: error:Decompile: Control did not flow through
enterblock_out

cause is that LOCAL_ASSERT for some reason does a return statement.

Comment 1

[Brian Crowder]

I think we need to provide an alternate macro here for LOCAL_ASSERT_RV that jumps to a label.  We've used a similar pattern elsewhere.

Comment 2

[Robert Sayre]

Crowder points out this is debug-only and should never happen anyway. Might be nice to keep the analysis clean, though.

Comment 3

[Robert Sayre]

No, I'm wrong, it turns into a return; in a release build.

Comment 4

[Brian Crowder]

Yeah, sorry if I was unclear.  My point is that these should never happen in debug OR in release.  In debug, these are crashes.  In release, they -should- never happen.  They probably do, so we should probably recover correctly, but I don't think the error is a severe one here.

Comment 5

[(dormant account)]

Attachment: Another macro hack to avoid an invisible return

Comment 6

[Igor Bukanov]

Comment on attachment 334381 [details] [diff] [review]
Another macro hack to avoid an invisible return

>-#define LOCAL_ASSERT_RV(expr, rv)                                             \
>+#define LOCAL_ASSERT_CUSTOM(expr, BAD_EXIT)                                   \
>     JS_BEGIN_MACRO                                                            \
>         JS_ASSERT(expr);                                                      \
>-        if (!(expr)) return (rv);                                             \
>+        if (!(expr)) BAD_EXIT;                                                \
                   ^^^^^^

that should be 

    if (!(expr)) { BAD_EXIT; } 

so the following does what it intends:

    LOCAL_ASSERT_CUSTOM(expr, ok = JS_FALSE; goto enterblock_out)

IMO from the point of readability using just 

  LOCAL_ASSERT(condition, return JS_FALSE);
  LOCAL_ASSERT_CUSTOM(expr, ok = JS_FALSE; goto enterblock_out);
  etc.

without any LOCAL_ASSERT_RV etc. shortcuts would make the purpose of the macro very explicit and exposes for the reader the gotos/returns. But that can be done in another bug.

Comment 7

[(dormant account)]

Attachment: avoid invisible return

I agree that it would be helpful to have a policy against returns in macros that don't imply that in the name.

Comment 8

[Igor Bukanov]

Comment on attachment 336528 [details] [diff] [review]
avoid invisible return

>diff --git a/js/src/jsopcode.cpp b/js/src/jsopcode.cpp
>--- a/js/src/jsopcode.cpp
>+++ b/js/src/jsopcode.cpp
>@@ -1192,11 +1192,14 @@ DecompileSwitch(SprintStack *ss, TableEn
>     return JS_TRUE;
> }
> 
>-#define LOCAL_ASSERT_RV(expr, rv)                                             \
>+#define LOCAL_ASSERT_CUSTOM(expr, BAD_EXIT)                                   \
>     JS_BEGIN_MACRO                                                            \
>         JS_ASSERT(expr);                                                      \
>-        if (!(expr)) return (rv);                                             \
>+        if (!(expr)) { BAD_EXIT; }                                            \
>     JS_END_MACRO
>+
>+#define LOCAL_ASSERT_RV(expr, rv)                                             \
>+    LOCAL_ASSERT_CUSTOM(expr, return (rv))
> 
> static JSAtom *
> GetArgOrVarAtom(JSPrinter *jp, uintN slot)
>@@ -2544,11 +2547,13 @@ Decompile(SprintStack *ss, jsbytecode *p
> 
>                 /* From here on, control must flow through enterblock_out. */
>                 MUST_FLOW_THROUGH("enterblock_out");
>+#define LOCAL_ASSERT_OUT(expr) LOCAL_ASSERT_CUSTOM(expr, ok = JS_FALSE; \
>+                                                   goto enterblock_out)
>                 for (sprop = OBJ_SCOPE(obj)->lastProp; sprop;
>                      sprop = sprop->parent) {
>                     if (!(sprop->flags & SPROP_HAS_SHORTID))
>                         continue;
>-                    LOCAL_ASSERT(sprop->shortid < argc);
>+                    LOCAL_ASSERT_OUT(sprop->shortid < argc);
>                     atomv[sprop->shortid] = JSID_TO_ATOM(sprop->id);
>                 }
>                 ok = JS_TRUE;
>@@ -2584,7 +2589,7 @@ Decompile(SprintStack *ss, jsbytecode *p
> 
>                     pc2 = pc;
>                     pc += oplen;
>-                    LOCAL_ASSERT(*pc == JSOP_EXCEPTION);
>+                    LOCAL_ASSERT_OUT(*pc == JSOP_EXCEPTION);
>                     pc += JSOP_EXCEPTION_LENGTH;
>                     todo = Sprint(&ss->sprinter, exception_cookie);
>                     if (todo < 0 || !PushOff(ss, todo, JSOP_EXCEPTION)) {
>@@ -2600,7 +2605,7 @@ Decompile(SprintStack *ss, jsbytecode *p
>                              * It is emitted only when the catch head contains
>                              * an exception guard.
>                              */
>-                            LOCAL_ASSERT(js_GetSrcNoteOffset(sn, 0) != 0);
>+                            LOCAL_ASSERT_OUT(js_GetSrcNoteOffset(sn, 0) != 0);
>                             pc += JSOP_DUP_LENGTH;
>                             todo = Sprint(&ss->sprinter, exception_cookie);
>                             if (todo < 0 ||
>@@ -2618,13 +2623,13 @@ Decompile(SprintStack *ss, jsbytecode *p
>                             ok = JS_FALSE;
>                             goto enterblock_out;
>                         }
>-                        LOCAL_ASSERT(*pc == JSOP_POP);
>+                        LOCAL_ASSERT_OUT(*pc == JSOP_POP);
>                         pc += JSOP_POP_LENGTH;
>                         lval = PopStr(ss, JSOP_NOP);
>                         js_puts(jp, lval);
>                     } else {
> #endif
>-                        LOCAL_ASSERT(*pc == JSOP_SETLOCALPOP);
>+                        LOCAL_ASSERT_OUT(*pc == JSOP_SETLOCALPOP);
>                         i = GET_SLOTNO(pc) - jp->script->nfixed;
>                         pc += JSOP_SETLOCALPOP_LENGTH;
>                         atom = atomv[i - OBJ_BLOCK_DEPTH(cx, obj)];
>@@ -2642,19 +2647,19 @@ Decompile(SprintStack *ss, jsbytecode *p
>                      * guarded catch head) off the stack now.
>                      */
>                     rval = PopStr(ss, JSOP_NOP);
>-                    LOCAL_ASSERT(strcmp(rval, exception_cookie) == 0);
>+                    LOCAL_ASSERT_OUT(strcmp(rval, exception_cookie) == 0);
> 
>                     len = js_GetSrcNoteOffset(sn, 0);
>                     if (len) {
>                         len -= PTRDIFF(pc, pc2, jsbytecode);
>-                        LOCAL_ASSERT(len > 0);
>+                        LOCAL_ASSERT_OUT(len > 0);
>                         js_printf(jp, " if ");
>                         ok = Decompile(ss, pc, len, JSOP_NOP) != NULL;
>                         if (!ok)
>                             goto enterblock_out;
>                         js_printf(jp, "%s", POP_STR());
>                         pc += len;
>-                        LOCAL_ASSERT(*pc == JSOP_IFEQ || *pc == JSOP_IFEQX);
>+                        LOCAL_ASSERT_OUT(*pc == JSOP_IFEQ || *pc == JSOP_IFEQX);
>                         pc += js_CodeSpec[*pc].length;
>                     }
> 
>@@ -2668,6 +2673,7 @@ Decompile(SprintStack *ss, jsbytecode *p
> 
>                 todo = -2;
> 
>+#undef LOCAL_ASSERT_OUT
>               enterblock_out:
>                 if (atomv != smallv)
>                     JS_free(cx, atomv);

Comment 9

[(dormant account)]

pushed rev c0f37eff7ea6