<div dir="ltr">On 15 September 2013 00:10, Francisco Jerez <<a href="mailto:currojerez@riseup.net" target="_blank">currojerez@riseup.net</a>> wrote: <div class="gmail_extra"><div class="gmail_quote"> <blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">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); } </blockquote><div> </div><div>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. </div><div>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. </div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> 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); } </blockquote><div> </div><div>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. </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> 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); } </blockquote><div> </div><div>The exact same situation exists here (yay code duplication). </div><div>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. </div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> 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 <a href="mailto:mesa-dev@lists.freedesktop.org">mesa-dev@lists.freedesktop.org</a> <a href="http://lists.freedesktop.org/mailman/listinfo/mesa-dev" target="_blank">http://lists.freedesktop.org/mailman/listinfo/mesa-dev</a> </blockquote></div> </div></div>