[PATCH v2 0/3] MR615: vkd3d-shader: Fix a couple of memory leaks.
-- 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
From: Giovanni Mascellani <gmascellani(a)codeweavers.com> --- 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))) -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/615
From: Giovanni Mascellani <gmascellani(a)codeweavers.com> --- 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) -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/615
From: Giovanni Mascellani <gmascellani(a)codeweavers.com> --- 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; } -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/615
I don't think we care about memory-leaks in vkd3d-compiler, inasmuch as it is a short-lived program? -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/615#note_59512
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.
-- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/615#note_59516
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. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/615#note_59518
This merge request was approved by Giovanni Mascellani. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/615
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. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/615#note_59561
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.
-- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/615#note_59562
This merge request was approved by Francisco Casas. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/615
This merge request was approved by Henri Verbeet. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/615
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. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/615#note_59601
participants (5)
-
Francisco Casas (＠fcasas) -
Giovanni Mascellani
-
Giovanni Mascellani (＠giomasce)
-
Henri Verbeet (＠hverbeet)
-
Zebediah Figura (＠zfigura)