[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:27:41 PDT 2015
On 07/30/2015 08:25 AM, Tapani Pälli wrote:
> 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.
>
Woops, make it "ES31-CTS.program_interface_query.uniform-types", not
"ES31-CTS.program_interface_query.uniform". It looks to me that most
tests fail just because of not figuring out the correct index for the
resource and that can be fixed by a valid_program_resource_index_name
change.
>
> 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);
>> }
>> }
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
More information about the mesa-dev
mailing list