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

[Timothy Arceri]

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

I meant to include a quote from the ARB_shader_image_load_store spec:

    Memory accesses to image variables declared using the "readonly" qualifier
    may only read the underlying memory, which is treated as read-only memory 
    and cannot be written to. It is an error to pass an image variable
qualified 
    with "readonly" to imageStore() or other built-in functions that modify 
    image memory.

    Memory accesses to image variables declared using the "writeonly"
qualifier 
    may only write the underlying memory; the underlying memory cannot be
read. 
    It is an error to pass an image variable qualified with "writeonly" to 
    imageLoad() or other built-in functions that read image memory.



> 
> > 
> > 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) {
> > > 
> > 
> > 
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev