Last Comment Bug 441362 - LOCAL_ASSERT causes out: label to be bypassed
: LOCAL_ASSERT causes out: label to be bypassed
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: x86 Linux
: -- normal (vote)
: ---
Assigned To: general
:
Mentors:
Depends on:
Blocks: 432917
  Show dependency treegraph
 
Reported: 2008-06-23 11:34 PDT by (dormant account)
Modified: 2008-10-12 18:25 PDT (History)
5 users (show)
bob: in‑testsuite-
bob: in‑litmus-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Another macro hack to avoid an invisible return (4.75 KB, patch)
2008-08-18 17:29 PDT, (dormant account)
no flags Details | Diff | Splinter Review
avoid invisible return (4.75 KB, patch)
2008-09-02 12:23 PDT, (dormant account)
igor: review+
Details | Diff | Splinter Review

Description (dormant account) 2008-06-23 11:34:56 PDT
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 2008-06-23 11:40:28 PDT
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 2008-07-02 15:43:04 PDT
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 2008-07-02 15:49:06 PDT
No, I'm wrong, it turns into a return; in a release build.
Comment 4 Brian Crowder 2008-07-02 16:10:39 PDT
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) 2008-08-18 17:29:34 PDT
Created attachment 334381 [details] [diff] [review]
Another macro hack to avoid an invisible return
Comment 6 Igor Bukanov 2008-08-19 02:51:01 PDT
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) 2008-09-02 12:23:18 PDT
Created attachment 336528 [details] [diff] [review]
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 2008-09-02 12:38:37 PDT
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) 2008-09-02 13:35:35 PDT
pushed rev c0f37eff7ea6

Note You need to log in before you can comment on or make changes to this bug.