[Mesa-dev] [PATCH 6/8] glsl: Fallback gracefully if ir_set_program_inouts sees unexpected indexing.

[Kenneth Graunke]

On 07/31/2013 02:17 PM, Paul Berry wrote:
> The code in ir_set_program_inouts that marks just a portion of a
> variable as used (rather than the whole variable) only works on a few
> kinds of indexing operations:
>
> - Indexing into matrices
> - Indexing into arrays of matrices, vectors, or scalars.
>
> Fortunately these are the only kinds of indexing operations that we
> expect to see; everything else is either handled by a
> previously-executed lowering pass or prohibited by GLSL.
>
> However, that could conceivably change in the future (the GLSL rules
> might change, or we might modify the lowering passes).  To avoid
> mysterious bugs in the future, let's have ir_set_program_inouts report
> an assertion failure if it ever encounters an unexpected kind of
> indexing operation (and in release builds, fall back to just marking
> the whole variable as used).
> ---
>   src/glsl/ir_set_program_inouts.cpp | 23 +++++++++++++++++++++++
>   1 file changed, 23 insertions(+)
>
> diff --git a/src/glsl/ir_set_program_inouts.cpp b/src/glsl/ir_set_program_inouts.cpp
> index 8774338..b4588a7 100644
> --- a/src/glsl/ir_set_program_inouts.cpp
> +++ b/src/glsl/ir_set_program_inouts.cpp
> @@ -144,6 +144,29 @@ ir_set_program_inouts_visitor::try_mark_partial_variable(ir_variable *var,
>   {
>      const glsl_type *type = var->type;
>
> +   /* The code below only handles:
> +    *
> +    * - Indexing into matrices
> +    * - Indexing into arrays of (matrices, vectors, or scalars)
> +    *
> +    * All other possibilities are either prohibited by GLSL (vertex inputs and
> +    * fragment outputs can't be structs) or should have been eliminated by
> +    * lowering passes (do_vec_index_to_swizzle() gets rid of indexing into
> +    * vectors, and lower_packed_varyings() gets rid of structs that occur in
> +    * varyings).
> +    */
> +   if (!(type->is_matrix() ||
> +        (type->is_array() &&
> +         (type->fields.array->is_numeric() ||
> +          type->fields.array->is_boolean())))) {
> +      assert(!"Unexpected indexing in ir_set_program_inouts");
> +
> +      /* For safety in release builds, in case we ever encounter unexpected
> +       * indexing, give up and let the caller mark the whole variable as used.
> +       */
> +      return false;

Since you have a completely reasonable path to take if the assertion 
doesn't hold, I'd prefer to see it be a _mesa_problem() rather than an 
assertion.

Otherwise, for the series:
Reviewed-by: Kenneth Graunke <kenneth at whitecape.org>

> +   }
> +
>      ir_constant *index_as_constant = index->as_constant();
>      if (!index_as_constant)
>         return false;
>