Skip to content

[SYCL] protect against possible data corruption cause by std::locale side effect #18578

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

Conversation

cperkinsintel
Copy link
Contributor

std::locale::global() in a user app can impact the SYCL library by side-effect, causing numbers (include hex) to be formatted with commas and periods where we might not expect such. In most cases, our use of std streams is for user facing messages. But in some cases they are used in places where unexpected formatting could cause errors. This PR attempts to shore these places up.

…de-effect, causing numbers (include hex) to be formatted with commas and periods where we might not expect such. In most cases, our use of std streams is for user facing messages. But in some cases they are used in places where formatting could cause errors. This PR attempts to shore these places up.

Signed-off-by: Chris Perkins <[email protected]>
@cperkinsintel cperkinsintel marked this pull request as ready for review May 21, 2025 16:11
@cperkinsintel cperkinsintel requested review from a team as code owners May 21, 2025 16:11
@ProGTX
Copy link
Contributor

ProGTX commented May 21, 2025

Seems like this would impact #18561 as well?

@cperkinsintel
Copy link
Contributor Author

@ProGTX - I'm guessing yes. If you output numbers onto a stream, then you are at risk for the localization side effect. Use .imbue() to prevent that.

Copy link
Contributor

@uditagarwal97 uditagarwal97 left a comment

Choose a reason for hiding this comment

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

LGTM. But, can we have a small test to reproduce the original issue and validate the fix?

@cperkinsintel
Copy link
Contributor Author

@uditagarwal97 - I have a related fix with test over here: #18550
For these other cases it's hard to inject the values into the pipelines to reproduce, and our unit test framework doesn't have affordances (neither does syclcompat). I'm not sure how to write a test.

@uditagarwal97
Copy link
Contributor

@uditagarwal97 - I have a related fix with test over here: #18550

The test in #18550 is good enough for me - it demonstrates the original issue with using std::locale

@uditagarwal97 uditagarwal97 merged commit b91d3e2 into intel:sycl May 22, 2025
24 checks passed
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.

3 participants