On 15 September 2013 00:10, Francisco Jerez <curroje...@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@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
