Re: [Piglit] [PATCH] glsl-1.50: add linker test for unused in out blocks

[Józef Kucia]

On Wed, May 31, 2017 at 9:42 PM, Ian Romanick  wrote:
> On 05/30/2017 07:23 AM, Józef Kucia wrote:
>> This test exposes a Mesa GLSL linker bug. The test fails with the
>
> Is there a bugzilla for this issue?  Was this encountered in a app or ... ?

I filed a bug report for this issue today. It's bug 101247 [1].

>> following error message:
>>
>>   error: Input block `blk' is not an output of the previous stage
>>
>> Section 4.3.4 (Inputs) of the GLSL 1.50 spec says:
>>
>> "Only the input variables that are actually read need to be written
>> by the previous stage; it is allowed to have superfluous
>> declarations of input variables."
>
> There are some additional rules for separate shader objects programs,
> and it sounds like we may be applying them too broadly.
>
>> ---
>>  .../interstage-multiple-shader-objects.shader_test | 38 
>> ++
>>  1 file changed, 38 insertions(+)
>>  create mode 100644 
>> tests/spec/glsl-1.50/linker/interstage-multiple-shader-objects.shader_test
>>
>> diff --git 
>> a/tests/spec/glsl-1.50/linker/interstage-multiple-shader-objects.shader_test 
>> b/tests/spec/glsl-1.50/linker/interstage-multiple-shader-objects.shader_test
>> new file mode 100644
>> index 000..66a46d5
>> --- /dev/null
>> +++ 
>> b/tests/spec/glsl-1.50/linker/interstage-multiple-shader-objects.shader_test
>> @@ -0,0 +1,38 @@
>> +# Exercises a Mesa GLSL linker bug.
>> +#
>> +# Note that the output block is not used and it is not declared in the main
>> +# shader object.
>> +
>> +[require]
>> +GLSL >= 1.50
>> +
>> +[vertex shader]
>> +out blk {
>> +  vec4 foo;
>> +} inst;
>> +
>> +void set_output(vec4 v)
>> +{
>> +  gl_Position = v;
>> +}
>> +
>> +[vertex shader]
>
> Are there supposed to be two vertex shaders?  Is that necessary to
> reproduce the link failure?

Two vertex shaders are necessary to reproduce the link failure. My
understanding of the issue is that when an output block is declared in
a shader object other than the one with the main() function, and the
output block is unused then there won't be any trace of the original
block declaration in the linked shader produced by
link_intrastage_shaders(). The linker fails while trying to find the
matching output block in the linked shader.

However, a stronger test case is probably also a valid GLSL program.
My understanding of the GL spec suggests that a similar GLSL program
should link even if the matching output block is not declared in a
vertex shader at all, because there is no static use of the input
block in the fragment shader.

[1] - https://bugs.freedesktop.org/show_bug.cgi?id=101247
___
Piglit mailing list
Piglit@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/piglit

Re: [Piglit] [PATCH] textureSize: Silence GCC maybe-uninitialized warning.

[Ian Romanick]

Okay... probably ignore this.  I don't know why a message from 2015 just
popped up as "new and unread" in my inbox.  Weird.

On 05/31/2017 12:54 PM, Ian Romanick wrote:
> Reviewed-by: Ian Romanick 
> 
> On 01/08/2015 12:42 PM, Vinson Lee wrote:
>> textureSize.c: In function 'generate_GLSL':
>> textureSize.c:367:22: warning: 'gs' may be used uninitialized in this 
>> function [-Wmaybe-uninitialized]
>>   if (!vs || (gs_code && !gs) || !fs)
>>   ^
>>
>> Signed-off-by: Vinson Lee 
>> ---
>>  tests/texturing/shaders/textureSize.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/tests/texturing/shaders/textureSize.c 
>> b/tests/texturing/shaders/textureSize.c
>> index ee64b07..4013b49 100644
>> --- a/tests/texturing/shaders/textureSize.c
>> +++ b/tests/texturing/shaders/textureSize.c
>> @@ -247,7 +247,7 @@ has_lod(void)
>>  int
>>  generate_GLSL(enum shader_target test_stage)
>>  {
>> -int vs, gs, fs;
>> +int vs, gs = 0, fs;
>>  int prog;
>>  
>>  static char *vs_code;
>>
> 

___
Piglit mailing list
Piglit@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/piglit

Re: [Piglit] [PATCH] textureSize: Silence GCC maybe-uninitialized warning.

[Ian Romanick]

Reviewed-by: Ian Romanick 

On 01/08/2015 12:42 PM, Vinson Lee wrote:
> textureSize.c: In function 'generate_GLSL':
> textureSize.c:367:22: warning: 'gs' may be used uninitialized in this 
> function [-Wmaybe-uninitialized]
>   if (!vs || (gs_code && !gs) || !fs)
>   ^
> 
> Signed-off-by: Vinson Lee 
> ---
>  tests/texturing/shaders/textureSize.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tests/texturing/shaders/textureSize.c 
> b/tests/texturing/shaders/textureSize.c
> index ee64b07..4013b49 100644
> --- a/tests/texturing/shaders/textureSize.c
> +++ b/tests/texturing/shaders/textureSize.c
> @@ -247,7 +247,7 @@ has_lod(void)
>  int
>  generate_GLSL(enum shader_target test_stage)
>  {
> - int vs, gs, fs;
> + int vs, gs = 0, fs;
>   int prog;
>  
>   static char *vs_code;
> 

___
Piglit mailing list
Piglit@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/piglit

Re: [Piglit] [PATCH] texturing: Add decompression test for S3TC DXT1

[Ian Romanick]

On 05/15/2017 04:08 PM, Nanley Chery wrote:
> Cc: Kenneth Graunke 
> Cc: Mark Janes 
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=100925
> Signed-off-by: Nanley Chery 
> ---
>  tests/all.py  |  1 +
>  tests/texturing/CMakeLists.gl.txt |  1 +
>  tests/texturing/s3tc-targeted.c   | 99 
> +++
>  3 files changed, 101 insertions(+)
>  create mode 100644 tests/texturing/s3tc-targeted.c
> 
> diff --git a/tests/all.py b/tests/all.py
> index 49a8c9fbe..38aabc103 100644
> --- a/tests/all.py
> +++ b/tests/all.py
> @@ -3303,6 +3303,7 @@ with profile.test_list.group_manager(
>  g(['arb_texture_compression-invalid-formats', 's3tc'], 'invalid formats')
>  g(['gen-compressed-teximage'], run_concurrent=False)
>  g(['s3tc-errors'])
> +g(['s3tc-targeted'])
>  g(['s3tc-teximage'], run_concurrent=False)
>  g(['s3tc-texsubimage'], run_concurrent=False)
>  g(['getteximage-targets', '2D', 'S3TC'])
> diff --git a/tests/texturing/CMakeLists.gl.txt 
> b/tests/texturing/CMakeLists.gl.txt
> index 9ab4d7ac8..e5d41e432 100644
> --- a/tests/texturing/CMakeLists.gl.txt
> +++ b/tests/texturing/CMakeLists.gl.txt
> @@ -64,6 +64,7 @@ ENDIF (UNIX)
>  piglit_add_executable (s3tc-teximage s3tc-teximage.c)
>  piglit_add_executable (fxt1-teximage fxt1-teximage.c)
>  piglit_add_executable (s3tc-texsubimage s3tc-texsubimage.c)
> +piglit_add_executable (s3tc-targeted s3tc-targeted.c)
>  piglit_add_executable (s3tc-errors s3tc-errors.c)
>  piglit_add_executable (sampler-cube-shadow sampler-cube-shadow.c)
>  piglit_add_executable (streaming-texture-leak streaming-texture-leak.c)
> diff --git a/tests/texturing/s3tc-targeted.c b/tests/texturing/s3tc-targeted.c
> new file mode 100644
> index 0..b86a326dc
> --- /dev/null
> +++ b/tests/texturing/s3tc-targeted.c
> @@ -0,0 +1,99 @@
> +/*
> + * Copyright © 2017 Intel Corporation
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and associated documentation files (the "Software"),
> + * to deal in the Software without restriction, including without limitation
> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> + * and/or sell copies of the Software, and to permit persons to whom the
> + * Software is furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice (including the next
> + * paragraph) shall be included in all copies or substantial portions of the
> + * Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER 
> DEALINGS
> + * IN THE SOFTWARE.
> + */
> +
> +/** @file s3tc-targeted.c
> + *
> + * Tests the cases of S3TC DXT1 decompression in which the bitmap contains 
> the
> + * value b'11. The chosen tests help to determine that the color comparison
> + * portion of decompression works correctly and that any internal driver
> + * swizzling of the alpha channel is performed correctly.
> + *
> + * Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=100925
> + */
> +
> +#include "piglit-util-gl.h"
> +
> +PIGLIT_GL_TEST_CONFIG_BEGIN
> +
> + /* We need OpenGL 1.3 for the *TexImage* functions used in this file. */

I'm assuming you mean glCompressed*TexImage* functions?  An alternative
would be to require GL_ARB_texture_compression and use the functions
with ARB suffixes.

> + config.supports_gl_compat_version = 13;
> + config.requires_displayed_window = false;
> +
> +PIGLIT_GL_TEST_CONFIG_END
> +
> +static bool
> +test_block(GLenum internal_fmt, const char * base_fmt_str,
> +const uint8_t * dxt1_block, uint16_t expected_result)
> +{
> + /* Upload the DXT1 block. */
> + glCompressedTexImage2D(GL_TEXTURE_2D, 0, internal_fmt, 1, 1, 0,
> +8 /* 64 bits */, dxt1_block);
> +
> + /* Decompress the only defined pixel in the DXT1 block. */
> + uint16_t actual_pixel = 0xBEEF;
> + glGetTexImage(GL_TEXTURE_2D, 0, GL_RGBA, GL_UNSIGNED_SHORT_4_4_4_4,
> +   _pixel);
> +
> + /* Test the result. */
> + if (actual_pixel != expected_result) {
> + fprintf(stderr, "Sampled %#.4x (R4G4B4A4_PACK32), but "
> + "expected %#.4x from %s DXT1 texture.\n",
> + actual_pixel, expected_result, base_fmt_str);
> + return false;
> + }
> +
> + return true;
> +}
> +
> +#define TEST(fmt, block, pixel) \
> + 

Re: [Piglit] [PATCH] glsl-1.50: add linker test for unused in out blocks

[Ian Romanick]

On 05/30/2017 07:23 AM, Józef Kucia wrote:
> This test exposes a Mesa GLSL linker bug. The test fails with the

Is there a bugzilla for this issue?  Was this encountered in a app or ... ?

> following error message:
> 
>   error: Input block `blk' is not an output of the previous stage
> 
> Section 4.3.4 (Inputs) of the GLSL 1.50 spec says:
> 
> "Only the input variables that are actually read need to be written
> by the previous stage; it is allowed to have superfluous
> declarations of input variables."

There are some additional rules for separate shader objects programs,
and it sounds like we may be applying them too broadly.

> ---
>  .../interstage-multiple-shader-objects.shader_test | 38 
> ++
>  1 file changed, 38 insertions(+)
>  create mode 100644 
> tests/spec/glsl-1.50/linker/interstage-multiple-shader-objects.shader_test
> 
> diff --git 
> a/tests/spec/glsl-1.50/linker/interstage-multiple-shader-objects.shader_test 
> b/tests/spec/glsl-1.50/linker/interstage-multiple-shader-objects.shader_test
> new file mode 100644
> index 000..66a46d5
> --- /dev/null
> +++ 
> b/tests/spec/glsl-1.50/linker/interstage-multiple-shader-objects.shader_test
> @@ -0,0 +1,38 @@
> +# Exercises a Mesa GLSL linker bug.
> +#
> +# Note that the output block is not used and it is not declared in the main
> +# shader object.
> +
> +[require]
> +GLSL >= 1.50
> +
> +[vertex shader]
> +out blk {
> +  vec4 foo;
> +} inst;
> +
> +void set_output(vec4 v)
> +{
> +  gl_Position = v;
> +}
> +
> +[vertex shader]

Are there supposed to be two vertex shaders?  Is that necessary to
reproduce the link failure?

> +void set_output(vec4 v);
> +
> +void main()
> +{
> +  set_output(vec4(1.0));
> +}
> +
> +[fragment shader]
> +in blk {
> +  vec4 foo;
> +} inst;
> +
> +void main()
> +{
> +  gl_FragColor = vec4(1.0);
> +}
> +
> +[test]
> +link success
> 

___
Piglit mailing list
Piglit@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/piglit

Re: [Piglit] [PATCH 1/8] fbo: Fix trivial typo in comment

[Brian Paul]

On 05/30/2017 11:33 PM, Ian Romanick wrote:

From: Ian Romanick 

Signed-off-by: Ian Romanick 
---
  tests/fbo/fbo-incomplete-invalid-texture.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/fbo/fbo-incomplete-invalid-texture.c 
b/tests/fbo/fbo-incomplete-invalid-texture.c
index cd8adaf..ac183b4 100644
--- a/tests/fbo/fbo-incomplete-invalid-texture.c
+++ b/tests/fbo/fbo-incomplete-invalid-texture.c
@@ -61,7 +61,7 @@ piglit_init(int argc, char **argv)
glGenFramebuffers(1, );
glBindFramebuffer(GL_DRAW_FRAMEBUFFER, fbo);

-   /* The format of the pixel data is invalide for the specified
+   /* The format of the pixel data is invalid for the specified
 * internalFormat.  This should fail and generate
 * GL_INVALID_OPERATION.  This leaves the texture in a weird, broken
 * state.



For the series,
Reviewed-by: Brian Paul 

___
Piglit mailing list
Piglit@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/piglit