[Mesa-dev] [PATCH 2/2] mesa: remove duplicate array index validation and extra uniform location lookup
Timothy Arceri
t_arceri at yahoo.com.au
Fri Jul 3 03:55:48 PDT 2015
This change removes 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 merged saving an extra hash table lookup (and yet another validation call).
Cc: Martin Peres <martin.peres at linux.intel.com>
Cc: Tapani Pälli <tapani.palli at intel.com>
---
This clean-up was done to allow AoA program interface query support to be
implemented more easily. When applied to my latest AoA work all but one
AoA program interface query subtest for the CTS now passes.
No Piglit regressions.
I also ran the following tests in the CTS without regression:
- ES31-CTS.program_interface_query*
- ES3-CTS.gtf.GL3Tests.explicit_attrib_location.explicit_attrib_location_room
- ES3-CTS.gtf.GL3Tests.draw_instanced.draw_instanced_max_vertex_attribs
src/mesa/main/program_resource.c | 66 ++-----------
src/mesa/main/shader_query.cpp | 206 ++++++++++++++++-----------------------
src/mesa/main/shaderapi.h | 7 +-
src/mesa/main/uniform_query.cpp | 75 --------------
src/mesa/main/uniforms.c | 7 +-
src/mesa/main/uniforms.h | 4 -
6 files changed, 100 insertions(+), 265 deletions(-)
diff --git a/src/mesa/main/program_resource.c b/src/mesa/main/program_resource.c
index d857b84..0a306b8 100644
--- a/src/mesa/main/program_resource.c
+++ b/src/mesa/main/program_resource.c
@@ -173,32 +173,12 @@ is_xfb_marker(const char *str)
return false;
}
-/**
- * Checks if given name index is legal for GetProgramResourceIndex,
- * check is written to be compatible with GL_ARB_array_of_arrays.
- */
-static bool
-valid_program_resource_index_name(const GLchar *name)
-{
- const char *array = strstr(name, "[");
- const char *close = strrchr(name, ']');
-
- /* Not array, no need for the check. */
- if (!array)
- return true;
-
- /* Last array index has to be zero. */
- if (!close || *--close != '0')
- return false;
-
- return true;
-}
-
GLuint GLAPIENTRY
_mesa_GetProgramResourceIndex(GLuint program, GLenum programInterface,
const GLchar *name)
{
GET_CURRENT_CONTEXT(ctx);
+ unsigned array_index;
struct gl_program_resource *res;
struct gl_shader_program *shProg =
_mesa_lookup_shader_program_err(ctx, program,
@@ -221,12 +201,9 @@ _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);
+ res = _mesa_program_resource_find_name(shProg, programInterface, name,
+ &array_index, true);
if (!res)
return GL_INVALID_INDEX;
@@ -300,36 +277,6 @@ _mesa_GetProgramResourceiv(GLuint program, GLenum programInterface,
propCount, props, bufSize, length, params);
}
-/**
- * Function verifies syntax of given name for GetProgramResourceLocation
- * and GetProgramResourceLocationIndex for the following cases:
- *
- * "array element portion of a string passed to GetProgramResourceLocation
- * or GetProgramResourceLocationIndex must not have, a "+" sign, extra
- * leading zeroes, or whitespace".
- *
- * Check is written to be compatible with GL_ARB_array_of_arrays.
- */
-static bool
-invalid_array_element_syntax(const GLchar *name)
-{
- char *first = strchr(name, '[');
- char *last = strrchr(name, '[');
-
- if (!first)
- return false;
-
- /* No '+' or ' ' allowed anywhere. */
- if (strchr(first, '+') || strchr(first, ' '))
- return true;
-
- /* Check that last array index is 0. */
- if (last[1] == '0' && last[2] != ']')
- return true;
-
- return false;
-}
-
static struct gl_shader_program *
lookup_linked_program(GLuint program, const char *caller)
{
@@ -356,7 +303,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. */
@@ -383,7 +330,8 @@ _mesa_GetProgramResourceLocation(GLuint program, GLenum programInterface,
_mesa_lookup_enum_by_nr(programInterface), name);
}
- return _mesa_program_resource_location(shProg, programInterface, name);
+ return _mesa_program_resource_location(shProg, programInterface, name,
+ true);
}
/**
@@ -397,7 +345,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 a6246a3..46901cc 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.
@@ -189,63 +190,6 @@ _mesa_GetActiveAttrib(GLhandleARB program, GLuint desired_index,
(GLint *) type, "glGetActiveAttrib");
}
-/* Locations associated with shader variables (array or non-array) can be
- * queried using its base name or using the base name appended with the
- * valid array index. For example, in case of below vertex shader, valid
- * queries can be made to know the location of "xyz", "array", "array[0]",
- * "array[1]", "array[2]" and "array[3]". In this example index reurned
- * will be 0, 0, 0, 1, 2, 3 respectively.
- *
- * [Vertex Shader]
- * layout(location=0) in vec4 xyz;
- * layout(location=1) in vec4[4] array;
- * void main()
- * { }
- *
- * This requirement came up with the addition of ARB_program_interface_query
- * to OpenGL 4.3 specification. See page 101 (page 122 of the PDF) for details.
- *
- * This utility function is used by:
- * _mesa_GetAttribLocation
- * _mesa_GetFragDataLocation
- * _mesa_GetFragDataIndex
- *
- * Returns 0:
- * if the 'name' string matches var->name.
- * Returns 'matched index':
- * if the 'name' string matches var->name appended with valid array index.
- */
-int static inline
-get_matching_index(const ir_variable *const var, const char *name) {
- unsigned idx = 0;
- const char *const paren = strchr(name, '[');
- const unsigned len = (paren != NULL) ? paren - name : strlen(name);
-
- if (paren != NULL) {
- if (!var->type->is_array())
- return -1;
-
- char *endptr;
- idx = (unsigned) strtol(paren + 1, &endptr, 10);
- const unsigned idx_len = endptr != (paren + 1) ? endptr - paren - 1 : 0;
-
- /* Validate the sub string representing index in 'name' string */
- if ((idx > 0 && paren[1] == '0') /* leading zeroes */
- || (idx == 0 && idx_len > 1) /* all zeroes */
- || paren[1] == ' ' /* whitespace */
- || endptr[0] != ']' /* closing brace */
- || endptr[1] != '\0' /* null char */
- || idx_len == 0 /* missing index */
- || idx >= var->type->length) /* exceeding array bound */
- return -1;
- }
-
- if (strncmp(var->name, name, len) == 0 && var->name[len] == '\0')
- return idx;
-
- return -1;
-}
-
GLint GLAPIENTRY
_mesa_GetAttribLocation(GLhandleARB program, const GLcharARB * name)
{
@@ -271,13 +215,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, false);
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
@@ -455,13 +401,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, false);
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
@@ -525,39 +473,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, bool is_program_query)
{
- 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)
@@ -567,26 +508,36 @@ _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:
- 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:
+ 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)) {
+ if (is_program_query && *array_index > 0)
+ return NULL;
+ 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;
@@ -736,17 +687,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;
-
- 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;
@@ -757,15 +700,29 @@ 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_UNI(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_UNI(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;
+ /* 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;
+ }
/* From the GL_ARB_uniform_buffer_object spec:
*
* "The value -1 will be returned if <name> does not correspond to an
@@ -778,7 +735,7 @@ program_resource_location(struct gl_shader_program *shProg,
return -1;
/* location in remap table + array element offset */
- return RESOURCE_UNI(res)->remap_location + offset;
+ return RESOURCE_UNI(res)->remap_location + array_index;
default:
return -1;
@@ -787,22 +744,23 @@ program_resource_location(struct gl_shader_program *shProg,
/**
* Function implements following location queries:
- * glGetAttribLocation
- * glGetFragDataLocation
* glGetUniformLocation
*/
GLint
_mesa_program_resource_location(struct gl_shader_program *shProg,
- GLenum programInterface, const char *name)
+ GLenum programInterface, const char *name,
+ bool is_resource_query)
{
+ 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, false);
/* Resource not found. */
if (!res)
return -1;
- return program_resource_location(shProg, res, name);
+ return program_resource_location(shProg, res, name, array_index);
}
/**
@@ -814,7 +772,8 @@ _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, false);
/* Non-existent variable or resource is not referenced by fragment stage. */
if (!res || !(res->StageReferences & (1 << MESA_SHADER_FRAGMENT)))
@@ -886,7 +845,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, false);
if (!uni)
continue;
(*val)++;
@@ -896,7 +856,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, false);
if (!uni)
continue;
*val++ =
@@ -1034,7 +995,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.h b/src/mesa/main/shaderapi.h
index aba6d5d..186b10d 100644
--- a/src/mesa/main/shaderapi.h
+++ b/src/mesa/main/shaderapi.h
@@ -232,7 +232,9 @@ _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,
+ bool is_resource_query);
extern struct gl_program_resource *
_mesa_program_resource_find_index(struct gl_shader_program *shProg,
@@ -246,7 +248,8 @@ _mesa_get_program_resource_name(struct gl_shader_program *shProg,
extern GLint
_mesa_program_resource_location(struct gl_shader_program *shProg,
- GLenum programInterface, const char *name);
+ GLenum programInterface, const char *name,
+ bool is_resource_query);
extern GLint
_mesa_program_resource_location_index(struct gl_shader_program *shProg,
diff --git a/src/mesa/main/uniform_query.cpp b/src/mesa/main/uniform_query.cpp
index cab5083..680ba16 100644
--- a/src/mesa/main/uniform_query.cpp
+++ b/src/mesa/main/uniform_query.cpp
@@ -978,81 +978,6 @@ _mesa_uniform_matrix(struct gl_context *ctx, struct gl_shader_program *shProg,
}
-/**
- * Called via glGetUniformLocation().
- *
- * Returns the uniform index into UniformStorage (also the
- * glGetActiveUniformsiv uniform index), and stores the referenced
- * array offset in *offset, or GL_INVALID_INDEX (-1).
- */
-extern "C" unsigned
-_mesa_get_uniform_location(struct gl_shader_program *shProg,
- const GLchar *name,
- unsigned *out_offset)
-{
- /* Page 80 (page 94 of the PDF) of the OpenGL 2.1 spec says:
- *
- * "The first element of a uniform array is identified using the
- * name of the uniform array appended with "[0]". Except if the last
- * part of the string name indicates a uniform array, then the
- * location of the first element of that array can be retrieved by
- * either using the name of the uniform array, or the name of the
- * uniform array appended with "[0]"."
- *
- * Note: since uniform names are not allowed to use whitespace, and array
- * indices within uniform names are not allowed to use "+", "-", or leading
- * zeros, it follows that each uniform has a unique name up to the possible
- * ambiguity with "[0]" noted above. Therefore we don't need to worry
- * about mal-formed inputs--they will properly fail when we try to look up
- * the uniform name in shProg->UniformHash.
- */
-
- const GLchar *base_name_end;
- long offset = parse_program_resource_name(name, &base_name_end);
- bool array_lookup = offset >= 0;
- char *name_copy;
-
- if (array_lookup) {
- name_copy = (char *) malloc(base_name_end - name + 1);
- memcpy(name_copy, name, base_name_end - name);
- name_copy[base_name_end - name] = '\0';
- } else {
- name_copy = (char *) name;
- offset = 0;
- }
-
- unsigned location = 0;
- const bool found = shProg->UniformHash->get(location, name_copy);
-
- assert(!found
- || strcmp(name_copy, shProg->UniformStorage[location].name) == 0);
-
- /* Free the temporary buffer *before* possibly returning an error.
- */
- if (name_copy != name)
- free(name_copy);
-
- if (!found)
- return GL_INVALID_INDEX;
-
- /* If the uniform is built-in, fail. */
- if (shProg->UniformStorage[location].builtin)
- return GL_INVALID_INDEX;
-
- /* If the uniform is an array, fail if the index is out of bounds.
- * (A negative index is caught above.) This also fails if the uniform
- * is not an array, but the user is trying to index it, because
- * array_elements is zero and offset >= 0.
- */
- if (array_lookup
- && offset >= (long) shProg->UniformStorage[location].array_elements) {
- return GL_INVALID_INDEX;
- }
-
- *out_offset = offset;
- return location;
-}
-
extern "C" bool
_mesa_sampler_uniforms_are_valid(const struct gl_shader_program *shProg,
char *errMsg, size_t errMsgLength)
diff --git a/src/mesa/main/uniforms.c b/src/mesa/main/uniforms.c
index 5548d1d..7c9ff7e 100644
--- a/src/mesa/main/uniforms.c
+++ b/src/mesa/main/uniforms.c
@@ -930,7 +930,7 @@ _mesa_GetUniformLocation(GLuint programObj, const GLcharARB *name)
return -1;
}
- return _mesa_program_resource_location(shProg, GL_UNIFORM, name);
+ return _mesa_program_resource_location(shProg, GL_UNIFORM, name, false);
}
GLuint GLAPIENTRY
@@ -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, false);
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, false);
uniformIndices[i] = _mesa_program_resource_index(shProg, res);
}
}
diff --git a/src/mesa/main/uniforms.h b/src/mesa/main/uniforms.h
index bd7b05e..e62eaa5 100644
--- a/src/mesa/main/uniforms.h
+++ b/src/mesa/main/uniforms.h
@@ -343,10 +343,6 @@ void GLAPIENTRY
_mesa_ProgramUniformMatrix4x3dv(GLuint program, GLint location, GLsizei count,
GLboolean transpose, const GLdouble *value);
-unsigned
-_mesa_get_uniform_location(struct gl_shader_program *shProg,
- const GLchar *name, unsigned *offset);
-
void
_mesa_uniform(struct gl_context *ctx, struct gl_shader_program *shader_program,
GLint location, GLsizei count,
--
2.4.3
More information about the mesa-dev
mailing list