On Wed, Jul 29, 2015 at 6:56 AM, Timothy Arceri <t_arc...@yahoo.com.au> 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.pa...@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)
parse_program_resource_name returns a long, which you're storing in an
unsigned and the comparing with < 0. I think something's broken here,
but I don't know what.
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
