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

[Alejandro Piñeiro]

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.

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