[PATCH vkd3d v2 06/10] vkd3d-shader/hlsl: Properly free parse_variable_def memory in declare_vars().
Giovanni Mascellani
gmascellani at codeweavers.com
Tue Jan 11 10:42:15 CST 2022
Hi,
my understanding is the aim of this patch is to fix memory leaks that
happen in error branches, taking care of not freeing stuff a reference
to which was stored somewhere else. I don't like how this is achieved.
First, I think it is useful to have just one clearly identified
destructor for each type (struct parse_variable_def in this case).
Destroying objects in many different places makes it harder to maintain
the code when new fields are added, because it is easy to forget to
destroy them somewhere (possibly what already happened here). So I think
that objects of type struct parse_variable_def should always be
destroyed in free_parse_variable_def(), as you did in your first iteration.
The problem with that if some pointer was copied somewhere else, we
mustn't free it any more. In other words, we have to make pointer
ownership more clear. To do that, I would suggest to set pointers to
NULL once ownership has been transferred somewhere else. So, for
example, after a successful hlsl_new_var() you'd set v->name and
v->semantic.name to NULL. In the destructor, vkd3d_free() is already
able to deal with NULL pointers, ignoring them (things are a little bit
more complicated with the "initializer" field, which is a sub-object and
not a pointer; either we make it a pointer, or you have to set to NULL
its fields after ownership transfer; I don't like the latter because it
is another encapsulation violation, that is likely to eventually cause
problems analogous to the one you're trying to solve).
Be aware that there is a non-negligible probability that somebody with
higher sign-off power than me might disagree with me.
Giovanni.
On 10/01/22 20:33, Francisco Casas wrote:
> Signed-off-by: Francisco Casas <fcasas at codeweavers.com>
> ---
> libs/vkd3d-shader/hlsl.y | 10 ++++++++++
> 1 file changed, 10 insertions(+)
>
> diff --git a/libs/vkd3d-shader/hlsl.y b/libs/vkd3d-shader/hlsl.y
> index 636882c4..988e0743 100644
> --- a/libs/vkd3d-shader/hlsl.y
> +++ b/libs/vkd3d-shader/hlsl.y
> @@ -1424,6 +1424,9 @@ static struct list *declare_vars(struct hlsl_ctx *ctx, struct hlsl_type *basic_t
> hlsl_error(ctx, &v->loc, VKD3D_SHADER_ERROR_HLSL_MISSING_INITIALIZER,
> "Const variable \"%s\" is missing an initializer.", var->name);
> hlsl_free_var(var);
> + free_parse_initializer(&v->initializer);
> + if (v->arrays.count)
> + vkd3d_free(v->arrays.sizes);
> vkd3d_free(v);
> continue;
> }
> @@ -1436,6 +1439,9 @@ static struct list *declare_vars(struct hlsl_ctx *ctx, struct hlsl_type *basic_t
> "Variable \"%s\" was already declared in this scope.", var->name);
> hlsl_note(ctx, &old->loc, VKD3D_SHADER_LOG_ERROR, "\"%s\" was previously declared here.", old->name);
> hlsl_free_var(var);
> + free_parse_initializer(&v->initializer);
> + if (v->arrays.count)
> + vkd3d_free(v->arrays.sizes);
> vkd3d_free(v);
> continue;
> }
> @@ -1485,6 +1491,7 @@ static struct list *declare_vars(struct hlsl_ctx *ctx, struct hlsl_type *basic_t
> {
> hlsl_fixme(ctx, &v->loc, "Array initializer.");
> free_parse_initializer(&v->initializer);
> + vkd3d_free(v->arrays.sizes);
> vkd3d_free(v);
> continue;
> }
> @@ -1507,6 +1514,9 @@ static struct list *declare_vars(struct hlsl_ctx *ctx, struct hlsl_type *basic_t
> list_move_tail(statements_list, v->initializer.instrs);
> vkd3d_free(v->initializer.instrs);
> }
> +
> + if (v->arrays.count)
> + vkd3d_free(v->arrays.sizes);
> vkd3d_free(v);
> }
> vkd3d_free(var_list);
More information about the wine-devel
mailing list