Fix samples memory leak and cleanup issues #137
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Fix memory leaks and cleanup logic in the following sample programs: - samples/core/binaries - samples/core/blur - samples/core/reduce - samples/core/saxpy - samples/core/multi-device - samples/extensions/khr/externalmemory Also fix minor logic and error-handling inconsistencies such as: - Ensuring proper release of cl_event objects - Correct ordering of Vulkan cleanup calls - Improved version string checks and CLI option allocation Signed-off-by: Xin Jin <xin.jin@arm.com>
|
|
Contributor
|
I'll do a more thorough review once the CLA is signed, but I can confirm that I do not see any OpenCL object leaks after these changes (verified with the OpenCL Intercept Layer and LeakChecking). Thanks! |
Contributor
Author
|
thanks @bashbaug , I just signed the CLA. |
Fix a use-after-free warning reported by GCC:
error: pointer 'dev_version' may be used after 'free'
else if (opencl_version_contains(dev_version, "2."))
Previously, dev_version was freed immediately after checking for
OpenCL 1.0/1.1, but later reused for determining whether to add -cl-std
compiler options. This triggered -Werror=use-after-free under GCC with
strict diagnostics.
This patch:
- Defers free(dev_version) to a new 'ver:' cleanup label to avoid
premature free.
- Replaces raw malloc/free with MEM_CHECK for safety and consistency.
- Adds snprintf-based -cl-std option handling with bounds checking.
- Moves compiler_options logic earlier to avoid stale pointer usage.
- Removes old compiler_options block near clBuildProgram.
The refactoring ensures correctness, eliminates UB, and prepares for
cleaner version-based behavior in the future.
Signed-off-by: Xin Jin <xin.jin@arm.com>
To address review comment Signed-off-by: Xin Jin <xin.jin@arm.com>
bashbaug
approved these changes
Aug 4, 2025
Contributor
bashbaug
left a comment
bashbaug
left a comment
There was a problem hiding this comment.
Thanks! LGTM, aside from one nitpick.
Calling free() on a NULL pointer is well-defined and has no effect. This simplifies the code by removing an unnecessary conditional. Signed-off-by: Xin Jin <xin.jin@arm.com>
Contributor
|
I filed an issue for the macOS CI failures, which seem unrelated to your changes: #144. I'm going to merge these changes as-is. Thank you again for the fixes! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fix memory leaks and cleanup logic in the following sample programs:
Also fix minor logic and error-handling inconsistencies such as: