[Mesa-dev] [PATCH V2 3/4] mesa: remove overlapping array index validation and extra uniform location lookup
Timothy Arceri
t_arceri at yahoo.com.au
Fri Jul 3 15:48:55 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 simplified saving an extra hash table lookup (and yet another
validation call).
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 | 66 ++-----------
src/mesa/main/shader_query.cpp | 204 ++++++++++++++++-----------------------
src/mesa/main/shaderapi.h | 7 +-
src/mesa/main/uniforms.c | 7 +-
4 files changed, 100 insertions(+), 184 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 0473c2e..3b08ea1 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_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;
+ /* 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;
@@ -791,16 +748,19 @@ program_resource_location(struct gl_shader_program *shProg,
*/
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);
}
/**
@@ -812,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)))
@@ -884,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)++;
@@ -894,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++ =
@@ -1032,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/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);
}
}
--
2.4.3
More information about the mesa-dev
mailing list