[Mesa-dev] [PATCH v3 1/2] glsl: add is_lhs bool on ast_expression

[Timothy Arceri]

On Mon, 2016-03-28 at 20:50 +0200, Alejandro Piñeiro wrote:
> Useful to know if a expression is the recipient of an assignment
> or not, that would be used to (for example) raise warnings of
> "use of uninitialized variable" without getting a false positive
> when assigning first a variable.
> 
> By default the value is false, and it is assigned to true on
> the following cases:
>  * The lhs assignments subexpression
>  * At ast_array_index, on the array itself.
>  * While handling the method on an array, to avoid the warning
>    calling array.length
>  * When computed the cached test expression at test_to_hir, to
>    avoid a duplicate warning on the test expression of a switch.
> 
> set_is_lhs setter is added, because in some cases (like
> ast_field_selection)
> the value need to be propagated on the expression tree. To avoid
> doing the
> propatagion if not needed, it skips if no
> primary_expression.identifier is
> available.
> 
> v2: use a new bool on ast_expression, instead of a new parameter
>     on ast_expression::hir (Timothy Arceri)
> 
> v3: fix style and some typos on comments, initialize is_lhs default
> value
>     on constructor, to avoid a c++11 feature (Ian Romanick)
> 
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=94129

Its hard to know if this will work correctly in all scenarios but I
don't see much point in holding it back any longer. It looks pretty
good to me at this point lets see if anyone spots any issues with it in
action.

With the couple of minor comments below addressed both patches are:

Reviewed-by: Timothy Arceri <timothy.arceri at collabora.com>

> ---
>  src/compiler/glsl/ast.h                  |  6 ++++++
>  src/compiler/glsl/ast_function.cpp       |  4 ++++
>  src/compiler/glsl/ast_to_hir.cpp         | 33
> ++++++++++++++++++++++++++++++++
>  src/compiler/glsl/glsl_parser_extras.cpp |  1 +
>  4 files changed, 44 insertions(+)
> 
> diff --git a/src/compiler/glsl/ast.h b/src/compiler/glsl/ast.h
> index 727aa43..9f46340 100644
> --- a/src/compiler/glsl/ast.h
> +++ b/src/compiler/glsl/ast.h
> @@ -214,6 +214,7 @@ public:
>        subexpressions[2] = NULL;
>        primary_expression.identifier = identifier;
>        this->non_lvalue_description = NULL;
> +      this->is_lhs = false;
>     }
>  
>     static const char *operator_string(enum ast_operators op);
> @@ -263,6 +264,11 @@ public:
>      * This pointer may be \c NULL.
>      */
>     const char *non_lvalue_description;
> +
> +   void set_is_lhs(bool new_value);
> +
> +private:
> +   bool is_lhs;
>  };
>  
>  class ast_expression_bin : public ast_expression {
> diff --git a/src/compiler/glsl/ast_function.cpp
> b/src/compiler/glsl/ast_function.cpp
> index 1a44020..db68d5d 100644
> --- a/src/compiler/glsl/ast_function.cpp
> +++ b/src/compiler/glsl/ast_function.cpp
> @@ -1727,6 +1727,10 @@
> ast_function_expression::handle_method(exec_list *instructions,
>     const char *method;
>     method = field->primary_expression.identifier;
>  
> +   /* This would prevent to raise "uninitialized variable" warnings
> when
> +    * calling array.length.
> +    */
> +   field->subexpressions[0]->set_is_lhs(true);
>     op = field->subexpressions[0]->hir(instructions, state);
>     if (strcmp(method, "length") == 0) {
>        if (!this->expressions.is_empty()) {
> diff --git a/src/compiler/glsl/ast_to_hir.cpp
> b/src/compiler/glsl/ast_to_hir.cpp
> index 35def8e..eb45f29 100644
> --- a/src/compiler/glsl/ast_to_hir.cpp
> +++ b/src/compiler/glsl/ast_to_hir.cpp
> @@ -1248,6 +1248,24 @@ ast_expression::hir_no_rvalue(exec_list
> *instructions,
>     do_hir(instructions, state, false);
>  }
>  
> +void
> +ast_expression::set_is_lhs(bool new_value)
> +{
> +   /* is_lhs is tracked only to print "variable used uninitialized"
> warnings,
> +    * if we lack a identifier we can just skip, so also skipping
> going through
> +    * subexpressions (see below)
> +    */

I find the text "so also skipping going through subexpressions (see
below)" a little confusing I don't think it adds much value so I'd
suggest just removing it and replacing it with "it." i.e "we can just
skip it."

> +   if (this->primary_expression.identifier == NULL)
> +      return;
> +
> +   this->is_lhs = new_value;
> +
> +   /* We need to go through the subexpressions tree to cover cases
> like
> +    * ast_field_selection */
                             ^-- Put this on the next line


> +   if (this->subexpressions[0] != NULL)
> +      this->subexpressions[0]->set_is_lhs(new_value);
> +}
> +
>  ir_rvalue *
>  ast_expression::do_hir(exec_list *instructions,
>                         struct _mesa_glsl_parse_state *state,
> @@ -1323,6 +1341,7 @@ ast_expression::do_hir(exec_list *instructions,
>        break;
>  
>     case ast_assign: {
> +      this->subexpressions[0]->set_is_lhs(true);
>        op[0] = this->subexpressions[0]->hir(instructions, state);
>        op[1] = this->subexpressions[1]->hir(instructions, state);
>  
> @@ -1592,6 +1611,7 @@ ast_expression::do_hir(exec_list *instructions,
>     case ast_div_assign:
>     case ast_add_assign:
>     case ast_sub_assign: {
> +      this->subexpressions[0]->set_is_lhs(true);
>        op[0] = this->subexpressions[0]->hir(instructions, state);
>        op[1] = this->subexpressions[1]->hir(instructions, state);
>  
> @@ -1618,6 +1638,7 @@ ast_expression::do_hir(exec_list *instructions,
>     }
>  
>     case ast_mod_assign: {
> +      this->subexpressions[0]->set_is_lhs(true);
>        op[0] = this->subexpressions[0]->hir(instructions, state);
>        op[1] = this->subexpressions[1]->hir(instructions, state);
>  
> @@ -1640,6 +1661,7 @@ ast_expression::do_hir(exec_list *instructions,
>  
>     case ast_ls_assign:
>     case ast_rs_assign: {
> +      this->subexpressions[0]->set_is_lhs(true);
>        op[0] = this->subexpressions[0]->hir(instructions, state);
>        op[1] = this->subexpressions[1]->hir(instructions, state);
>        type = shift_result_type(op[0]->type, op[1]->type, this->oper, 
> state,
> @@ -1658,6 +1680,7 @@ ast_expression::do_hir(exec_list *instructions,
>     case ast_and_assign:
>     case ast_xor_assign:
>     case ast_or_assign: {
> +      this->subexpressions[0]->set_is_lhs(true);
>        op[0] = this->subexpressions[0]->hir(instructions, state);
>        op[1] = this->subexpressions[1]->hir(instructions, state);
>        type = bit_logic_result_type(op[0], op[1], this->oper, state,
> &loc);
> @@ -1839,6 +1862,11 @@ ast_expression::do_hir(exec_list
> *instructions,
>     case ast_array_index: {
>        YYLTYPE index_loc = subexpressions[1]->get_location();
>  
> +      /* Getting if an array is being used uninitialized is beyond
> what we get
> +       * from ir_value.data.assigned. Setting is_lhs as true would
> force to
> +       * not raise a uninitialized warning when using an array
> +       */
> +      subexpressions[0]->set_is_lhs(true);
>        op[0] = subexpressions[0]->hir(instructions, state);
>        op[1] = subexpressions[1]->hir(instructions, state);
>  
> @@ -5746,6 +5774,11 @@ ast_switch_statement::test_to_hir(exec_list
> *instructions,
>  {
>     void *ctx = state;
>  
> +   /* set to true to avoid a duplicate "use of uninitialized
> variable" warning
> +    * on the switch test case. The first one would be already raised
> when
> +    * getting the test_expression at ast_switch_statement::hir
> +    */
> +   test_expression->set_is_lhs(true);
>     /* Cache value of test expression. */
>     ir_rvalue *const test_val =
>        test_expression->hir(instructions,
> diff --git a/src/compiler/glsl/glsl_parser_extras.cpp
> b/src/compiler/glsl/glsl_parser_extras.cpp
> index 9fcca21..8a4ffcb 100644
> --- a/src/compiler/glsl/glsl_parser_extras.cpp
> +++ b/src/compiler/glsl/glsl_parser_extras.cpp
> @@ -1204,6 +1204,7 @@ ast_expression::ast_expression(int oper,
>     this->subexpressions[1] = ex1;
>     this->subexpressions[2] = ex2;
>     this->non_lvalue_description = NULL;
> +   this->is_lhs = false;
>  }
>  
>