[Mesa-dev] [PATCH v4 (part2) 49/59] glsl: Do not allow assignments to read-only variables

[Iago Toral]

On Wed, 2015-08-05 at 20:04 +1000, Timothy Arceri wrote:
> On Wed, 2015-08-05 at 10:30 +0200, Iago Toral Quiroga wrote:
> > ---
> >  src/glsl/ast_to_hir.cpp | 9 ++++++++-
> >  1 file changed, 8 insertions(+), 1 deletion(-)
> > 
> > diff --git a/src/glsl/ast_to_hir.cpp b/src/glsl/ast_to_hir.cpp
> > index e834a46..518612d 100644
> > --- a/src/glsl/ast_to_hir.cpp
> > +++ b/src/glsl/ast_to_hir.cpp
> > @@ -811,8 +811,15 @@ do_assignment(exec_list *instructions, struct 
> > _mesa_glsl_parse_state *state,
> >     }
> >  
> >     ir_variable *lhs_var = lhs->variable_referenced();
> > -   if (lhs_var)
> > +   if (lhs_var) {
> > +      if (lhs_var->data.image_read_only) {
> 
> It looks like data.read_only is always set to true for images so wouldn't this
> already be caught already by the existing read-only check?
> 
>       else if (lhs_var != NULL && lhs_var->data.read_only) {
>          _mesa_glsl_error(&lhs_loc, state,
>                           "assignment to read-only variable '%s'",
>                           lhs_var->name);

Not as it is now, because with SSBOs we only set image_read_only and not
read_only when the readonly qualifier is used. I suppose this is what we
are expected to do since the SSBO spec says that behavior for these
qualifiers on SSBOs is the same as for images:
https://www.opengl.org/registry/specs/ARB/shader_storage_buffer_object.txt

"Modify Section 4.10, Memory Qualifiers (p. 71)"
(...)
"(insert after third paragraph, p. 73) The memory qualifiers "coherent",
"volatile", "restrict", "readonly", and "writeonly" may be used in the
declaration of buffer variables (i.e., members of shader storage blocks).
When a buffer variable is declared with a memory qualifier, the behavior
specified for memory accesses involving image variables described above
applies identically to memory accesses involving that buffer variable.  It
is an error to assign to a buffer variable qualified with "readonly" or to
read from a buffer variable qualified with "writeonly".

What is a bit confusing for me is that images seem to set
image_read_only depending on whether we used the readonly qualifier or
not (like ssbos) but then they also set read_only to true
unconditionally, so I guess there is a difference between both fields,
but I don't know what it is exactly, specially since you can also use
writeonly on images, for example.

In any case, since we have both read_only and image_read_only in
ir_variable at present, I think it makes sense to have checks for both
of them, if one of them ends up being redundant the right thing to do
would be to kill it completely I guess, otherwise it only gets (even)
more confusing.

Iago

> 
> > +         _mesa_glsl_error(&lhs_loc, state,
> > +                          "assignment to read-only variable `%s'",
> > +                          lhs_var->name);
> > +         error_emitted = true;
> > +      }
> >        lhs_var->data.assigned = true;
> > +   }
> >  
> >     if (!error_emitted) {
> >        if (non_lvalue_description != NULL) {
>