[SYCL] Don't use legacy ANSI-only Windows API for loading plugins #10943
Conversation
againull
changed the title
|
Failing testing jobs are present in the main branch in post-commit testing. i.e. not caused by this PR. |
| reinterpret_cast<HMODULE>(ExeModuleHandle == Handle ? 0 : Handle), | ||
| reinterpret_cast<LPSTR>(&Path), sizeof(Path)); | ||
| reinterpret_cast<LPWSTR>(&Path), sizeof(Path)); |
There was a problem hiding this comment.
Do you really need this cast?
There was a problem hiding this comment.
Indeed, redundant. Removed.
| (void)Ret; | ||
|
|
||
| BOOL RetCode = PathRemoveFileSpecA(reinterpret_cast<LPSTR>(&Path)); | ||
| assert(RetCode && "PathRemoveFileSpecA failed"); | ||
| BOOL RetCode = PathRemoveFileSpec(reinterpret_cast<LPWSTR>(&Path)); |
|
|
||
| using OSModuleHandle = intptr_t; | ||
| static constexpr OSModuleHandle ExeModuleHandle = -1; | ||
| static OSModuleHandle getOSModuleHandle(const void *VirtAddr) { |
There was a problem hiding this comment.
| static OSModuleHandle getOSModuleHandle(const void *VirtAddr) { | |
| inline OSModuleHandle getOSModuleHandle(const void *VirtAddr) { |
| #include <shlwapi.h> | ||
|
|
||
| using OSModuleHandle = intptr_t; | ||
| static constexpr OSModuleHandle ExeModuleHandle = -1; |
There was a problem hiding this comment.
| static constexpr OSModuleHandle ExeModuleHandle = -1; | |
| constexpr OSModuleHandle ExeModuleHandle = -1; |
or inline conxtexpr if necessary (I think it's inline by default).
| // Implementation of this function is OS specific. Please see windows_pi.cpp and | ||
| // posix_pi.cpp. | ||
| // TODO: refactor code when support matrix for DPCPP changes and <filesystem> is | ||
| // available on all supported systems. | ||
| std::vector<std::tuple<std::string, backend, void *>> | ||
| loadPlugins(const std::vector<std::pair<std::string, backend>> &&PluginNames); |
There was a problem hiding this comment.
Why not in pi.hpp?
There was a problem hiding this comment.
As @bader noticed, currently pi.hpp is in sycl/include/detail directory which means that it is exposed to the user. It seems like some of the functions from there need to be moved to the sycl/source/detail, because no need to expose them to the user, they used only internally.
This function is also used only internally, so I was trying to avoid adding this function to pi.hpp. And since it's only single usage, I didn't create header for it in sycl/source/detail.
sycl/source/detail/windows_pi.cpp
Outdated
| void *Library = getPreloadedPlugin(LibSYCLDir / PluginName.first); | ||
| LoadedPlugins.push_back(std::make_tuple(std::move(PluginName.first), | ||
| std::move(PluginName.second), | ||
| std::move(Library))); |
There was a problem hiding this comment.
| std::move(Library))); | |
| Library)); |
Moving a pointer is overcomplicated a little bit :)
Currently to load PI plugins we use legacy ANSI-only versions of Windows API like GetModuleFileNameA, PathRemoveFileSpecA etc. Problem is that if path containing PI plugins has any non-ANSI symbols then PI plugins are not found and not loaded.
In this patch get rid of legacy API calls, for example, use GetModuleFileName instead of GetModuleFileNameA. GetModuleFileName is an alias which automatically selects the ANSI or Unicode version of this function.
Another difference is that GetModuleFileName and other similar aliases work with wchar_t to be able to handle unicode on Windows (in contrast to legacy GetModuleFileNameA which works with char_t).
So, use std::filesystem:path to work with library paths for convenience (instead of storing path in std::string or std::wstring) because it allows to handle paths without caring about format, can be constructed from string/wstring/.. and can be converted to string/wstring ...
DPCPP is supported on some linux systems where default compiler is gcc 7.5 which doesn't provide
<filesystem>support. On Windows, minimal supported version of Visual Studio is 2019 where<filesystem> is available (supported since Visual Studio 2017 version 15.7).That's why use filesystem::path only on Windows for now, added TODO to do the same on Linux when matrix support changes.