[Mesa-dev] [PATCH] glsl/ast: Fix loss of error_emitted value due to reassignment

[Ian Romanick]

On 07/18/2018 01:53 AM, Danylo Piliaiev wrote:
> Signed-off-by: Danylo Piliaiev <danylo.piliaiev at globallogic.com>
> ---
>  src/compiler/glsl/ast_to_hir.cpp | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/src/compiler/glsl/ast_to_hir.cpp b/src/compiler/glsl/ast_to_hir.cpp
> index dd60a2a87f..8a4cc56511 100644
> --- a/src/compiler/glsl/ast_to_hir.cpp
> +++ b/src/compiler/glsl/ast_to_hir.cpp
> @@ -1938,7 +1938,7 @@ ast_expression::do_hir(exec_list *instructions,
>        result = get_lvalue_copy(instructions, op[0]->clone(ctx, NULL));
>  
>        ir_rvalue *junk_rvalue;
> -      error_emitted =
> +      error_emitted |=
>           do_assignment(instructions, state,
>                         this->subexpressions[0]->non_lvalue_description,
>                         op[0]->clone(ctx, NULL), temp_rhs,
> 

Are there any tests that encounter this?  It seems like do_assignment
should always generate an error if either of the operands have errors.

I notice that the ast_pre_inc / ast_pre_dec case just before this only
sets error_emitted once.  Whatever we decided, ast_pre_* and ast_post_*
should do it the same way.  Intuitively, I think the ast_pre_inc /
ast_pre_dec method is correct, but you'll have to confirm or refute that
with testing. :)

Since this is a Boolean value, I have a mild preference for

      error_emitted =
         do_assignment(instructions, state,
                       this->subexpressions[0]->non_lvalue_description,
                       op[0]->clone(ctx, NULL), temp_rhs,
                       &junk_rvalue, false, false,
                       this->subexpressions[0]->get_location()) ||
         error_emitted;

or (as is done in the ast_array_index case just below this):

      if (do_assignment(instructions, state,
                        this->subexpressions[0]->non_lvalue_description,
                        op[0]->clone(ctx, NULL), temp_rhs,
                        &junk_rvalue, false, false,
                        this->subexpressions[0]->get_location()))
         error_emitted = true;