The attached patch consolidates most of the existing stack-unwinding code into Continuation:invoke; previously, RetContinuation:invoke and find_exception_handler also did stack-unwinding, and none of the three did it quite the same way.
Here are the effects: 1. Improved code sharing, a prerequisite for improving semantics. 2. Actions are now triggered when a continuation is called, fixing two TODO cases (and also two others which were looking for the buggy behavior). Previously, this only worked for exception handling and RetContinuation. 3. Actions are no longer triggered as a side-effect of discovering that there are no active exception handlers, which is more in the spirit of PDD23. 4. Variable names are now more consistent between Continuation:invoke and RetContinuation:invoke (or what's left of it). Questions: 1. I notice that parameter passing is handled by Continuation:invoke but not by RetContinuation:invoke. Currently I have to use some cumbersome "Am I a RetContinuation?" logic in what is supposed to be the generic method in order to deal with this. It would be nicer if they both passed parameters in the same way. Is there some reason this can't be done? 2. The exception handling case is not as efficient as it could be (quadratic handler search followed by re-searching for the handler's stack entry). This would be helped by splitting the unwinding code into a separate routine, and calling it from each of the invoke methods, rather than trying to use inheritance. But I assume this will need to be rewritten anyway when PDD23 is fully implemented. With that in mind, is this OK for now? 3. Around src/exceptions.c:268 (unpatched) there is an assignment of e->entry_type to NO_STACK_ENTRY_TYPE, after a comment explaining why. The patch disables this assignment, with no ill effect. If the code is still needed for some reason, it will have to be moved to RetContinuation:invoke, but I don't have a test case to ensure that I've done this right. Should I just chuck it? Suggestions are cordially invited. If I hear no complaints by tomorrow night, I will assume all answers are "yes," and commit this (or something close to it). -- Bob Rogers http://rgrjr.dyndns.org/
Index: src/pmc/retcontinuation.pmc =================================================================== --- src/pmc/retcontinuation.pmc (revision 14640) +++ src/pmc/retcontinuation.pmc (working copy) @@ -76,55 +76,19 @@ =item C<void *invoke(void *next)> -Restores the "context" by calling the superclass's C<invoke()> and places -the frame pointer in the stack frame cache. +Transfers control to the calling context, and frees the current context. =cut */ void* invoke (void* next) { - Stack_Chunk_t *ctr_stack, *stack_now; struct Parrot_cont * cc = PMC_cont(SELF); - parrot_context_t *caller_ctx; - struct PackFile_ByteCode *seg; /* bytecode segment */ - /* - * unwind control stack - */ - stack_now = CONTEXT(INTERP->ctx)->control_stack; - caller_ctx = cc->to_ctx; - ctr_stack = caller_ctx->control_stack; - while (stack_now != ctr_stack) { - if (!stack_now || !ctr_stack) - internal_exception(1, "Control stack damaged"); - /* - * this automagically runs all pushed action - * handler during pop - see cleanup stuff - * in stack_pop - */ - (void)stack_pop(INTERP, &stack_now, - NULL, NO_STACK_ENTRY_TYPE); - } + parrot_context_t *from_ctx = cc->from_ctx; + struct PackFile_ByteCode * const seg = cc->seg; - /* debug print before context is switched */ - if (Interp_trace_TEST(INTERP, PARROT_TRACE_SUB_CALL_FLAG)) { - Interp *tracer; - - PMC *to_sub = caller_ctx->current_sub; - PMC *from_sub = cc->from_ctx->current_sub; - - tracer = interpreter->debugger ? - interpreter->debugger : interpreter; - PIO_eprintf(tracer, "# Back in sub '%Ss' from '%Ss'\n", - Parrot_full_sub_name(INTERP, to_sub), - Parrot_full_sub_name(INTERP, from_sub)); - } - CONTEXT(INTERP->ctx) = caller_ctx; - INTERP->ctx.bp = caller_ctx->bp; - INTERP->ctx.bp_ps = caller_ctx->bp_ps; - next = cc->address; - Parrot_free_context(INTERP, cc->from_ctx, 1); - seg = cc->seg; + next = SUPER(next); + Parrot_free_context(INTERP, from_ctx, 1); #ifdef NDEBUG /* the continuation is dead - delete and destroy it */ mem_sys_free(cc); @@ -147,7 +111,6 @@ */ cc->to_ctx = NULL; #endif - INTERP->current_args = NULL; if (INTERP->code != seg) { Parrot_switch_to_cs(INTERP, seg, 1); } Index: src/pmc/continuation.pmc =================================================================== --- src/pmc/continuation.pmc (revision 14640) +++ src/pmc/continuation.pmc (working copy) @@ -224,15 +224,25 @@ void* invoke (void* next) { struct Parrot_cont * cc = PMC_cont(SELF); - parrot_context_t *caller_ctx; - parrot_context_t *ctx; - opcode_t *pc; + Stack_Chunk_t *stack_target; + parrot_context_t *from_ctx = CONTEXT(INTERP->ctx); + parrot_context_t *to_ctx = cc->to_ctx; + opcode_t *pc = cc->address; + /* [bug: these should be 'isa' tests. -- rgr, 17-Sep-06.] */ + int exception_continuation_p + = SELF->vtable->base_type == enum_class_Exception_Handler; + int ret_continuation_p + = SELF->vtable->base_type == enum_class_RetContinuation; if (interpreter->current_runloop_id != cc->runloop_id /* it's ok if we are exiting to "runloop 0"; there is no such runloop, but the only continuation that thinks it came from runloop 0 is for the return from the initial sub call. */ - && cc->runloop_id != 0) { + && cc->runloop_id != 0 + /* since a RetContinuation [currently] only returns to the next + outer frame, exiting to the inner run loop does the right thing, + since it normally returns to the next outer runloop anyway. */ + && ! ret_continuation_p) { fprintf(stderr, "[oops; continuation %p of type %d " "is trying to jump from runloop %d to runloop %d]\n", SELF, (int) SELF->vtable->base_type, @@ -242,34 +252,89 @@ if (Interp_debug_TEST(interpreter, PARROT_CTX_DESTROY_DEBUG_FLAG)) { fprintf(stderr, "[invoke cont %p, to_ctx %p, from_ctx %p (refs %d)]\n", - SELF, cc->to_ctx, cc->from_ctx, - (int) cc->from_ctx->ref_count); + SELF, to_ctx, from_ctx, (int) from_ctx->ref_count); } #endif - if (! cc->to_ctx) { + if (! to_ctx) { real_exception(interpreter, NULL, INVALID_OPERATION, "Continuation invoked after deactivation."); } + + /* figure out how far to unwind. */ + if (exception_continuation_p) { + /* if we are an exception handler, then we must pop ourself and + everything above. */ + stack_target = from_ctx->control_stack; + while (stack_target + && UVal_pmc(((Stack_Entry_t *)STACK_DATAP(stack_target))->entry) != SELF) { + stack_target = stack_target->prev; + } + if (! stack_target) { + internal_exception(ERROR_STACK_EMPTY, + "Can't find handler on control stack!"); + } + /* move down one more to pop the EH itself. */ + stack_target = stack_target->prev; + } + else { + stack_target = to_ctx->control_stack; + } + + /* + * unwind control stack + */ + while (from_ctx->control_stack != stack_target) { + PMC *cleanup_sub = NULL; + Stack_Entry_t *e; + + if (! from_ctx->control_stack) + internal_exception(1, "Control stack damaged"); + e = stack_entry(interpreter, from_ctx->control_stack, 0); + if (e->entry_type == STACK_ENTRY_ACTION) { + /* + * Disable automatic cleanup routine execution in stack_pop so + * that we can run the action subroutine manually. This is + * because we have to run the sub AFTER it has been popped, lest + * a new error in the sub cause an infinite loop when invoking + * an error handler. + */ + cleanup_sub = UVal_pmc(e->entry); + e->cleanup = STACK_CLEANUP_NULL; + } + (void)stack_pop(INTERP, &from_ctx->control_stack, + NULL, NO_STACK_ENTRY_TYPE); + if (cleanup_sub) { + /* Now it's safe to run. */ + Parrot_runops_fromc_args(interpreter, cleanup_sub, + "vI", exception_continuation_p); + } + } + /* debug print before context is switched */ if (Interp_trace_TEST(INTERP, PARROT_TRACE_SUB_CALL_FLAG)) { - PMC *sub = cc->to_ctx->current_sub; + PMC *sub = to_ctx->current_sub; PIO_eprintf(INTERP, "# Back in sub '%Ss'\n", Parrot_full_sub_name(INTERP, sub)); } - caller_ctx = CONTEXT(INTERP->ctx); + /* * set context */ - CONTEXT(INTERP->ctx) = ctx = cc->to_ctx; - INTERP->ctx.bp = ctx->bp; - INTERP->ctx.bp_ps = ctx->bp_ps; + CONTEXT(INTERP->ctx) = to_ctx; + INTERP->ctx.bp = to_ctx->bp; + INTERP->ctx.bp_ps = to_ctx->bp_ps; + if (ret_continuation_p) { + /* RetContinuation arg passing is handled elsewhere. */ + return pc; + } + + /* pass args */ if (cc->current_results) { /* where caller wants result */ - ctx->current_results = cc->current_results; + to_ctx->current_results = cc->current_results; } - pc = cc->address; - if (ctx->current_results && INTERP->current_args) { + if (to_ctx->current_results && INTERP->current_args) { /* * the register pointer is already switched back * to the caller, therefore the registers of the @@ -279,11 +344,13 @@ */ Parrot_block_DOD(INTERP); parrot_pass_args(INTERP, - caller_ctx, - ctx, + from_ctx, + to_ctx, PARROT_OP_set_args_pc); Parrot_unblock_DOD(INTERP); } + + /* switch segment */ INTERP->current_args = NULL; if (INTERP->code != cc->seg) { Parrot_switch_to_cs(INTERP, cc->seg, 1); Index: src/exceptions.c =================================================================== --- src/exceptions.c (revision 14640) +++ src/exceptions.c (working copy) @@ -226,33 +226,18 @@ STRING *message; char *m; int exit_status, print_location; + int depth = 0; + Stack_Entry_t * const e; + /* for now, we don't check the exception class and we don't - * look for matching handlers + * look for matching handlers. [this is being redesigned anyway.] */ message = VTABLE_get_string_keyed_int(interpreter, exception, 0); - do { - PMC *cleanup_sub = NULL; - Stack_Entry_t * const e = stack_entry(interpreter, - CONTEXT(interpreter->ctx)->control_stack, 0); - - if (!e) - break; - if (e->entry_type == STACK_ENTRY_ACTION) { - /* - * Disable automatic cleanup routine execution in stack_pop so that - * we can run the action subroutine manually with an INTVAL argument - * of 1. Note that we have to run the sub AFTER it has been popped, - * lest a new error in the sub cause an infinite loop. - */ - cleanup_sub = UVal_pmc(e->entry); - e->cleanup = STACK_CLEANUP_NULL; - } - (void)stack_pop(interpreter, &CONTEXT(interpreter->ctx)->control_stack, - NULL, e->entry_type); - if (cleanup_sub) { - /* Now it's safe to run. */ - Parrot_runops_fromc_args(interpreter, cleanup_sub, "vI", 1); - } + /* [TODO: replace quadratic search with something linear, hopefully without + trashing abstraction layers. -- rgr, 17-Sep-06.] */ + while (e = stack_entry(interpreter, + CONTEXT(interpreter->ctx)->control_stack, + depth)) { if (e->entry_type == STACK_ENTRY_PMC) { /* * During interpreter creation there is an initial context @@ -265,14 +250,17 @@ * * So invalidate entry_type. */ - e->entry_type = NO_STACK_ENTRY_TYPE; + /* [i think this is probably broken. -- rgr, 17-Sep-06.] */ + /* e->entry_type = NO_STACK_ENTRY_TYPE; */ handler = UVal_pmc(e->entry); if (handler && handler->vtable->base_type == enum_class_Exception_Handler) { return handler; } } - } while (1); + depth++; + } + /* flush interpreter output to get things printed in order */ PIO_flush(interpreter, PIO_STDOUT(interpreter)); PIO_flush(interpreter, PIO_STDERR(interpreter)); Index: t/pmc/exception.t =================================================================== --- t/pmc/exception.t (revision 14640) +++ t/pmc/exception.t (working copy) @@ -433,11 +433,12 @@ .end .sub action - print "never\n" + print "unwind\n" .end CODE main foo +unwind back OUTPUT @@ -459,36 +460,17 @@ .end .sub action - print "never\n" + print "unwind\n" .end CODE main foo +unwind back OUTPUT -pir_output_is(<<'CODE', <<'OUTPUT', "register corruption - implicit P5"); +pir_output_is(<<'CODE', <<'OUTPUT', 'cleanup global: continuation'); .sub main :main - null P0 - null P1 - null P2 - null P3 - I0 = 1 - I1 = 2 - push_eh coke - $P0 = global 'no coke' -coke: - print_item I0 - print_item I1 - print_newline -.end -CODE -1 2 -OUTPUT - -## this is broken; continuation calling does not execute actions when unwinding. -pir_output_is(<<'CODE', <<'OUTPUT', 'cleanup global: continuation', todo => 'bug'); -.sub main :main .local pmc outer, cont outer = new String outer = "Outer value\n" @@ -735,13 +717,10 @@ .end CODE /^main -at_exit, flag = 1 No exception handler/ OUTPUT -# creating the Closure converts RetContinuation to Continuations -# which dont trigger acction handlers -pir_output_is(<<'CODE', <<'OUTPUT', "pushaction as closure", todo => 'Continuation'); +pir_output_is(<<'CODE', <<'OUTPUT', "pushaction as closure"); .sub main :main .local pmc a .lex 'a', a @@ -770,6 +749,38 @@ a = 42 OUTPUT +# exception handlers are still run in an inferior runloop, which messes up +# nonlocal exit from within handlers. +pir_output_like(<<'CODE', <<'OUTPUT', "pushaction: error while handling error", todo => 'runloop shenanigans'); +.sub main :main + push_eh h + print "main\n" + .const .Sub at_exit = "exit_handler" + pushaction at_exit + $P1 = new .Exception + throw $P1 + print "never 1\n" +h: + ## this is never actually reached, because exit_handler throws an unhandled + ## exception before the handler is entered. + print "in outer handler\n" +.end + +.sub exit_handler :outer(main) + .param int flag + print_item "at_exit, flag =" + print_item flag + print_newline + $P2 = new .Exception + throw $P2 + print "never 2\n" +.end +CODE +/^main +at_exit, flag = 1 +No exception handler/ +OUTPUT + pir_output_is(<<'CODE', <<'OUTPUT', "exit_handler via exit exception"); .sub main :main .local pmc a