Fix a leak in pi_unified_runtime.cpp. #12589
Conversation
yingcong-wu
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
yingcong-wu
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
| @@ -1349,6 +1349,7 @@ __SYCL_EXPORT pi_result piPluginInit(pi_plugin *PluginInit) { | |||
| } | |||
|
|
|||
| HANDLE_ERRORS(urLoaderInit(0, LoaderConfig)); | |||
| HANDLE_ERRORS(urLoaderConfigRelease(LoaderConfig)); | |||
There was a problem hiding this comment.
This is an improvement, but if urLoaderConfigEnableLayer or urLoaderInit fail I think we still leak. Do we need to fix this RAII-style instead?
There was a problem hiding this comment.
I did recently add RAII wrappers to the UR CTS, there's deffo a case for making those first class citizens so they can be reused where needed.
There was a problem hiding this comment.
I will look into the RAII wrappers. Hi @kbenzie , do you have some links to some documents/commit that I can start with? Thanks.
Maybe I misunderstand your comment. Is there RAII wrapper for LoaderConfig type? Or do you mean you plan to add them later?
There was a problem hiding this comment.
Hi @kbenzie , did UR already provide such RAII wrappers or did you mean that you plan to add such wrappers to UR?
There was a problem hiding this comment.
We do not currently provide public RAII wrappers, I've created oneapi-src/unified-runtime#1334 to address this.
I don't think this should block this PR.
|
@ldrumm could you please review the answers & approve if no objections? Would be great to get this fix merged since it causes failures of 9 SYCL tests. |
|
Hi @kbenzie, if the RAII wrapper does not block this PR, could you please help land this patch? Thank you. |
|
@intel/llvm-gatekeepers please merge |
LoaderConfigis created and stored in a local pointer and never released when done using, causing it to be leaked.This patch releases the
LoaderConfigwhen finished using it.