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

[Chad Versace]

I like this patch. It removes a lot of boilerplate. And, it makes object
destruction easier to reason about, making it easier to reason about tricky
destruction bugs.

On 09/17/2013 11:59 AM, Paul Berry wrote:
> On 15 September 2013 00:10, Francisco Jerez <currojerez at riseup.net> wrote:
>
>> This patch introduces a pair of helper functions providing a common
>> implementation of the "new" and "delete" operators for all C++ classes
>> that are allocated by ralloc via placement new.  The 'ralloc_new'
>> helper function takes care of setting up an ralloc destructor callback
>> that will call the appropriate destructor before the memory allocated
>> to an object is released.
>>
>> Until now objects needed to call 'ralloc_set_destructor' explicitly
>> with a pointer to a static method which in turn called the actual
>> destructor in order to get something that should be transparent to
>> them.  After this patch they'll only need to call 'ralloc_new' from
>> the new operator and 'ralloc_delete' from the delete operator, turning
>> all overloads of new and delete into one-liners.
>> ---
>>   src/glsl/ast.h                                     | 26 +++----------
>>   src/glsl/glsl_parser_extras.h                      |  9 +----
>>   src/glsl/glsl_symbol_table.cpp                     |  7 +---
>>   src/glsl/glsl_symbol_table.h                       | 23 +----------
>>   src/glsl/glsl_types.h                              | 11 +-----
>>   src/glsl/ir_function_detect_recursion.cpp          | 11 +-----
>>   src/glsl/list.h                                    | 22 ++---------
>>   src/glsl/loop_analysis.h                           | 14 +------
>>   src/glsl/ralloc.h                                  | 44
>> ++++++++++++++++++++++
>>   src/mesa/drivers/dri/i965/brw_cfg.h                | 14 +------
>>   src/mesa/drivers/dri/i965/brw_fs.h                 | 21 ++---------
>>   src/mesa/drivers/dri/i965/brw_fs_live_variables.h  |  7 +---
>>   src/mesa/drivers/dri/i965/brw_vec4.h               | 21 ++---------
>>   .../drivers/dri/i965/brw_vec4_live_variables.h     |  7 +---
>>   src/mesa/program/ir_to_mesa.cpp                    |  7 +---
>>   src/mesa/state_tracker/st_glsl_to_tgsi.cpp         |  7 +---
>>   16 files changed, 77 insertions(+), 174 deletions(-)
>>


>> diff --git a/src/mesa/drivers/dri/i965/brw_cfg.h
>> b/src/mesa/drivers/dri/i965/brw_cfg.h
>> index 95a18e9..f8037a9 100644
>> --- a/src/mesa/drivers/dri/i965/brw_cfg.h
>> +++ b/src/mesa/drivers/dri/i965/brw_cfg.h
>> @@ -41,12 +41,7 @@ class bblock_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<bblock_t>(size, ctx);
>>      }
>>
>>      bblock_link *make_list(void *mem_ctx);

Post-patch, it's safe for bblock_t to lack an override of operator delete
only because it's destructor is a no-op. However, it remains a no-op only if
the exec_list destructor remains a no-op. I'd like to see bblock_t future-proofed
now against any double-destructor bugs by overriding operator delete.

>> @@ -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.
>
> 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.

Whatever you do with cfg_t's destructor, I'd like to an override of operator
delete for the same reason I stated for bblock_t.

Actually, for each class that this patch touches, I want to see an override
of operator delete if one does not already exist, lest a future someone introduces
a bug by changing that class's destructor from trivial to non-trivial but forgets
to override delete as well.