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

[Timothy Arceri]

This seems reasonable, thanks for fixing. Maybe just add a comment in 
the code also.

/* 12 * 4 == (max # of FS outputs) * max components */

Or something similar.

Reviewed-by: Timothy Arceri <tarceri at itsqueeze.com>

On 18/09/17 19:30, Nicolai Hähnle wrote:
> 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;
>         }
>