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 <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/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.
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
