Compiler memory leak

Hi,

I've found a memory leak in the compiler: there are extra
INC_(SAVE/RESTORE)_POINTER
in compile_block and _gst_make_constant that create a leak.

Once the object is allocated and protected in a context (ie created by
save_pointer)
once leaving the function they should be either protected or added in a
protected object
otherwise they could be GCed

The patch remove those extra INC_(SAVE/RESTORE)_POINTER protection

Gwen



_______________________________________________
help-smalltalk mailing list
[hidden email]
https://lists.gnu.org/mailman/listinfo/help-smalltalk

Il 16/07/2013 09:43, Gwenaël Casaccio ha scritto:
> Hi,
>
> I've found a memory leak in the compiler: there are extra
> INC_(SAVE/RESTORE)_POINTER
> in compile_block and _gst_make_constant that create a leak.

Not a leak, a dangling pointer. :)

> Once the object is allocated and protected in a context (ie created by
> save_pointer)
> once leaving the function they should be either protected or added in a
> protected object
> otherwise they could be GCed
>
> The patch remove those extra INC_(SAVE/RESTORE)_POINTER protection

As discussed on IRC, incubator contexts should be as small as possible,
just the time to prepare an object and pass it back to the caller.  The
caller can then decide to incubate it, or put it in an object that is
alive, or put it in an object that is incubated.

So I think it is better to leave these INC_SAVE/RESTORE_POINTER calls in
place.  In fact, I think those in _gst_make_constant_oop are fine.

You did find a bug in compile_block.  add_literal's addition of
blockOOP/blockClosureOOP is within compile_block's incubator context,
which breaks because the literal can disappear as soon as compile_block
exits.  As usual, you wonder how come we never encountered this situation.

To catch more bugs like this, we need to assert that add_literal is
called in the outermost incubator context.  We can add an
inc_context_depth() function (which really can just return
INC_SAVE_POINTER()), store the value in _gst_compile_method and compare
it in add_literal.  Let me try to do a patch.

Paolo

_______________________________________________
help-smalltalk mailing list
[hidden email]
https://lists.gnu.org/mailman/listinfo/help-smalltalk

On 16/07/2013 16:07, Paolo Bonzini wrote: You know I've changed my mind, using them as a kind of global
stack for allocation could be nice and simplify the Smalltalk objects
allocation logic. Yes but you capture only one kind of bugs tracking allocation with a
"global" incubator is easier.
> Paolo

Gwen


_______________________________________________
help-smalltalk mailing list
[hidden email]
https://lists.gnu.org/mailman/listinfo/help-smalltalk

Il 16/07/2013 21:11, Gwenaël Casaccio ha scritto:
>> To catch more bugs like this, we need to assert that add_literal is
>> called in the outermost incubator context.  We can add an
>> inc_context_depth() function (which really can just return
>> INC_SAVE_POINTER()), store the value in _gst_compile_method and compare
>> it in add_literal.  Let me try to do a patch.
>
> Yes but you capture only one kind of bugs tracking allocation with a
> "global" incubator is easier.

There is a global incubator, that's _gst_register_oop and
_gst_register_oop_array.  INC_SAVE_POINTER/INC_RESTORE_POINTER is
explicitly for stack-based operation.  add_literal is at the bottom of
the stack because the objects it incubates are for the outermost object
(the CompiledMethod).  So the invariant makes sense, and the fragility
might only be apparent (i.e. it actually helps finding bugs).

That said, I'm glad to evaluate a patch to remove incubator usage from
add_literal, and instead use _gst_register_oop_array.

Paolo


_______________________________________________
help-smalltalk mailing list
[hidden email]
https://lists.gnu.org/mailman/listinfo/help-smalltalk