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

[Emil Velikov]

Hi gents,

On 9 June 2015 at 14:09, Francisco Jerez <currojerez at riseup.net> wrote:
> Francisco Jerez <currojerez at riseup.net> writes:
>
>> Tapani Pälli <tapani.palli at intel.com> writes:
>>
>>> Desktop GLSL < 130 and GLSL ES < 300 allow sampler array indexing where
>>> index can contain a loop induction variable. This extra check will warn
>>> during linking if some of the indexes could not be turned in to constant
>>> expressions.
>>>
>>> v2: warning instead of error for backends that did not enable
>>>     UnrollSamplerArrayDynamicIndexing option (have dynamic indexing)
>>>
>>> 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 | 77 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>>>  1 file changed, 77 insertions(+)
>>>
>>> diff --git a/src/glsl/linker.cpp b/src/glsl/linker.cpp
>>> index 9978380..27d7c18 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,40 @@ 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).
>>> + */
>>> +static bool
>>> +validate_sampler_array_indexing(struct gl_context *ctx,
>>> +                                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;
>>> +
>>> +      bool no_dynamic_indexing =
>>> +         ctx->Const.ShaderCompilerOptions[i].UnrollSamplerArrayDynamicIndexing;
>>> +
>>> +      /* Search for array derefs in shader. */
>>> +      v.run(prog->_LinkedShaders[i]->ir);
>>> +      if (v.uses_dynamic_sampler_array_indexing()) {
>>> +         const char *msg = "sampler arrays indexed with non-constant "
>>> +                           "expressions is forbidden in GLSL %s %u";
>>
>> For the sake of clarity, maybe add that it's sampler array indexing with
>> *general* non-constant expressions what is forbidden, loop induction
>> variables are allowed and they are technically a kind of non-constant
>> expression.
>>
>>> +         /* Backend has indicated that it has no dynamic indexing support. */
>>> +         if (no_dynamic_indexing) {
>>> +            linker_error(prog, msg, prog->IsES ? "ES" : "", prog->Version);
>>> +            return false;
>>> +         } else {
>>> +            linker_warning(prog, msg, prog->IsES ? "ES" : "", prog->Version);
>>
>> It seems a bit mean to spam the user with another warning at link time,
>> you've already warned in PATCH 01 that this feature will be removed in
>> more recent GLSL versions.  If you drop the warning:
>>
> Er, nevermind, the warning here is indeed subtly different (you are
> doing a kind of indexing not considered under the
> constant-index-expression wording), disregard my comment about dropping
> the warning, it seems fine to warn the user twice.
>
>> Reviewed-by: Francisco Jerez <currojerez at riseup.net>
>>
If I understand things correctly the series (patches 1/4 and 4/4 from
stable pov) are reviewed but are not in master. Are there any
obstacles/objections against doing so ?

Thanks
Emil