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

isNan and isInf in Constant.cpp produce wrong results on big-endian systems #2802

Closed
mhillenbrand opened this issue Nov 9, 2021 · 3 comments · Fixed by #2803 or #2814
Closed

isNan and isInf in Constant.cpp produce wrong results on big-endian systems #2802

mhillenbrand opened this issue Nov 9, 2021 · 3 comments · Fixed by #2803 or #2814

Comments

@mhillenbrand
Copy link
Contributor

Both attempt to decode a double into high and low 32-bit ints via a union:

typedef union {
double d;
int i[2];
} DoubleIntUnion;

On little-endian systems that works as intended. On big-endian systems, such as s390x, the 32-bit ints end up in reversed order and thus the two functions fail to detect Infs or NaNs.

Reproduction: glslangValidator -C -i Test/constFold.frag and compare out10 and out11 to expected values -- on s390x, they are all 0 since none of the NaN or Inf conditions are detected correctly.

mhillenbrand added a commit to mhillenbrand/glslang that referenced this issue Nov 9, 2021
There were two implementations of isInf() and isNan(), in Constant.cpp
and in intermOut.cpp. The former only works on little-endian systems,
the latter is a wrapper for library functions and works regardless of
endianness. Move the second version into Common.h and adopt it in both
places. Thereby avoid the duplication and fix for big-endian systems.

On s390x, this fixes the test case
Glsl/CompileToAstTest.FromFile/constFold_frag.

TODO regression test on x86.

Fixes KhronosGroup#2802
mhillenbrand added a commit to mhillenbrand/glslang that referenced this issue Nov 9, 2021
There were two implementations of isInf() and isNan(), in Constant.cpp
and in intermOut.cpp. The former only works on little-endian systems,
the latter is a wrapper for library functions and works regardless of
endianness. Move the second version into Common.h and adopt it in both
places. Thereby avoid the duplication and fix for big-endian systems.

On s390x, this fixes the test case
Glsl/CompileToAstTest.FromFile/constFold_frag.

Fixes KhronosGroup#2802
@dneto0
Copy link
Contributor

dneto0 commented Nov 9, 2021

I believe that the use of the union exercised undefined behaviour. It's a type-based alias analysis violation.
C++14 3.10 "Lvalues and rvalues" says what types can be loaded from memory locations, depending on what types were used to write those locations.

Thanks for fixing this!

@greg-lunarg
Copy link
Contributor

Had to revert #2803 with #2809; see issue #2808.

@greg-lunarg greg-lunarg reopened this Nov 10, 2021
mhillenbrand added a commit to mhillenbrand/glslang that referenced this issue Nov 11, 2021
There were two implementations of isInf() and isNan(), in Constant.cpp
and in intermOut.cpp. The former only works on little-endian systems,
the latter is a wrapper for library functions and works regardless of
endianness. Move the second version into Common.h and adopt it in both
places. Thereby avoid the duplication and fix for big-endian systems.

On s390x, this fixes the test case
Glsl/CompileToAstTest.FromFile/constFold_frag.

TODO regression test on x86.

TODO build-test on Windows!!

Fixes KhronosGroup#2802
@mhillenbrand
Copy link
Contributor Author

mhillenbrand commented Nov 11, 2021

In my patch, I missed moving the #include <cfloat> that is required on Windows. Fixing and testing...
https://docs.microsoft.com/en-us/cpp/c-runtime-library/reference/fpclass-fpclassf?view=msvc-170

mhillenbrand added a commit to mhillenbrand/glslang that referenced this issue Nov 11, 2021
…ing (updated)

There were two implementations of isInf() and isNan(), in Constant.cpp
and in intermOut.cpp. The former only works on little-endian systems,
the latter is a wrapper for library functions and works regardless of
endianness. Move the second version into Common.h and adopt it in both
places. Thereby avoid the duplication and fix for big-endian systems.

A previous commit with the same intent and purpose had missed a required
header for builds on Windows.

On s390x, this fixes the test case
Glsl/CompileToAstTest.FromFile/constFold_frag.

Fixes KhronosGroup#2802
mhillenbrand added a commit to mhillenbrand/glslang that referenced this issue Nov 12, 2021
There were two implementations of isInf() and isNan(), in Constant.cpp
and in intermOut.cpp. The former only works on little-endian systems,
the latter is a wrapper for library functions and works regardless of
endianness. Move the second version into Common.h and adopt it in both
places. Thereby avoid the duplication and fix for big-endian systems.

On s390x, this fixes the test case
Glsl/CompileToAstTest.FromFile/constFold_frag.

Fixes KhronosGroup#2802
mhillenbrand added a commit to mhillenbrand/glslang that referenced this issue Nov 12, 2021
…ing (updated)

There were two implementations of isInf() and isNan(), in Constant.cpp
and in intermOut.cpp. The former only works on little-endian systems,
the latter is a wrapper for library functions and works regardless of
endianness. Move the second version into Common.h and adopt it in both
places. Thereby avoid the duplication and fix for big-endian systems.

A previous commit with the same intent and purpose had missed a required
header for builds on Windows.

On s390x, this fixes the test case
Glsl/CompileToAstTest.FromFile/constFold_frag.

Fixes KhronosGroup#2802
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants