[Mesa-dev] [PATCH] glsl/linker: fix output variable overlap check

[Mark Janes]

This patch regressed piglit and gl cts tests for the i965 driver

https://bugs.freedesktop.org/show_bug.cgi?id=102904

Nicolai Hähnle <nhaehnle at gmail.com> writes:

> From: Nicolai Hähnle <nicolai.haehnle at amd.com>
>
> Prevent an overflow caused by too many output variables. To limit the
> scope of the issue, write to the assigned array only for the non-ES
> fragment shader path, which is the only place where it's needed.
>
> Since the function will bail with an error when output variables with
> overlapping components are found, (max # of FS outputs) * 4 is an upper
> limit to the space we need.
>
> Found by address sanitizer.
>
> Fixes dEQP-GLES3.functional.attribute_location.bind_aliasing.*
>
> Cc: mesa-stable at lists.freedesktop.org
> ---
>  src/compiler/glsl/linker.cpp | 17 +++++++++++------
>  1 file changed, 11 insertions(+), 6 deletions(-)
>
> diff --git a/src/compiler/glsl/linker.cpp b/src/compiler/glsl/linker.cpp
> index 5c3f1d12bbc..ddd8301a739 100644
> --- a/src/compiler/glsl/linker.cpp
> +++ b/src/compiler/glsl/linker.cpp
> @@ -2647,26 +2647,28 @@ assign_attribute_or_color_locations(void *mem_ctx,
>        {
>           const temp_attr *const l = (const temp_attr *) a;
>           const temp_attr *const r = (const temp_attr *) b;
>  
>           /* Reversed because we want a descending order sort below. */
>           return r->slots - l->slots;
>        }
>     } to_assign[32];
>     assert(max_index <= 32);
>  
> -   /* Temporary array for the set of attributes that have locations assigned.
> +   /* Temporary array for the set of attributes that have locations assigned,
> +    * for the purpose of checking overlapping slots/components of (non-ES)
> +    * fragment shader outputs.
>      */
> -   ir_variable *assigned[16];
> +   ir_variable *assigned[12 * 4];
> +   unsigned assigned_attr = 0;
>  
>     unsigned num_attr = 0;
> -   unsigned assigned_attr = 0;
>  
>     foreach_in_list(ir_instruction, node, sh->ir) {
>        ir_variable *const var = node->as_variable();
>  
>        if ((var == NULL) || (var->data.mode != (unsigned) direction))
>           continue;
>  
>        if (var->data.explicit_location) {
>           var->data.is_unmatched_generic_inout = 0;
>           if ((var->data.location >= (int)(max_index + generic_base))
> @@ -2878,20 +2880,26 @@ assign_attribute_or_color_locations(void *mem_ctx,
>                          if (assigned_component_mask & component_mask) {
>                             linker_error(prog, "overlapping component is "
>                                          "assigned to %ss %s and %s "
>                                          "(component=%d)\n",
>                                          string, assigned[i]->name, var->name,
>                                          var->data.location_frac);
>                             return false;
>                          }
>                       }
>                    }
> +
> +                  /* At most one variable per fragment output component should
> +                   * reach this. */
> +                  assert(assigned_attr < ARRAY_SIZE(assigned));
> +                  assigned[assigned_attr] = var;
> +                  assigned_attr++;
>                 } else if (target_index == MESA_SHADER_FRAGMENT ||
>                            (prog->IsES && prog->data->Version >= 300)) {
>                    linker_error(prog, "overlapping location is assigned "
>                                 "to %s `%s' %d %d %d\n", string, var->name,
>                                 used_locations, use_mask, attr);
>                    return false;
>                 } else {
>                    linker_warning(prog, "overlapping location is assigned "
>                                   "to %s `%s' %d %d %d\n", string, var->name,
>                                   used_locations, use_mask, attr);
> @@ -2917,23 +2925,20 @@ assign_attribute_or_color_locations(void *mem_ctx,
>               *
>               * Mark this attribute slot as taking up twice as much space
>               * so we can count it properly against limits.  According to
>               * issue (3) of the GL_ARB_vertex_attrib_64bit behavior, this
>               * is optional behavior, but it seems preferable.
>               */
>              if (var->type->without_array()->is_dual_slot())
>                 double_storage_locations |= (use_mask << attr);
>           }
>  
> -         assigned[assigned_attr] = var;
> -         assigned_attr++;
> -
>           continue;
>        }
>  
>        if (num_attr >= max_index) {
>           linker_error(prog, "too many %s (max %u)",
>                        target_index == MESA_SHADER_VERTEX ?
>                        "vertex shader inputs" : "fragment shader outputs",
>                        max_index);
>           return false;
>        }
> -- 
> 2.11.0
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev