Skip to content

[SYCL] fix for std::locale::global interfering with allowlist check #18550

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.

Sign up for GitHub

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
merged 3 commits into from
May 28, 2025
Merged

[SYCL] fix for std::locale::global interfering with allowlist check #18550

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
1d95857
fix for std::locale::global interfering with allowlist check
cperkinsintel May 19, 2025
e1a3289
try/catch in event en_US not available
cperkinsintel May 20, 2025
e72cbe8
Windows vs posix
cperkinsintel May 20, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions sycl/source/detail/allowlist.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -428,6 +428,8 @@ void applyAllowList(std::vector<ur_device_handle_t> &UrDevices,
uint32_t DeviceVendorIdUInt =
DeviceImpl.get_info<info::device::vendor_id>();
std::stringstream DeviceVendorIdHexStringStream;
// To avoid commas or other locale-specific modifications, call imbue().
DeviceVendorIdHexStringStream.imbue(std::locale::classic());
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems like we do this for a couple other stringstream objects. Could we make a derived class that automatically sets this in the ctor? Might also want to use it in getUint32PropAsOptStr in sycl/source/detail/program_manager/program_manager.cpp.

Copy link
Contributor Author

@cperkinsintel cperkinsintel May 27, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I considered making a derived class, but decided against it. The other places ( #18578 ) are few and are in both sycl and syclcompat, and just creating a class doesn't mean people will actually use it.
We've already had a "use custom iostream alternate" phase and "remove custom iostream alternate" phase in the years I've been on this project.

DeviceVendorIdHexStringStream << "0x" << std::hex << DeviceVendorIdUInt;
const auto &DeviceVendorIdValue = DeviceVendorIdHexStringStream.str();
DeviceDesc[DeviceVendorIdKeyName] = DeviceVendorIdValue;
Expand Down
34 changes: 34 additions & 0 deletions sycl/unittests/allowlist/DeviceIsAllowed.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,14 @@
//===----------------------------------------------------------------------===//

#include <detail/allowlist.hpp>
#include <sycl/platform.hpp>

#include <gtest/gtest.h>

#ifdef _WIN32
#include <windows.h> // SetEnvironmentVariable
#endif

constexpr char SyclDeviceAllowList[] =
"BackendName:opencl,DeviceType:gpu,DeviceVendorId:0x8086,DriverVersion:{{("
"19\\.(4[3-9]|[5-9]\\d)\\..*)|([2-9][0-9]\\.\\d+\\..*)|(\\d+\\.\\d+\\."
Expand Down Expand Up @@ -85,6 +90,35 @@ TEST(DeviceIsAllowedTests, CheckSupportedOpenCLGPUDeviceIsAllowed) {
EXPECT_EQ(Actual, true);
}

TEST(DeviceIsAllowedTests, CheckLocalizationDoesNotImpact) {
// The localization can affect std::stringstream output.
// We want to make sure that DeviceVenderId doesn't have a comma
// inserted (ie "0x8,086" ), which will break the platform retrieval.

try {
auto previous = std::locale::global(std::locale("en_US.UTF-8"));
#ifdef _WIN32
SetEnvironmentVariableA("SYCL_DEVICE_ALLOWLIST", SyclDeviceAllowList);
#else
setenv("SYCL_DEVICE_ALLOWLIST", SyclDeviceAllowList, 1);
#endif

auto post_platforms = sycl::platform::get_platforms();
std::locale::global(previous);
#ifdef _WIN32
SetEnvironmentVariableA("SYCL_DEVICE_ALLOWLIST", nullptr);
#else
unsetenv("SYCL_DEVICE_ALLOWLIST");
#endif

EXPECT_NE(size_t{0}, post_platforms.size());
} catch (...) {
// It is possible that the en_US locale is not available.
// In this case, we just skip the test.
GTEST_SKIP() << "Locale en_US.UTF-8 not available.";
}
}

TEST(DeviceIsAllowedTests, CheckSupportedOpenCLCPUDeviceIsAllowed) {
bool Actual = sycl::detail::deviceIsAllowed(
OpenCLCPUDeviceDesc, sycl::detail::parseAllowList(SyclDeviceAllowList));
Expand Down