Bug 94129

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

FWIW, although it is true that NVIDIA driver seems more verbose with respect to warnings, I have just tested on GL_VERSION = 4.5.0 NVIDIA 352.55, and I didn't see any warning on a shader like this:

#version 130

in vec3 color;
out vec4 outColor;

void main() {
  vec4 undefined;

  outColor = vec4(color, 1.0) + undefined;
}

That shows a lot of crap on string.

> We should too.

If you don't mind I will take a look to this bug.
 
> This is fairly easy to detect in NIR - see if ssa_undefs are used. 

Will start looking here.

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

Ok.

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

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

The AST is a bit awkward to work with beyond basic validation. You would likely find it easier to add this in GLSL IR somewhere.

I'm not sure how hard it would be to detect if something is initialized, but there are plenty of examples where we walk over the IR and do some type of validation/modification, you may even be able to hook into an existing pass.

Also do be afraid to send what you have as a RFC to the mailing list for feedback and ideas. You will get much more feedback that way rather than commenting on a bug.

(In reply to Timothy Arceri from comment #4)
> The AST is a bit awkward to work with beyond basic validation. You would
> likely find it easier to add this in GLSL IR somewhere.

Well, ast_to_hir is a kind of "in the middle" between AST and IR, as the translation between one and the other is being done there.

Additionally, as I mentioned on my comment, ir_variable is already storing if a variable was assigned (that is mostly equivalent to initialized), but as the comment documenting it says:

       * This answers whether the variable was assigned in any path of
       * the shader during ast_to_hir.  This doesn't answer whether it is
       * still written after dead code removal, nor is it maintained in
       * non-ast_to_hir.cpp (GLSL parsing) paths.

So initially I preferred to try to reuse what it was already available.

> I'm not sure how hard it would be to detect if something is initialized, but
> there are plenty of examples where we walk over the IR and do some type of
> validation/modification, you may even be able to hook into an existing pass.

Ok, I will try this option if Im not able to advance on my current approach. But as said, I would prefer to try first with what IR already provides.

> Also do be afraid to send what you have as a RFC to the mailing list for
> feedback and ideas. You will get much more feedback that way rather than
> commenting on a bug.

Yes, that was also my idea, but as mentioned my working patch was still too dirty for that. I preferred to try to clean it first.

Thanks for all the feedback.

After some extra code checking on both AST and IR.

(In reply to Timothy Arceri from comment #4)
> The AST is a bit awkward to work with beyond basic validation. You would
> likely find it easier to add this in GLSL IR somewhere.

There is another reason to try to do this on the ast_to_hir pass. As in the NIR

After some extra code checking on both AST and IR.

(In reply to Timothy Arceri from comment #4)
> The AST is a bit awkward to work with beyond basic validation. You would
> likely find it easier to add this in GLSL IR somewhere.

There is another reason to try to do this on the ast_to_hir pass. As in the NIR case, at GLSL IR we don't have available the source location. This is needed if we want to add additional information, for example, in which source line:column number the uninitialized value is being used. All the error/warnings raised at GLSL IR (not a lot) doesn't provide that info, and need to provide a fake location. For example:
   memset(&loc, 0, sizeof(loc));
   _mesa_glsl_error(&loc, state,
		    "function `%s' has static recursion",
		    proto);

I guess that for this case, a message "var %s used initialized" is better that nothing, but I think that is more useful if it points to the line:column number as other warnings/errors (like undeclared variable).

(And sorry for the previous not finished comment, pressed the wrong key)

Just sent an RFC series to the list:
https://lists.freedesktop.org/archives/mesa-dev/2016-February/108442.html

(patchwork is down, so using a ml link)

(In reply to Alejandro Piñeiro (freenode IRC: apinheiro) from comment #8)
> Just sent an RFC series to the list:
> https://lists.freedesktop.org/archives/mesa-dev/2016-February/108442.html
> 
> (patchwork is down, so using a ml link)

And now set a normal (normal as no RFC anymore) series to the list:
https://patchwork.freedesktop.org/series/3851/

(In reply to Alejandro Piñeiro (freenode IRC: apinheiro) from comment #9)
> (In reply to Alejandro Piñeiro (freenode IRC: apinheiro) from comment #8)
> > Just sent an RFC series to the list:
> > https://lists.freedesktop.org/archives/mesa-dev/2016-February/108442.html
> > 
> > (patchwork is down, so using a ml link)
> 
> And now set a normal (normal as no RFC anymore) series to the list:
> https://patchwork.freedesktop.org/series/3851/

After some ping the patches got reviewed, and I just pushed them:

https://cgit.freedesktop.org/cgit/?url=mesa/mesa/commit/&id=8568d02498d12ebde6a6245056eebfbfe18aaf8f

https://cgit.freedesktop.org/cgit/?url=mesa/mesa/commit/&id=dcd41ca87a06199184eb8ada654aec985185189c

With this "uninitialized variable" was added, so we can close the bug.

Thanks everybody involved.