[SYCL]Replaced LoadLibraryA() with LoadLibraryEx() #10914
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
Signed-off-by: Kim, Euna <[email protected]>
Signed-off-by: Kim, Euna <[email protected]>
stdale-intel
changed the title
eunakim0103
commented
Aug 22, 2023
sycl/source/detail/windows_pi.cpp
Outdated
| @@ -31,7 +31,8 @@ void *loadOsLibrary(const std::string &LibraryPath) { | |||
| if (!SetDllDirectoryA("")) { | |||
| assert(false && "Failed to update DLL search path"); | |||
| } | |||
| auto Result = (void *)LoadLibraryA(LibraryPath.c_str()); | |||
| // auto Result = (void *)LoadLibraryA(LibraryPath.c_str()); | |||
There was a problem hiding this comment.
Will remove this comment
againull
reviewed
Aug 23, 2023
| @@ -151,25 +151,26 @@ void preloadLibraries() { | |||
| MapT &dllMap = getDllMap(); | |||
|
|
|||
| std::string ocl_path = LibSYCLDir + __SYCL_OPENCL_PLUGIN_NAME; | |||
| dllMap.emplace(ocl_path, LoadLibraryA(ocl_path.c_str())); | |||
| dllMap.emplace(ocl_path, LoadLibraryExA(ocl_path.c_str(), NULL, NULL)); | |||
There was a problem hiding this comment.
I don't mind this change, but it seems like if flags are NULL then LoadLibraryExA is doing the same as LoadLibraryA.
See https://learn.microsoft.com/en-us/windows/win32/api/libloaderapi/nf-libloaderapi-loadlibraryexa
If no flags are specified, the behavior of this function is identical to that of the LoadLibrary function. This parameter can be one of the following values.
againull
approved these changes
Aug 23, 2023
|
My fix passed all tests before and I removed one comment afterwards. The test fail in SYCL E2E on AWS CUDA is not relevant to my code. This PR is ready to merge @intel/llvm-gatekeepers. |
|
@eunakim0103 post-merge nit comment: avoid using |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Replaced LoadLibraryA() with LoadLibraryEx() and added SetDllDirectoryA() to remove the working directory from the standard search path. LoadLibraryExA() was used because "path" is of type c-string.
The libloaderapi.h header defines LoadLibraryEx as an alias which automatically selects the ANSI or Unicode version of this function based on the definition of the UNICODE preprocessor constant.
https://learn.microsoft.com/en-us/windows/win32/api/libloaderapi/nf-libloaderapi-loadlibraryexw
HMODULE LoadLibraryEx(
[in] LPCSTR lpLibFileName, //==> library path as a string
HANDLE hFile, //==> reserved as NULL for future use
[in] DWORD dwFlags //==> dwFlags determine the behavior of LoadLibaryEx(). When it is NULL, it behaves same as LoadLibaryA()
);
https://learn.microsoft.com/en-us/windows/win32/api/libloaderapi/nf-libloaderapi-loadlibraryexa