A dedicated per-context stack for bsr/jsr/ret
Bob Rogers
the exclusive use of the C<bsr/jsr/ret> ops. There is a minor boost in
functionality (i.e. C<push_eh> and C<clear_eh> no longer have to nest
with respect to C<bsr> and C<ret> because the patch gives them different
stacks), but the real reason for wanting to do this is to get the return
addresses out of the C<control_stack>, so that it only has entries that
are dynamically scoped.
The C<return_stack> is not initialized until it is needed, which is
optimal for the majority of Parrotcode that doesn't use this feature,
but makes the implementation somewhat uglier (IMHO, anyway). I tried to
initialize C<return_stack> along with the other context stuff, but
couldn't get it to fly. Presumably this is because context
initialization happens before pool initialization, and pools are
required for C<Stack_Chunk> allocation. If that is correct, and
C<Stack_Chunk> objects are truly managed by GC (something that only
dawned on me just now), then my comments about C<stack_destroy> are off
the mark [1], but the patch is probably OK otherwise. Is that right?
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?
One final note: You will need to do "make clean" after applying this
patch, as otherwise changing include/parrot/interpreter.h does not force
anything in src/dynpmc/ or src/dynoplibs/ to be recompiled. It would be
great if somebody who understands the build system (i.e. better than I)
could investigate. Thanks,
-- Bob Rogers
http://rgrjr.dyndns.org/
[1] Probably off the sweep as well.
Leopold Toetsch
> The attached patch creates a C<return_stack> in C<Parrot_Context> for
Separating stacks is a good thing, as code paths are simplified.
Q: do we really want or need a return stack per context?
bsr/ret ought to be as slim as possible and it'll not mix with CPS anyway.
> 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
Bob Rogers
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.