[Bug 109532] ir_variable has maximum access out of bounds -- but it's not out of bounds

[bugzilla-daemon at freedesktop.org]

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

--- Comment #19 from andrii simiklit <andrey.simiklit.1989 at gmail.com> ---
(In reply to Ian Romanick from comment #18)
> (In reply to asimiklit from comment #7)
> > (In reply to Ilia Mirkin from comment #2)
> > > Looks like this is happening in link_uniform_blocks. The type gets updated:
> > > 
> > >       if (b->array != NULL &&
> > >           (b->type->without_array()->interface_packing ==
> > >            GLSL_INTERFACE_PACKING_PACKED)) {
> > >          b->type = resize_block_array(b->type, b->array);
> > >          b->var->type = b->type;
> > >       }
> > > 
> > > But no change to the corresponding max_data_access. However doing the naive
> > > thing here didn't help -- the shader fails. I didn't investigate why. I
> > > think create_buffer_blocks also needs to be involved somehow.
> > 
> > Thanks a lot your message was very helpful to find the start point of
> > investigation.
> > 
> > Looks like this thing is helped:
> > 
> > ---
> >  src/compiler/glsl/link_uniform_blocks.cpp | 7 +++++++
> >  1 file changed, 7 insertions(+)
> > 
> > diff --git a/src/compiler/glsl/link_uniform_blocks.cpp
> > b/src/compiler/glsl/link_uniform_blocks.cpp
> > index 0b890586298..5197870a6d9 100644
> > --- a/src/compiler/glsl/link_uniform_blocks.cpp
> > +++ b/src/compiler/glsl/link_uniform_blocks.cpp
> > @@ -438,8 +438,15 @@ link_uniform_blocks(void *mem_ctx,
> >        if (b->array != NULL &&
> >            (b->type->without_array()->interface_packing ==
> >             GLSL_INTERFACE_PACKING_PACKED)) {
> > +         const int new_max_array_access =
> > (int)(b->array->num_array_elements - 1);
> >           b->type = resize_block_array(b->type, b->array);
> >           b->var->type = b->type;
> > +
> > +         assert(((int)b->array->array_elements[new_max_array_access] ==
> > +                b->var->data.max_array_access ||
> > +                b->var->data.max_array_access == -1) && "Is last index is
> > proper");
> > +
> > +         b->var->data.max_array_access = new_max_array_access;
> >        }
> >  
> >        block_size.num_active_uniforms = 0;
> 
> I don't think this has any chance of being correct.  max_array_access is set
> based on actual, known accesses to the array.  Changing that arbitrarily is
> just lying to the rest of the compiler.  Nothing good can come from that. 
> Since the type of the array is now wrong, my guess is that the test fails
> (instead of crashes) because the part of the array that has the data the
> shader wants is optimized away.
This is changed not arbitrarily. All elements of the new optimized array type
are 100% used so index of the last element of this array is a max_array_access,
no?

> 
> How does the mismatch between max_array_access and the type occur in the
> first place?  My guess is that this is the source of the bug.  They *must*
> agree at some point, or an earlier part of the compile would have generated
> an error (as required by the spec).
Do you know where the location of the code which should leads them to
agreement?
Unfortunately I didn't find this code. 
Shouldn't it be correct to lead them to agreement here?

https://gitlab.freedesktop.org/mesa/mesa/blob/master/src/compiler/glsl/link_uniform_blocks.cpp#L441
Seems like a new optimized array type should contain used/active instances only
so it should be correct to suppose that last element of this array is 100% used
and max_array_access can points to this element. Didn't I consider something
here?



About deqp test, it fails in such cases:

layout(local_size_x = 1) in;

layout(packed, binding = 0) buffer BlockB
{
        float b[1];
} blockB[2];

void main (void)
{
    blockB[1].b[0] = 77777.77777;
}

and looks like problem is here:
https://android.googlesource.com/platform/external/deqp/+/master/modules/gles31/functional/es31fSSBOLayoutCase.cpp#2249

deqp test uses binding point 1 for BlockB[1] but when we use 'packed' layout
the BlockB[0] instance
is removed due to unuse and there is just one array instance BlockB[1] with
assigned binding point 0 instead of 1.

Looks like it should be something like this:
   if (layoutNdx >= 0)
   {
      const BlockLocation& blockLoc = blockLocations[layoutNdx];
      if (blockLoc.size > 0)
         gl.bindBufferRange(GL_SHADER_STORAGE_BUFFER, bindingPoint,
buffers[blockLoc.index].buffer, blockLoc.offset, blockLoc.size);

      bindingPoint += 1;
   }
   else if((block.getFlags() & LAYOUT_PACKED) == 0)
   {
      bindingPoint += 1;
   }

I read the spec and looks like it is acceptable to remove the instance of array
if it isn't active.
I will provide the patch (to test it on Intel CI if possible) and more details
on Monday
(because I left everything on work machine).

Thanks,
Andrii.

-- 
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/20190209/60902562/attachment.html>