[Mesa-dev] [PATCH v2 2/2] glsl: raise warning when using uninitialized variables

[Alejandro Piñeiro]

On 18/04/16 16:11, Alejandro Piñeiro wrote:
> On 18/04/16 00:47, Ilia Mirkin wrote:
>> On Mon, Mar 28, 2016 at 2:50 PM, Alejandro Piñeiro <apinheiro at igalia.com> wrote:
>>> v2:
>>>  * Take into account out varyings too (Timothy Arceri)
>>>  * Fix style (Timothy Arceri)
>>>  * Use a new ast_expression variable, instead of an
>>>    ast_expression::hir new parameter (Timothy Arceri)
>>>
>>> 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 eb45f29..e38ab10 100644
>>> --- a/src/compiler/glsl/ast_to_hir.cpp
>>> +++ b/src/compiler/glsl/ast_to_hir.cpp
>>> @@ -1901,6 +1901,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 || var->data.mode == ir_var_shader_out)
>>> +             && !this->is_lhs
>>> +             && result->variable_referenced()->data.assigned != true) {
>>> +            _mesa_glsl_warning(&loc, state, "`%s' used uninitialized",
>>> +                               this->primary_expression.identifier);
>> This also appears to warn in the case of
>>
>> void foo(out float x) { x = 1.0; }
>>
>> float bar;
>> foo(bar);
>>
>> It thinks that bar is being used uninitialized. How do we fix this? It
>> happens a ton in Talos Principle.
> After checking a little: initially I thought it would be easy to solve
> because there is already some checks related to the parameters modes
> (in, out, inout) at verify_parameter_modes (ast_function.cpp). This
> method compares the formal parameter (x in your example) and the actual
> parameter (bar in your example), and checks that the modes are correct
> and other stuff, like for example setting assigned to true on the actual
> parameter. The problem is that this is done after the actual variable is
> processed, that is where the warning is raised. And if fact, in order to
> work, the actual parameter needs to be already processed, as the modes
> are part of the ir variable, not of the ast_node.

Correcting myself, although it is true that we need the modes coming, in
this case we need the modes from the formal parameter, that was
processed before. So for the one we are processing now (the actual one)
we can use the ast_node (that is what that method is using). So perhaps
it would be possible to add a previous step before processing (something
like "preverify_parameter_modes"), using only the ast nodes of the
actual variable. That would allow to set is_lhs there, and avoid having
two places raising the same warning. If not possible I will fallback on
what I mentioned on my previous email.

>
> I think that a way to solve that would be set is_lhs for any function
> parameter, and raise the uninitialized warning too on
> verify_parameter_modes. That would mean that we raise the warning in two
> different places, but I think that it would not be a big issue, because
> as mentioned, that code is already checking errors related with the
> actual and formal parameters and their modes.
>
> I will try to do this tomorrow.
>
> BR
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev