Investigate memory leak in field initializers

[bandurvp]

Currently the GC transformation is not applied at all for field initializers. This seems to result in memory leaks if a field is initialized with, for instance, the result of a function call such as a := add(1, 2); The resulting generated code will contain newInt(1) and newInt(2) which can not be reclaimed by the garbage collector or the corresponding call to the enclosing class free function. If the parameters of add appear as newIntGC(1, NULL) and newIntGC(2, NULL) then the memory can be reclaimed.

[peterwvj]

I see your point. So is the solution simply to apply the GC transformation to field initialisers?

If we do that the field initialisers fory : nat := f(3) would be:

 static  TVP _Z17fieldInitializer4EV(ACLASS this)	{
  TVP embeding_1 = newIntGC(3, NULL);
  TVP ret_1 = CALL_FUNC_PTR(A, A, this, CLASS_A__Z1fEN, embeding_1);
  return ret_1;
}

Another thing: I noticed that the field initialiser is invoked in the following way: this_ptr->m_A_y= _Z17fieldInitializer4EV(this_ptr). Does that not cause the state of active objects to be corrupted when vdm_gc() is invoked? I mean, f(3) returns a GC value. Wouldn't it be better to deep copy the value returned by the field initialiser: this_ptr->m_A_y= deepCopy(_Z17fieldInitializer4EV(this_ptr))?

[bandurvp]

Only applying the GC transformation for initializers will not work because the GC will collect them when it's called, whereas we want them to persist. But applying the GC transformation and also wrapping in a deep copy I think will work. It's only a matter of using vdmClone as the deep copy mechanism.

[peterwvj]

OK, issue fixed in pvj/ovt-254. Now, when the GC is enabled, the field initializer for y : nat := f(3) is translated to:

static  TVP _Z17fieldInitializer4EV(ACLASS this)	{
  TVP embeding_1 = newIntGC(3, NULL);
  TVP ret_1 = CALL_FUNC_PTR(A, A, this, CLASS_A__Z1fEN, embeding_1);

  return vdmClone(ret_1);
}

Essentially what happens is that the garbage collection transformation is applied to field initializer. In addition to that, expressions that are returned in field initializers are cloned. @bandurvp please confirm that the generated code looks alright.

[bandurvp]

Hi @peterwvj , I've discovered a bug that I think is a result of this fix. In this model

class A
end A

class B
instance variables
public static a : A := new A();

operations
public op : () ==> ()
op() == a := new A();	 
end B

The assignment to a should not be a GC one, otherwise the instance variable gets reclaimed when the GC is run. Before the fix the assignment was

g_B_a = _Z1AEV(NULL);

but now it is

g_B_a = _Z1AEVGC(NULL);

Notice also that the second NULL is missing the the second case because of my change for issue #123. This part is correct.

[peterwvj]

Yeah, I see what you mean. Is the solution simply to clone the right-hand-side of assignments when fields (instance + static instance variables) are assigned to? That is, the code should be generated as:

g_B_a = vdmClone(_Z1AEVGC(NULL));

Can you confirm that this is the solution @bandurvp ?

I mean, it's not safe to just revert the "fix" (changing it back to "Z1AEV(NULL)") because then the following model snippet will produce code that leaks:

(
  op();
  op();
)

[bandurvp]

Hi @peterwvj , that would work, but it would introduce another layer of cloning. Could an analysis be done and omit the GC versions in assignments to fields because they need to persist for the lifetime of the object?

[peterwvj]

Hi @bandurvp Yeah, we could, but is that really a good solution? I mean, the example above (or below) would cause a leak the second time op is executed since the value is constructed using "Z1AEV(NULL)".

(
  op();
  op();
)

The other approach would introduce another layer of cloning (that's true), but it would be correct. Do you see what I mean?

[bandurvp]

Hi @peterwvj , in normal field assignments, the macros SET_FIELD_PTR and SET_FIELD_PTR_GC call vdmFree on the field variable before they assign the new value. Could something similar be done for the above? The assignment would become

vdmFree(g_B_a);
g_B_a = _Z1AEV(NULL);

[peterwvj]

Can you clarify what you mean by "normal field assignment". Is g_B_a =... considered a normal field assignment?

[bandurvp]

Sorry, that wasn't very clear of me. What I meant is that non-static fields receive assignments like this:

TVP field_tmp_1 = _Z1AEVGC(NULL);
SET_FIELD_PTR(B, B, this, b, field_tmp_1);
vdmFree(field_tmp_1);

where the macro SET_FIELD_PTR first does a vdmFree on the field being assigned to (this is actually done in the inner macro SET_STRUCT_FIELD which is called by this one.) So I was thinking we can do something similar to assignments to static fields, by freeing first and then assigning the new value which is not under GC control.

[peterwvj]

Thanks for clarifying. Okay, so the transformation will only cover static fields. I guess this transformation should ignore field initializers? Are there more things to ignore?

Now, just to make sure I understand, we want the following bit of code

g_B_a = _Z1AEVGC(NULL);

to be generated as

vdmFree(g_B_a);
g_B_a = _Z1AEV(NULL);

Sure, we can do that. I don't think it's that difficult.

What about nested values? I mean, what if the assignment in the VDM model looks like a := new A(42);. I assume that these "nested" values would need to use the explicit constructors as well. Would the code then be?:

vdmFree(g_B_a);
g_B_a = _Z1AEV(NULL, newInt(42));

But does this approach work if the argument is the result produced by (say) a function? For example, let's say that the assignment to generate is a := new A(f()); where f() returns as value that the garbage collector may reclaim. Do you see my point?

[bandurvp]

Agreed that the initial implementation of this should be:

vdmFree(g_B_a);
g_B_a = vdmClone(_Z1AEVGC(NULL, newIntGC(42)));

[peterwvj]

Hi @bandurvp . There's a fix in pvj/development. Could you try it out and let me know if it works as expected.