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

[Ian Romanick]

On 02/26/2016 07:09 AM, 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)
> 
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=94129
> ---
>  src/compiler/glsl/ast.h            |  5 +++++
>  src/compiler/glsl/ast_function.cpp |  3 +++
>  src/compiler/glsl/ast_to_hir.cpp   | 30 ++++++++++++++++++++++++++++++
>  3 files changed, 38 insertions(+)
> 
> diff --git a/src/compiler/glsl/ast.h b/src/compiler/glsl/ast.h
> index 9aa5bb9..d07b595 100644
> --- a/src/compiler/glsl/ast.h
> +++ b/src/compiler/glsl/ast.h
> @@ -263,6 +263,11 @@ public:
>      * This pointer may be \c NULL.
>      */
>     const char *non_lvalue_description;
> +
> +   void set_is_lhs(bool new_value);
> +
> +private:
> +   bool is_lhs = false;

This is valid C++?  I thought you could only initialize static members
in this way, and everything else had to be initialized by the constructor.

>  };
>  
>  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..49afccc 100644
> --- a/src/compiler/glsl/ast_function.cpp
> +++ b/src/compiler/glsl/ast_function.cpp
> @@ -1727,6 +1727,9 @@ ast_function_expression::handle_method(exec_list *instructions,
>     const char *method;
>     method = field->primary_expression.identifier;
>  
> +   /* This would prevent to raise "unitialized variable" warnings when calling
                                      uninitialized

> +    * array.length. */

Here and elsewhere, the closing */ of a multiline comment goes on its
own line.

> +   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 db5ec9a..49e4858 100644
> --- a/src/compiler/glsl/ast_to_hir.cpp
> +++ b/src/compiler/glsl/ast_to_hir.cpp
> @@ -1248,6 +1248,23 @@ 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 unitialized warnings", if
                                                        uninitialized

> +    * we lack a identifier we can just skip, so also skipping going through
> +    * subexpressions (see below) */
> +   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 */
> +   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 +1340,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 +1610,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 +1637,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 +1660,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 +1679,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 +1861,10 @@ 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 unintialized is beyond what we get
                                              uninitialized

> +       * from ir_value.data.assigned. Setting is_lhs as true would force to
> +       * not raise a unitialized warning when using an array*/
                        uninitialized

> +      subexpressions[0]->set_is_lhs(true);
>        op[0] = subexpressions[0]->hir(instructions, state);
>        op[1] = subexpressions[1]->hir(instructions, state);
>  
> @@ -5732,6 +5758,10 @@ ast_switch_statement::test_to_hir(exec_list *instructions,
>  {
>     void *ctx = state;
>  
> +   /* set to true to avoid a duplicate "use of unitialized variable" warning
                                                  uninitialized

> +    * 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,
>