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

[Timothy Arceri]

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?

> 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