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

[Timothy Arceri]

On Sat, 2015-08-08 at 14:25 +0300, Francisco Jerez wrote:
> Iago Toral <itoral at igalia.com> writes:
> 
> > On Wed, 2015-08-05 at 22:22 +1000, Timothy Arceri wrote:
> > > On Wed, 2015-08-05 at 13:45 +0200, Iago Toral wrote:
> > > > 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,
> > > 
> > > Asking what the difference is was originally going to be my first 
> > > question to
> > > you :)
> > > 
> > > > but I don't know what it is exactly, specially since you can also use
> > > > writeonly on images, for example.
> > > 
> > > So I really dont know much about images but after some reading the 
> > > conclusion
> > > I've come to is the qualifiers (image_read_only) are meant to limit how 
> > > you
> > > can use imageStore(), imageLoad() and imageAtomic*() etc.
> > 
> > Looking at ARB_shader_image_load_store that seems consistent...  In that
> > case I imagine that we could just set read_only for buffer variables
> > with the readonly qualifier instead of image_read_only and drop this
> > patch. We will need to add, at least, write_only to ir_variable as well
> > I guess... I imagine that the 3 other fields (image_coherent,
> > image_restrict, image_volatile) do not have image-specific semantics
> > like image_read_only and image_write_oly and can be shared with ssbos
> > we do not have to replicate them in ir_variable as well (in that case we
> > might want to rename them so it is clear that image_read_only and
> > image_write_only really are special and specific to images)
> > 
> > Curro, what do you think?
> > 
> Yes, the image_* fields (which we should definitely rename now that they
> also affect SSBO variables) are just the memory qualifiers defined by
> the GLSL spec, in the case of images the difference between these and
> the original ir_variable::read_only is clear: The former apply to the
> memory pointed to by the variable, while the latter refers to the
> variable itself -- So in principle one could have a fixed image uniform
> (fixed in the sense of always pointing to the same image unit) pointing
> to some write-only image (i.e. only ir_variable::read_only set), or a
> image uniform mutated during the execution of the program pointing to a
> read-only image (i.e. only ir_variable::image_read_only set -- Although
> in practice this latter possibility is disallowed by the spec, which is
> why images currently always have ir_variable::read_only set).
> 
> For buffer variables the distinction is blurred because syntactically
> the variable is both pointer-to-storage and storage at the same time, so
> AFAICT the approach you've taken of checking whether image_read_only is
> set for the LHS of an assignment seems reasonable to me, because this
> way all image_* memory qualifiers apply to the storage of buffer
> variables consistently.  But please let's make the check conditional on
> the LHS being a buffer variable because this confusion between variable
> and pointed-to-storage is SSBO-specific.

Right I think I wasn't fully understanding the difference between SSBO and UBO
's when I said it wasn't needed although I think I helped tease out some
issues :)

Just one more comment when you add the new checks it would be nice to just add
it to the existing "assignment to read-only variable" if statement rather than
copying an pasting the error message. This also keeps the error checking code
grouped together a little nicer.


> 
> > > On the other hand read_only is the usual uniform restriction stoping you 
> > > from
> > > assigning to the variable directly e.g myImage = 1; which is why its 
> > > always
> > > set to true.
> > > 
> > > If I'm correct I dont think this patch is needed.
> > > 
> > > > 
> > > > 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) {
> > > > > 
> > > > 
> > > > 
> > >