glsl: update assumption in array out of bounds test

Submitted by Timothy Arceri on March 30, 2017, 11:53 p.m.

Details

Message ID 20170330235342.3296-1-tarceri@itsqueeze.com
State New
Headers show
Series "glsl: update assumption in array out of bounds test" ( rev: 1 ) in Piglit

Not browsing as part of any series.

Commit Message

Timothy Arceri March 30, 2017, 11:53 p.m.
Section 5.7 of the GLSL 4.5 spec says:

   "Behavior is undefined if a shader subscripts an array with an
   index less than 0 or greater than or equal to the size the array
   was declared with."

So we cannot be sure which path the shader will take. Update the
test so that both branches end in the same result.

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=96684
---
 tests/shaders/glsl-array-bounds-01.shader_test | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Patch hide | download patch | download mbox

diff --git a/tests/shaders/glsl-array-bounds-01.shader_test b/tests/shaders/glsl-array-bounds-01.shader_test
index 2e7c762..a06fef7 100644
--- a/tests/shaders/glsl-array-bounds-01.shader_test
+++ b/tests/shaders/glsl-array-bounds-01.shader_test
@@ -15,21 +15,21 @@  void main()
  */
 #version 120
 
 float array[] = float [] (1.0, 2.0, 3.0, 4.0);
 
 void main()
 {
    int idx = 20;
 
    if (array[idx] == 5.0)
-      gl_FragColor = vec4(1.0, 0.0, 0.0, 1.0);
+      gl_FragColor = vec4(0.0, 1.0, 0.0, 1.0);
    else
       gl_FragColor = vec4(0.0, 1.0, 0.0, 1.0);
 }
 
 [test]
 clear color 0.0 0.0 0.0 0.0
 clear
 ortho
 draw rect 10 10 10 10
 probe rgb 15 15 0.0 1.0 0.0

Comments

On 31.03.2017 01:53, Timothy Arceri wrote:
> Section 5.7 of the GLSL 4.5 spec says:
>
>    "Behavior is undefined if a shader subscripts an array with an
>    index less than 0 or greater than or equal to the size the array
>    was declared with."
>
> So we cannot be sure which path the shader will take. Update the
> test so that both branches end in the same result.
>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=96684
> ---
>  tests/shaders/glsl-array-bounds-01.shader_test | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/tests/shaders/glsl-array-bounds-01.shader_test b/tests/shaders/glsl-array-bounds-01.shader_test
> index 2e7c762..a06fef7 100644
> --- a/tests/shaders/glsl-array-bounds-01.shader_test
> +++ b/tests/shaders/glsl-array-bounds-01.shader_test
> @@ -15,21 +15,21 @@ void main()
>   */
>  #version 120
>
>  float array[] = float [] (1.0, 2.0, 3.0, 4.0);
>
>  void main()
>  {
>     int idx = 20;
>
>     if (array[idx] == 5.0)
> -      gl_FragColor = vec4(1.0, 0.0, 0.0, 1.0);
> +      gl_FragColor = vec4(0.0, 1.0, 0.0, 1.0);
>     else
>        gl_FragColor = vec4(0.0, 1.0, 0.0, 1.0);
>  }

I'm a bit concerned that an optimizer will do tail-merging followed by 
eliminating the load. Maybe do something silly like

float tmp = array[idx];

gl_FragColor = vec4(0.0, 1.0 + abs(tmp), 0.0, 1.0);

instead? Then the compiler can't optimize the load away, because the 
shader has to work with un-clamped floating point framebuffers.

On second thought, what about NaNs?

Cheers,
Nicolai

>
>  [test]
>  clear color 0.0 0.0 0.0 0.0
>  clear
>  ortho
>  draw rect 10 10 10 10
>  probe rgb 15 15 0.0 1.0 0.0
>
On 31/03/17 17:30, Nicolai Hähnle wrote:
> On 31.03.2017 01:53, Timothy Arceri wrote:
>> Section 5.7 of the GLSL 4.5 spec says:
>>
>>    "Behavior is undefined if a shader subscripts an array with an
>>    index less than 0 or greater than or equal to the size the array
>>    was declared with."
>>
>> So we cannot be sure which path the shader will take. Update the
>> test so that both branches end in the same result.
>>
>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=96684
>> ---
>>  tests/shaders/glsl-array-bounds-01.shader_test | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/tests/shaders/glsl-array-bounds-01.shader_test
>> b/tests/shaders/glsl-array-bounds-01.shader_test
>> index 2e7c762..a06fef7 100644
>> --- a/tests/shaders/glsl-array-bounds-01.shader_test
>> +++ b/tests/shaders/glsl-array-bounds-01.shader_test
>> @@ -15,21 +15,21 @@ void main()
>>   */
>>  #version 120
>>
>>  float array[] = float [] (1.0, 2.0, 3.0, 4.0);
>>
>>  void main()
>>  {
>>     int idx = 20;
>>
>>     if (array[idx] == 5.0)
>> -      gl_FragColor = vec4(1.0, 0.0, 0.0, 1.0);
>> +      gl_FragColor = vec4(0.0, 1.0, 0.0, 1.0);
>>     else
>>        gl_FragColor = vec4(0.0, 1.0, 0.0, 1.0);
>>  }
>
> I'm a bit concerned that an optimizer will do tail-merging followed by
> eliminating the load. Maybe do something silly like
>
> float tmp = array[idx];
>
> gl_FragColor = vec4(0.0, 1.0 + abs(tmp), 0.0, 1.0);
>
> instead? Then the compiler can't optimize the load away, because the
> shader has to work with un-clamped floating point framebuffers.
>
> On second thought, what about NaNs?

Well we are testing undefined behavior, the correct thing might be to 
just delete the test?



>
> Cheers,
> Nicolai
>
>>
>>  [test]
>>  clear color 0.0 0.0 0.0 0.0
>>  clear
>>  ortho
>>  draw rect 10 10 10 10
>>  probe rgb 15 15 0.0 1.0 0.0
>>
>
>
On 03.04.2017 02:01, Timothy Arceri wrote:
>
>
> On 31/03/17 17:30, Nicolai Hähnle wrote:
>> On 31.03.2017 01:53, Timothy Arceri wrote:
>>> Section 5.7 of the GLSL 4.5 spec says:
>>>
>>>    "Behavior is undefined if a shader subscripts an array with an
>>>    index less than 0 or greater than or equal to the size the array
>>>    was declared with."
>>>
>>> So we cannot be sure which path the shader will take. Update the
>>> test so that both branches end in the same result.
>>>
>>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=96684
>>> ---
>>>  tests/shaders/glsl-array-bounds-01.shader_test | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/tests/shaders/glsl-array-bounds-01.shader_test
>>> b/tests/shaders/glsl-array-bounds-01.shader_test
>>> index 2e7c762..a06fef7 100644
>>> --- a/tests/shaders/glsl-array-bounds-01.shader_test
>>> +++ b/tests/shaders/glsl-array-bounds-01.shader_test
>>> @@ -15,21 +15,21 @@ void main()
>>>   */
>>>  #version 120
>>>
>>>  float array[] = float [] (1.0, 2.0, 3.0, 4.0);
>>>
>>>  void main()
>>>  {
>>>     int idx = 20;
>>>
>>>     if (array[idx] == 5.0)
>>> -      gl_FragColor = vec4(1.0, 0.0, 0.0, 1.0);
>>> +      gl_FragColor = vec4(0.0, 1.0, 0.0, 1.0);
>>>     else
>>>        gl_FragColor = vec4(0.0, 1.0, 0.0, 1.0);
>>>  }
>>
>> I'm a bit concerned that an optimizer will do tail-merging followed by
>> eliminating the load. Maybe do something silly like
>>
>> float tmp = array[idx];
>>
>> gl_FragColor = vec4(0.0, 1.0 + abs(tmp), 0.0, 1.0);
>>
>> instead? Then the compiler can't optimize the load away, because the
>> shader has to work with un-clamped floating point framebuffers.
>>
>> On second thought, what about NaNs?
>
> Well we are testing undefined behavior, the correct thing might be to
> just delete the test?

That's a good point. Though robust buffer access changes this, so that 
only the returned value is undefined. So instead of deleting the test, 
it should probably be modified / moved to become a test for robust 
buffer access.

Cheers,
Nicolai

>
>
>
>>
>> Cheers,
>> Nicolai
>>
>>>
>>>  [test]
>>>  clear color 0.0 0.0 0.0 0.0
>>>  clear
>>>  ortho
>>>  draw rect 10 10 10 10
>>>  probe rgb 15 15 0.0 1.0 0.0
>>>
>>
>>