[1/2] glsl: Avoid propagating incompatible type of initializer

Submitted by Danylo Piliaiev on Aug. 15, 2018, 12:46 p.m.

Details

Message ID 20180815124639.12600-1-danylo.piliaiev@globallogic.com
State Accepted
Commit 6f3c7374b11299c21d829db794fad3b756af60fb
Headers show
Series "Series without cover letter" ( rev: 1 ) in Mesa

Browsing this patch as part of:
"Series without cover letter" rev 1 in Mesa
<< prev patch [1/2] next patch >>

Commit Message

Danylo Piliaiev Aug. 15, 2018, 12:46 p.m.
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@globallogic.com>
---
 src/compiler/glsl/ast_to_hir.cpp | 62 +++++++++++++++++---------------
 1 file changed, 33 insertions(+), 29 deletions(-)

Patch hide | download patch | download mbox

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

Comments

Series:

Reviewed-by: Timothy Arceri <tarceri@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@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;
>      }
>
Hi,

Thank you for the review.

On 9/15/18 2:15 AM, Timothy Arceri wrote:
> Series:
>
> Reviewed-by: Timothy Arceri <tarceri@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@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;
>>      }
>>