[Mesa-dev] [PACH v2] glsl: SSBO unsized array declarations, if present, must be declared last

[Iago Toral]

On Sun, 2016-10-16 at 17:00 +1100, Timothy Arceri wrote:
> On Fri, 2016-10-14 at 14:30 +0200, Iago Toral Quiroga wrote:
> > 
> > From the ARB_shader_storage_buffer_object spec:
> > 
> > "In a shader storage block, the last member may be declared without
> > an explicit
> >  size.  In this case, the effective array size is inferred at run-
> > time from
> >  the size of the data store backing the interface block.  Such
> > unsized
> >  arrays may be indexed with general integer expressions, but may
> > not
> > be
> >  passed as an argument to a function or indexed with a negative
> > constant
> >  expression."
> > 
> > dEQP tests that SSBOs that declare field members after an unsized
> > array
> > declaration fail to compile.
> > 
> > Fixes the remaining subcase of the following dEQP tests:
> > dEQP-
> > GLES31.functional.debug.negative_coverage.callbacks.shader.compile_
> > co
> > mpute_shader
> > dEQP-
> > GLES31.functional.debug.negative_coverage.get_error.shader.compile_
> > co
> > mpute_shader
> > dEQP-
> > GLES31.functional.debug.negative_coverage.log.shader.compile_comput
> > e_
> > shader
> > 
> > v2: only update has_unsized_array while we have not found a
> > previous
> > unsized
> >     array declaration.
> > 
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=98132
> > ---
> >  src/compiler/glsl/ast_to_hir.cpp | 8 ++++++++
> >  1 file changed, 8 insertions(+)
> > 
> > diff --git a/src/compiler/glsl/ast_to_hir.cpp
> > b/src/compiler/glsl/ast_to_hir.cpp
> > index c3c8cef..462838a 100644
> > --- a/src/compiler/glsl/ast_to_hir.cpp
> > +++ b/src/compiler/glsl/ast_to_hir.cpp
> > @@ -6645,6 +6645,7 @@
> > ast_process_struct_or_iface_block_members(exec_list *instructions,
> >  
> >     bool first_member = true;
> >     bool first_member_has_explicit_location = false;
> > +   bool has_unsized_array = false;
> >  
> >     unsigned i = 0;
> >     foreach_list_typed (ast_declarator_list, decl_list, link,
> > declarations) {
> > @@ -6840,6 +6841,13 @@
> > ast_process_struct_or_iface_block_members(exec_list *instructions,
> >  
> >           const struct glsl_type *field_type =
> >              process_array_type(&loc, decl_type, decl-
> > > 
> > > array_specifier, state);
> > +
> > +         if (has_unsized_array && var_mode ==
> > ir_var_shader_storage)
> > +            _mesa_glsl_error(&loc, state, "SSBO member declared "
> > +                             "after unsized array.");
> > +         else if (!has_unsized_array)
> > +            has_unsized_array = field_type->is_unsized_array();
> > +
> I suspect this fixes named ifc blocks? There is code to detect these
> for unnamed ifc blocks and an is_unsized_array_last_element()
> function.

No, the dEQP test case is not a named interface block.

> I think you want to merge to code from the two of these checks (I
> think
> here is the correct spot) and probably remove
> the is_unsized_array_last_element() helper.

I have been looking into this and the problem seems to be more complex:

The problem with that code is that it is not handling this case, it is
simply producing an error for any unsized array that is not in an SSBO,
but it is not doing anything if the array is in an SSBO. It looks like
Dave changed this and moved the ssbo check to the linker instead
because the desktop spec allows to use unsized arrays that are not last
if the unsized array is implicitly sized. See 5b2675093e863a52.

With this we should be producing an error, but we do it at link time,
not at compile time, which is what dEQP expects. I have been discussing
this with Samuel and the relevant aspect of the spec is this:

"Except for the last declared member of a shader storage block (section
4.3.9 “Interface Blocks”), the size of an array must be declared
(explicitly sized) before it is indexed with anything other than an
integral constant expression. (...) Violation of any of these rules
result in compile-time errors."

So according to this a shader that declares an unsized array and never
uses it (which is the case of this dEQP test), is fine, because we
never try to index it. That would mean that the dEQP test is actually
bogus as is, since it has an empty main function. That said, if the
array is used, is not implicitly sized and is not the last member of
the ssbo, then spec would expect a compile-time error instead of a
link-time error.

I'll see if I can write a patch for this.

> > 
> >           validate_array_dimensions(field_type, state, &loc);
> >           fields[i].type = field_type;
> >           fields[i].name = decl->identifier;