[Mesa-dev] [PATCH 2/2] glsl: struct constructors/initializers only allow implicit conversions

[Timothy Arceri]

On Sun, 2016-07-31 at 18:43 +0300, Andres Gomez wrote:
> When an argument for a structure constructor or initializer doesn't
> match the expected type, only Section 4.1.10 “Implicit Conversions”
> are allowed to try to match that expected type.
> 
> From page 32 (page 38 of the PDF) of the GLSL 1.20 spec:
> 
>   " The arguments to the constructor will be used to set the
> structure's
>     fields, in order, using one argument per field. Each argument
> must
>     be the same type as the field it sets, or be a type that can be
>     converted to the field's type according to Section 4.1.10
> “Implicit
>     Conversions.”"
> 
> From page 35 (page 41 of the PDF) of the GLSL 4.20 spec:
> 
>   " In all cases, the innermost initializer (i.e., not a list of
>     initializers enclosed in curly braces) applied to an object must
>     have the same type as the object being initialized or be a type
> that
>     can be converted to the object's type according to section 4.1.10
>     "Implicit Conversions". In the latter case, an implicit
> conversion
>     will be done on the initializer before the assignment is done."
> 
> Fixes GL44-CTS.shading_language_420pack.initializer_list_negative
> 
> Signed-off-by: Andres Gomez <agomez at igalia.com>
> ---
>  src/compiler/glsl/ast_function.cpp | 62
> ++++++++++++++++++++++++++++----------
>  1 file changed, 46 insertions(+), 16 deletions(-)
> 
> diff --git a/src/compiler/glsl/ast_function.cpp
> b/src/compiler/glsl/ast_function.cpp
> index 9dcec50..9b09cb6 100644
> --- a/src/compiler/glsl/ast_function.cpp
> +++ b/src/compiler/glsl/ast_function.cpp
> @@ -31,10 +31,6 @@
>  static ir_rvalue *
>  convert_component(ir_rvalue *src, const glsl_type *desired_type);
>  
> -bool
> -apply_implicit_conversion(const glsl_type *to, ir_rvalue * &from,
> -                          struct _mesa_glsl_parse_state *state);
> -

I'd put this in a third patch when you remove this and
make apply_implicit_conversion() static.

>  static unsigned
>  process_parameters(exec_list *instructions, exec_list
> *actual_parameters,
>  		   exec_list *parameters,
> @@ -1703,6 +1699,23 @@ process_record_constructor(exec_list
> *instructions,
>                             struct _mesa_glsl_parse_state *state)
>  {
>     void *ctx = state;
> +   /* From page 32 (page 38 of the PDF) of the GLSL 1.20 spec:
> +    *
> +    *    "The arguments to the constructor will be used to set the
> structure's
> +    *     fields, in order, using one argument per field. Each
> argument must
> +    *     be the same type as the field it sets, or be a type that
> can be
> +    *     converted to the field's type according to Section 4.1.10
> “Implicit
> +    *     Conversions.”"
> +    *
> +    * From page 35 (page 41 of the PDF) of the GLSL 4.20 spec:
> +    *
> +    *    "In all cases, the innermost initializer (i.e., not a list
> of
> +    *     initializers enclosed in curly braces) applied to an
> object must
> +    *     have the same type as the object being initialized or be a
> type that
> +    *     can be converted to the object's type according to section
> 4.1.10
> +    *     "Implicit Conversions". In the latter case, an implicit
> conversion
> +    *     will be done on the initializer before the assignment is
> done."
> +    */
>     exec_list actual_parameters;
>  
>     process_parameters(instructions, &actual_parameters,
> @@ -1710,7 +1723,7 @@ process_record_constructor(exec_list
> *instructions,
>  
>     exec_node *node = actual_parameters.get_head_raw();
>     for (unsigned i = 0; i < constructor_type->length; i++) {
> -      ir_rvalue *ir = (ir_rvalue *) node;
> +      ir_rvalue *result = (ir_rvalue *) node;

Maybe its just me but I find this naming really confusing when trying
to read the following code, I'd just leave it as ir.

>  
>        if (node->is_tail_sentinel()) {
>           _mesa_glsl_error(loc, state,
> @@ -1719,18 +1732,35 @@ process_record_constructor(exec_list
> *instructions,
>           return ir_rvalue::error_value(ctx);
>        }
>  
> -      if (apply_implicit_conversion(constructor_type-
> >fields.structure[i].type,
> -                                 ir, state)) {
> -         node->replace_with(ir);
> -      } else {
> -         _mesa_glsl_error(loc, state,
> -                          "parameter type mismatch in constructor
> for `%s.%s' "
> -                          "(%s vs %s)",
> -                          constructor_type->name,
> -                          constructor_type-
> >fields.structure[i].name,
> -                          ir->type->name,
> -                          constructor_type-
> >fields.structure[i].type->name);
> +      const glsl_base_type element_base_type =
> +         constructor_type->fields.structure[i].type->base_type;
> +
> +      /* Apply implicit conversions (not the scalar constructor
> rules!). See
> +       * the spec quote above. */

*/ should be on the next line

> +      if (element_base_type != result->type->base_type) {
> +         const glsl_type *desired_type =
> +            glsl_type::get_instance(element_base_type,
> +                                    result->type->vector_elements,
> +                                    result->type->matrix_columns);
> +
> +	 if (result->type->can_implicitly_convert_to(desired_type,
> state)) {
> +	    /* Even though convert_component() implements the
> constructor
> +	     * conversion rules (not the implicit conversion rules),
> its safe
> +	     * to use it here because we already checked that the
> implicit
> +	     * conversion is legal.
> +	     */
> +	    result = convert_component(result, desired_type);
> +	 }

You are adding a bunch of tabs above please remove them. It would be
nice if you could add a forth patch to the series that removes the
remain tabs in this file since there are not many remaining.

> +      }
> +
> +      if (result->type != constructor_type-
> >fields.structure[i].type) {
> +	 _mesa_glsl_error(loc, state, "type error in array
> constructor: "
> +			  "expected: %s, found %s",
> +			  constructor_type-
> >fields.structure[i].type->name,
> +			  result->type->name);
>           return ir_rvalue::error_value(ctx);
> +      } else {
> +         node->replace_with(result);
>        }

This if-else should be inside the previous if.

 The node->replace_with(result); should be after
 result = convert_component(result, desired_type);

Otherwise you are replacing the node with itself.

The error message can be added as an else to if (result->type-
>can_implicitly_convert_to())

>  
>        node = node->next;

I'm still not entirely sure I understand why this patch is different
from using apply_implicit_conversion() can you explain to me why
apply_implicit_conversion() does not match the rules correctly? Thanks
:)