[PATCH v2 0/3] MR615: vkd3d-shader: Fix a couple of memory leaks.

[Giovanni Mascellani (＠giomasce)]

--
  v2: vkd3d-shader/spirv: Free binary SPIR-V code (Valgrind).
      vkd3d-compiler: Free compilation options (Valgrind).

https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/615

[PATCH v2 1/3] vkd3d-compiler: Exit the main function through a common code path.

[Giovanni Mascellani]

From: Giovanni Mascellani

---
 programs/vkd3d-compiler/main.c | 22 +++++++++++++---------
 1 file changed, 13 insertions(+), 9 deletions(-)

diff --git a/programs/vkd3d-compiler/main.c b/programs/vkd3d-compiler/main.c
index 4aa368b04..f55eff6a9 100644
--- a/programs/vkd3d-compiler/main.c
+++ b/programs/vkd3d-compiler/main.c
@@ -717,12 +717,13 @@ int main(int argc, char **argv)
     int ret;

if (!parse_command_line(argc, argv, &options))
-        return 1;
+        goto done;

if (options.print_help)
     {
         print_usage(argv[0]);
-        return 0;
+        fail = 0;
+        goto done;
     }

if (options.print_version)
@@ -730,13 +731,15 @@ int main(int argc, char **argv)
         const char *version = vkd3d_shader_get_version(NULL, NULL);

fprintf(stdout, "vkd3d shader compiler version " PACKAGE_VERSION " using %s\n", version);
-        return 0;
+        fail = 0;
+        goto done;
     }

if (options.print_source_types)
     {
         print_source_types();
-        return 0;
+        fail = 0;
+        goto done;
     }

if (options.print_target_types)
@@ -744,11 +747,12 @@ int main(int argc, char **argv)
         if (options.source_type == VKD3D_SHADER_SOURCE_NONE)
         {
             fprintf(stderr, "--print-target-types requires an explicitly specified source type.\n");
-            return 1;
+            goto done;
         }

print_target_types(options.source_type);
-        return 0;
+        fail = 0;
+        goto done;
     }

if (!(input = open_input(options.filename, &close_input)))
@@ -774,7 +778,7 @@ int main(int argc, char **argv)
             if (token == TAG_DXBC)
             {
                 if (!parse_dxbc_source_type(&info.source, &options.source_type))
-                    return 1;
+                    goto done;
             }
             else if ((token & 0xfffe0000) == 0xfffe0000)
                 options.source_type = VKD3D_SHADER_SOURCE_D3D_BYTECODE;
@@ -791,13 +795,13 @@ int main(int argc, char **argv)
         fprintf(stderr, "Target type '%s' is invalid for source type '%s'.\n",
                 get_target_type_info(options.target_type)->name,
                 get_source_type_info(options.source_type)->name);
-        return 1;
+        goto done;
     }

if (!options.preprocess_only && options.source_type == VKD3D_SHADER_SOURCE_HLSL && !options.profile)
     {
         fprintf(stderr, "You need to specify a profile when compiling from HLSL source.\n");
-        return 1;
+        goto done;
     }

if (!options.filename && get_source_type_info(options.source_type)->is_binary && isatty(fileno(input)))

[PATCH v2 2/3] vkd3d-compiler: Free compilation options (Valgrind).

[Giovanni Mascellani]

From: Giovanni Mascellani

---
 programs/vkd3d-compiler/main.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/programs/vkd3d-compiler/main.c b/programs/vkd3d-compiler/main.c
index f55eff6a9..3fa54aada 100644
--- a/programs/vkd3d-compiler/main.c
+++ b/programs/vkd3d-compiler/main.c
@@ -879,6 +879,7 @@ int main(int argc, char **argv)
     fail = 0;
     vkd3d_shader_free_shader_code(&output_code);
 done:
+    free(options.compile_options);
     if (close_output)
         fclose(output);
     if (close_input)

[PATCH v2 3/3] vkd3d-shader/spirv: Free binary SPIR-V code (Valgrind).

[Giovanni Mascellani]

From: Giovanni Mascellani

---
 libs/vkd3d-shader/spirv.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/libs/vkd3d-shader/spirv.c b/libs/vkd3d-shader/spirv.c
index 298ad31d9..70046d980 100644
--- a/libs/vkd3d-shader/spirv.c
+++ b/libs/vkd3d-shader/spirv.c
@@ -9981,6 +9981,7 @@ static int spirv_compiler_generate_spirv(struct spirv_compiler *compiler,
         enum vkd3d_shader_spirv_environment environment = spirv_compiler_get_target_environment(compiler);
         if (vkd3d_spirv_binary_to_text(spirv, environment, compiler->formatting, &text) != VKD3D_OK)
             return VKD3D_ERROR;
+        vkd3d_shader_free_shader_code(spirv);
         *spirv = text;
     }

[Zebediah Figura (＠zfigura)]

I don't think we care about memory-leaks in vkd3d-compiler, inasmuch as it is a short-lived program?

[Francisco Casas (＠fcasas)]

On Tue Jan 30 18:01:52 2024 +0000, Zebediah Figura wrote:
> I don't think we care about memory-leaks in vkd3d-compiler, inasmuch as
> it is a short-lived program?

IMO, cleaning memory leaks has the benefit of clearing valgrind output, which I think can make real errors more detectable.

[Francisco Casas (＠fcasas)]

Francisco Casas (@fcasas) commented about programs/vkd3d-compiler/main.c:
>  int ret;
>
>  if (!parse_command_line(argc, argv, &options))
> -
>    return 1;
> -
>    goto done;
>
> if (options.print_help)
>    {
>        print_usage(argv[0]);
> -
>    return 0;
> -
>    fail = 0;

It may be just me, but I think that it is more intuitive to set `fail = 1` on errors rather than `fail = 0` on regular code paths.

[PATCH v2 0/3] MR615: vkd3d-shader: Fix a couple of memory leaks. - approved

[Giovanni Mascellani (＠giomasce)]

This merge request was approved by Giovanni Mascellani.

[Giovanni Mascellani (＠giomasce)]

> IMO, cleaning memory leaks has the benefit of clearing valgrind output, which I think can make real errors more detectable.

Yeah, that's precisely how I got there.

Also, it seems that somebody cared about memory leaks at some point, because memory is freed on many code paths (both exceptional and not). But others are missing.

[Giovanni Mascellani (＠giomasce)]

On Tue Jan 30 19:05:32 2024 +0000, Francisco Casas wrote:
> It may be just me, but I think that it is more intuitive to set `fail =
> 1` on errors rather than `fail = 0` on regular code paths.

I mainly adopted the same style already in the code.

[PATCH v2 0/3] MR615: vkd3d-shader: Fix a couple of memory leaks. - approved

[Francisco Casas (＠fcasas)]

This merge request was approved by Francisco Casas.

[PATCH v2 0/3] MR615: vkd3d-shader: Fix a couple of memory leaks. - approved

[Henri Verbeet (＠hverbeet)]

This merge request was approved by Henri Verbeet.

[Henri Verbeet (＠hverbeet)]

One (perhaps obvious) thing to point out here is that main() is starting to become large enough that moving some parts into separate functions would likely simplify error handling a bit.