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

[Timothy Arceri]

Series:

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

Are there piglit tests to go with this?

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;
>      }
>