[PATCH vkd3d 1/6] 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 | 12 ++++++++++++ 1 file changed, 12 insertions(+)
diff --git a/libs/vkd3d-shader/hlsl.y b/libs/vkd3d-shader/hlsl.y index 459ad03b..4ec36b76 100644 --- a/libs/vkd3d-shader/hlsl.y +++ b/libs/vkd3d-shader/hlsl.y @@ -1467,6 +1467,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; } @@ -1479,6 +1482,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; } @@ -1521,6 +1527,8 @@ static struct list *declare_vars(struct hlsl_ctx *ctx, struct hlsl_type *basic_t { FIXME("Initializers for non scalar/struct variables not supported yet.\n"); free_parse_initializer(&v->initializer); + if (v->arrays.count) + vkd3d_free(v->arrays.sizes); vkd3d_free(v); continue; } @@ -1528,6 +1536,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; } @@ -1550,6 +1559,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);
Signed-off-by: Giovanni Mascellani gmascellani@codeweavers.com --- I still think that a nearly 200 lines long function with relatively complicated execution flow and such a messy memory management is a bomb ready to go off, and that this patch proves my point.
Il 15/02/22 21:17, Francisco Casas ha scritto:
Signed-off-by: Francisco Casas fcasas@codeweavers.com
libs/vkd3d-shader/hlsl.y | 12 ++++++++++++ 1 file changed, 12 insertions(+)
diff --git a/libs/vkd3d-shader/hlsl.y b/libs/vkd3d-shader/hlsl.y index 459ad03b..4ec36b76 100644 --- a/libs/vkd3d-shader/hlsl.y +++ b/libs/vkd3d-shader/hlsl.y @@ -1467,6 +1467,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; }@@ -1479,6 +1482,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; }@@ -1521,6 +1527,8 @@ static struct list *declare_vars(struct hlsl_ctx *ctx, struct hlsl_type *basic_t { FIXME("Initializers for non scalar/struct variables not supported yet.\n"); free_parse_initializer(&v->initializer);
if (v->arrays.count)vkd3d_free(v->arrays.sizes); vkd3d_free(v); continue; }@@ -1528,6 +1536,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; }@@ -1550,6 +1559,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 Wed, Feb 16, 2022 at 9:47 AM Giovanni Mascellani gmascellani@codeweavers.com wrote:
Signed-off-by: Giovanni Mascellani gmascellani@codeweavers.com
I still think that a nearly 200 lines long function with relatively complicated execution flow and such a messy memory management is a bomb ready to go off, and that this patch proves my point.
Splitting off the function or otherwise simplifying the code are options on the table, sure. It's not that clear to me how to do that in a sensible manner though (not that I tried very hard to think about it).
Hi,
Il 24/02/22 15:07, Matteo Bruni ha scritto:
Splitting off the function or otherwise simplifying the code are options on the table, sure. It's not that clear to me how to do that in a sensible manner though (not that I tried very hard to think about it).
I gave my proposal in https://www.winehq.org/pipermail/wine-devel/2022-January/204393.html. Basically, the idea is to always free the fields of struct parse_variable_def in a single function, appropriately marking those that have reused elsewhere (for example setting them to NULL) to avoid freeing stuff that is still in use.
Giovanni.
-
Francisco Casas -
Giovanni Mascellani -
Matteo Bruni