[PATCH vkd3d v2 06/10] vkd3d-shader/hlsl: Properly free parse_variable_def memory in declare_vars().
Signed-off-by: Francisco Casas fcasas@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);
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@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);
On 1/11/22 10:42, Giovanni Mascellani wrote:
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.
I'm afraid I don't agree in this case. The principle is fine in general; the problem here is that struct parse_variable_def isn't semantically an object; it's a collection of information that's grouped into a struct only because yacc requires it. The fact that the code ends up using it in a relatively piecewise fashion is a good indication of this.
Hi,
On 11/01/22 18:34, Zebediah Figura (she/her) wrote:
I'm afraid I don't agree in this case. The principle is fine in general; the problem here is that struct parse_variable_def isn't semantically an object; it's a collection of information that's grouped into a struct only because yacc requires it. The fact that the code ends up using it in a relatively piecewise fashion is a good indication of this.
It is of course a completely legitimate point of view, but I think it's more prone to mistakes: for instance, the many missing calls to free() that Francisco is fixing.
Also, if we insist in keeping this point of view, then we should remove free_parse_variable_def(), because that's precisely one of the gadgets that indicates that the structure is meant to be an encapsulated object.
Giovanni.
On 1/11/22 12:23, Giovanni Mascellani wrote:
Hi,
On 11/01/22 18:34, Zebediah Figura (she/her) wrote:
I'm afraid I don't agree in this case. The principle is fine in general; the problem here is that struct parse_variable_def isn't semantically an object; it's a collection of information that's grouped into a struct only because yacc requires it. The fact that the code ends up using it in a relatively piecewise fashion is a good indication of this.
It is of course a completely legitimate point of view, but I think it's more prone to mistakes: for instance, the many missing calls to free() that Francisco is fixing.
I'm not convinced that treating it as an object would really help here. Leaks are just hard.
Also, if we insist in keeping this point of view, then we should remove free_parse_variable_def(), because that's precisely one of the gadgets that indicates that the structure is meant to be an encapsulated object.
I don't see anything wrong with having helpers like that.
On Tue, Jan 11, 2022 at 7:44 PM Zebediah Figura (she/her) zfigura@codeweavers.com wrote:
On 1/11/22 12:23, Giovanni Mascellani wrote:
Hi,
On 11/01/22 18:34, Zebediah Figura (she/her) wrote:
I'm afraid I don't agree in this case. The principle is fine in general; the problem here is that struct parse_variable_def isn't semantically an object; it's a collection of information that's grouped into a struct only because yacc requires it. The fact that the code ends up using it in a relatively piecewise fashion is a good indication of this.
It is of course a completely legitimate point of view, but I think it's more prone to mistakes: for instance, the many missing calls to free() that Francisco is fixing.
I'm not convinced that treating it as an object would really help here. Leaks are just hard.
Also, if we insist in keeping this point of view, then we should remove free_parse_variable_def(), because that's precisely one of the gadgets that indicates that the structure is meant to be an encapsulated object.
I don't see anything wrong with having helpers like that.
I tend to lean towards Zeb's view on both points.
FWIW I agree with Giovanni in that it is good for the code maintainability that the memory has a clear ownership, and I think his proposal is good.
However, I accept that in this particular case it may be better to move forward and maybe consider it as a future batch of patches if we find more uses for parse_variable_def.
January 11, 2022 2:34 PM, "Zebediah Figura (she/her)" zfigura@codeweavers.com wrote:
On 1/11/22 10:42, Giovanni Mascellani wrote:
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.
I'm afraid I don't agree in this case. The principle is fine in general; the problem here is that struct parse_variable_def isn't semantically an object; it's a collection of information that's grouped into a struct only because yacc requires it. The fact that the code ends up using it in a relatively piecewise fashion is a good indication of this.
-
Francisco Casas -
Giovanni Mascellani -
Matteo Bruni -
Zebediah Figura (she/her)