[Mesa-dev] [PATCH 1/2] glsl: Avoid propagating incompatible type of initializer

[Danylo Piliaiev]

Hi,

Thank you for the review.

On 9/15/18 2:15 AM, Timothy Arceri wrote:
> Series:
>
> Reviewed-by: Timothy Arceri <tarceri at itsqueeze.com>
>
> Are there piglit tests to go with this?
Yes, there is a test "[PATCH] glsl-1.10: add a 
'initialization-incompatible-type-propagation' test":
https://patchwork.freedesktop.org/patch/244676/
>
> On 15/8/18 10:46 pm, Danylo Piliaiev wrote:
>> do_assignment validated assigment but when rhs type was not compatible
>> it proceeded without issues and returned error_emitted = false.
>> On the other hand process_initializer expected do_assignment to always
>> return compatible type and never fail.
>>
>> As a result when variable was initialized with incompatible type
>> the type of variable changed to the incompatible one.
>> This manifested in unnecessary error messages and in one case in crash.
>>
>> Example GLSL:
>>   vec4 tmp = vec2(0.0);
>>   tmp.z -= 1.0;
>>
>> Past error messages:
>>   initializer of type vec2 cannot be assigned to variable of type vec4
>>   invalid swizzle / mask `z'
>>   type mismatch
>>   operands to arithmetic operators must be numeric
>>
>> After this patch:
>>   initializer of type vec2 cannot be assigned to variable of type vec4
>>
>> In the other case when we initialize variable with incompatible struct,
>> accessing variable's field leaded to a crash. Example:
>>   uniform struct {float field;} data;
>>   ...
>>   vec4 tmp = data;
>>   tmp.x -= 1.0;
>>
>> After the patch there is only error line without a crash:
>>   initializer of type #anon_struct cannot be assigned to variable of
>>    type vec4
>>
>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=107547
>>
>> Signed-off-by: Danylo Piliaiev <danylo.piliaiev at globallogic.com>
>> ---
>>   src/compiler/glsl/ast_to_hir.cpp | 62 +++++++++++++++++---------------
>>   1 file changed, 33 insertions(+), 29 deletions(-)
>>
>> diff --git a/src/compiler/glsl/ast_to_hir.cpp 
>> b/src/compiler/glsl/ast_to_hir.cpp
>> index 5d3f10b682..93e7c8ec33 100644
>> --- a/src/compiler/glsl/ast_to_hir.cpp
>> +++ b/src/compiler/glsl/ast_to_hir.cpp
>> @@ -1012,6 +1012,8 @@ do_assignment(exec_list *instructions, struct 
>> _mesa_glsl_parse_state *state,
>>            mark_whole_array_access(rhs);
>>            mark_whole_array_access(lhs);
>>         }
>> +   } else {
>> +     error_emitted = true;
>>      }
>>        /* Most callers of do_assignment (assign, add_assign, 
>> pre_inc/dec,
>> @@ -4562,41 +4564,43 @@ process_initializer(ir_variable *var, 
>> ast_declaration *decl,
>>         /* Never emit code to initialize a uniform.
>>          */
>>         const glsl_type *initializer_type;
>> +      bool error_emitted = false;
>>         if (!type->qualifier.flags.q.uniform) {
>> -         do_assignment(initializer_instructions, state,
>> -                       NULL,
>> -                       lhs, rhs,
>> -                       &result, true,
>> -                       true,
>> -                       type->get_location());
>> +         error_emitted =
>> +            do_assignment(initializer_instructions, state,
>> +                          NULL, lhs, rhs,
>> +                          &result, true, true,
>> +                          type->get_location());
>>            initializer_type = result->type;
>>         } else
>>            initializer_type = rhs->type;
>>   -      var->constant_initializer = 
>> rhs->constant_expression_value(mem_ctx);
>> -      var->data.has_initializer = true;
>> +      if (!error_emitted) {
>> +         var->constant_initializer = 
>> rhs->constant_expression_value(mem_ctx);
>> +         var->data.has_initializer = true;
>>   -      /* If the declared variable is an unsized array, it must 
>> inherrit
>> -       * its full type from the initializer.  A declaration such as
>> -       *
>> -       *     uniform float a[] = float[](1.0, 2.0, 3.0, 3.0);
>> -       *
>> -       * becomes
>> -       *
>> -       *     uniform float a[4] = float[](1.0, 2.0, 3.0, 3.0);
>> -       *
>> -       * The assignment generated in the if-statement (below) will also
>> -       * automatically handle this case for non-uniforms.
>> -       *
>> -       * If the declared variable is not an array, the types must
>> -       * already match exactly.  As a result, the type assignment
>> -       * here can be done unconditionally.  For non-uniforms the call
>> -       * to do_assignment can change the type of the initializer (via
>> -       * the implicit conversion rules).  For uniforms the initializer
>> -       * must be a constant expression, and the type of that expression
>> -       * was validated above.
>> -       */
>> -      var->type = initializer_type;
>> +         /* If the declared variable is an unsized array, it must 
>> inherrit
>> +         * its full type from the initializer.  A declaration such as
>> +         *
>> +         *     uniform float a[] = float[](1.0, 2.0, 3.0, 3.0);
>> +         *
>> +         * becomes
>> +         *
>> +         *     uniform float a[4] = float[](1.0, 2.0, 3.0, 3.0);
>> +         *
>> +         * The assignment generated in the if-statement (below) will 
>> also
>> +         * automatically handle this case for non-uniforms.
>> +         *
>> +         * If the declared variable is not an array, the types must
>> +         * already match exactly.  As a result, the type assignment
>> +         * here can be done unconditionally.  For non-uniforms the call
>> +         * to do_assignment can change the type of the initializer (via
>> +         * the implicit conversion rules).  For uniforms the 
>> initializer
>> +         * must be a constant expression, and the type of that 
>> expression
>> +         * was validated above.
>> +         */
>> +         var->type = initializer_type;
>> +      }
>>           var->data.read_only = temp;
>>      }
>>