[Piglit] [PATCH] gl-2.1: Verify values of GLSL built-in constants that are visible to the GL API

[Ian Romanick]

(WTF is up with Thunderbird mangling whitespace?!?)

On 02/02/2012 02:52 AM, Kenneth Graunke wrote:
> On 02/01/2012 02:41 PM, Ian Romanick wrote:
>> From: Ian Romanick<ian.d.romanick at intel.com>
>>
>> Values are verified by attempting to compile a shader that sizes an
>> array based on a comparison between the built-in and its expected
>> value. If the values do not match, the array will have a size of -1
>> and will cause a compilation error. If the values do match, the array
>> will have a size of 1.
>>
>> For safety, the shaders are checked for compilation errors and for
>> linking errors. This catched implementations that only generate
>> errors during linking. This means that the array is also accessed by
>> the shader body so that it is not eliminated (thereby eliminating the
>> error).
>>
>> Signed-off-by: Ian Romanick<ian.d.romanick at intel.com>
>> ---
>> If this looks right, I can generate a similar test for gl-3.0 and
>> gles-2.0. I intentionally chose to keep these separate because it
>> will make it easier to port to other APIs. For example, the shaders
>> for gl-3.0 and gles-2.0 will look different
>>
>> tests/all.tests | 1 +
>> tests/spec/gl-2.1/CMakeLists.gl.txt | 1 +
>> tests/spec/gl-2.1/built-in-constants.c | 174
>> ++++++++++++++++++++++++++++++++
>> 3 files changed, 176 insertions(+), 0 deletions(-)
>> create mode 100644 tests/spec/gl-2.1/built-in-constants.c
>
> This looks great. I really like the idea of using the array size trick
> to make it a compile test rather than a rendering test.
>
> A couple of nitpicks below...
>
>> diff --git a/tests/all.tests b/tests/all.tests
>> index 2312ee0..862d63f 100644
>> --- a/tests/all.tests
>> +++ b/tests/all.tests
>> @@ -830,6 +830,7 @@ add_concurrent_test(gl20, 'vertex-program-two-side')
>> gl21 = Group()
>> spec['!OpenGL 2.1'] = gl21
>> gl21['minmax'] = concurrent_test('gl-2.1-minmax')
>> +gl21['GLSL built-in constant values'] =
>> concurrent_test('gl-2.1-built-in-constants')
>>
>> gl30 = Group()
>> spec['!OpenGL 3.0'] = gl30
>> diff --git a/tests/spec/gl-2.1/CMakeLists.gl.txt
>> b/tests/spec/gl-2.1/CMakeLists.gl.txt
>> index 9a4afbe..4d604bb 100644
>> --- a/tests/spec/gl-2.1/CMakeLists.gl.txt
>> +++ b/tests/spec/gl-2.1/CMakeLists.gl.txt
>> @@ -14,3 +14,4 @@ link_libraries (
>> )
>>
>> add_executable (gl-2.1-minmax minmax.c)
>> +add_executable (gl-2.1-built-in-constants built-in-constants.c)
>> diff --git a/tests/spec/gl-2.1/built-in-constants.c
>> b/tests/spec/gl-2.1/built-in-constants.c
>> new file mode 100644
>> index 0000000..a23fb29
>> --- /dev/null
>> +++ b/tests/spec/gl-2.1/built-in-constants.c
>> @@ -0,0 +1,174 @@
>> +/* Copyright © 2012 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 gl-2.1-built-in-constants.c
>> + * Verify values of GLSL built-in constants that are visible to the
>> GL API.
>> + *
>> + * Values are verified by attempting to compile a shader that sizes
>> an array
>> + * based on a comparison between the built-in and its expected value.
>> If the
>> + * values do not match, the array will have a size of -1 and will
>> cause a
>> + * compilation error. If the values do match, the array will have a
>> size of
>> + * 1.
>> + *
>> + * For safety, the shaders are checked for compilation errors and for
>> linking
>> + * errors. This catched implementations that only generate errors during
>
> "catched" is not a word. I think you mean "catches" or "will catch"?

It was supposed to be "catches."

>> + * linking. This means that the array is also accessed by the shader
>> body so
>> + * that it is not eliminated (thereby eliminating the error).
>
> I don't follow. Your assignment to gl_FragColor ensures that the array
> is used, but the fact that the test checks for compilation and linker
> errors does not. Missed a sentence here?

I think I just misworded the sentence that's there.  If an 
implementation only generates errors during linking, it may not generate 
an error for something that's eliminated.  If the array is not accessed, 
the compiler or linker may eliminate it.  If it's eliminated before 
checking for errors, no error will be generated.

I think s/array is also/array must be/ mostly fixes the comment that's 
there.

>> + */
>> +
>> +#include "piglit-util.h"
>> +
>> +static const char *fs_template =
>> + "#version 120\n"
>> + "uniform vec4 a[%s == %d ? 1 : -1];\n"
>> + "void main() {\n"
>> + " gl_FragColor = a[0];\n"
>> + "}\n"
>> + ;
>> +
>> +static const char *vs_template =
>> + "#version 120\n"
>> + "uniform vec4 a[%s == %d ? 1 : -1];\n"
>> + "void main() {\n"
>> + " gl_Position = a[0];\n"
>> + "}\n"
>> + ;
>> +
>> +static const struct {
>> + const char *name;
>> + GLenum pname;
>> +} built_ins[] = {
>> + {
>> + "gl_MaxLights",
>> + GL_MAX_LIGHTS
>> + },
>> + {
>> + "gl_MaxClipPlanes",
>> + GL_MAX_CLIP_PLANES
>> + },
>> + {
>> + "gl_MaxTextureUnits",
>> + GL_MAX_TEXTURE_UNITS
>> + },
>> + {
>> + "gl_MaxTextureCoords",
>> + GL_MAX_TEXTURE_COORDS
>> + },
>> + {
>> + "gl_MaxVertexAttribs",
>> + GL_MAX_VERTEX_ATTRIBS
>> + },
>> + {
>> + "gl_MaxVertexUniformComponents",
>> + GL_MAX_VERTEX_UNIFORM_COMPONENTS
>> + },
>> + {
>> + "gl_MaxVaryingFloats",
>> + GL_MAX_VARYING_FLOATS
>> + },
>> + {
>> + "gl_MaxVertexTextureImageUnits",
>> + GL_MAX_VERTEX_TEXTURE_IMAGE_UNITS
>> + },
>> + {
>> + "gl_MaxCombinedTextureImageUnits",
>> + GL_MAX_COMBINED_TEXTURE_IMAGE_UNITS
>> + },
>> + {
>> + "gl_MaxTextureImageUnits",
>> + GL_MAX_TEXTURE_IMAGE_UNITS
>> + },
>> + {
>> + "gl_MaxFragmentUniformComponents",
>> + GL_MAX_FRAGMENT_UNIFORM_COMPONENTS
>> + },
>> + {
>> + "gl_MaxDrawBuffers",
>> + GL_MAX_DRAW_BUFFERS
>> + }
>> +};
>> +
>> +int piglit_width = 10, piglit_height = 10;
>> +int piglit_window_mode = GLUT_RGB;
>> +
>> +enum piglit_result
>> +piglit_display()
>> +{
>> + /* UNREACHED */
>> + return PIGLIT_FAIL;
>> +}
>> +
>> +void
>> +piglit_init(int argc, char **argv)
>> +{
>> + bool pass = true;
>> + char buf[512];
>> + unsigned i;
>> +
>> + for (i = 0; i< ARRAY_SIZE(built_ins); i++) {
>> + int value;
>> +
>> + glGetIntegerv(built_ins[i].pname,&value);
>> + if (!piglit_check_gl_error(GL_NO_ERROR)) {
>> + fprintf(stderr, "Query of 0x%04 generated an error.\n",
>> + built_ins[i].name);
>> + pass = false;
>> + } else {
>
> I might just use "continue" here to avoid having to indent the whole
> body of the loop an additional level...which ends up forcing you to wrap
> the piglit_compile_shader_text lines.

Good idea.

>> + GLuint fs, vs, prog;
>> +
>> + snprintf(buf, sizeof(buf), fs_template,
>> + built_ins[i].name, value);
>
> Jose actually added asprintf support a while back, even on Windows, so
> you can just use that rather than a fixed size buffer. It's already
> available via your #include "piglit-util.h".

But then I have to remember to free the buffers and, technically 
speaking, check for errors.  I'm not sure that's better.

>> + fs = piglit_compile_shader_text(GL_FRAGMENT_SHADER,
>> + buf);
>> + if (fs == 0) {
>> + fprintf(stderr,
>> + "Check of %s failed in fragment "
>> + "shader\n",
>> + built_ins[i].name);
>> + pass = false;
>> + }
>> +
>> + snprintf(buf, sizeof(buf), vs_template,
>> + built_ins[i].name, value);
>
> Ditto.
>
>> + vs = piglit_compile_shader_text(GL_VERTEX_SHADER,
>> + buf);
>> + if (vs == 0) {
>> + fprintf(stderr,
>> + "Check of %s failed in vertex "
>> + "shader\n",
>> + built_ins[i].name);
>> + pass = false;
>> + }
>> +
>> + prog = piglit_link_simple_program(fs, vs);
>> + if (prog == 0) {
>> + fprintf(stderr,
>> + "Check of %s failed to link\n",
>> + built_ins[i].name);
>> + pass = false;
>> + }
>> + }
>> + }
>> +
>> + piglit_report_result(pass ? PIGLIT_PASS : PIGLIT_FAIL);
>> +}
>
> Again, glad to see this.
>
> Reviewed-by: Kenneth Graunke <kenneth at whitecape.org>