[Piglit] [PATCH] Add test glsl-uniform-out-of-bounds-2.c

[Ian Romanick]

On 11/05/2012 11:39 AM, Frank Henigman wrote:
> This test shows the problem fixed by my "add bounds checking for
> uniform array access" patch, sent to mesa-dev on Nov. 2.
>
> Check behavior of glGetUniformLocation, glGetUniform and glUniform(Matrix)
> when attempting to access non-existent array elements.

There should be some text in the body of the test explaining what is 
being tested.  I had to look at it for quite some time to figure out 
that it's really testing three different things.  If I'm trying to debug 
a test failure, for example, I shouldn't have to work that hard just to 
figure out what is being tested.

It looks like it's testing:

1. Cross validate glGetActiveUniform with glGetUniformlocation. 
Uniforms returned by glGetActiveUniform have a valid location (as 
returned by glGetUniformLocation).  All array elements of an active 
uniform (as returned in the size parameter of glGetActiveUniform) must 
also have a valid location.  Array elements outside that range must not.

2. Validate that uniform data set by glUniform (or glUniformMatrix) can 
be retrieved by glGetUniform.

3. Validate that invalid uniform locations cannot be set by glUnifor / 
glUniformMatirx or queried by glGetUniform.

> ---
>   tests/all.tests                              |    1 +
>   tests/shaders/CMakeLists.gl.txt              |    1 +
>   tests/shaders/glsl-uniform-out-of-bounds-2.c |  155 ++++++++++++++++++++++++++
>   3 files changed, 157 insertions(+), 0 deletions(-)
>   create mode 100644 tests/shaders/glsl-uniform-out-of-bounds-2.c
>
> diff --git a/tests/all.tests b/tests/all.tests
> index 0a8a5aa..63bf1f6 100644
> --- a/tests/all.tests
> +++ b/tests/all.tests
> @@ -240,6 +240,7 @@ add_plain_test(shaders, 'glsl-novertexdata')
>   add_plain_test(shaders, 'glsl-preprocessor-comments')
>   add_plain_test(shaders, 'glsl-reload-source')
>   add_plain_test(shaders, 'glsl-uniform-out-of-bounds')
> +add_plain_test(shaders, 'glsl-uniform-out-of-bounds-2')
>   add_plain_test(shaders, 'glsl-uniform-update')
>   add_plain_test(shaders, 'glsl-unused-varying')
>   add_plain_test(shaders, 'glsl-fs-bug25902')
> diff --git a/tests/shaders/CMakeLists.gl.txt b/tests/shaders/CMakeLists.gl.txt
> index 6ac0230..93cbf7b 100644
> --- a/tests/shaders/CMakeLists.gl.txt
> +++ b/tests/shaders/CMakeLists.gl.txt
> @@ -66,6 +66,7 @@ piglit_add_executable (glsl-reload-source glsl-reload-source.c)
>   piglit_add_executable (glsl-unused-varying glsl-unused-varying.c)
>   piglit_add_executable (glsl-uniform-update glsl-uniform-update.c)
>   piglit_add_executable (glsl-uniform-out-of-bounds glsl-uniform-out-of-bounds.c)
> +piglit_add_executable (glsl-uniform-out-of-bounds-2 glsl-uniform-out-of-bounds-2.c)
>   piglit_add_executable (glsl-fs-bug25902 glsl-fs-bug25902.c)
>   piglit_add_executable (glsl-fs-color-matrix glsl-fs-color-matrix.c)
>   piglit_add_executable (glsl-fs-exp2 glsl-fs-exp2.c)
> diff --git a/tests/shaders/glsl-uniform-out-of-bounds-2.c b/tests/shaders/glsl-uniform-out-of-bounds-2.c
> new file mode 100644
> index 0000000..94a9f88
> --- /dev/null
> +++ b/tests/shaders/glsl-uniform-out-of-bounds-2.c
> @@ -0,0 +1,155 @@
> +/*
> + * Copyright 2012 Google Inc.
> + *
> + * This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2.1 of the License, or (at your option) any later version.
> + *
> + * This library is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with this library; if not, write to the Free Software
> + * Foundation Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA
> + *
> + * Authors:
> + *   Frank Henigman <fjhenigman at google.com>
> + */
> +
> +#include "piglit-util-gl-common.h"
> +
> +PIGLIT_GL_TEST_CONFIG_BEGIN
> +
> +	config.supports_gl_compat_version = 10;
> +
> +	config.window_width = 100;
> +	config.window_height = 100;

Since this test doesn't do any rendering, there's no need to set the 
window dimensions.  Apparently width less than 150 causes problems in 
Windows8.

> +	config.window_visual = PIGLIT_GL_VISUAL_RGB | PIGLIT_GL_VISUAL_DOUBLE;
> +
> +PIGLIT_GL_TEST_CONFIG_END
> +
> +static GLint prog;
> +
> +enum piglit_result
> +piglit_display(void)
> +{
> +	/* unreached */
> +	return PIGLIT_FAIL;
> +}
> +
> +void
> +piglit_init(int argc, char **argv)
> +{
> +	GLint vs, fs, len;
> +	GLchar name[99];
> +	int size;
> +	GLenum type;
> +	int i, j, activeIdx;
> +	enum piglit_result result = PIGLIT_PASS;
> +	GLint min = -1, max = -1;
> +	GLint numActive;
> +	GLfloat v[16];
> +
> +	piglit_require_gl_version(20);
> +
> +	vs = piglit_compile_shader_text(GL_VERTEX_SHADER,
> +		"attribute vec4 p;\n"
> +		"void main() { gl_Position = p; }\n"
> +	);
> +	fs = piglit_compile_shader_text(GL_FRAGMENT_SHADER,
> +		"uniform vec4 v[4];\n"
> +		"uniform mat4 m[4];\n"
> +		"void main() { gl_FragColor = v[1] + m[1][1]; }\n"
> +	);
> +
> +	prog = piglit_link_simple_program(vs, fs);
> +	glUseProgram(prog);
> +	glGetProgramiv(prog, GL_ACTIVE_UNIFORMS, &numActive);
> +	printf("num active %d\n", numActive);
> +
> +	for ( activeIdx = 0; activeIdx < numActive; ++activeIdx ) {
> +		glGetActiveUniform(prog, activeIdx, sizeof(name)/sizeof(name[0]), NULL, &len, &type, name);

Use the ARRAY_SIZE macro (from piglit_util.h).

> +		printf("name %s length %d\n", name, len);
> +		if (name[1] != 0) continue;

Please put the body and the if-statement on separate lines.  There are a 
couple other cases of this.

> +		if (name[0] == 'v') {
> +			size = 4;
> +		} else if (name[0] == 'm' ) {
> +			size = 16;
> +		} else {
> +			continue;
> +		}
> +
> +		// try each location in array, plus some outside
> +		for ( i = -2; i < len + 2; ++i ) {
> +			GLchar buf[9];
> +			sprintf(buf, "%c[%d]", name[0], i);
> +			GLint loc = glGetUniformLocation(prog, buf);
> +			int isActive;

bool (from stdbool.h)?

> +			if (loc == -1) {
> +				printf("no index %d\n", i);
> +				isActive = 0;
> +			} else {
> +				isActive = 1;
> +				if (min == -1 || loc < min) min = loc;
> +				if (max == -1 || loc > max) max = loc;
> +
> +				// write, read back, compare
> +				for ( j = 0; j < 16; ++j ) v[j] = j + 11;

Please put the body of the loop and the for-statement on separate lines. 
  This is really hard to read.

Also, why not just do this initialization once or make v a static const 
array with a static initializer?  (See also my next comments.)

> +				if (size == 4 ) {
> +					glUniform4fv(loc, 1, v);
> +				} else {
> +					glUniformMatrix4fv(loc, 1, GL_FALSE, v);
> +				}
> +				glGetUniformfv(prog, loc, v);
> +				for ( j = 0; j < size; ++j ) {
> +					if (v[j] != j + 11) {

This is a bad way to verify this.  glGetUniformfv could be a no-op, and 
this check would pass.  You should glGetUniformfv into a different 
buffer that is preinitialized with garbage and compare the two arrays.

> +						printf("FAIL: value at index %d\n", i);
> +						result = PIGLIT_FAIL;
> +						break;
> +					}
> +				}
> +			}
> +
> +			// does glGetActiveUniform agree with glGetUniformLocation?
> +			if (isActive ^ (0 <= i && i < len)) {

After changing isActive to bool, this could be

		if (isActive == (i >= 0 && i < len)) {

We don't use the literal-on-the-left style.

A comment would also be useful here.  Something like

	/* The array element should be active iff the index queried
	 * is within the array size previously returned by
	 * glGetActiveUniform.
	 */

> +				printf("FAIL: inconsistent reporting of active array elements\n");
> +				result = PIGLIT_FAIL;
> +			}
> +		}
> +	}
> +
> +	// make up some bogus locations
> +	GLint bogus[99];
> +	int nbogus = 0;

Mixing code and declarations will fail on MSVC builds.

> +	for ( i = 1; i < 6; ++i ) {
> +		bogus[nbogus++] = min - i;
> +		bogus[nbogus++] = max + i;
> +		bogus[nbogus++] = max + (1<<16) + i - 3;
> +	}
> +
> +	// test bogus locations
> +	for ( i = 0; i < nbogus; ++i ) {
> +		if (bogus[i] == -1) continue;
> +		printf("try bogus location %d\n", bogus[i]);
> +		glUniform4fv(bogus[i], 1, v);
> +		if (glGetError() != GL_INVALID_OPERATION) {
> +			printf("FAIL: wrote bogus location\n");
> +			result = PIGLIT_FAIL;
> +		}

Use the piglit_check_gl_error function here.

	if (!piglit_check_gl_error(GL_INVALID_OPERATION))
		result = PIGLIT_FAIL;

Most piglit tests use a boolean "pass" flag and only select PIGLIT_PASS 
or PIGLIT_FAIL at the end.  That allows simpler code like:

	pass = piglit_check_gl_error(GL_INVALID_OPERATION) && pass;

> +		glUniformMatrix4fv(bogus[i], 1, GL_FALSE, v);
> +		if (glGetError() != GL_INVALID_OPERATION) {
> +			printf("FAIL: wrote bogus location (matrix)\n");
> +			result = PIGLIT_FAIL;
> +		}
> +		glGetUniformfv(prog, bogus[i], v);
> +		if (glGetError() != GL_INVALID_OPERATION) {
> +			printf("FAIL: read bogus location\n");
> +			result = PIGLIT_FAIL;
> +		}
> +	}
> +
> +	piglit_report_result(result);
> +}
>