[Mesa-dev] [RFC 3/3] glsl: raise warning when using uninitialized variables

[Timothy Arceri]

On Thu, 2016-02-25 at 11:08 +0100, Alejandro Piñeiro wrote:
> On 25/02/16 10:41, Timothy Arceri wrote:
> > On Thu, 2016-02-25 at 09:09 +0100, Alejandro Piñeiro wrote:
> > > On 25/02/16 00:27, Timothy Arceri wrote:
> > > > On Wed, 2016-02-24 at 20:04 +0100, Alejandro Piñeiro wrote:
> > > > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=94129
> > > > > ---
> > > > >  src/compiler/glsl/ast_to_hir.cpp | 7 +++++++
> > > > >  1 file changed, 7 insertions(+)
> > > > > 
> > > > > diff --git a/src/compiler/glsl/ast_to_hir.cpp
> > > > > b/src/compiler/glsl/ast_to_hir.cpp
> > > > > index ee5485c..296c845 100644
> > > > > --- a/src/compiler/glsl/ast_to_hir.cpp
> > > > > +++ b/src/compiler/glsl/ast_to_hir.cpp
> > > > > @@ -1885,6 +1885,13 @@ ast_expression::do_hir(exec_list
> > > > > *instructions,
> > > > >        if (var != NULL) {
> > > > >           var->data.used = true;
> > > > >           result = new(ctx) ir_dereference_variable(var);
> > > > > +
> > > > > +         if (var->data.mode == ir_var_auto
> > > > I think we would also maybe want to warn for output varyings???
> > > > 
> > > > var->data.mode == ir_var_shader_out
> > > Ok, I will check this one.
> > > 
> > > 
> > > > > +             && !assignment_recipient
> > > > > +             && result->variable_referenced()->data.assigned 
> > > > > !=
> > > > > true) {
> > > > > +            _mesa_glsl_warning(& loc, state, "`%s' used 
> > > >                                 ---^
> > > > We don't really use this style anymore   
> > > Well, I just used the style of the mesa_glsl_error three lines
> > > below
> > > :/.
> > > Thanks for pointing it, I would not have realized it, I will
> > > update
> > > the
> > > code.
> > > 
> > > > > uninitialized",
> > > > > +                               this-
> > > > > > primary_expression.identifier);
> > > > > +         }
> > > > I think what you have will work but I don't really like passing
> > > > around assignment_recipient. Maybe it would be better just to
> > > > add a
> > > > new
> > > > bool lhs_var; to the ast_expression class that defaults to
> > > > false
> > > > and
> > > > use that instead.
> > > At some point I thought on that idea too, but as I mentioned on
> > > the
> > > cover letter, on the ast tree we don't have access to the parent
> > > expression, that is the one that would get lhs_var assigned. 
> > But isn't the lhs always just a variable so you would just set the
> > flag
> > to true on the lhs subexpression for an assignment. The lhs is
> > never
> > actually an expression or I'm I missing something?
> 
> Well, when is_lhs is assigned, it is an expression, but you are
> right, I
> just checked and it is with just the one variable we want. I thought
> that there were more "expression layers" between the node with
> this->oper==ast_identifier, and the one that does the assignment.

Thinking about it some more you may also have to pass it down one level
to ast_array_index if your assigning to an array element.

But I think you could just do something like:

subexpressions[0]->lhs_var = this->lhs_var;

You might have to handle ast_field_selection somehow too, for structs
and named interfaces.

> 
> Thanks for the advices, I think that I have everything I need now.
> > 
> > > The only
> > > solution to that problem that I found is that when being assigned
> > > to
> > > a
> > > expression, it would need to go for all the children and
> > > assigning
> > > that
> > > value, something that was also somewhat intrusive and probably
> > > slower
> > > compared to what I finally implemented. In any case, I will try
> > > to
> > > implement that variant, so reviewers would decide what is the
> > > best
> > > option.
> > > 
> > > Thanks for all the feedback.
> > > 
> > > BR
> 
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev