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

[Danylo Piliaiev]

On 19.07.18 21:41, Ian Romanick wrote:
> 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.
Looked at it once more, and indeed error will be emitted by
do_assignment in this case, so first assignment to error_emitted
in ast_post_inc/dec is just superfluous.
I found the reassignment by running cppcheck, should have paid more 
attention fixing it...
So I'll just remove the assignment.
>
> 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;