[Mesa-dev] [PATCH 01/20] mesa: fix and simplify resource query for arrays
Timothy Arceri
t_arceri at yahoo.com.au
Sat Aug 1 18:40:06 PDT 2015
On Sat, 2015-08-01 at 15:58 -0700, Matt Turner wrote:
> On Wed, Jul 29, 2015 at 6:56 AM, Timothy Arceri <t_arceri at 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.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)
>
> 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.
Right thanks. Not sure how this didn't cause any tests to fail, I know there
are tests for it, maybe gcc is smarter than me.
parse_program_resource_name returns -1 when the index is invalid this needs to
be tested before assigning the value to array_index.
In link_varyings.cpp after the -1 check is done the value is just assigned to
an unsigned variable so it seems long is just used so we can return the -1
rather than actually expecting index values to be ridiculously large.
More information about the mesa-dev
mailing list