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

[Alejandro Piñeiro]

On 25/02/16 11:54, Timothy Arceri wrote:
> 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.

I didn't need to do anything specific for this case.

> 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.

Ok, thanks for pointing this specific case.

>
>> 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