Skip to content
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

conversions: fix undefined behaviour in DataInfoSpec #1786

Merged
merged 2 commits into from
Jul 14, 2023

Conversation

svenvh
Copy link
Member

@svenvh svenvh commented Jul 13, 2023

For conversion from integers to floating point, the DataInfoSpec constructor tries to convert CL_FLT_MAX to an integer. The float value cannot be represented as an integer, which is undefined behaviour.

Fix by only doing this conversion when InType is a floating point value.

The refactoring of the conversions test dropped the workaround added by 59a1204 ("Fix for test_conversions failure with Clang build on Linux #1057 (#1062)", 2021-05-11).

While at it, prefer static_cast for the conversions.

For conversion from integers to float, the DataInfoSpec constructor
tries to convert `CL_FLT_MAX` to an integer.  The float value cannot
be represented as an integer, which is undefined behaviour.

Fix by only doing this conversion when `InType` is a floating point
value.

While at it, use `static_cast` for the conversions.

Signed-off-by: Sven van Haastregt <sven.vanhaastregt@arm.com>
The refactoring of the conversions test dropped the workaround added
by 59a1204 ("Fix for test_conversions failure with Clang build on
Linux KhronosGroup#1057 (KhronosGroup#1062)", 2021-05-11).

Signed-off-by: Sven van Haastregt <sven.vanhaastregt@arm.com>
Copy link
Contributor

@alan-baker alan-baker left a comment

Choose a reason for hiding this comment

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

Tested this internally and it fixed the regressions I was seeing.

@svenvh svenvh merged commit 19bddc9 into KhronosGroup:main Jul 14, 2023
6 checks passed
@svenvh svenvh deleted the conv-ub branch July 14, 2023 14:50
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.

2 participants