Memory fix

[David Hakim]

Hey Stefano!

I'm taking a look at your patch now. I replicated at least one
interpreter memory leak in ary3.moto . Is this the one you are
referring to ? I haven't gone through the other perf tests yet. First I
want to add this one to the main test list. Then fix it. Then look at
the others :)

Hi Dave,
I'm working to fix some memory problems. Please give a look at this
patch and
see if it makes sense... I've fixed a bug in motoi_new (the resulting
MotoVal
of motoi_callMDF was not free'd before) and I've create a new strdup
routine
for variable names, to be able to free them separately when a frame is
free'd
(or when a dereference name is pushed/popped as MotoVar). There are
however
some place where we leak memory... I would like to be able to run all
the
tests in perfetest.tar.gz in interpreted mode (doesn't matter for how
long
:)) without getting memory errors... The funny thing with this patch
is that
things go slightly faster!
The patch applies with -p1 on moto_0_20_0...
Stefano<motomem.patch>

[David Hakim]

So the root of the problem in ary3.moto is that in motoi_assign we are
evaluating the lvalue twice but only throwing the MotoVar returned from
the evaluation away once.

At the start of motoi_assign we evaluate the lvalue as a MotoVar
(assigning it to dest) so we can assign to it later

UnionCell *lval = uc_operand(p,0);
int op = uc_operand(p, 1)->opcell.opcode;
MotoVar * dest = NULL;

/* Save rval UnionCell */
env->rval_uc = uc_operand(p, 2);

/* First ... either get value name or process operand 0 */

if (lval->type == VALUE_TYPE && lval->valuecell.kind == ID_VALUE) {

char *destn = uc_str(p, 0);
dest = moto_getVar(env, destn);

} else if((lval->type == OP_TYPE && lval->opcell.opcode == ARRAY_LVAL)
||
(lval->type == OP_TYPE && lval->opcell.opcode == DEREFERENCE_LVAL)) {

dest = NULL;

/* Evaluate the first operand to get the Member MotoVar */
motoi(lval);

dest= (MotoVar*)opstack_pop(env);

}

Then, if we are dealing with a potentially overloaded operator we

else {

/* do a math operation and leave the result on the top of the stack */
MotoVal *v1;
MotoVal *v2;

switch (op) {
case MATH_ADD: /* add-assignment */
case MATH_SUB: /* sub-assignment */
case MATH_MULT: /* mult-assignment */
case MATH_DIV: /* div-assignment */
case MATH_MOD: /* mod-assignment */
if (env->rval_uc != NULL) {
motoi(uc_operand(p, 2));
env->rval_uc = NULL;
}

/* FIXME!!! nasty but nice */

if( lval->opcell.opcode == DEREFERENCE_LVAL )
motoi_dereference_rval(uc_operand(p, 0));
else
motoi(uc_operand(p, 0));

v1 = opstack_pop(env);
v2 = opstack_pop(env);

if(motox_try_overloaded_method_op(op,v1,v2,NULL) != MOTO_OK) {
motoi_domath(v1, v2, op);
moto_freeVal(env,v1);
moto_freeVal(env,v2);
}
break;
}

}

The really bad thing here is

if( lval->opcell.opcode == DEREFERENCE_LVAL )
motoi_dereference_rval(uc_operand(p, 0));
else
motoi(uc_operand(p, 0));

But in theory we should not need to re-evaluate lval operand if its a
DEREFERENCE_LVAL or an ARRAY_LVAL since we already did that earlier (to
get the dest MotoVar). Moreover since we are not doing a specific check
here for ARRAY_LVAL (which pops a MotoVar as opposed to a MotoVal onto
the opstack) we get into deeper trouble when we execute:

v1 = opstack_pop(env);

since we've just assigned a MotoVal* a MotoVar* ...

More investigation is definitely required but that's what I see so far

-Dave

Post by David Hakim
Hey Stefano!
I'm taking a look at your patch now. I replicated at least one
interpreter memory leak in ary3.moto . Is this the one you are
referring to ? I haven't gone through the other perf tests yet. First
I want to add this one to the main test list. Then fix it. Then look
at the others :)

Hi Dave,
I'm working to fix some memory problems. Please give a look at this
patch and
see if it makes sense... I've fixed a bug in motoi_new (the resulting
MotoVal
of motoi_callMDF was not free'd before) and I've create a new strdup
routine
for variable names, to be able to free them separately when a frame
is free'd
(or when a dereference name is pushed/popped as MotoVar). There are
however
some place where we leak memory... I would like to be able to run all
the
tests in perfetest.tar.gz in interpreted mode (doesn't matter for how
long
:)) without getting memory errors... The funny thing with this patch
is that
things go slightly faster!
The patch applies with -p1 on moto_0_20_0...
Stefano<motomem.patch>

[David Hakim]

Ok, so I fixed the behavior of the += *= -= /= and %= operators when
the lvalue is a subscripted array

This fixes both the memory leak and the functional issues

Post by David Hakim
if( lval->opcell.opcode == DEREFERENCE_LVAL )
motoi_dereference_rval(uc_operand(p, 0));
else
motoi(uc_operand(p, 0));
v1 = opstack_pop(env);

with

if( lval->opcell.opcode == DEREFERENCE_LVAL ||
lval->opcell.opcode == ARRAY_LVAL ) {
v1 = moto_getVarVal(env,dest);
} else {
motoi(uc_operand(p, 0));
v1 = opstack_pop(env);
}

I'm not sure if this fix is going to wreak any havoc with the operator
overloading stuff. Stefano, you might want to take a look at any issues
there. However all tests pass and I have added a new test to the tests
directory

arr_assign_succ

to make sure this issue doesn't pop up again

-Dave

Post by David Hakim
So the root of the problem in ary3.moto is that in motoi_assign we are
evaluating the lvalue twice but only throwing the MotoVar returned
from the evaluation away once.
At the start of motoi_assign we evaluate the lvalue as a MotoVar
(assigning it to dest) so we can assign to it later
UnionCell *lval = uc_operand(p,0);
int op = uc_operand(p, 1)->opcell.opcode;
MotoVar * dest = NULL;
/* Save rval UnionCell */
env->rval_uc = uc_operand(p, 2);
/* First ... either get value name or process operand 0 */
if (lval->type == VALUE_TYPE && lval->valuecell.kind == ID_VALUE) {
char *destn = uc_str(p, 0);
dest = moto_getVar(env, destn);
} else if((lval->type == OP_TYPE && lval->opcell.opcode ==
ARRAY_LVAL) ||
(lval->type == OP_TYPE && lval->opcell.opcode == DEREFERENCE_LVAL)) {
dest = NULL;
/* Evaluate the first operand to get the Member MotoVar */
motoi(lval);
dest= (MotoVar*)opstack_pop(env);
}
Then, if we are dealing with a potentially overloaded operator we
else {
/* do a math operation and leave the result on the top of the stack */
MotoVal *v1;
MotoVal *v2;
switch (op) {
case MATH_ADD: /* add-assignment */
case MATH_SUB: /* sub-assignment */
case MATH_MULT: /* mult-assignment */
case MATH_DIV: /* div-assignment */
case MATH_MOD: /* mod-assignment */
if (env->rval_uc != NULL) {
motoi(uc_operand(p, 2));
env->rval_uc = NULL;
}
/* FIXME!!! nasty but nice */
if( lval->opcell.opcode == DEREFERENCE_LVAL )
motoi_dereference_rval(uc_operand(p, 0));
else
motoi(uc_operand(p, 0));
v1 = opstack_pop(env);
v2 = opstack_pop(env);
if(motox_try_overloaded_method_op(op,v1,v2,NULL) != MOTO_OK) {
motoi_domath(v1, v2, op);
moto_freeVal(env,v1);
moto_freeVal(env,v2);
}
break;
}
}
The really bad thing here is
if( lval->opcell.opcode == DEREFERENCE_LVAL )
motoi_dereference_rval(uc_operand(p, 0));
else
motoi(uc_operand(p, 0));
But in theory we should not need to re-evaluate lval operand if its a
DEREFERENCE_LVAL or an ARRAY_LVAL since we already did that earlier
(to get the dest MotoVar). Moreover since we are not doing a specific
check here for ARRAY_LVAL (which pops a MotoVar as opposed to a
v1 = opstack_pop(env);
since we've just assigned a MotoVal* a MotoVar* ...
More investigation is definitely required but that's what I see so far
-Dave

Post by David Hakim
Hey Stefano!
I'm taking a look at your patch now. I replicated at least one
interpreter memory leak in ary3.moto . Is this the one you are
referring to ? I haven't gone through the other perf tests yet. First
I want to add this one to the main test list. Then fix it. Then look
at the others :)

Hi Dave,
I'm working to fix some memory problems. Please give a look at this
patch and
see if it makes sense... I've fixed a bug in motoi_new (the
resulting MotoVal
of motoi_callMDF was not free'd before) and I've create a new strdup
routine
for variable names, to be able to free them separately when a frame
is free'd
(or when a dereference name is pushed/popped as MotoVar). There are
however
some place where we leak memory... I would like to be able to run
all the
tests in perfetest.tar.gz in interpreted mode (doesn't matter for
how long
:)) without getting memory errors... The funny thing with this patch
is that
things go slightly faster!
The patch applies with -p1 on moto_0_20_0...
Stefano<motomem.patch>