[Mesa-dev] [PATCH 01/20] mesa: fix and simplify resource query for arrays
Tapani Pälli
tapani.palli at intel.com
Wed Jul 29 22:25:14 PDT 2015
Hi Timothy;
Would it be OK for you to wait a bit with these 3 first patches? I'm
currently going through failing PIQ tests and I have a 1 line fix to
valid_program_resource_index_name() that together with your patch "glsl:
set stage flag for structs and arrays in resource list" will fix all of
the failing "ES31-CTS.program_interface_query.uniform" tests. Frankly I
don't see the value of such a big refactor necessary for this.
On 07/29/2015 04:56 PM, Timothy Arceri wrote:
> This removes the need for multiple functions designed to validate an array
> subscript and replaces them with a call to a single function.
>
> The change also means that validation is now only done once and the index
> is retrived at the same time, as a result the getUniformLocation code can
> be simplified saving an extra hash table lookup (and yet another
> validation call).
>
> This chage also fixes some tests in:
> ES31-CTS.program_interface_query.uniform
>
> V3: rebase on subroutines, and move the resource index array == 0
> check into _mesa_GetProgramResourceIndex() to simplify things further
>
> V2: Fix bounds checks for program input/output, split unrelated comment fix
> and _mesa_get_uniform_location() removal into their own patch.
>
> Cc: Tapani Pälli <tapani.palli at intel.com>
> ---
> src/mesa/main/program_resource.c | 14 ++--
> src/mesa/main/shader_query.cpp | 174 +++++++++++++++++++++------------------
> src/mesa/main/shaderapi.c | 2 +-
> src/mesa/main/shaderapi.h | 3 +-
> src/mesa/main/uniforms.c | 5 +-
> 5 files changed, 106 insertions(+), 92 deletions(-)
>
> diff --git a/src/mesa/main/program_resource.c b/src/mesa/main/program_resource.c
> index 641ef22..fdbd5b3 100644
> --- a/src/mesa/main/program_resource.c
> +++ b/src/mesa/main/program_resource.c
> @@ -229,6 +229,7 @@ _mesa_GetProgramResourceIndex(GLuint program, GLenum programInterface,
> const GLchar *name)
> {
> GET_CURRENT_CONTEXT(ctx);
> + unsigned array_index = 0;
> struct gl_program_resource *res;
> struct gl_shader_program *shProg =
> _mesa_lookup_shader_program_err(ctx, program,
> @@ -268,13 +269,10 @@ _mesa_GetProgramResourceIndex(GLuint program, GLenum programInterface,
> case GL_PROGRAM_OUTPUT:
> case GL_UNIFORM:
> case GL_TRANSFORM_FEEDBACK_VARYING:
> - /* Validate name syntax for array variables */
> - if (!valid_program_resource_index_name(name))
> - return GL_INVALID_INDEX;
> - /* fall-through */
> case GL_UNIFORM_BLOCK:
> - res = _mesa_program_resource_find_name(shProg, programInterface, name);
> - if (!res)
> + res = _mesa_program_resource_find_name(shProg, programInterface, name,
> + &array_index);
> + if (!res || array_index > 0)
> return GL_INVALID_INDEX;
>
> return _mesa_program_resource_index(shProg, res);
> @@ -403,7 +401,7 @@ _mesa_GetProgramResourceLocation(GLuint program, GLenum programInterface,
> struct gl_shader_program *shProg =
> lookup_linked_program(program, "glGetProgramResourceLocation");
>
> - if (!shProg || !name || invalid_array_element_syntax(name))
> + if (!shProg || !name)
> return -1;
>
> /* Validate programInterface. */
> @@ -453,7 +451,7 @@ _mesa_GetProgramResourceLocationIndex(GLuint program, GLenum programInterface,
> struct gl_shader_program *shProg =
> lookup_linked_program(program, "glGetProgramResourceLocationIndex");
>
> - if (!shProg || !name || invalid_array_element_syntax(name))
> + if (!shProg || !name)
> return -1;
>
> /* From the GL_ARB_program_interface_query spec:
> diff --git a/src/mesa/main/shader_query.cpp b/src/mesa/main/shader_query.cpp
> index 3e52a09..dd11b81 100644
> --- a/src/mesa/main/shader_query.cpp
> +++ b/src/mesa/main/shader_query.cpp
> @@ -44,7 +44,8 @@ extern "C" {
>
> static GLint
> program_resource_location(struct gl_shader_program *shProg,
> - struct gl_program_resource *res, const char *name);
> + struct gl_program_resource *res, const char *name,
> + unsigned array_index);
>
> /**
> * Declare convenience functions to return resource data in a given type.
> @@ -272,13 +273,15 @@ _mesa_GetAttribLocation(GLhandleARB program, const GLcharARB * name)
> if (shProg->_LinkedShaders[MESA_SHADER_VERTEX] == NULL)
> return -1;
>
> + unsigned array_index = 0;
> struct gl_program_resource *res =
> - _mesa_program_resource_find_name(shProg, GL_PROGRAM_INPUT, name);
> + _mesa_program_resource_find_name(shProg, GL_PROGRAM_INPUT, name,
> + &array_index);
>
> if (!res)
> return -1;
>
> - GLint loc = program_resource_location(shProg, res, name);
> + GLint loc = program_resource_location(shProg, res, name, array_index);
>
> /* The extra check against against 0 is made because of builtin-attribute
> * locations that have offset applied. Function program_resource_location
> @@ -456,13 +459,15 @@ _mesa_GetFragDataLocation(GLuint program, const GLchar *name)
> if (shProg->_LinkedShaders[MESA_SHADER_FRAGMENT] == NULL)
> return -1;
>
> + unsigned array_index = 0;
> struct gl_program_resource *res =
> - _mesa_program_resource_find_name(shProg, GL_PROGRAM_OUTPUT, name);
> + _mesa_program_resource_find_name(shProg, GL_PROGRAM_OUTPUT, name,
> + &array_index);
>
> if (!res)
> return -1;
>
> - GLint loc = program_resource_location(shProg, res, name);
> + GLint loc = program_resource_location(shProg, res, name, array_index);
>
> /* The extra check against against 0 is made because of builtin-attribute
> * locations that have offset applied. Function program_resource_location
> @@ -552,39 +557,32 @@ _mesa_program_resource_array_size(struct gl_program_resource *res)
> return 0;
> }
>
> -static int
> -array_index_of_resource(struct gl_program_resource *res,
> - const char *name)
> +/**
> + * Checks if array subscript is valid and if so sets array_index.
> + */
> +static bool
> +valid_array_index(const GLchar *name, unsigned *array_index)
> {
> - assert(res->Data);
> + unsigned idx = 0;
> + const GLchar *out_base_name_end;
>
> - switch (res->Type) {
> - case GL_PROGRAM_INPUT:
> - case GL_PROGRAM_OUTPUT:
> - return get_matching_index(RESOURCE_VAR(res), name);
> - default:
> - assert(!"support for resource type not implemented");
> - return -1;
> - }
> + idx = parse_program_resource_name(name, &out_base_name_end);
> + if (idx < 0)
> + return false;
> +
> + if (array_index)
> + *array_index = idx;
> +
> + return true;
> }
>
> /* Find a program resource with specific name in given interface.
> */
> struct gl_program_resource *
> _mesa_program_resource_find_name(struct gl_shader_program *shProg,
> - GLenum programInterface, const char *name)
> + GLenum programInterface, const char *name,
> + unsigned *array_index)
> {
> - GET_CURRENT_CONTEXT(ctx);
> - const char *full_name = name;
> -
> - /* When context has 'VertexID_is_zero_based' set, gl_VertexID has been
> - * lowered to gl_VertexIDMESA.
> - */
> - if (name && ctx->Const.VertexID_is_zero_based) {
> - if (strcmp(name, "gl_VertexID") == 0)
> - full_name = "gl_VertexIDMESA";
> - }
> -
> struct gl_program_resource *res = shProg->ProgramResourceList;
> for (unsigned i = 0; i < shProg->NumProgramResourceList; i++, res++) {
> if (res->Type != programInterface)
> @@ -594,38 +592,46 @@ _mesa_program_resource_find_name(struct gl_shader_program *shProg,
> const char *rname = _mesa_program_resource_name(res);
> unsigned baselen = strlen(rname);
>
> - switch (programInterface) {
> - case GL_TRANSFORM_FEEDBACK_VARYING:
> - case GL_UNIFORM_BLOCK:
> - case GL_UNIFORM:
> - case GL_VERTEX_SUBROUTINE_UNIFORM:
> - case GL_GEOMETRY_SUBROUTINE_UNIFORM:
> - case GL_FRAGMENT_SUBROUTINE_UNIFORM:
> - case GL_COMPUTE_SUBROUTINE_UNIFORM:
> - case GL_TESS_CONTROL_SUBROUTINE_UNIFORM:
> - case GL_TESS_EVALUATION_SUBROUTINE_UNIFORM:
> - case GL_VERTEX_SUBROUTINE:
> - case GL_GEOMETRY_SUBROUTINE:
> - case GL_FRAGMENT_SUBROUTINE:
> - case GL_COMPUTE_SUBROUTINE:
> - case GL_TESS_CONTROL_SUBROUTINE:
> - case GL_TESS_EVALUATION_SUBROUTINE:
> - if (strncmp(rname, name, baselen) == 0) {
> + if (strncmp(rname, name, baselen) == 0) {
> + switch (programInterface) {
> + case GL_UNIFORM_BLOCK:
> /* Basename match, check if array or struct. */
> if (name[baselen] == '\0' ||
> name[baselen] == '[' ||
> name[baselen] == '.') {
> return res;
> }
> + break;
> + case GL_TRANSFORM_FEEDBACK_VARYING:
> + case GL_UNIFORM:
> + case GL_VERTEX_SUBROUTINE_UNIFORM:
> + case GL_GEOMETRY_SUBROUTINE_UNIFORM:
> + case GL_FRAGMENT_SUBROUTINE_UNIFORM:
> + case GL_COMPUTE_SUBROUTINE_UNIFORM:
> + case GL_TESS_CONTROL_SUBROUTINE_UNIFORM:
> + case GL_TESS_EVALUATION_SUBROUTINE_UNIFORM:
> + case GL_VERTEX_SUBROUTINE:
> + case GL_GEOMETRY_SUBROUTINE:
> + case GL_FRAGMENT_SUBROUTINE:
> + case GL_COMPUTE_SUBROUTINE:
> + case GL_TESS_CONTROL_SUBROUTINE:
> + case GL_TESS_EVALUATION_SUBROUTINE:
> + if (name[baselen] == '.') {
> + return res;
> + }
> + /* fall-through */
> + case GL_PROGRAM_INPUT:
> + case GL_PROGRAM_OUTPUT:
> + if (name[baselen] == '\0') {
> + return res;
> + } else if (name[baselen] == '[' &&
> + valid_array_index(name, array_index)) {
> + return res;
> + }
> + break;
> + default:
> + assert(!"not implemented for given interface");
> }
> - break;
> - case GL_PROGRAM_INPUT:
> - case GL_PROGRAM_OUTPUT:
> - if (array_index_of_resource(res, full_name) >= 0)
> - return res;
> - break;
> - default:
> - assert(!"not implemented for given interface");
> }
> }
> return NULL;
> @@ -787,19 +793,9 @@ _mesa_get_program_resource_name(struct gl_shader_program *shProg,
>
> static GLint
> program_resource_location(struct gl_shader_program *shProg,
> - struct gl_program_resource *res, const char *name)
> + struct gl_program_resource *res, const char *name,
> + unsigned array_index)
> {
> - unsigned index, offset;
> - int array_index = -1;
> - long offset_ret;
> - const GLchar *base_name_end;
> -
> - if (res->Type == GL_PROGRAM_INPUT || res->Type == GL_PROGRAM_OUTPUT) {
> - array_index = array_index_of_resource(res, name);
> - if (array_index < 0)
> - return -1;
> - }
> -
> /* Built-in locations should report GL_INVALID_INDEX. */
> if (is_gl_identifier(name))
> return GL_INVALID_INDEX;
> @@ -810,13 +806,22 @@ program_resource_location(struct gl_shader_program *shProg,
> */
> switch (res->Type) {
> case GL_PROGRAM_INPUT:
> + /* If the input is an array, fail if the index is out of bounds. */
> + if (array_index > 0
> + && array_index >= RESOURCE_VAR(res)->type->length) {
> + return -1;
> + }
> return RESOURCE_VAR(res)->data.location + array_index - VERT_ATTRIB_GENERIC0;
> case GL_PROGRAM_OUTPUT:
> + /* If the output is an array, fail if the index is out of bounds. */
> + if (array_index > 0
> + && array_index >= RESOURCE_VAR(res)->type->length) {
> + return -1;
> + }
> return RESOURCE_VAR(res)->data.location + array_index - FRAG_RESULT_DATA0;
> case GL_UNIFORM:
> - index = _mesa_get_uniform_location(shProg, name, &offset);
> -
> - if (index == GL_INVALID_INDEX)
> + /* If the uniform is built-in, fail. */
> + if (RESOURCE_UNI(res)->builtin)
> return -1;
>
> /* From the GL_ARB_uniform_buffer_object spec:
> @@ -830,17 +835,21 @@ program_resource_location(struct gl_shader_program *shProg,
> RESOURCE_UNI(res)->atomic_buffer_index != -1)
> return -1;
>
> - /* location in remap table + array element offset */
> - return RESOURCE_UNI(res)->remap_location + offset;
> -
> + /* fallthrough */
> case GL_VERTEX_SUBROUTINE_UNIFORM:
> case GL_GEOMETRY_SUBROUTINE_UNIFORM:
> case GL_FRAGMENT_SUBROUTINE_UNIFORM:
> case GL_COMPUTE_SUBROUTINE_UNIFORM:
> case GL_TESS_CONTROL_SUBROUTINE_UNIFORM:
> case GL_TESS_EVALUATION_SUBROUTINE_UNIFORM:
> - offset_ret = parse_program_resource_name(name, &base_name_end);
> - return RESOURCE_UNI(res)->remap_location + ((offset_ret != -1) ? offset_ret : 0);
> + /* If the uniform is an array, fail if the index is out of bounds. */
> + if (array_index > 0
> + && array_index >= RESOURCE_UNI(res)->array_elements) {
> + return -1;
> + }
> +
> + /* location in remap table + array element offset */
> + return RESOURCE_UNI(res)->remap_location + array_index;
> default:
> return -1;
> }
> @@ -854,14 +863,16 @@ GLint
> _mesa_program_resource_location(struct gl_shader_program *shProg,
> GLenum programInterface, const char *name)
> {
> + unsigned array_index = 0;
> struct gl_program_resource *res =
> - _mesa_program_resource_find_name(shProg, programInterface, name);
> + _mesa_program_resource_find_name(shProg, programInterface, name,
> + &array_index);
>
> /* Resource not found. */
> if (!res)
> return -1;
>
> - return program_resource_location(shProg, res, name);
> + return program_resource_location(shProg, res, name, array_index);
> }
>
> /**
> @@ -873,7 +884,7 @@ _mesa_program_resource_location_index(struct gl_shader_program *shProg,
> GLenum programInterface, const char *name)
> {
> struct gl_program_resource *res =
> - _mesa_program_resource_find_name(shProg, programInterface, name);
> + _mesa_program_resource_find_name(shProg, programInterface, name, NULL);
>
> /* Non-existent variable or resource is not referenced by fragment stage. */
> if (!res || !(res->StageReferences & (1 << MESA_SHADER_FRAGMENT)))
> @@ -949,7 +960,8 @@ get_buffer_property(struct gl_shader_program *shProg,
> for (unsigned i = 0; i < RESOURCE_UBO(res)->NumUniforms; i++) {
> const char *iname = RESOURCE_UBO(res)->Uniforms[i].IndexName;
> struct gl_program_resource *uni =
> - _mesa_program_resource_find_name(shProg, GL_UNIFORM, iname);
> + _mesa_program_resource_find_name(shProg, GL_UNIFORM, iname,
> + NULL);
> if (!uni)
> continue;
> (*val)++;
> @@ -959,7 +971,8 @@ get_buffer_property(struct gl_shader_program *shProg,
> for (unsigned i = 0; i < RESOURCE_UBO(res)->NumUniforms; i++) {
> const char *iname = RESOURCE_UBO(res)->Uniforms[i].IndexName;
> struct gl_program_resource *uni =
> - _mesa_program_resource_find_name(shProg, GL_UNIFORM, iname);
> + _mesa_program_resource_find_name(shProg, GL_UNIFORM, iname,
> + NULL);
> if (!uni)
> continue;
> *val++ =
> @@ -1099,7 +1112,8 @@ _mesa_program_resource_prop(struct gl_shader_program *shProg,
> case GL_PROGRAM_INPUT:
> case GL_PROGRAM_OUTPUT:
> *val = program_resource_location(shProg, res,
> - _mesa_program_resource_name(res));
> + _mesa_program_resource_name(res),
> + 0);
> return 1;
> default:
> goto invalid_operation;
> diff --git a/src/mesa/main/shaderapi.c b/src/mesa/main/shaderapi.c
> index 0887c68..b702dcd 100644
> --- a/src/mesa/main/shaderapi.c
> +++ b/src/mesa/main/shaderapi.c
> @@ -2229,7 +2229,7 @@ _mesa_GetSubroutineIndex(GLuint program, GLenum shadertype,
> }
>
> resource_type = _mesa_shader_stage_to_subroutine(stage);
> - res = _mesa_program_resource_find_name(shProg, resource_type, name);
> + res = _mesa_program_resource_find_name(shProg, resource_type, name, NULL);
> if (!res) {
> _mesa_error(ctx, GL_INVALID_OPERATION, "%s", api_name);
> return -1;
> diff --git a/src/mesa/main/shaderapi.h b/src/mesa/main/shaderapi.h
> index c081f74..0a10191 100644
> --- a/src/mesa/main/shaderapi.h
> +++ b/src/mesa/main/shaderapi.h
> @@ -232,7 +232,8 @@ _mesa_program_resource_index(struct gl_shader_program *shProg,
>
> extern struct gl_program_resource *
> _mesa_program_resource_find_name(struct gl_shader_program *shProg,
> - GLenum programInterface, const char *name);
> + GLenum programInterface, const char *name,
> + unsigned *array_index);
>
> extern struct gl_program_resource *
> _mesa_program_resource_find_index(struct gl_shader_program *shProg,
> diff --git a/src/mesa/main/uniforms.c b/src/mesa/main/uniforms.c
> index 6ba746e..ff1df72 100644
> --- a/src/mesa/main/uniforms.c
> +++ b/src/mesa/main/uniforms.c
> @@ -952,7 +952,7 @@ _mesa_GetUniformBlockIndex(GLuint program,
>
> struct gl_program_resource *res =
> _mesa_program_resource_find_name(shProg, GL_UNIFORM_BLOCK,
> - uniformBlockName);
> + uniformBlockName, NULL);
> if (!res)
> return GL_INVALID_INDEX;
>
> @@ -987,7 +987,8 @@ _mesa_GetUniformIndices(GLuint program,
>
> for (i = 0; i < uniformCount; i++) {
> struct gl_program_resource *res =
> - _mesa_program_resource_find_name(shProg, GL_UNIFORM, uniformNames[i]);
> + _mesa_program_resource_find_name(shProg, GL_UNIFORM, uniformNames[i],
> + NULL);
> uniformIndices[i] = _mesa_program_resource_index(shProg, res);
> }
> }
More information about the mesa-dev
mailing list