-
Notifications
You must be signed in to change notification settings - Fork 734
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix a leak in pi_unified_runtime.cpp. #12589
Conversation
@@ -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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 |
LoaderConfig
is created and stored in a local pointer and never released when done using, causing it to be leaked.This patch releases the
LoaderConfig
when finished using it.