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

Paul Berry stereotype441 at gmail.com
Tue Sep 17 11:59:23 PDT 2013


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/glsl/ast.h b/src/glsl/ast.h
> index 1c7fc63..26c4701 100644
> --- a/src/glsl/ast.h
> +++ b/src/glsl/ast.h
> @@ -53,19 +53,12 @@ public:
>      * easier to just ralloc_free 'ctx' (or any of its ancestors). */
>     static void* operator new(size_t size, void *ctx)
>     {
> -      void *node;
> -
> -      node = rzalloc_size(ctx, size);
> -      assert(node != NULL);
> -
> -      return node;
> +      return ralloc_new<ast_node>(size, ctx);
>     }
>
> -   /* If the user *does* call delete, that's OK, we will just
> -    * ralloc_free in that case. */
> -   static void operator delete(void *table)
> +   static void operator delete(void *p)
>     {
> -      ralloc_free(table);
> +      ralloc_delete(p);
>     }
>
>     /**
> @@ -367,19 +360,12 @@ struct ast_type_qualifier {
>      * easier to just ralloc_free 'ctx' (or any of its ancestors). */
>     static void* operator new(size_t size, void *ctx)
>     {
> -      void *node;
> -
> -      node = rzalloc_size(ctx, size);
> -      assert(node != NULL);
> -
> -      return node;
> +      return ralloc_new<ast_type_qualifier>(size, ctx);
>     }
>
> -   /* If the user *does* call delete, that's OK, we will just
> -    * ralloc_free in that case. */
> -   static void operator delete(void *table)
> +   static void operator delete(void *p)
>     {
> -      ralloc_free(table);
> +      ralloc_delete(p);
>     }
>
>     union {
> diff --git a/src/glsl/glsl_parser_extras.h b/src/glsl/glsl_parser_extras.h
> index 2e2440a..6c2a63e 100644
> --- a/src/glsl/glsl_parser_extras.h
> +++ b/src/glsl/glsl_parser_extras.h
> @@ -77,17 +77,12 @@ struct _mesa_glsl_parse_state {
>      * easier to just ralloc_free 'ctx' (or any of its ancestors). */
>     static void* operator new(size_t size, void *ctx)
>     {
> -      void *mem = rzalloc_size(ctx, size);
> -      assert(mem != NULL);
> -
> -      return mem;
> +      return ralloc_new<_mesa_glsl_parse_state>(size, ctx);
>     }
>
> -   /* If the user *does* call delete, that's OK, we will just
> -    * ralloc_free in that case. */
>     static void operator delete(void *mem)
>     {
> -      ralloc_free(mem);
> +      ralloc_delete(mem);
>     }
>
>     /**
> diff --git a/src/glsl/glsl_symbol_table.cpp
> b/src/glsl/glsl_symbol_table.cpp
> index 4c96620..11fe06e 100644
> --- a/src/glsl/glsl_symbol_table.cpp
> +++ b/src/glsl/glsl_symbol_table.cpp
> @@ -30,15 +30,12 @@ public:
>      * easier to just ralloc_free 'ctx' (or any of its ancestors). */
>     static void* operator new(size_t size, void *ctx)
>     {
> -      void *entry = ralloc_size(ctx, size);
> -      assert(entry != NULL);
> -      return entry;
> +      return ralloc_new<symbol_table_entry>(size, ctx);
>     }
>
> -   /* If the user *does* call delete, that's OK, we will just
> ralloc_free. */
>     static void operator delete(void *entry)
>     {
> -      ralloc_free(entry);
> +      ralloc_delete(entry);
>     }
>
>     bool add_interface(const glsl_type *i, enum ir_variable_mode mode)
> diff --git a/src/glsl/glsl_symbol_table.h b/src/glsl/glsl_symbol_table.h
> index 62d26b8..f850d9f 100644
> --- a/src/glsl/glsl_symbol_table.h
> +++ b/src/glsl/glsl_symbol_table.h
> @@ -43,35 +43,16 @@ class symbol_table_entry;
>   * type safe and some symbol table invariants.
>   */
>  struct glsl_symbol_table {
> -private:
> -   static void
> -   _glsl_symbol_table_destructor (glsl_symbol_table *table)
> -   {
> -      table->~glsl_symbol_table();
> -   }
> -
> -public:
>     /* Callers of this ralloc-based new need not call delete. It's
>      * easier to just ralloc_free 'ctx' (or any of its ancestors). */
>     static void* operator new(size_t size, void *ctx)
>     {
> -      void *table;
> -
> -      table = ralloc_size(ctx, size);
> -      assert(table != NULL);
> -
> -      ralloc_set_destructor(table, (void (*)(void*))
> _glsl_symbol_table_destructor);
> -
> -      return table;
> +      return ralloc_new<glsl_symbol_table>(size, ctx);
>     }
>
> -   /* If the user *does* call delete, that's OK, we will just
> -    * ralloc_free in that case. Here, C++ will have already called the
> -    * destructor so tell ralloc not to do that again. */
>     static void operator delete(void *table)
>     {
> -      ralloc_set_destructor(table, NULL);
> -      ralloc_free(table);
> +      ralloc_delete(table);
>     }
>
>     glsl_symbol_table();
> diff --git a/src/glsl/glsl_types.h b/src/glsl/glsl_types.h
> index 9f61eee..acdf48f 100644
> --- a/src/glsl/glsl_types.h
> +++ b/src/glsl/glsl_types.h
> @@ -103,19 +103,12 @@ struct glsl_type {
>          assert(glsl_type::mem_ctx != NULL);
>        }
>
> -      void *type;
> -
> -      type = ralloc_size(glsl_type::mem_ctx, size);
> -      assert(type != NULL);
> -
> -      return type;
> +      return ralloc_new<glsl_type>(size, glsl_type::mem_ctx);
>     }
>
> -   /* If the user *does* call delete, that's OK, we will just
> -    * ralloc_free in that case. */
>     static void operator delete(void *type)
>     {
> -      ralloc_free(type);
> +      ralloc_delete(type);
>     }
>
>     /**
> diff --git a/src/glsl/ir_function_detect_recursion.cpp
> b/src/glsl/ir_function_detect_recursion.cpp
> index 280c473..1b21672 100644
> --- a/src/glsl/ir_function_detect_recursion.cpp
> +++ b/src/glsl/ir_function_detect_recursion.cpp
> @@ -144,19 +144,12 @@ public:
>      * easier to just ralloc_free 'ctx' (or any of its ancestors). */
>     static void* operator new(size_t size, void *ctx)
>     {
> -      void *node;
> -
> -      node = ralloc_size(ctx, size);
> -      assert(node != NULL);
> -
> -      return node;
> +      return ralloc_new<function>(size, ctx);
>     }
>
> -   /* If the user *does* call delete, that's OK, we will just
> -    * ralloc_free in that case. */
>     static void operator delete(void *node)
>     {
> -      ralloc_free(node);
> +      ralloc_delete(node);
>     }
>
>     ir_function_signature *sig;
> diff --git a/src/glsl/list.h b/src/glsl/list.h
> index 1d46365..dfd6378 100644
> --- a/src/glsl/list.h
> +++ b/src/glsl/list.h
> @@ -80,19 +80,12 @@ struct exec_node {
>      * easier to just ralloc_free 'ctx' (or any of its ancestors). */
>     static void* operator new(size_t size, void *ctx)
>     {
> -      void *node;
> -
> -      node = ralloc_size(ctx, size);
> -      assert(node != NULL);
> -
> -      return node;
> +      return ralloc_new<exec_node>(size, ctx);
>     }
>
> -   /* If the user *does* call delete, that's OK, we will just
> -    * ralloc_free in that case. */
>     static void operator delete(void *node)
>     {
> -      ralloc_free(node);
> +      ralloc_delete(node);
>     }
>
>     exec_node() : next(NULL), prev(NULL)
> @@ -289,19 +282,12 @@ struct exec_list {
>      * easier to just ralloc_free 'ctx' (or any of its ancestors). */
>     static void* operator new(size_t size, void *ctx)
>     {
> -      void *node;
> -
> -      node = ralloc_size(ctx, size);
> -      assert(node != NULL);
> -
> -      return node;
> +      return ralloc_new<exec_list>(size, ctx);
>     }
>
> -   /* If the user *does* call delete, that's OK, we will just
> -    * ralloc_free in that case. */
>     static void operator delete(void *node)
>     {
> -      ralloc_free(node);
> +      ralloc_delete(node);
>     }
>
>     exec_list()
> diff --git a/src/glsl/loop_analysis.h b/src/glsl/loop_analysis.h
> index 769d626..2a10211 100644
> --- a/src/glsl/loop_analysis.h
> +++ b/src/glsl/loop_analysis.h
> @@ -143,19 +143,7 @@ public:
>
>     static void* operator new(size_t size, void *ctx)
>     {
> -      void *lvs = ralloc_size(ctx, size);
> -      assert(lvs != NULL);
> -
> -      ralloc_set_destructor(lvs, (void (*)(void*)) destructor);
> -
> -      return lvs;
> -   }
> -
> -private:
> -   static void
> -   destructor(loop_variable_state *lvs)
> -   {
> -      lvs->~loop_variable_state();
> +      return ralloc_new<loop_variable_state>(size, ctx);
>     }
>  };
>
> diff --git a/src/glsl/ralloc.h b/src/glsl/ralloc.h
> index 67eb938..6df4b89 100644
> --- a/src/glsl/ralloc.h
> +++ b/src/glsl/ralloc.h
> @@ -402,6 +402,50 @@ bool ralloc_vasprintf_append(char **str, const char
> *fmt, va_list args);
>
>  #ifdef __cplusplus
>  } /* end of extern "C" */
> +
> +namespace detail {
> +   template<typename T>
> +   void
> +   ralloc_destroy(void *ptr) {
> +      reinterpret_cast<T *>(ptr)->~T();
> +   }
> +}
> +
> +/**
> + * Helper function intended to be used as implementation of an
> + * overload of the new operator for some specific type \p T.
> + *
> + * The allocated object will be owned by the given ralloc context \p
> + * ctx: The object's destructor will be executed and its memory will
> + * be deallocated automatically when the parent object \p ctx is
> + * released.
> + */
> +template<typename T>
> +void *
> +ralloc_new(size_t size, const void *ctx) {
> +   void *ptr = ralloc_size(ctx, size);
> +   assert(ptr != NULL);
> +   ralloc_set_destructor(ptr, detail::ralloc_destroy<T>);
> +   return ptr;
> +}
> +
> +/**
> + * Helper function that should be called from the delete operator
> + * overload in classes using \c ralloc_new as implementation of the
> + * new operator.
> + *
> + * \c ralloc_free should NOT be called directly by the delete
> + * overload, because it will result in the object destructor being
> + * called twice when the user deletes the object explicitly: first by
> + * the compiler before the delete overload is executed and then by \c
> + * ralloc_free.
> + */
> +inline void
> +ralloc_delete(void *ptr) {
> +   ralloc_set_destructor(ptr, NULL);
> +   ralloc_free(ptr);
> +}
> +
>  #endif
>
>  #endif
> 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);
> @@ -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.


>
>     cfg_t(backend_visitor *v);
> diff --git a/src/mesa/drivers/dri/i965/brw_fs.h
> b/src/mesa/drivers/dri/i965/brw_fs.h
> index cb4ac3b..7cd9fd8 100644
> --- a/src/mesa/drivers/dri/i965/brw_fs.h
> +++ b/src/mesa/drivers/dri/i965/brw_fs.h
> @@ -61,12 +61,7 @@ public:
>      * easier to just ralloc_free 'ctx' (or any of its ancestors). */
>     static void* operator new(size_t size, void *ctx)
>     {
> -      void *node;
> -
> -      node = ralloc_size(ctx, size);
> -      assert(node != NULL);
> -
> -      return node;
> +      return ralloc_new<fs_reg>(size, ctx);
>     }
>
>     void init();
> @@ -124,12 +119,7 @@ class ip_record : public exec_node {
>  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<ip_record>(size, ctx);
>     }
>
>     ip_record(int ip)
> @@ -146,12 +136,7 @@ public:
>      * easier to just ralloc_free 'ctx' (or any of its ancestors). */
>     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_inst>(size, ctx);
>     }
>
>     void init();
> 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.h
> b/src/mesa/drivers/dri/i965/brw_vec4.h
> index f0ab53d..c177d05 100644
> --- a/src/mesa/drivers/dri/i965/brw_vec4.h
> +++ b/src/mesa/drivers/dri/i965/brw_vec4.h
> @@ -122,12 +122,7 @@ public:
>      * easier to just ralloc_free 'ctx' (or any of its ancestors). */
>     static void* operator new(size_t size, void *ctx)
>     {
> -      void *node;
> -
> -      node = ralloc_size(ctx, size);
> -      assert(node != NULL);
> -
> -      return node;
> +      return ralloc_new<src_reg>(size, ctx);
>     }
>
>     void init();
> @@ -160,12 +155,7 @@ public:
>      * easier to just ralloc_free 'ctx' (or any of its ancestors). */
>     static void* operator new(size_t size, void *ctx)
>     {
> -      void *node;
> -
> -      node = ralloc_size(ctx, size);
> -      assert(node != NULL);
> -
> -      return node;
> +      return ralloc_new<dst_reg>(size, ctx);
>     }
>
>     void init();
> @@ -192,12 +182,7 @@ public:
>      * easier to just ralloc_free 'ctx' (or any of its ancestors). */
>     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_instruction>(size, ctx);
>     }
>
>     vec4_instruction(vec4_visitor *v, enum opcode opcode,
> 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).

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.


>
>     vec4_live_variables(vec4_visitor *v, cfg_t *cfg);
> diff --git a/src/mesa/program/ir_to_mesa.cpp
> b/src/mesa/program/ir_to_mesa.cpp
> index 8bc5412..41274a2 100644
> --- a/src/mesa/program/ir_to_mesa.cpp
> +++ b/src/mesa/program/ir_to_mesa.cpp
> @@ -153,12 +153,7 @@ public:
>      * easier to just ralloc_free 'ctx' (or any of its ancestors). */
>     static void* operator new(size_t size, void *ctx)
>     {
> -      void *node;
> -
> -      node = rzalloc_size(ctx, size);
> -      assert(node != NULL);
> -
> -      return node;
> +      return ralloc_new<ir_to_mesa_instruction>(size, ctx);
>     }
>
>     enum prog_opcode op;
> diff --git a/src/mesa/state_tracker/st_glsl_to_tgsi.cpp
> b/src/mesa/state_tracker/st_glsl_to_tgsi.cpp
> index ff1ebd5..cd4802b 100644
> --- a/src/mesa/state_tracker/st_glsl_to_tgsi.cpp
> +++ b/src/mesa/state_tracker/st_glsl_to_tgsi.cpp
> @@ -221,12 +221,7 @@ public:
>      * easier to just ralloc_free 'ctx' (or any of its ancestors). */
>     static void* operator new(size_t size, void *ctx)
>     {
> -      void *node;
> -
> -      node = rzalloc_size(ctx, size);
> -      assert(node != NULL);
> -
> -      return node;
> +      return ralloc_new<glsl_to_tgsi_instruction>(size, ctx);
>     }
>
>     unsigned op;
> --
> 1.8.3.4
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20130917/7665a8b4/attachment-0001.html>


More information about the mesa-dev mailing list