Skip to content

[SYCL] Check if PluginInitializeFunction is NULL before accessing it. #963

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

Merged

Conversation

garimagu
Copy link
Contributor

Signed-off-by: Garima Gupta [email protected]

@garimagu garimagu requested a review from romanovvlad December 23, 2019 18:13
@@ -79,6 +79,9 @@ bool bindPlugin(void *Library) {

decltype(::piPluginInit) *PluginInitializeFunction = (decltype(
&::piPluginInit))(getOsLibraryFuncAddress(Library, "piPluginInit"));
if (PluginInitializeFunction == nullptr)
return false;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you think about returning some error code instead of just boolean from this function? By doing so, you can report better messages to the user (which might be helpful during debugging).

Right now I see at least two possible breakage points here:

  • failed to get piPluginInit address, i.e. PluginIintializeFunction == nullptr
    PluginInitializeFunction returned non-success error code, i.e. err != PI_SUCCESS

P.S. there is even TODO for this at line 92

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right. But this is needed at multiple places.
Whole PI functionality currently is not debugging friendly.
Is it ok if it is taken as a separate patch? @smaslov-intel and @romanovvlad have been suggesting a similar change.

@garimagu
Copy link
Contributor Author

@smaslov-intel please review.

@romanovvlad romanovvlad merged commit 25c9790 into intel:sycl Dec 24, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants