The attached patch creates a C<return_stack> in C<Parrot_Context> for
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.
Diffs between last version checked in and current workfile(s):
Index: include/parrot/interpreter.h
===================================================================
--- include/parrot/interpreter.h (revision 14695)
+++ include/parrot/interpreter.h (working copy)
@@ -213,6 +213,7 @@
struct Stack_Chunk *user_stack; /* Base of the scratch stack */
struct Stack_Chunk *control_stack; /* Base of the flow control stack */
+ struct Stack_Chunk *return_stack; /* Base of the bsr/jsr/ret stack */
PMC *lex_pad; /* LexPad PMC */
struct Parrot_Context *outer_ctx; /* outer context, if a closure */
UINTVAL warns; /* Keeps track of what warnings
Index: src/register.c
===================================================================
--- src/register.c (revision 14695)
+++ src/register.c (working copy)
@@ -234,6 +234,9 @@
ctx->current_method = NULL; /* XXX who clears it? */
ctx->current_object = NULL; /* XXX who clears it? */
ctx->current_HLL = 0;
+ /* each context gets its own return stack, but we may not be able to
+ initialize it yet if we're still in create_initial_context. */
+ ctx->return_stack = NULL;
if (old) {
/* some items should better be COW copied */
ctx->constants = old->constants;
@@ -393,6 +396,12 @@
ptr = ctxp;
slot = ctxp->regs_mem_size >> 3;
+ if (ctxp->return_stack) {
+ /* [stack_destroy is currently a noop. -- rgr, 23-Sep-06.] */
+ stack_destroy(ctxp->return_stack);
+ ctxp->return_stack = NULL;
+ }
+
assert(slot < interpreter->ctx_mem.n_free_slots);
*(void **)ptr = interpreter->ctx_mem.free_list[slot];
interpreter->ctx_mem.free_list[slot] = ptr;
Index: src/inter_create.c
===================================================================
--- src/inter_create.c (revision 14695)
+++ src/inter_create.c (working copy)
@@ -416,8 +416,13 @@
/* deinit op_lib */
(void) PARROT_CORE_OPLIB_INIT(0);
+ /* [stack_destroy is currently a noop, otherwise these would probably fail,
+ as the entries are shared with other contexts. -- rgr, 23-Sep-06.]
+ */
stack_destroy(CONTEXT(interpreter->ctx)->user_stack);
stack_destroy(CONTEXT(interpreter->ctx)->control_stack);
+ if (CONTEXT(interpreter->ctx)->control_stack)
+ stack_destroy(CONTEXT(interpreter->ctx)->control_stack);
destroy_context(interpreter);
Index: src/ops/core.ops
===================================================================
--- src/ops/core.ops (revision 14695)
+++ src/ops/core.ops (working copy)
@@ -203,7 +203,13 @@
=cut
inline op bsr (label INT) :base_core,check_event {
- stack_push(interpreter, &CONTEXT(interpreter->ctx)->control_stack,
+ struct Parrot_Context *context = CONTEXT(interpreter->ctx);
+ if (! context->return_stack) {
+ /* this is necessary in order to work around strange context
+ initiation rites. -- rgr, 21-Sep-06. */
+ context->return_stack = new_stack(interpreter, "Return");
+ }
+ stack_push(interpreter, &context->return_stack,
expr NEXT(), STACK_ENTRY_DESTINATION, STACK_CLEANUP_NULL);
goto OFFSET($1);
}
@@ -230,7 +236,13 @@
inline op jsr(label INT) :base_core,check_event {
opcode_t * loc;
- stack_push(interpreter, &CONTEXT(interpreter->ctx)->control_stack,
+ struct Parrot_Context *context = CONTEXT(interpreter->ctx);
+ if (! context->return_stack) {
+ /* this is necessary in order to work around strange context
+ initiation rites. -- rgr, 21-Sep-06. */
+ context->return_stack = new_stack(interpreter, "Return");
+ }
+ stack_push(interpreter, &context->return_stack,
expr NEXT(), STACK_ENTRY_DESTINATION, STACK_CLEANUP_NULL);
loc = INTVAL2PTR(opcode_t *, $1);
goto ADDRESS(loc);
Index: src/stacks.c
===================================================================
--- src/stacks.c (revision 14695)
+++ src/stacks.c (working copy)
@@ -363,7 +363,13 @@
/* We don't mind the extra call, so we do this: (previous comment
* said we *do* mind, but I say let the compiler decide) */
void *dest;
- (void)stack_pop(interpreter, &CONTEXT(interpreter->ctx)->control_stack,
+ struct Parrot_Context *context = CONTEXT(interpreter->ctx);
+
+ if (! context->return_stack) {
+ real_exception(interpreter, NULL, INVALID_OPERATION,
+ "Return address stack is empty.");
+ }
+ (void)stack_pop(interpreter, &CONTEXT(interpreter->ctx)->return_stack,
&dest, STACK_ENTRY_DESTINATION);
return dest;
}
Index: src/sub.c
===================================================================
--- src/sub.c (revision 14695)
+++ src/sub.c (working copy)
@@ -40,6 +40,8 @@
mark_stack(interpreter, ctx->user_stack);
mark_stack(interpreter, ctx->control_stack);
+ if (ctx->return_stack)
+ mark_stack(interpreter, ctx->return_stack);
mark_register_stack(interpreter, ctx->reg_stack);
obj = (PObj*)ctx->current_sub;
if (obj)
End of diffs.
