[Mesa-dev] [PATCH 16/19] glsl: add AoA support to resource name parsing
Timothy Arceri
t_arceri at yahoo.com.au
Sat Jun 20 07:06:34 PDT 2015
Grrr. Not sure how I missed it but this patch breaks transform feedback.
It doesn't seem like a good idea to share this code between the two
codepaths any more, seems like Paul had plains to use it in more places
when it was added but it never happened.
I'll send a version 2 of this patch tomorrow.
On Sat, 2015-06-20 at 22:33 +1000, Timothy Arceri wrote:
> Updated to parse arrays of arrays and return the correct offset.
>
> We are now also validating the array subscript rather than potentially
> returning an offset that will be out of bounds.
> ---
> src/glsl/link_uniforms.cpp | 2 +-
> src/glsl/link_varyings.cpp | 7 +--
> src/glsl/link_varyings.h | 3 +-
> src/glsl/linker.cpp | 108 ++++++++++++++++++++++++++++++++--------
> src/glsl/program.h | 3 +-
> src/mesa/main/uniform_query.cpp | 2 +-
> 6 files changed, 97 insertions(+), 28 deletions(-)
>
> diff --git a/src/glsl/link_uniforms.cpp b/src/glsl/link_uniforms.cpp
> index 6b6b197..38400d9 100644
> --- a/src/glsl/link_uniforms.cpp
> +++ b/src/glsl/link_uniforms.cpp
> @@ -621,7 +621,7 @@ private:
> }
>
> this->uniforms[id].name = ralloc_strdup(this->uniforms, name);
> - this->uniforms[id].type = base_type;
> + this->uniforms[id].type = type;
> this->uniforms[id].initialized = 0;
> this->uniforms[id].num_driver_storage = 0;
> this->uniforms[id].driver_storage = NULL;
> diff --git a/src/glsl/link_varyings.cpp b/src/glsl/link_varyings.cpp
> index 6cb55b3..d97af8f 100644
> --- a/src/glsl/link_varyings.cpp
> +++ b/src/glsl/link_varyings.cpp
> @@ -292,7 +292,7 @@ cross_validate_outputs_to_inputs(struct gl_shader_program *prog,
> */
> void
> tfeedback_decl::init(struct gl_context *ctx, const void *mem_ctx,
> - const char *input)
> + const char *input, struct gl_shader_program *shProg)
> {
> /* We don't have to be pedantic about what is a valid GLSL variable name,
> * because any variable with an invalid name can't exist in the IR anyway.
> @@ -329,7 +329,8 @@ tfeedback_decl::init(struct gl_context *ctx, const void *mem_ctx,
>
> /* Parse a declaration. */
> const char *base_name_end;
> - long subscript = parse_program_resource_name(input, &base_name_end);
> + long subscript = parse_program_resource_name(input, &base_name_end,
> + shProg);
> this->var_name = ralloc_strndup(mem_ctx, input, base_name_end - input);
> if (this->var_name == NULL) {
> _mesa_error_no_memory(__func__);
> @@ -574,7 +575,7 @@ parse_tfeedback_decls(struct gl_context *ctx, struct gl_shader_program *prog,
> char **varying_names, tfeedback_decl *decls)
> {
> for (unsigned i = 0; i < num_names; ++i) {
> - decls[i].init(ctx, mem_ctx, varying_names[i]);
> + decls[i].init(ctx, mem_ctx, varying_names[i], prog);
>
> if (!decls[i].is_varying())
> continue;
> diff --git a/src/glsl/link_varyings.h b/src/glsl/link_varyings.h
> index afc16a8..443d1ca 100644
> --- a/src/glsl/link_varyings.h
> +++ b/src/glsl/link_varyings.h
> @@ -91,7 +91,8 @@ struct tfeedback_candidate
> class tfeedback_decl
> {
> public:
> - void init(struct gl_context *ctx, const void *mem_ctx, const char *input);
> + void init(struct gl_context *ctx, const void *mem_ctx,
> + const char *input, struct gl_shader_program *shProg);
> static bool is_same(const tfeedback_decl &x, const tfeedback_decl &y);
> bool assign_location(struct gl_context *ctx,
> struct gl_shader_program *prog);
> diff --git a/src/glsl/linker.cpp b/src/glsl/linker.cpp
> index 3494464..66d5706 100644
> --- a/src/glsl/linker.cpp
> +++ b/src/glsl/linker.cpp
> @@ -377,18 +377,18 @@ linker_warning(gl_shader_program *prog, const char *fmt, ...)
>
> /**
> * Given a string identifying a program resource, break it into a base name
> - * and an optional array index in square brackets.
> + * and optional array indices in square brackets.
> *
> - * If an array index is present, \c out_base_name_end is set to point to the
> - * "[" that precedes the array index, and the array index itself is returned
> - * as a long.
> + * If array indices are present, \c out_base_name_end is set to point to the
> + * "[" that precedes the first array index, and the an array offset is
> + * returned as a long.
> *
> * If no array index is present (or if the array index is negative or
> * mal-formed), \c out_base_name_end, is set to point to the null terminator
> * at the end of the input string, and -1 is returned.
> *
> - * Only the final array index is parsed; if the string contains other array
> - * indices (or structure field accesses), they are left in the base name.
> + * Only the final array indices are parsed; if the string contains other array
> + * indices such as structure field accesses, they are left in the base name.
> *
> * No attempt is made to check that the base name is properly formed;
> * typically the caller will look up the base name in a hash table, so
> @@ -396,7 +396,8 @@ linker_warning(gl_shader_program *prog, const char *fmt, ...)
> */
> long
> parse_program_resource_name(const GLchar *name,
> - const GLchar **out_base_name_end)
> + const GLchar **out_base_name_end,
> + struct gl_shader_program *shProg)
> {
> /* Section 7.3.1 ("Program Interfaces") of the OpenGL 4.3 spec says:
> *
> @@ -406,31 +407,96 @@ parse_program_resource_name(const GLchar *name,
> * string will not include white space anywhere in the string."
> */
>
> + long offset = 0;
> const size_t len = strlen(name);
> *out_base_name_end = name + len;
>
> if (len == 0 || name[len-1] != ']')
> return -1;
>
> - /* Walk backwards over the string looking for a non-digit character. This
> - * had better be the opening bracket for an array index.
> - *
> - * Initially, i specifies the location of the ']'. Since the string may
> - * contain only the ']' charcater, walk backwards very carefully.
> - */
> - unsigned i;
> - for (i = len - 1; (i > 0) && isdigit(name[i-1]); --i)
> - /* empty */ ;
> + /* count number of dimensions, and validate the subscripts */
> + unsigned i = len - 1;
> + unsigned num_dimensions = 0;
> + bool subscript_end = true;
> + for (num_dimensions=0; i > 0; --i ) {
> + if (subscript_end) {
> + if (name[i] == ']') {
> + num_dimensions++;
> + subscript_end = false;
> + } else {
> + break; /* no more subscripts */
> + }
> + } else {
> + if (isdigit(name[i]))
> + continue;
>
> - if ((i == 0) || name[i-1] != '[')
> + if (name[i] == '[') {
> + subscript_end = true;
> + } else {
> + /* invalid subscript, just bail out here
> + * e.g. "myUniform23]","myUniform2][3]"
> + */
> + return -1;
> + }
> + }
> + }
> +
> + if((name[i] == ']' || name[i] == '[' || isdigit(name[i])) && i == 0) {
> + /* invalid name e.g. "][23]","][2][3]"
> + * or "[23]","[2][3]" or 1[23]","1[2][3]","123]","12][3]"
> + */
> return -1;
> + }
> +
> + *out_base_name_end = name + i + 1;
> +
> + const glsl_type *type;
> + char *name_copy = (char *) malloc(*out_base_name_end - name + 1);
> + memcpy(name_copy, name, *out_base_name_end - name);
> + name_copy[*out_base_name_end - name] = '\0';
> +
> + unsigned location = 0;
> + const bool UNUSED found = shProg->UniformHash->get(location, name_copy);
> +
> + assert(!found
> + || strcmp(name_copy, shProg->UniformStorage[location].name) == 0);
> +
> + type = shProg->UniformStorage[location].type;
> + free(name_copy);
> +
> + /* validate against the found uniform and calculate the offset */
> + i = len - 1;
> + unsigned curr_dim = num_dimensions;
> + while(curr_dim--) {
> + /* walk backwards to find the first digit in the subscript */
> + for (; (i > 0) && isdigit(name[i-1]); --i)
> + /* empty */ ;
> +
> + long array_index = strtol(&name[i], NULL, 10);
> + if (array_index < 0)
> + return -1;
> +
> + /* the subscript dimensions don't match the uniform we found */
> + if (!type->is_array())
> + return -1;
> +
> + offset += pow(type->length, curr_dim) * array_index;
> + type = type->fields.array;
> +
> + /* check for arrays of arrays */
> + if (i > 2 && name[i-2] == ']') {
> + i-=2;
> + } else {
> + i--;
> + break;
> + }
> + }
>
> - long array_index = strtol(&name[i], NULL, 10);
> - if (array_index < 0)
> + /* the subscript dimensions don't match the uniform we found */
> + if (type->is_array())
> return -1;
>
> - *out_base_name_end = name + (i - 1);
> - return array_index;
> + return offset;
> }
>
>
> diff --git a/src/glsl/program.h b/src/glsl/program.h
> index f15113a..81c1bad 100644
> --- a/src/glsl/program.h
> +++ b/src/glsl/program.h
> @@ -49,4 +49,5 @@ linker_warning(struct gl_shader_program *prog, const char *fmt, ...)
>
> extern long
> parse_program_resource_name(const GLchar *name,
> - const GLchar **out_base_name_end);
> + const GLchar **out_base_name_end,
> + struct gl_shader_program *shProg);
> diff --git a/src/mesa/main/uniform_query.cpp b/src/mesa/main/uniform_query.cpp
> index c8b0b58..f7a5c87 100644
> --- a/src/mesa/main/uniform_query.cpp
> +++ b/src/mesa/main/uniform_query.cpp
> @@ -1013,7 +1013,7 @@ _mesa_get_uniform_location(struct gl_shader_program *shProg,
> */
>
> const GLchar *base_name_end;
> - long offset = parse_program_resource_name(name, &base_name_end);
> + long offset = parse_program_resource_name(name, &base_name_end, shProg);
> bool array_lookup = offset >= 0;
> char *name_copy;
>
More information about the mesa-dev
mailing list