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.

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.

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

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

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 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.

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

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);

pushed rev c0f37eff7ea6