[Mesa-stable] [PATCH 2/2] glsl: validate sampler array indexing for 'constant-index-expression'

[Tapani Pälli]

Hello;

I'd like to ping if this approach would be ok. We've had some 
discussions with Curro about it and overall it would seem nicer to move 
this check to happen at compile time. However, this seems quite a 
problematic move. I'll try explain below why;

The overall problem with the failing use cases in the bug is that loop 
unroll does not happen. It does not happen because loop analysis does 
not know how to deal with functions and there is a texture2D call inside 
the loop.

Now what follows is that because unroll does not happen the array index 
(loop induction variable) does not become constant during compilation, 
this will happen only after linking (where unroll finally happens due to 
function inlining which allows further optimization).

I have a hacky patch where I force unroll to happen early when only 
builtin calls are found inside loop and it works *but* unfortunately it 
does not help since in the unrolled result we still have sampler array 
indexing with a non-constant variable 'i', it will be constant only 
later after linking phase when function inlining and further 
optimizations happen. It looks like this (I modified ir print output a 
bit to fit in email):

1st round:
assign var_ref i constant_int 0
call texture2D (constant int 0)

2nd round:
assign var_ref i (var_ref i + constant_int 1)
call texture2D (var_ref i)

So at this point I got a bit tired of this approach. IMO linker check is 
sufficient and according the spec. Spec does not explicitly specify a 
compiler or linker error for this case but it does say:

GLSL ES 1.0.17 spec (4.1.9 Arrays):

"Reading from or writing to an array with a non-constant index that is 
less than zero or greater than or equal to the array's size results in 
undefined behavior. It is platform dependent how bounded this undefined 
behavior may be. It is possible that it leads to instability of the 
underlying system or corruption of memory. However, a particular 
platform may bound the behavior such that this is not the case."

So according to spec, we should not really be checking anything but here 
I'm offering undefined behavior as extra linker check allowed by the 
last clause.

Any opinions appreciated;

// Tapani


On 05/19/2015 03:01 PM, Tapani Pälli wrote:
> Desktop GLSL < 130 and GLSL ES < 300 allow sampler array indexing where
> index can contain a loop induction variable. This extra check makes sure
> that all these indexes turn in to constant expressions during
> compilation/linking.
>
> Signed-off-by: Tapani Pälli <tapani.palli at intel.com>
> Cc: "10.5" and "10.6" <mesa-stable at lists.freedesktop.org>
> ---
>   src/glsl/linker.cpp | 71 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 71 insertions(+)
>
> diff --git a/src/glsl/linker.cpp b/src/glsl/linker.cpp
> index ecdc025..729b27f 100644
> --- a/src/glsl/linker.cpp
> +++ b/src/glsl/linker.cpp
> @@ -346,6 +346,39 @@ private:
>      bool uses_non_zero_stream;
>   };
>
> +/* Class that finds array derefs and check if indexes are dynamic. */
> +class dynamic_sampler_array_indexing_visitor : public ir_hierarchical_visitor
> +{
> +public:
> +   dynamic_sampler_array_indexing_visitor() :
> +      dynamic_sampler_array_indexing(false)
> +   {
> +   }
> +
> +   ir_visitor_status visit_enter(ir_dereference_array *ir)
> +   {
> +      if (!ir->variable_referenced())
> +         return visit_continue;
> +
> +      if (!ir->variable_referenced()->type->contains_sampler())
> +         return visit_continue;
> +
> +      if (!ir->array_index->constant_expression_value()) {
> +         dynamic_sampler_array_indexing = true;
> +         return visit_stop;
> +      }
> +      return visit_continue;
> +   }
> +
> +   bool uses_dynamic_sampler_array_indexing()
> +   {
> +      return dynamic_sampler_array_indexing;
> +   }
> +
> +private:
> +   bool dynamic_sampler_array_indexing;
> +};
> +
>   } /* anonymous namespace */
>
>   void
> @@ -2736,6 +2769,34 @@ build_program_resource_list(struct gl_context *ctx,
>       */
>   }
>
> +/**
> + * This check is done to make sure we allow only constant expression
> + * indexing and "constant-index-expression" (indexing with an expression
> + * that includes loop induction variable).
> + */
> +bool
> +validate_sampler_array_indexing(struct gl_shader_program *prog)
> +{
> +   dynamic_sampler_array_indexing_visitor v;
> +   for (unsigned i = 0; i < MESA_SHADER_STAGES; i++) {
> +      if (prog->_LinkedShaders[i] == NULL)
> +	 continue;
> +
> +      /* Search for array derefs in shader. */
> +      v.run(prog->_LinkedShaders[i]->ir);
> +
> +      if (v.uses_dynamic_sampler_array_indexing()) {
> +         linker_error(prog,
> +                      "sampler arrays indexed with non-constant "
> +                      "expressions is forbidden in GLSL %s %u",
> +                      prog->IsES ? "ES" : "", prog->Version);
> +         return false;
> +      }
> +   }
> +
> +   return true;
> +}
> +
>
>   void
>   link_shaders(struct gl_context *ctx, struct gl_shader_program *prog)
> @@ -2948,6 +3009,16 @@ link_shaders(struct gl_context *ctx, struct gl_shader_program *prog)
>         lower_const_arrays_to_uniforms(prog->_LinkedShaders[i]->ir);
>      }
>
> +   /* Validation for special cases where we allow sampler array indexing
> +    * with loop induction variable. This makes sure that all such cases
> +    * have been turned in to constant expressions.
> +    */
> +   if ((!prog->IsES && prog->Version < 130) ||
> +       (prog->IsES && prog->Version < 300)) {
> +      if (!validate_sampler_array_indexing(prog))
> +         goto done;
> +   }
> +
>      /* Check and validate stream emissions in geometry shaders */
>      validate_geometry_shader_emissions(ctx, prog);
>
>