[perl #41583] Tail calls from within vtable methods broken

3 views
Skip to first unread message

Bram Geron

unread,
Feb 22, 2007, 12:29:20 PM2/22/07
to bugs-bi...@rt.perl.org
# New Ticket Created by Bram Geron
# Please include the string: [perl #41583]
# in the subject line of all future correspondence about this issue.
# <URL: http://rt.perl.org/rt3/Ticket/Display.html?id=41583 >


---
osname= linux
osvers= 2.6.18.3
arch= i486-linux-gnu-thread-multi
cc= cc
---
Flags:
category=core
severity=medium
ack=no
---
Tail calls from within v-table methods are broken, the tail-called sub
(or method) will not return correct values.

When method A tailcalls sub B, B's set_returns stores its opcode
number (and with it, which registers should be returned), but the
low-level vtable code gets the registers from A's context.
(Runops_args stores a pointer to A's context just before it is called,
wrongly assuming A has the final set_returns. Runops_args returns the
context to a function that then does return value passing on it.)

Maybe the solution is to store the current context in a new field in
the interp structure; I don't know, I'm rather bad at C.

Example:
This should print 2, but it prints 13.

-----

.sub main :main
$P1 = newclass "Foo"
$P2 = new "Foo"

## Should return 2, but doesn't.
$I1 = elements $P2
$S1 = $I1
say $S1
.end

.namespace ["Foo"]

.sub elements :method :vtable
I0 = 13
I1 = 2
.return identity(I1)
.end

.sub identity
.param int arg
## arg is I0, taken from the elements context (which is set
## to 13). If we put "I0 = 14" here and don't optimize, we
## return 2. (elements's context's I1)
.return (arg)
.end

-----

This happens in svn head.

Regards,
Bram Geron.

---
Summary of my parrot 0.4.8 (r16890) configuration:
configdate='Sat Feb 10 16:50:38 2007'
Platform:
osname=linux, archname=i486-linux-gnu-thread-multi
jitcapable=1, jitarchname=i386-linux,
jitosname=LINUX, jitcpuarch=i386
execcapable=1
perl=/usr/bin/perl
Compiler:
cc='cc', ccflags='-D_REENTRANT -D_GNU_SOURCE -DTHREADS_HAVE_PIDS
-DDEBIAN -pipe -I/usr/local/include -D_LARGEFILE_SOURCE
-D_FILE_OFFSET_BITS=64 -I /usr/include',
Linker and Libraries:
ld='cc', ldflags=' -L/usr/local/lib',
cc_ldflags='',
libs='-ldl -lm -lpthread -lcrypt -lrt -lgmp -lreadline -lncurses'
Dynamic Linking:
share_ext='.so', ld_share_flags='-shared -L/usr/local/lib -fPIC',
load_ext='.so', ld_load_flags='-shared -L/usr/local/lib -fPIC'
Types:
iv=long, intvalsize=4, intsize=4, opcode_t=long, opcode_t_size=4,
ptrsize=4, ptr_alignment=1 byteorder=1234,
nv=double, numvalsize=8, doublesize=8

---
Environment:
HOME LANG LANGUAGE LD_LIBRARY_PATH LOGDIR PATH SHELL

Alek Storm

unread,
Mar 4, 2007, 1:56:32 PM3/4/07
to Jonathan Worthington, perl6-i...@perl.org
I'm almost done with a different patch that preserves the parent context for
the purpose of returning values into it. All further tailcalled contexts
are freed as normal. That's pretty vague, but it's easier just to see the
code. I just haven't had time to finish and release it.

Thanks,
Alek Storm

On 3/4/07, Jonathan Worthington <jona...@jnthn.net> wrote:


>
> Bram Geron (via RT) wrote:
> > Tail calls from within v-table methods are broken, the tail-called sub
> > (or method) will not return correct values.
> >
> > When method A tailcalls sub B, B's set_returns stores its opcode
> > number (and with it, which registers should be returned), but the
> > low-level vtable code gets the registers from A's context.
> > (Runops_args stores a pointer to A's context just before it is called,
> > wrongly assuming A has the final set_returns. Runops_args returns the
> > context to a function that then does return value passing on it.)
> >
> > Maybe the solution is to store the current context in a new field in
> > the interp structure; I don't know, I'm rather bad at C.
> >
> > Example:
> > This should print 2, but it prints 13.
> >
> >

> The problem is that when we make a call over the C boundary (e.g. call
> back into PIR), we do that by getting the invoke v-table method of the
> sub PMC to create the context etc. for the call for us, and then capture
> that context. We then return that context after the sub has executed,
> and in theory can get the parameters from it.
>
> Enter tail calls. The assumption that the context of the sub we called
> is the context with the results we want in it is now invalid, since a
> tailcall now replaces the context. That means we end up holding a
> reference to a freed context (essentially, freed memory!)
>
> My attempt at fixing this is to construct a fake context for the sub we
> call to set_returns into. Even if it does a tail call, then it will
> still end up putting the return values there - it's just normal calling
> semantics.
>
> Happily, the attached patch makes your program work. Unfortunately, it
> breaks other stuff and I'm having a hard time seeing why right now. The
> breakage includes PGE. So something I've done is quite wrong somehow,
> but I'm not sure where. So I'm throwing the patch out on the list in
> hope someone may stop something stupid I've done. Perhaps someone who's
> more familiar with the whole contexts thing than I.
>
> Thanks,
>
> Jonathan
>
>
>
> Index: src/ops/core.ops
> ===================================================================
> --- src/ops/core.ops (revision 17321)
> +++ src/ops/core.ops (working copy)
> @@ -564,14 +564,9 @@
> ctx = CONTEXT(interp->ctx);
> ccont = ctx->current_cont;
>
> - if (PMC_cont(ccont)->address) {
> - /* else it's from runops_fromc */
> + /* If we have a context to put the return values in... */
> + if (PMC_cont(ccont)->to_ctx) {
> parrot_context_t * const caller_ctx = PMC_cont(ccont)->to_ctx;
> - if (! caller_ctx) {
> - /* there is no point calling real_exception here, because
> - PDB_backtrace can't deal with a missing to_ctx either. */
> - internal_exception(1, "No caller_ctx for continuation %p.",
> ccont);
> - }
>
> src_indexes = interp->current_returns;
> dest_indexes = caller_ctx->current_results;
> @@ -581,6 +576,13 @@
>
> parrot_pass_args(interp, ctx, caller_ctx, src_indexes,
> dest_indexes, PARROT_PASS_RESULTS);
> }
> + else if (!PMC_cont(ccont)->address) {
> + /* Problem - we have a return bytecode address, but no context!
> There
> + * is no point calling real_exception here, because PDB_backtrace
> + * can't deal with a missing to_ctx either. */
> + internal_exception(1, "No caller_ctx for continuation %p.",
> ccont);
> + }
> +
> argc = SIG_ELEMS(signature);
> goto OFFSET(argc + 2);
> }
> Index: src/inter_run.c
> ===================================================================
> --- src/inter_run.c (revision 17321)
> +++ src/inter_run.c (working copy)
> @@ -29,6 +29,7 @@
> runops(Interp *interp, size_t offset)>
>
> Run parrot ops. Set exception handler and/or resume after exception.
> +This is the low level run ops routine that just takes an offset.
>
> =cut
>
> @@ -156,18 +157,74 @@
> opcode_t offset, *dest;
> parrot_context_t *ctx;
> parrot_context_t *old_ctx;
> + INTVAL *fake_context_registers;
> + opcode_t fake_return_bytecode[1] = { 0 };
> + int i;
> +
> /*
> * FIXME argument count limited - check strlen of sig
> */
> char new_sig[10];
> const char *sig_p;
>
> + /* Set the object we're calling with, if any. */
> + interp->current_object = obj;
> +
> + /* We set up a new return continuation with a NULL address, so we
> will
> + * eventually fall back out of the runloop to here. */
> old_ctx = CONTEXT(interp->ctx);
> - interp->current_cont = new_ret_continuation_pmc(interp, NULL);
> - interp->current_object = obj;
> + interp->current_cont = new_ret_continuation_pmc(interp, NULL);
> +
> + /* We may need to capture the return values too. We set up a fake
> register
> + * frame within a context to put them in to - one register of each
> type. */
> + fake_context_registers = mem_sys_allocate(4 * sizeof(INTVAL));
> + for (i = 0; i < 4; i++)
> + fake_context_registers[i] = 1;
> + ctx = Parrot_alloc_context(interp, fake_context_registers);
> + ctx->caller_ctx = NULL;
> + ctx->current_sub = NULL;
> + ctx->current_results = fake_return_bytecode;
> +
> + /* Also need to build a fake results signature. */
> + ctx->results_signature = pmc_new(interp,
> enum_class_FixedIntegerArray);
> + dod_register_pmc(interp, ctx->results_signature);
> + if (strlen(sig) > 0) {
> + VTABLE_set_integer_native(interp, ctx->results_signature, 1);
> + switch (sig[0])
> + {
> + case 'I':
> + VTABLE_set_integer_keyed_int(interp, ctx->results_signature,
> 0, 0);
> + break;
> + case 'S':
> + VTABLE_set_integer_keyed_int(interp, ctx->results_signature,
> 0, 1);
> + break;
> + case 'P':
> + VTABLE_set_integer_keyed_int(interp, ctx->results_signature,
> 0, 2);
> + break;
> + case 'N':
> + VTABLE_set_integer_keyed_int(interp, ctx->results_signature,
> 0, 3);
> + break;
> + }
> + }
> + else {
> + VTABLE_set_integer_native(interp, ctx->results_signature, 0);
> + }
> +
> + /* Set this as the context for the return continuation. */
> + PMC_cont(interp->current_cont)->to_ctx = ctx;
> + ctx->current_cont = interp->current_cont;
> +
> + /* Call the invoke v-table method to give us the address in the
> bytecode. */
> dest = VTABLE_invoke(interp, sub, NULL);
> if (!dest)
> internal_exception(1, "Subroutine returned a NULL address");
> +
> + /* Set the caller context, now invoke has set the context in the
> interpreter. */
> + CONTEXT(interp->ctx)->caller_ctx = ctx;
> +
> + /* Build the call signature. If we have an object, need to make sure
> we
> + * get an O as the first parameter (the final else branch does this).
> + * We always skip over the first character since that's the return
> type. */
> if (PMC_IS_NULL(obj)) {
> /* skip over the return type */
> sig_p = sig + 1;
> @@ -184,7 +241,8 @@
> strcpy(new_sig + 1, sig + 1);
> sig_p = new_sig;
> }
> -
> +
> + /* If we have arguments, do the passing of them. */
> if (*sig_p && dest[0] == PARROT_OP_get_params_pc) {
> dest = parrot_pass_args_fromc(interp, sig_p, dest, old_ctx, ap);
> }
> @@ -197,9 +255,17 @@
> }
> */
>
> - ctx = CONTEXT(interp->ctx);
> + /* Compute the offset into the bytecode and let rip. */
> offset = dest - interp->code->base.data;
> runops(interp, offset);
> +
> + /* Clean up. */
> + dod_unregister_pmc(interp, ctx->results_signature);
> + CONTEXT(interp->ctx) = old_ctx;
> + interp->ctx.bp = old_ctx->bp;
> + interp->ctx.bp_ps = old_ctx->bp_ps;
> +
> + /* Hand back the context so we can get the args out of it. */
> return ctx;
> }
>
> @@ -280,17 +346,8 @@
> PMC *sub, PMC *obj, STRING *meth)
> {
> parrot_context_t *ctx;
> - opcode_t offset, *dest;
> -
> - interp->current_cont = new_ret_continuation_pmc(interp, NULL);
> - interp->current_object = obj;
> - dest = VTABLE_invoke(interp, sub, (void*)1);
> - if (!dest)
> - internal_exception(1, "Subroutine returned a NULL address");
> - ctx = CONTEXT(interp->ctx);
> - offset = dest - interp->code->base.data;
> - runops(interp, offset);
> - return set_retval(interp, 0, ctx);
> + ctx = runops_args(interp, sub, obj, meth, "vO", NULL);
> + return NULL; /* No sig, assume void. */
> }
>
> void *
> @@ -303,7 +360,8 @@
> va_start(args, sig);
> ctx = runops_args(interp, sub, PMCNULL, NULL, sig, args);
> va_end(args);
> - return set_retval(interp, *sig, ctx);
> + return sig[0] == 'S' ? (void*)CTX_REG_STR(ctx, 0) :
> + sig[0] == 'P' ? (void*)CTX_REG_PMC(ctx, 0) : NULL;
> }
>
> void *
> @@ -328,7 +386,8 @@
> va_start(args, sig);
> ctx = runops_args(interp, sub, PMCNULL, NULL, sig, args);
> va_end(args);
> - retval = set_retval(interp, *sig, ctx);
> + retval = sig[0] == 'S' ? (void*)CTX_REG_STR(ctx, 0) :
> + sig[0] == 'P' ? (void*)CTX_REG_PMC(ctx, 0) : NULL;
>
> interp->current_args = cargs;
> interp->current_params = params;
> @@ -347,7 +406,7 @@
> va_start(args, sig);
> ctx = runops_args(interp, sub, PMCNULL, NULL, sig, args);
> va_end(args);
> - return set_retval_i(interp, *sig, ctx);
> + return CTX_REG_INT(ctx, 0);
> }
>
> FLOATVAL
> @@ -360,7 +419,7 @@
> va_start(args, sig);
> ctx = runops_args(interp, sub, PMCNULL, NULL, sig, args);
> va_end(args);
> - return set_retval_f(interp, *sig, ctx);
> + return CTX_REG_NUM(ctx, 0);
> }
>
> void*
> @@ -373,7 +432,8 @@
> va_start(args, sig);
> ctx = runops_args(interp, sub, obj, meth, sig, args);
> va_end(args);
> - return set_retval(interp, *sig, ctx);
> + return sig[0] == 'S' ? (void*)CTX_REG_STR(ctx, 0) :
> + sig[0] == 'P' ? (void*)CTX_REG_PMC(ctx, 0) : NULL;
> }
>
> INTVAL
> @@ -386,7 +446,7 @@
> va_start(args, sig);
> ctx = runops_args(interp, sub, obj, meth, sig, args);
> va_end(args);
> - return set_retval_i(interp, *sig, ctx);
> + return CTX_REG_INT(ctx, 0);
> }
>
> FLOATVAL
> @@ -399,7 +459,7 @@
> va_start(args, sig);
> ctx = runops_args(interp, sub, obj, meth, sig, args);
> va_end(args);
> - return set_retval_f(interp, *sig, ctx);
> + return CTX_REG_NUM(ctx, 0);
> }
>
> void *
> @@ -409,7 +469,8 @@
> parrot_context_t *ctx;
>
> ctx = runops_args(interp, sub, PMCNULL, NULL, sig, args);
> - return set_retval(interp, *sig, ctx);
> + return sig[0] == 'S' ? (void*)CTX_REG_STR(ctx, 0) :
> + sig[0] == 'P' ? (void*)CTX_REG_PMC(ctx, 0) : NULL;
> }
>
> INTVAL
> @@ -419,7 +480,7 @@
> parrot_context_t *ctx;
>
> ctx = runops_args(interp, sub, PMCNULL, NULL, sig, args);
> - return set_retval_i(interp, *sig, ctx);
> + return CTX_REG_INT(ctx, 0);
> }
>
> FLOATVAL
> @@ -429,7 +490,7 @@
> parrot_context_t *ctx;
>
> ctx = runops_args(interp, sub, PMCNULL, NULL, sig, args);
> - return set_retval_f(interp, *sig, ctx);
> + return CTX_REG_NUM(ctx, 0);
> }
>
> void*
> @@ -439,7 +500,8 @@
> parrot_context_t *ctx;
>
> ctx = runops_args(interp, sub, obj, meth, sig, args);
> - return set_retval(interp, *sig, ctx);
> + return sig[0] == 'S' ? (void*)CTX_REG_STR(ctx, 0) :
> + sig[0] == 'P' ? (void*)CTX_REG_PMC(ctx, 0) : NULL;
> }
>
> INTVAL
> @@ -449,7 +511,7 @@
> parrot_context_t *ctx;
>
> ctx = runops_args(interp, sub, obj, meth, sig, args);
> - return set_retval_i(interp, *sig, ctx);
> + return CTX_REG_INT(ctx, 0);
> }
>
> FLOATVAL
> @@ -459,7 +521,7 @@
> parrot_context_t *ctx;
>
> ctx = runops_args(interp, sub, obj, meth, sig, args);
> - return set_retval_f(interp, *sig, ctx);
> + return CTX_REG_NUM(ctx, 0);
> }
>
> /*
>
>

Bram Geron

unread,
May 7, 2007, 4:41:56 PM5/7/07
to parrotbug...@parrotcode.org
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

This patch copies the context to a new interp->fromc_result_ctx field on
a set_returns. As far as I know no new tests fail with this. This patch
retains most side-patching (comments etc.) by Jonathan. It also tests
in Parrot_free_context whether you want to free a context that is on
front of the free list, which would generate weird bugs. There is a
test case for this bug in t/op/calling.t.

Alek, I made the bulk of the patch before I dug through the other mails
and saw your mail. I'm sorry if you were still planning on sending it.

Thanks,
Bram Geron

diffstat:
include/parrot/interpreter.h | 5 +-
include/parrot/register.h | 6 +-
src/gc/register.c | 43 ++++++++++++++++++++
src/inter_call.c | 2
src/inter_run.c | 89
+++++++++++++++++++++++++++++++++++++------
src/ops/core.ops | 17 +++++++-
t/op/calling.t | 33 +++++++++++++++
7 files changed, 175 insertions(+), 20 deletions(-)


Alek Storm via RT wrote:
> I'm almost done with a different patch that preserves the parent context for
> the purpose of returning values into it. All further tailcalled contexts
> are freed as normal. That's pretty vague, but it's easier just to see the
> code. I just haven't had time to finish and release it.
>
> Thanks,
> Alek Storm
>
> On 3/4/07, Jonathan Worthington <jona...@jnthn.net> wrote:
>> Bram Geron (via RT) wrote:
>>> Tail calls from within v-table methods are broken, the tail-called sub
>>> (or method) will not return correct values.
>>>
>>> When method A tailcalls sub B, B's set_returns stores its opcode
>>> number (and with it, which registers should be returned), but the
>>> low-level vtable code gets the registers from A's context.
>>> (Runops_args stores a pointer to A's context just before it is called,
>>> wrongly assuming A has the final set_returns. Runops_args returns the
>>> context to a function that then does return value passing on it.)
>>>
>>> Maybe the solution is to store the current context in a new field in
>>> the interp structure; I don't know, I'm rather bad at C.
>>>
>>> Example:
>>> This should print 2, but it prints 13.
>>

>> (...)
- --
Bram Geron | GPG 0xE7B9E65E

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.6 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iD8DBQFGP48UvquQbee55l4RAqSuAJ9/93Fci2ztI9W9DykQngKHwjzZmACfQege
L/kpuaEOCJiQXJ2HLM06xKw=
=Yxh5
-----END PGP SIGNATURE-----

fromcfix.diff

Alek Storm

unread,
May 7, 2007, 5:03:40 PM5/7/07
to Bram Geron, parrotbug...@parrotcode.org
On 5/7/07, Bram Geron <bge...@gmail.com> wrote:
>
> Alek, I made the bulk of the patch before I dug through the other mails
> and saw your mail. I'm sorry if you were still planning on sending it.


Don't feel sorry at all. I got halfway through the patch before I got
distracted by other things. Great work. Thank you very much for
accomplishing what I failed to do :)

--
Alek Storm

Chromatic

unread,
Jun 14, 2008, 1:57:08 PM6/14/08
to parrot-...@perl.org, Bram Geron, bugs-bi...@rt.perl.org
I tried to apply Bram's patch from last year, but it had a lot of fuzz
(especially related to the removal of register stacks). Here's what I ended
up with.

It doesn't work, so I'm not applying it as it is... but if there's still a
problem, this might be a better starting point.

-- c

vtable_tail_calls.patch
Reply all
Reply to author
Forward
0 new messages