From: Leopold Toetsch <[EMAIL PROTECTED]>
Date: Sun, 24 Sep 2006 13:16:08 +0200
Am Sonntag, 24. September 2006 03:43 schrieb Bob Rogers:
> The attached patch creates a C<return_stack> in C<Parrot_Context> for
> the exclusive use of the C<bsr/jsr/ret> ops. herwise. Is that right?
Separating stacks is a good thing, as code paths are simplified.
Q: do we really want or need a return stack per context?
The more I think about it, the more it makes sense [but see my change of
heart below]. The return stack needs to be scoped to the context
anyway, so that the code doesn't accidentally C<ret> into the caller's
sub. Putting the stack in the context means that stack underflow is
detected naturally.
Also, it currently works to C<.return> from a C<bsr> subroutine, and
the return addresses are automatically popped. Whether or not any code
depends on that, and whether or not it should be spec'ed to do that,
we'd still have to add extra code to implement it either way.
bsr/ret ought to be as slim as possible and it'll not mix with CPS
anyway.
Absolutely. [Except that, as shown below, it does mix, being part of
the dynamic environment.]
> However, since C<return_stack> entries always contain a
> STACK_ENTRY_DESTINATION with STACK_CLEANUP_NULL, and are only ever
> referenced from their owning context, using a C<Stack_Chunk> to store
> them is way overkill. Would it be better to design something more
> specific to return addresses so that it can be lighter in weight?
Indeed. One ResizableIntegerArray in interpreter would do it.
leo
Heck, a dynamically-resized "void **" array and an index would do it.
Then it wouldn't require memory allocation at each call. And that
applies regardless of where we decide to put it.
Oops; stop the presses; I just thought of a problem. Having a
separate stack for return addresses means that the return stack state is
not captured by continuations. Neither is the control stack at present,
but this patch just splits that one bug into two.
Please bear with me here:
For a long time now, continuations have been imprecise about
restoring the dynamic environment (as currently implemented via the
control_stack slot). This is because the continuation points to the
context which in turn points to the dynamic environment, so when you
invoke a continuation, you return to the right context but with whatever
dynamic environment the context was using when it was last exited. This
is clearly a bug. It's not very noticeable when the only things on the
dynamic environment are error handlers (which have a different mechanism
and therefore work correctly anyway), actions, and C<bsr> return
addresses, but the consequences will get more serious when we add
dynamic bindings to the mix.
The attached patch adds three Continuation test cases, the first of
which illustrates this bug using C<bsr>. (I should probably also add
cases that use C<pushaction> and C<push_eh>, for completeness.) This is
an updated version from the patch I posted on 26-July and subsequently
withdrew [1]. As I mentioned then, the right thing to do to fix all of
this is to (a) move the dynamic environment binding slot into the
interpreter, and (b) capture it along with the current context when
taking a continuation. This is the next step on my "hit list" of
dynamic environment jobs (it's been there for nearly a year now), and if
C<Stack_Chunk> objects are indeed garbage-collected, it will be a lot
easier than I had feared.
But when that happens, if return addresses are sitting in a separate
stack (whether in the context or the interpreter), they will still be
subject to the same bug. Of course, the C<return_stack> state could
also be captured in each continuation, but that seems like overkill.
Unless you have strong opinions, I think I would prefer to keep
return addresses on the control stack. I'll also assume I should commit
the new continuation.t cases.
-- Bob
[1] See "Partial fix to make closures invoke actions". Last Tuesday's
commit to consolidate stack unwinding had a similar effect; I hope
some day to extend it to support dynamic binding and PDD23 error
handling without any continuation barriers.
Index: t/pmc/continuation.t
===================================================================
--- t/pmc/continuation.t (revision 14696)
+++ t/pmc/continuation.t (working copy)
@@ -6,7 +6,7 @@
use warnings;
use lib qw( . lib ../lib ../../lib );
use Test::More;
-use Parrot::Test tests => 1;
+use Parrot::Test tests => 4;
=head1 NAME
@@ -33,4 +33,165 @@
ok 1
OUT
+pir_output_is(<<'CODE', <<'OUT', 'continuations preserve bsr/ret state.', todo
=> "BUG: continuations don't preserve the control_stack.");
+## Here is a trace of execution, keyed by labels.
+## L1: bsr to rtn1
+## rtn1: create a continuation that directs us to L6, and (we expect) captures
+## captures the whole dynamic state, including the return address to L3.
+## L3: return back to main
+## L4: if we're here the first time, call rtn2
+## rtn2: call the continuation from that routine.
+## L6: print "Continuation called." and return, which should take us . . .
+## L3: here the second time, where we print "done." and exit.
+.sub test_control_cont :main
+L1:
+ .local int return_count
+ .local pmc cont
+ return_count = 0
+ bsr rtn1
+L3:
+ unless return_count goto L4
+ print "done.\n"
+ end
+L4:
+ inc return_count
+ bsr rtn2
+ print "Oops; shouldn't have returned from rtn2.\n"
+ end
+L6:
+ print "Continuation called.\n"
+ ret
+rtn1:
+ print "Taking continuation.\n"
+ cont = new .Continuation
+ set_addr cont, L6
+ ret
+rtn2:
+ print "Calling continuation.\n"
+ cont()
+ ret
+.end
+CODE
+Taking continuation.
+Calling continuation.
+Continuation called.
+done.
+OUT
+pir_output_is(<<'CODE', <<'OUT', 'continuations call actions.');
+## the test_cont_action sub creates a continuation and passes it to _test_1
+## twice: the first time returns normally, the second time returns via the
+## continuation.
+.sub test_cont_action :main
+ ## debug 0x80
+ .local pmc cont
+ cont = new .Continuation
+ set_addr cont, continued
+ _test_1(4, cont)
+ _test_1("bar", cont)
+ print "oops; no "
+continued:
+ print "continuation called.\n"
+.end
+
+## set up C<pushaction> cleanup, and pass our arguments to _test_2.
+.sub _test_1
+ .param pmc arg1
+ .param pmc cont
+ print "_test_1\n"
+ .const .Sub $P43 = "___internal_test_1_0_"
+ pushaction $P43
+ $P50 = _test_2(arg1, cont)
+ print "got "
+ print $P50
+ print "\n"
+ .return ($P50)
+.end
+
+## cleanup sub used by _test_1, which just shows whether or not the action was
+## called at the right time.
+.sub ___internal_test_1_0_
+ .local pmc arg1
+ print "unwinding\n"
+ .return ()
+.end
+
+## return 3*n if n is an integer, else invoke the continuation.
+.sub _test_2
+ .param pmc n
+ .param pmc cont
+ typeof $I40, n
+ if $I40 != .Integer goto L3
+ $P44 = n_mul n, 3
+ .return ($P44)
+L3:
+ cont()
+.end
+CODE
+_test_1
+got 12
+unwinding
+_test_1
+unwinding
+continuation called.
+OUT
+
+pir_output_like(<<'CODE', <<'OUT', 'continuation action context');
+## this makes sure that returning via the continuation causes the action to be
+## invoked in the right dynamic context (i.e. without the error handler).
+.sub test_cont_action :main
+ .local pmc cont
+ cont = new .Continuation
+ set_addr cont, continued
+ _test_1("bar", cont)
+ print "oops; no "
+continued:
+ print "continuation called.\n"
+.end
+
+## set up C<pushaction> cleanup, and pass our arguments to _test_2.
+.sub _test_1
+ .param pmc arg1
+ .param pmc cont
+ print "_test_1\n"
+ .const .Sub $P43 = "___internal_test_1_0_"
+ pushaction $P43
+ $P50 = _test_2(arg1, cont)
+ print "got "
+ print $P50
+ print "\n"
+ .return ($P50)
+.end
+
+## cleanup sub used by _test_1, which just shows whether or not the action was
+## called at the right time.
+.sub ___internal_test_1_0_
+ .local pmc arg1
+ print "unwinding\n"
+ $P0 = new .Exception
+ $P0["_message"] = "something happened"
+ throw $P0
+.end
+
+## invoke the continuation within an error handler.
+.sub _test_2
+ .param pmc n
+ .param pmc cont
+ push_eh L3
+ cont()
+ print "oops"
+L3:
+ .local pmc exception
+ .get_results (exception, $S0)
+ print "Error: "
+ print exception
+ print "\n"
+.end
+CODE
+/\A_test_1
+unwinding
+something happened
+current instr/
+OUT
+
+# end of tests.
