[Mesa-dev] [Mesa-stable] [PATCH] mesa: Fix querying location of nth element of an array variable

[Ian Romanick]

On 04/03/2014 04:13 PM, Anuj Phogat wrote:
> This patch changes the behavior of glGetAttribLocation(),
> glGetFragDataLocation() and glGetFragDataIndex() functions.
> 
> Code changes handle the cases described in following example:
> vertex shader:
> layout(location = 1)in vec4[4] a;
> void main()
> {
> }
> 
> fragment shader:
> layout (location = 1) out vec4[4] c;
> void main()
> {
> }
> 
> Currently glGetAttribLocation("a") and glGetFragDataLocation("c") returns
> 1. glGetAttribLocation("a[i]") and glGetFragDataLocation("c[i]"), where
> i = {0, 1, 2, 3}, returns -1. But expected locations for array indices
> are: 1, 2, 3 and 4 respectively.
> 
> OpenGL spec doesn't say anything specific about getting the location
> of an array element. But it looks sensible to allow such query.
> 
> Fixes failures in Khronos OpenGL CTS tests:
> explicit_attrib_location_room
> draw_instanced_max_vertex_attribs
> 
> Proprietary linux drivers of AMD and NVIDIA matches current output on
> Mesa (i.e. returns -1).

I think this is a change (clarification?) that occured when
ARB_program_interface_query was added in OpenGL 4.3.  (Did the drivers
you tested support OpenGL 4.3?)  Specifically, the page 345 (page 367 of
the PDF) of the OpenGL 4.4 spec says:

    "Otherwise, [GetAttribLocation] is equivalent to

        GetProgramResourceLocation(program, PROGRAM_INPUT, name);"

And page 107 (page 192 of the PDF) says:

    "A string provided to GetProgramResourceLocation or
    GetProgramResourceLocationIndex is considered to match an active
    variable if

    • the string exactly matches the name of the active variable;
    • if the string identifies the base name of an active array, where
      the string would exactly match the name of the variable if the
      suffix "[0]" were appended to the string; or
    • if the string identifies an active element of the array, where
      the string ends with the concatenation of the "[" character, an
      integer (with no "+" sign, extra leading zeroes, or whitespace)
      identifying an array element, and the "]" character, the integer
      is less than the number of active elements of the array variable,
      and where the string would exactly match the enumerated name of
      the array if the decimal integer were replaced with zero."

We should capture this information in the commit message.

I think this code is much more complex than necessary.  It seems like
the inner search looks should be something like:

   const char *const paren = strchr(name, '[');
   const unsigned len = (paren != NULL) ? paren - name : strlen(name);
   unsigned idx = 0;

   ...

   if (paren != NULL) {
      /* Validate format of index. */
      ...

      idx = (unsigned) strtol(paren + 1, 10, NULL);
   }

   for (...) {
      if (strncmp(var->name, name, len) && var->name[len] == '\0') {
         /* This part should be refactored. */
         if (var->type->is_array()) {
            if (idx >= var->type->length)
                /* error */

            /* return something sensible. */
         } else {
            /* Can't have an array index if variable isn't an array.
             */
            if (paren != NULL)
                /* error */

            /* return something sensible. */
         }
      }
   }

> Cc: <mesa-stable at lists.freedesktop.org>
> Signed-off-by: Anuj Phogat <anuj.phogat at gmail.com>
> ---
>  src/mesa/main/shader_query.cpp | 76 +++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 75 insertions(+), 1 deletion(-)
> 
> diff --git a/src/mesa/main/shader_query.cpp b/src/mesa/main/shader_query.cpp
> index e1afe53..2256482 100644
> --- a/src/mesa/main/shader_query.cpp
> +++ b/src/mesa/main/shader_query.cpp
> @@ -131,6 +131,59 @@ _mesa_GetActiveAttrib(GLhandleARB program, GLuint desired_index,
>     _mesa_error(ctx, GL_INVALID_VALUE, "glGetActiveAttrib(index)");
>  }
>  
> +/* This function checks if the 'name' matches with var->name appended with
> + * valid array index for the variable. For example the var->name = "color"
> + * is declared as an array in shader program:
> + * in vec4[4] color;
> + *
> + * Match the queried 'name' with "color[0]", "color[1]", "color[2]" and
> + * "color[3]" to return the matched index of array.
> + */
> +int static inline
> +get_matching_array_index(const ir_variable *const var,
> +                         const char *name,
> +                         const char *gl_func_name) {
> +   /* Allowed array length for an input attribute or fragment shader output
> +    * is not exceeding 100 anytime soon.
> +    */
> +   assert(var->type->length >= 0 && var->type->length <= 100);
> +   GET_CURRENT_CONTEXT(ctx);
> +
> +   const char *str1 = var->name;
> +   const char *str2 = "[0]";
> +   int len1 = strlen(str1);
> +   int len2 = strlen(str2);
> +   char *result = (char*) malloc(len1 + len2
> +                                 + 1 /* space for extra digit */
> +                                 + 1 /* space for null char */);
> +   if (result == NULL) {
> +      _mesa_error(ctx, GL_OUT_OF_MEMORY, gl_func_name);
> +      return -1;
> +   }
> +   memcpy(result, str1, len1);
> +   memcpy(result + len1, str2, len2 + 1);
> +   int len = strlen(result);
> +
> +   for (unsigned i = 0; i < var->type->length; i++) {
> +      /* Increment the array index */
> +      if (i < 10)
> +         result[len - 2] = (char)(((int)'0') + i);
> +      else {
> +         /* We allocated extra spaces to handle double digit indices */
> +         result[len - 2] = (char)(((int)'0') + i / 10);
> +         result[len - 1] = (char)(((int)'0') + i % 10);
> +         result[len] = ']';
> +         result[len + 1] = '\0';
> +      }
> +      if (strcmp(result, name) == 0) {
> +         free(result);
> +         return i;
> +      }
> +   }
> +   free(result);
> +   return -1;
> +}
> +
>  GLint GLAPIENTRY
>  _mesa_GetAttribLocation(GLhandleARB program, const GLcharARB * name)
>  {
> @@ -176,6 +229,14 @@ _mesa_GetAttribLocation(GLhandleARB program, const GLcharARB * name)
>  
>        if (strcmp(var->name, name) == 0)
>  	 return var->data.location - VERT_ATTRIB_GENERIC0;
> +
> +      if (var->type->length > 0) {
> +         int index = get_matching_array_index(var,
> +                                              (const char *) name,
> +                                              "glGetAttribLocation()");
> +         if (index >= 0)
> +            return var->data.location + index - VERT_ATTRIB_GENERIC0;
> +      }
>     }
>  
>     return -1;
> @@ -338,8 +399,13 @@ _mesa_GetFragDataIndex(GLuint program, const GLchar *name)
>            || var->data.location < FRAG_RESULT_DATA0)
>           continue;
>  
> -      if (strcmp(var->name, name) == 0)
> +      if (strcmp(var->name, name) == 0
> +          || (var->type->length > 0
> +              && get_matching_array_index(var,
> +                                          (const char *) name,
> +                                          "glGetFragDataIndex()") >= 0)) {
>           return var->data.index;
> +      }
>     }
>  
>     return -1;
> @@ -396,6 +462,14 @@ _mesa_GetFragDataLocation(GLuint program, const GLchar *name)
>  
>        if (strcmp(var->name, name) == 0)
>  	 return var->data.location - FRAG_RESULT_DATA0;
> +
> +      if (var->type->length > 0) {
> +         int index = get_matching_array_index(var,
> +                                              (const char *) name,
> +                                              "glGetFragDataLocation()");
> +         if (index >= 0)
> +            return var->data.location + index - FRAG_RESULT_DATA0;
> +      }
>     }
>  
>     return -1;
>