[Bug 94129] Mesa's compiler should warn about undefined values

[bugzilla-daemon at freedesktop.org]

https://bugs.freedesktop.org/show_bug.cgi?id=94129

--- Comment #3 from Alejandro Piñeiro (freenode IRC: apinheiro) <apinheiro at igalia.com> ---
(In reply to Kenneth Graunke from comment #2)
> (In reply to Alejandro Piñeiro (freenode IRC: apinheiro) from comment #1)
> > If you don't mind I will take a look to this bug.
> 
> Thanks!  That would be fantastic!

Sorry for the long time without notice. But glsl IR code is somewhat
complicated, and I didn't have too much previous experience on that. 

So some comments/questions.

TL;DR I have a patch that seems to work fine on the tests I made. It is also
too intrusive/dirty and I would like to find a better approach.

Going now for something more detailed:

First, I would like to note that although the bug summary says "warn about
undefined values", I assume that we are talking here about "warn about use of
uninitialized variables" (so an equivalent to gcc -Wuninitialized). 

And now in relation to Kenneth original comments:

(In reply to Kenneth Graunke from comment #0)
> Many WebGL programs accidentally use undefined values, expecting them to be
> zero initialized (which is not required).  NVidia apparently issues a
> compiler warning in this case.  We should too.
> 
> This is fairly easy to detect in NIR - see if ssa_undefs are used.  

Yes, it is easy to detect it on NIR checking for ssa_undef. For example, if you
check instr->type at nir_print.c:print_ssa_def, you could point when a undef
variable is used when printing the NIR tree. But ...

> But, it's not clear whether we can issue warnings from that level.

... this is also true. NIR is, imho, too high level at that point. And right
now it doesn't have direct access to the glsl info_log, where those warnings
should be added. And I don't think that it would be a good idea to allow NIR to
get access to that, again for being too high level.

Additionally, we also want to point on which line happens the warning.

For that reason, I think that the best place to raise the warning is at
ast_to_hir.cpp, where for example, the error of use of undeclared variables is
being raised.

And going on with that example, I tried to raise the warning around the same
place that error is raised, while checking ast_identifier on
ast_expression::do_hir (ast_to_hir.cpp line 1873). ir_variable has a flag
equivalent to checking against ssa_undef on IR, that is
ir_variable_data.assigned. So I check the value of assigned at that point, and
the ir_variable_mode of the variable to avoid false positives on params,
uniforms, etc.

But the problem with raising the warning at this point is that (AFAIK) we don't
know if we are on a right/left side of an assignment, leading to false
positives.

I have a patch that changes ast_expression::do_hir and all the ast_node::hir
signature in order to include that info. As far as I see (I need to do more
tests), this works fine. But it is also highly intrussive. And in higher level
structures, that value doesn't have a real meaning, so given any value until
going to specific node types.

So I will work now on trying to get a simpler way to get that info (or an
alternative place(s) to raise the warning). Any advice would be welcome though.

-- 
You are receiving this mail because:
You are the QA Contact for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/intel-3d-bugs/attachments/20160223/d10ddfee/attachment.html>