KhronosGroup / KhronosGroup/glslang

Glsl/CompileToAstTest.FromFile/array_frag fails on s390x because error-message depends on endianness

Open
#2,797 1 comment 1 reaction 0 assignees View on GitHub
bug Infrastructure
Dominant language
C++
Stars
3.6k
Forks
989
Avg merge
1d 2h
Merged PRs (30d)
31

Description

The test case Glsl/CompileToAstTest.FromFile/array_frag (i.e., compile `Test/array.frag`) fails on s390x because of a mismatch in one of the expected error message. The mismatch is caused by undefined behavior in `TParseContext::handleBracketDereference` which in practice translates to behavior that depends on endianness. As a result, the error message on big-endian s390x differs from the reference output (`Test/baseResults/array.frag.out`) from a little-endian system.

fwict, the test failure is a false positive. Since I am currently looking into other big-endian issues as well (e.g., #202), I would prefer to get the test suite as clean as possible. Error message, reproduction, and my analysis below:

From glslangtests's output:
```
...
[ RUN ] Glsl/CompileToAstTest.FromFile/array_frag
glslang/gtests/TestFixture.h:148: Failure
Expected equality of these values:
...
With diff:
@@ -12,5 +12,5 @@
ERROR: 0:56: '=' : cannot convert from ' const 2-element array of int' to ' temp 3-element array of int'
ERROR: 0:57: '[]' : scalar integer expression required·
-ERROR: 0:57: '[' : index out of range '-858993459'
+ERROR: 0:57: '[' : array index out of range '1074318540'
ERROR: 0:58: '[]' : scalar integer expression required·
ERROR: 0:61: '' : array size required
...
```
To reproduce outside the test suite, run `./StandAlone/glslangValidator ../Test/array.frag -i -C` in the build directory and compare the output to Test/baseResults/array.frag.out.

The error message refers to
https://github.com/KhronosGroup/glslang/blob/eb92526d5e04572fdf1d15d2f3ae10a967c2f46f/Test/array.frag#L57
which gets caught as a non-integer array index (first error about that line).

Then, nonetheless, handleBracketDereference extracts an `int indexValue` from the constant
https://github.com/KhronosGroup/glslang/blob/eb92526d5e04572fdf1d15d2f3ae10a967c2f46f/glslang/MachineIndependent/ParseHelper.cpp#L538
for calling `checkIndex` later. Here, the index is from a `TConstUnion` with `type = glslang::EbtDouble`, so the underlying union has been initialized via `dConst`
https://github.com/KhronosGroup/glslang/blob/eb92526d5e04572fdf1d15d2f3ae10a967c2f46f/glslang/Include/ConstantUnion.h#L883-L895
and reading `iConst` is undefined behavior. In practice, that access reads 4 bytes from the same address as the 8-byte `dConst`. With the same value `3.1` in dConst, the result of that read differs between big-endian and little-endian systems, making the extracted `indexValue` ambiguous.

Note that the boundary check against the resulting `int indexValue` does not make sense on little or big endian systems. In the error message, that value is not helpful and potentially confusing.

Contributor guide

Open the contributing guide

Research direction

Start by reproducing the failure with ./StandAlone/glslangValidator ../Test/array.frag -i -C from the build directory and compare it with Test/baseResults/array.frag.out. Read TParseContext::handleBracketDereference in glslang/MachineIndependent/ParseHelper.cpp and the relevant TConstUnion definition in glslang/Include/ConstantUnion.h. Done means the array_frag test produces a deterministic, appropriate error message without an endian-dependent mismatch.

Written by the indexing model from the issue text.

Assessment

Tech stack
cpp
Domain
compilers, testing-qa
Issue type
Bug
Difficulty
3/5
Estimated time
1-2 days
Activity status
Stale
Clarity
Clearly specified
Newbie friendliness
48/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.