[SYCL][PostLink] Fix Coverity issues in SYCLPostLink.cpp#21573
[SYCL][PostLink] Fix Coverity issues in SYCLPostLink.cpp#21573hchilama wants to merge 19 commits intointel:syclfrom
Conversation
This commit addresses critical Coverity static analysis issues:
1. Fixed memory leak in performPostLinkProcessing:
- Removed unnecessary MD.release() call that was leaking ModuleDesc
- The unique_ptr now properly manages memory cleanup
2. Fixed uninitialized data in saveModuleDesc:
- Initialize SplitModule with {} for proper value-initialization
These changes prevent potential memory leaks and undefined behavior.
Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
hchilama
force-pushed
the
fix/sycl-postlink-coverity-issues
branch
from
0b0ff45 to
0afc179
Compare
YuriPlyakhin
left a comment
YuriPlyakhin
left a comment
There was a problem hiding this comment.
I'm wondering was any AI tool used to prepare this PR?
|
Yes @YuriPlyakhin I just started using Claude and thought to try it on Coverity issues and it worked. |
@hchilama , could you, please, follow https://llvm.org/docs/AIToolPolicy.html |
| if (!SplitImageOrErr) | ||
| return SplitImageOrErr.takeError(); | ||
|
|
||
| MD.release(); |
There was a problem hiding this comment.
Can agree. release() function just returns the pointer so that a caller is responsible to destroy the resource which is obviously not happen here. The best solution would be to do:
MD.reset();This way unique_ptr would destroy the object.
In overall, this is done this way because IRs are fat and we want to release used RAM as soon as possible.
There was a problem hiding this comment.
I thought deleting the object when the pointer goes out of scope would be the safest if there is any early returns. But sure I can make sure the pointer to release sooner with reset() .
| llvm::sycl_post_link::saveModuleDesc(module_split::ModuleDesc &MD, | ||
| std::string Prefix, bool OutputAssembly) { | ||
| module_split::SplitModule SM; | ||
| module_split::SplitModule SM{}; |
There was a problem hiding this comment.
Can't agree. Coverity complains about the problem with NumBuckets being uninitialized in destructor which is true. I am not sure that your change resolves the problem.
It looks like the problem was introduced recently while DenseMap has been changing over last several months. It can be tracked in git blame or here llvm/llvm-project#168255
I think this is better to raise this concern to appropriate folks in LLVM by github issue or, probably, email to some author that has been doing that work recently.
There was a problem hiding this comment.
Yes, may be we can do that. I usually follow the Coverity BKM rules and make this as a "False Positive" in the scan. while the Coverity flags this as a uninitialized, but the members are safely initialized by the constructor. If that is ok, I can update the comments in the scan.
Thanks @YuriPlyakhin . I reviewed the document and edit the commit. Please take a look. |
There was a problem hiding this comment.
I'm ok with the fix, but I'll let @maksimsab to approve, as he was working on this code.
please, see below
| return SplitImageOrErr.takeError(); | ||
|
|
||
| MD.release(); | ||
| Md.reset(); |
3 participants
The below summary is generated by Claud but I followed the human in the middle policy to review each and every step that AI made.
Summary
This PR fixes critical Coverity static analysis issues in
SYCLPostLink.cpp:Issues Fixed
Memory Leak - Fixed resource leak in
performPostLinkProcessingMD.release()call at line 184 that was leakingModuleDescpointerunique_ptrnow properly manages memory cleanup automaticallyUninitialized Properties - Fixed uninitialized data access in
saveModuleDescSplitModulewith{}for proper value-initializationChanges Made
lib/SYCLPostLink/SYCLPostLink.cpp:SM;toSM{};for explicit initializationMD.release();that was causing memory leakTest Plan
python buildbot/compile.pyAssisted-by: Claude AI