From 12f3d51db5545cd019460e0aa74d1d6b01ad6857 Mon Sep 17 00:00:00 2001 From: Sami Vaarala Date: Mon, 30 Jan 2017 23:52:59 +0200 Subject: [PATCH] Minor cleanups for explicit scope objects * Rework 'with' statement object environment setup. * Checked allocation. --- src-input/duk_hthread_builtins.c | 8 +- src-input/duk_hthread_stacks.c | 58 +++------- src-input/duk_js_executor.c | 177 +++++++++++++++---------------- 3 files changed, 97 insertions(+), 146 deletions(-) diff --git a/src-input/duk_hthread_builtins.c b/src-input/duk_hthread_builtins.c index 4d8e5426..4d86ddcc 100644 --- a/src-input/duk_hthread_builtins.c +++ b/src-input/duk_hthread_builtins.c @@ -73,12 +73,8 @@ DUK_LOCAL void duk__duplicate_ram_global_object(duk_hthread *thr) { */ alloc_size = DUK_HOBJECT_P_ALLOC_SIZE(h_oldglobal); DUK_ASSERT(alloc_size > 0); - props = DUK_ALLOC(thr->heap, alloc_size); - if (DUK_UNLIKELY(props == NULL)) { - /* XXX: just use DUK_ALLOC_CHECKED()? */ - DUK_ERROR_ALLOC_FAILED(thr); - return; - } + props = DUK_ALLOC_CHECKED(thr, alloc_size); + DUK_ASSERT(props != NULL); DUK_ASSERT(DUK_HOBJECT_GET_PROPS(thr->heap, h_oldglobal) != NULL); DUK_MEMCPY((void *) props, (const void *) DUK_HOBJECT_GET_PROPS(thr->heap, h_oldglobal), alloc_size); diff --git a/src-input/duk_hthread_stacks.c b/src-input/duk_hthread_stacks.c index fa27925d..3887724a 100644 --- a/src-input/duk_hthread_stacks.c +++ b/src-input/duk_hthread_stacks.c @@ -145,9 +145,7 @@ DUK_INTERNAL void duk_hthread_callstack_unwind(duk_hthread *thr, duk_size_t new_ while (idx > new_top) { duk_activation *act; duk_hobject *func; -#if defined(DUK_USE_REFERENCE_COUNTING) duk_hobject *tmp; -#endif #if defined(DUK_USE_DEBUGGER_SUPPORT) duk_heap *heap; #endif @@ -236,7 +234,12 @@ DUK_INTERNAL void duk_hthread_callstack_unwind(duk_hthread *thr, duk_size_t new_ } /* func is NULL for lightfunc */ + /* Catch sites are required to clean up their environments + * in FINALLY part before propagating, so this should + * always hold here. + */ DUK_ASSERT(act->lex_env == act->var_env); + if (act->var_env != NULL) { DUK_DDD(DUK_DDDPRINT("closing var_env record %p -> %!O", (void *) act->var_env, (duk_heaphdr *) act->var_env)); @@ -244,22 +247,6 @@ DUK_INTERNAL void duk_hthread_callstack_unwind(duk_hthread *thr, duk_size_t new_ act = thr->callstack + idx; /* avoid side effect issues */ } -#if 0 - if (act->lex_env != NULL) { - if (act->lex_env == act->var_env) { - /* common case, already closed, so skip */ - DUK_DD(DUK_DDPRINT("lex_env and var_env are the same and lex_env " - "already closed -> skip closing lex_env")); - ; - } else { - DUK_DD(DUK_DDPRINT("closing lex_env record %p -> %!O", - (void *) act->lex_env, (duk_heaphdr *) act->lex_env)); - duk_js_close_environment_record(thr, act->lex_env); - act = thr->callstack + idx; /* avoid side effect issues */ - } - } -#endif - skip_env_close: /* @@ -272,44 +259,22 @@ DUK_INTERNAL void duk_hthread_callstack_unwind(duk_hthread *thr, duk_size_t new_ } /* - * Reference count updates - * - * Note: careful manipulation of refcounts. The top is - * not updated yet, so all the activations are reachable - * for mark-and-sweep (which may be triggered by decref). - * However, the pointers are NULL so this is not an issue. + * Reference count updates, using NORZ macros so we don't + * need to handle side effects. */ -#if defined(DUK_USE_REFERENCE_COUNTING) - tmp = act->var_env; -#endif + DUK_HOBJECT_DECREF_NORZ_ALLOWNULL(thr, act->var_env); act->var_env = NULL; -#if defined(DUK_USE_REFERENCE_COUNTING) - DUK_HOBJECT_DECREF_NORZ_ALLOWNULL(thr, tmp); - act = thr->callstack + idx; /* avoid side effect issues */ -#endif - -#if defined(DUK_USE_REFERENCE_COUNTING) - tmp = act->lex_env; -#endif + DUK_HOBJECT_DECREF_NORZ_ALLOWNULL(thr, act->lex_env); act->lex_env = NULL; -#if defined(DUK_USE_REFERENCE_COUNTING) - DUK_HOBJECT_DECREF_NORZ_ALLOWNULL(thr, tmp); - act = thr->callstack + idx; /* avoid side effect issues */ -#endif /* Note: this may cause a corner case situation where a finalizer * may see a currently reachable activation whose 'func' is NULL. */ -#if defined(DUK_USE_REFERENCE_COUNTING) tmp = DUK_ACT_GET_FUNC(act); -#endif - act->func = NULL; -#if defined(DUK_USE_REFERENCE_COUNTING) DUK_HOBJECT_DECREF_NORZ_ALLOWNULL(thr, tmp); - act = thr->callstack + idx; /* avoid side effect issues */ - DUK_UNREF(act); -#endif + DUK_UNREF(tmp); + act->func = NULL; } thr->callstack_top = new_top; @@ -486,6 +451,7 @@ DUK_INTERNAL void duk_hthread_catchstack_unwind(duk_hthread *thr, duk_size_t new env = act->lex_env; /* current lex_env of the activation (created for catcher) */ DUK_ASSERT(env != NULL); /* must be, since env was created when catcher was created */ act->lex_env = DUK_HOBJECT_GET_PROTOTYPE(thr->heap, env); /* prototype is lex_env before catcher created */ + DUK_HOBJECT_INCREF(thr, act->lex_env); DUK_HOBJECT_DECREF(thr, env); /* There is no need to decref anything else than 'env': if 'env' diff --git a/src-input/duk_js_executor.c b/src-input/duk_js_executor.c index 9cf98dd3..d6728f1d 100644 --- a/src-input/duk_js_executor.c +++ b/src-input/duk_js_executor.c @@ -908,7 +908,6 @@ DUK_LOCAL void duk__handle_catch(duk_hthread *thr, duk_size_t cat_idx, duk_tval if (DUK_CAT_HAS_CATCH_BINDING_ENABLED(&thr->catchstack[cat_idx])) { duk_hdecenv *new_env; - duk_hobject *act_lex_env; DUK_DDD(DUK_DDDPRINT("catcher has an automatic catch binding")); @@ -931,19 +930,12 @@ DUK_LOCAL void duk__handle_catch(duk_hthread *thr, duk_size_t cat_idx, duk_tval DUK_ASSERT(DUK_ACT_GET_FUNC(act) != NULL); DUK_UNREF(act); /* unreferenced without assertions */ - act = thr->callstack + thr->callstack_top - 1; - act_lex_env = act->lex_env; - act = NULL; /* invalidated */ - new_env = duk_hdecenv_alloc(thr, DUK_HOBJECT_FLAG_EXTENSIBLE | DUK_HOBJECT_CLASS_AS_FLAGS(DUK_HOBJECT_CLASS_DECENV)); DUK_ASSERT(new_env != NULL); duk_push_hobject(ctx, (duk_hobject *) new_env); DUK_ASSERT(DUK_HOBJECT_GET_PROTOTYPE(thr->heap, (duk_hobject *) new_env) == NULL); - DUK_HOBJECT_SET_PROTOTYPE(thr->heap, (duk_hobject *) new_env, act_lex_env); - DUK_HOBJECT_INCREF_ALLOWNULL(thr, act_lex_env); - DUK_DDD(DUK_DDDPRINT("new_env allocated: %!iO", (duk_heaphdr *) new_env)); /* Note: currently the catch binding is handled without a register @@ -958,8 +950,12 @@ DUK_LOCAL void duk__handle_catch(duk_hthread *thr, duk_size_t cat_idx, duk_tval duk_xdef_prop(ctx, -3, DUK_PROPDESC_FLAGS_W); /* writable, not configurable */ act = thr->callstack + thr->callstack_top - 1; + DUK_HOBJECT_SET_PROTOTYPE(thr->heap, (duk_hobject *) new_env, act->lex_env); act->lex_env = (duk_hobject *) new_env; DUK_HOBJECT_INCREF(thr, (duk_hobject *) new_env); /* reachable through activation */ + /* Net refcount change to act->lex_env is 0: incref for new_env's + * prototype, decref for act->lex_env overwrite. + */ DUK_CAT_SET_LEXENV_ACTIVE(&thr->catchstack[cat_idx]); @@ -3982,7 +3978,6 @@ DUK_LOCAL DUK_NOINLINE DUK_HOT void duk__js_execute_bytecode_inner(duk_hthread * duk_context *ctx = (duk_context *) thr; duk_activation *act; duk_catcher *cat; - duk_hobjenv *env; duk_tval *tv1; duk_small_uint_fast_t a; duk_small_uint_fast_t bc; @@ -4028,64 +4023,44 @@ DUK_LOCAL DUK_NOINLINE DUK_HOT void duk__js_execute_bytecode_inner(duk_hthread * a = DUK_DEC_A(ins); bc = DUK_DEC_BC(ins); - act = thr->callstack + thr->callstack_top - 1; - DUK_ASSERT(thr->callstack_top >= 1); - - /* 'with' target must be created first, in case we run out of memory */ - /* XXX: refactor out? */ - - if (a & DUK_BC_TRYCATCH_FLAG_WITH_BINDING) { - duk_hobject *target; - - DUK_DDD(DUK_DDDPRINT("need to initialize a with binding object")); - - if (act->lex_env == NULL) { - DUK_ASSERT(act->var_env == NULL); - DUK_DDD(DUK_DDDPRINT("delayed environment initialization")); - - /* must relookup act in case of side effects */ - duk_js_init_activation_environment_records_delayed(thr, act); - act = thr->callstack + thr->callstack_top - 1; - DUK_UNREF(act); /* 'act' is no longer accessed, scanbuild fix */ - } - DUK_ASSERT(act->lex_env != NULL); - DUK_ASSERT(act->var_env != NULL); - - env = duk_hobjenv_alloc(thr, - DUK_HOBJECT_FLAG_EXTENSIBLE | - DUK_HOBJECT_CLASS_AS_FLAGS(DUK_HOBJECT_CLASS_OBJENV)); - DUK_ASSERT(env != NULL); - DUK_ASSERT(DUK_HOBJECT_GET_PROTOTYPE(thr->heap, (duk_hobject *) env) == NULL); - duk_push_hobject(ctx, (duk_hobject *) env); /* Push to stabilize against side effects. */ - - duk_push_tval(ctx, DUK__REGP(bc)); - target = duk_to_hobject(ctx, -1); - DUK_ASSERT(target != NULL); - - /* always provideThis=true */ - env->target = target; - DUK_HOBJECT_INCREF(thr, target); - env->has_this = 1; - - DUK_ASSERT_HOBJENV_VALID(env); - - duk_pop(ctx); - - /* [ ... env ] */ + /* Registers 'bc' and 'bc + 1' are written in longjmp handling + * and if their previous values (which are temporaries) become + * unreachable -and- have a finalizer, there'll be a function + * call during error handling which is not supported now (GH-287). + * Ensure that both 'bc' and 'bc + 1' have primitive values to + * guarantee no finalizer calls in error handling. Scrubbing also + * ensures finalizers for the previous values run here rather than + * later. Error handling related values are also written to 'bc' + * and 'bc + 1' but those values never become unreachable during + * error handling, so there's no side effect problem even if the + * error value has a finalizer. + */ + duk_dup(ctx, bc); /* Stabilize value. */ + duk_to_undefined(ctx, bc); + duk_to_undefined(ctx, bc + 1); - DUK_DDD(DUK_DDDPRINT("environment for with binding: %!iT", - (duk_tval *) duk_get_tval(ctx, -1))); - } + /* Ensure a catchstack entry is available. One entry + * is guaranteed even if side effects cause function + * calls and the catchstack is shrunk because some + * spare room is left behind by a shrink operation. + */ + duk_hthread_catchstack_grow(thr); - /* allocate catcher and populate it (should be atomic) */ + /* Allocate catcher and populate it. Doesn't have to + * be fully atomic, but the catcher must be in a + * consistent state if side effects (such as finalizer + * calls) occur. + */ - duk_hthread_catchstack_grow(thr); - cat = thr->catchstack + thr->catchstack_top; DUK_ASSERT(thr->catchstack_top + 1 <= thr->catchstack_size); + cat = thr->catchstack + thr->catchstack_top; thr->catchstack_top++; cat->flags = DUK_CAT_TYPE_TCF; cat->h_varname = NULL; + cat->callstack_index = thr->callstack_top - 1; + cat->pc_base = (duk_instr_t *) curr_pc; /* pre-incremented, points to first jump slot */ + cat->idx_base = (duk_size_t) (thr->valstack_bottom - thr->valstack) + bc; if (a & DUK_BC_TRYCATCH_FLAG_HAVE_CATCH) { cat->flags |= DUK_CAT_FLAG_CATCH_ENABLED; @@ -4096,7 +4071,7 @@ DUK_LOCAL DUK_NOINLINE DUK_HOT void duk__js_execute_bytecode_inner(duk_hthread * if (a & DUK_BC_TRYCATCH_FLAG_CATCH_BINDING) { DUK_DDD(DUK_DDDPRINT("catch binding flag set to catcher")); cat->flags |= DUK_CAT_FLAG_CATCH_BINDING_ENABLED; - tv1 = DUK__REGP(bc); + tv1 = DUK_GET_TVAL_NEGIDX(thr, -1); DUK_ASSERT(DUK_TVAL_IS_STRING(tv1)); /* borrowed reference; although 'tv1' comes from a register, @@ -4105,55 +4080,68 @@ DUK_LOCAL DUK_NOINLINE DUK_HOT void duk__js_execute_bytecode_inner(duk_hthread * */ cat->h_varname = DUK_TVAL_GET_STRING(tv1); } else if (a & DUK_BC_TRYCATCH_FLAG_WITH_BINDING) { - /* env created above to stack top */ - duk_hobject *new_env; + duk_hobjenv *env; + duk_hobject *target; - DUK_DDD(DUK_DDDPRINT("lexenv active flag set to catcher")); - cat->flags |= DUK_CAT_FLAG_LEXENV_ACTIVE; + /* Delayed env initialization for activation (if needed). */ + act = thr->callstack + thr->callstack_top - 1; + DUK_ASSERT(thr->callstack_top >= 1); + if (act->lex_env == NULL) { + DUK_DDD(DUK_DDDPRINT("delayed environment initialization")); + DUK_ASSERT(act->var_env == NULL); - DUK_DDD(DUK_DDDPRINT("activating object env: %!iT", - (duk_tval *) duk_get_tval(ctx, -1))); + duk_js_init_activation_environment_records_delayed(thr, act); + act = thr->callstack + thr->callstack_top - 1; /* relookup, side effects */ + DUK_UNREF(act); /* 'act' is no longer accessed, scanbuild fix */ + } DUK_ASSERT(act->lex_env != NULL); - new_env = DUK_GET_HOBJECT_NEGIDX(ctx, -1); - DUK_ASSERT(new_env != NULL); + DUK_ASSERT(act->var_env != NULL); - act = thr->callstack + thr->callstack_top - 1; /* relookup (side effects) */ - DUK_ASSERT(DUK_HOBJECT_GET_PROTOTYPE(thr->heap, (duk_hobject *) new_env) == NULL); - DUK_HOBJECT_SET_PROTOTYPE_UPDREF(thr, new_env, act->lex_env); /* side effects */ + /* Coerce 'with' target. */ + target = duk_to_hobject(ctx, -1); + DUK_ASSERT(target != NULL); + + /* Create an object environment; it is not pushed + * so avoid side effects very carefully until it is + * referenced. + */ + env = duk_hobjenv_alloc(thr, + DUK_HOBJECT_FLAG_EXTENSIBLE | + DUK_HOBJECT_CLASS_AS_FLAGS(DUK_HOBJECT_CLASS_OBJENV)); + DUK_ASSERT(env != NULL); + DUK_ASSERT(DUK_HOBJECT_GET_PROTOTYPE(thr->heap, (duk_hobject *) env) == NULL); + env->target = target; /* always provideThis=true */ + DUK_HOBJECT_INCREF(thr, target); + env->has_this = 1; + DUK_ASSERT_HOBJENV_VALID(env); + DUK_DDD(DUK_DDDPRINT("environment for with binding: %!iO", env)); act = thr->callstack + thr->callstack_top - 1; /* relookup (side effects) */ - act->lex_env = new_env; - DUK_HOBJECT_INCREF(thr, new_env); - duk_pop(ctx); + DUK_ASSERT(DUK_HOBJECT_GET_PROTOTYPE(thr->heap, (duk_hobject *) env) == NULL); + DUK_ASSERT(act->lex_env != NULL); + DUK_HOBJECT_SET_PROTOTYPE(thr->heap, (duk_hobject *) env, act->lex_env); + act->lex_env = (duk_hobject *) env; /* Now reachable. */ + DUK_HOBJECT_INCREF(thr, (duk_hobject *) env); + /* Net refcount change to act->lex_env is 0: incref for env's + * prototype, decref for act->lex_env overwrite. + */ + + /* Set catcher lex_env active (affects unwind) + * only when the whole setup is complete. + */ + cat = thr->catchstack + thr->catchstack_top - 1; + cat->flags |= DUK_CAT_FLAG_LEXENV_ACTIVE; } else { ; } - /* Registers 'bc' and 'bc + 1' are written in longjmp handling - * and if their previous values (which are temporaries) become - * unreachable -and- have a finalizer, there'll be a function - * call during error handling which is not supported now (GH-287). - * Ensure that both 'bc' and 'bc + 1' have primitive values to - * guarantee no finalizer calls in error handling. Scrubbing also - * ensures finalizers for the previous values run here rather than - * later. Error handling related values are also written to 'bc' - * and 'bc + 1' but those values never become unreachable during - * error handling, so there's no side effect problem even if the - * error value has a finalizer. - */ - duk_to_undefined(ctx, bc); - duk_to_undefined(ctx, bc + 1); - - cat = thr->catchstack + thr->catchstack_top - 1; /* relookup (side effects) */ - cat->callstack_index = thr->callstack_top - 1; - cat->pc_base = (duk_instr_t *) curr_pc; /* pre-incremented, points to first jump slot */ - cat->idx_base = (duk_size_t) (thr->valstack_bottom - thr->valstack) + bc; - DUK_DDD(DUK_DDDPRINT("TRYCATCH catcher: flags=0x%08lx, callstack_index=%ld, pc_base=%ld, " "idx_base=%ld, h_varname=%!O", (unsigned long) cat->flags, (long) cat->callstack_index, (long) cat->pc_base, (long) cat->idx_base, (duk_heaphdr *) cat->h_varname)); + duk_pop(ctx); + curr_pc += 2; /* skip jump slots */ break; } @@ -4222,6 +4210,7 @@ DUK_LOCAL DUK_NOINLINE DUK_HOT void duk__js_execute_bytecode_inner(duk_hthread * DUK_ASSERT(prev_env != NULL); act->lex_env = DUK_HOBJECT_GET_PROTOTYPE(thr->heap, prev_env); DUK_CAT_CLEAR_LEXENV_ACTIVE(cat); + DUK_HOBJECT_INCREF(thr, act->lex_env); DUK_HOBJECT_DECREF(thr, prev_env); /* side effects */ }