[Mesa-dev] [PATCH 04/24] ralloc: Unify overloads of the new operator and guarantee object destruction.

[Francisco Jerez]

Paul Berry <stereotype441 at gmail.com> writes:

> On 15 September 2013 00:10, Francisco Jerez <currojerez at riseup.net> wrote:
>[...]
>> @@ -70,12 +65,7 @@ class cfg_t {
>>  public:
>>     static void* operator new(size_t size, void *ctx)
>>     {
>> -      void *node;
>> -
>> -      node = rzalloc_size(ctx, size);
>> -      assert(node != NULL);
>> -
>> -      return node;
>> +      return ralloc_new<cfg_t>(size, ctx);
>>     }
>
> I'm worried about this one--it introduces a behavioural change.
> Previously, if a cfg_t object was reclaimed through the standard ralloc
> mechanism, cfg_t::~cfg_t() would *not* be called.  Now it will be.  Since
> cfg_t::~cfg_t() calls ralloc_free(mem_ctx), and since cfg_t's mem_ctx is
> usually (always?) the same mem_ctx that the cfg_t is contained in, I think
> that will result in double-freeing of the mem_ctx.

That's really bad...  It looks like you are right, cfg_t sort of relies
on its destructor not being called in that case...

> However, looking further into the users of cfg_t, it looks like they all
> pass it a "borrowed" mem_ctx (in other words, the caller always retains
> responsibility for freeing the mem_ctx.  So I believe we have a
> pre-existing double-free bug.  The correct solution is probably to get rid
> of the cfg_t destructor entirely.

Probably this hasn't been a problem until now because cfg_t creates its
own ralloc context as a child of its allocator's context, the former is
what's being passed to ralloc_free() from ~cfg_t(), there was no
possibility for a double-free in all cases where cfg_t's destructor used
to end up getting called.

It looks like it might be useful to keep cfg_t's own allocation context
around because its children objects seem to have a shorter lifetime than
its parent context(s), removing the destructor would keep them alive for
a longer while until the parent is destroyed...  I think that now that
destructors are going to be called by ralloc it doesn't make sense
anymore to the have cfg_t's allocation context be a child of its
parent's allocation context, it should probably take care of destroying
it from its destructor as any other memory resource, to ensure all
objects it creates are destroyed timely.

>>[...]
>> diff --git a/src/mesa/drivers/dri/i965/brw_fs_live_variables.h
>> b/src/mesa/drivers/dri/i965/brw_fs_live_variables.h
>> index 1cde5f4..74542e5 100644
>> --- a/src/mesa/drivers/dri/i965/brw_fs_live_variables.h
>> +++ b/src/mesa/drivers/dri/i965/brw_fs_live_variables.h
>> @@ -55,12 +55,7 @@ class fs_live_variables {
>>  public:
>>     static void* operator new(size_t size, void *ctx)
>>     {
>> -      void *node;
>> -
>> -      node = rzalloc_size(ctx, size);
>> -      assert(node != NULL);
>> -
>> -      return node;
>> +      return ralloc_new<fs_live_variables>(size, ctx);
>>     }
>
> I'm worried about this one too, for similar reasons.  I believe in this
> case the appropriate solution is the same: get rid of the fs_live_variables
> destructor entirely.
>>
>>     fs_live_variables(fs_visitor *v, cfg_t *cfg);
>> diff --git a/src/mesa/drivers/dri/i965/brw_vec4_live_variables.h
>> b/src/mesa/drivers/dri/i965/brw_vec4_live_variables.h
>> index 438a03e..168da40 100644
>> --- a/src/mesa/drivers/dri/i965/brw_vec4_live_variables.h
>> +++ b/src/mesa/drivers/dri/i965/brw_vec4_live_variables.h
>> @@ -54,12 +54,7 @@ class vec4_live_variables {
>>  public:
>>     static void* operator new(size_t size, void *ctx)
>>     {
>> -      void *node;
>> -
>> -      node = rzalloc_size(ctx, size);
>> -      assert(node != NULL);
>> -
>> -      return node;
>> +      return ralloc_new<vec4_live_variables>(size, ctx);
>>     }
>
> The exact same situation exists here (yay code duplication).

These last two cases are only apparent problems because both objects
only seem to be allocated from the stack, the overloaded new operators
don't seem to be used at all.  I think for these two I'm going to remove
the unused new operators and fix the constructors to stop allocating
From its parent's context, if that sounds reasonable to you.

> Other than these double-free issues, I really like what you've done here.
> It's a substantial reduction in boilerplate code that's easy to get wrong.

Thank you for your very useful review. :)
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 229 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20130917/547d0ec6/attachment.pgp>