[Mesa-dev] [PATCH 3/3] glsl/linker: Check that re-declared, inter-shader built-in blocks match

[Timothy Arceri]

On Tue, 2017-01-31 at 10:28 +0100, Eduardo Lima Mitev wrote:
> On 01/27/2017 11:32 PM, Timothy Arceri wrote:
> > On Fri, 2017-01-27 at 17:42 +0100, Eduardo Lima Mitev wrote:
> > > From GLSL 4.5 spec, section "7.1 Built-In Language Variables",
> > > page
> > > 130 of
> > > the PDF states:
> > > 
> > >     "If multiple shaders using members of a built-in block
> > > belonging
> > > to
> > >      the same interface are linked together in the same program,
> > > they
> > > must
> > >      all redeclare the built-in block in the same way, as
> > > described
> > > in
> > >      section 4.3.9 “Interface Blocks” for interface-block
> > > matching,
> > > or a
> > >      link-time error will result."
> > > 
> > > Fixes:
> > > * GL45-CTS.CommonBugs.CommonBug_PerVertexValidation
> > 
> > 
> > I was looking at this test yesterday and noticed the the GS input
> > is
> > not used, so technically there is no reason the builtin can't be
> > optimised away. This means there is no need for validation between
> > the
> > GS interface and whatever output it is linked against, and is IMO a
> > bug
> > in the test.
> > 
> 
> This is a valid point and was also my first impression when I looked
> into the test. My argument to go and fix this in Mesa is that the
> fact
> that a driver optimizes out an unused variable is/should be
> transparent
> from a spec point of view. A different driver could very well fail
> linkage and would not be violating the spec either.

Usually implementations will only throw errors when a spec says to do
so, unless there is a technical reason we need to throw one. Being
restrictive when the spec doesn't say to usually leads to trouble as
something will work on one implementation and not on another. If there
is no technical reason not to allow something you don't want to be the
implementation that's failing to load the app.

If you really think it should be disallowed then I think you should
file a spec bug.


> 
> > Therefore I'm concerned that this series is forcing validation on
> > unused varyings which isn't required by the spec.
> > 
> 
> This is true. But the way I see it, there is a stricter and a relaxed
> way of looking at this. Your concern above materializes when a shader
> has a built-in block re-declared, it is incompatible with
> previous/next
> shader's interface, and the variable is unused. Failing linkage in
> this
> scenario is certainly not in the spec, but perhaps the most sensible
> thing to so. The shaders are wrong. If I was a shader developer, I
> think
> I would prefer linkage to fail here, just in case I have a legit bug
> in
> my shaders.
> 
> In any case, I already have a local fix for the test, which I plan
> submit. 

Did the fix cause the test to pass without your series? When I tried it
still failed but maybe I did something wrong.

> So I'm open to drop the series if we still think we should not
> be this strict.

As I said above I'd rather we not be more strict that the spec asks
for, if you really want to change it then please file a spec bug.

>  However, code gets factorized in patches 1 and 2
> regardless, so will send a v2 for those (dropping the copying of
> blocks).
> 
> Thanks for the feedback, Timothy!
> 
> Eduardo
> 
> > 
> > > ---
> > >  src/compiler/glsl/link_interface_blocks.cpp | 33
> > > ++++++++++++++++++++++++++++-
> > >  1 file changed, 32 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/src/compiler/glsl/link_interface_blocks.cpp
> > > b/src/compiler/glsl/link_interface_blocks.cpp
> > > index 7037c7776de..4c0278661fa 100644
> > > --- a/src/compiler/glsl/link_interface_blocks.cpp
> > > +++ b/src/compiler/glsl/link_interface_blocks.cpp
> > > @@ -376,11 +376,42 @@ validate_interstage_inout_blocks(struct
> > > gl_shader_program *prog,
> > >     /* Verify that the consumer's input interfaces match. */
> > >     foreach_in_list(ir_instruction, node, consumer->ir) {
> > >        ir_variable *var = node->as_variable();
> > > -      if (!var || !var->get_interface_type() || var->data.mode
> > > !=
> > > ir_var_shader_in)
> > > +      if (!var || !var->get_interface_type())
> > >           continue;
> > >  
> > >        ir_variable *producer_def = definitions.lookup(var);
> > >  
> > > +      /* Check that all built-in block re-declarations are
> > > compatible
> > > +       * across shaders: From OpenGL Shading Language 4.5,
> > > section
> > > +       * "7.1 Built-In Language Variables", page 130 of the PDF:
> > > +       *
> > > +       *    "If multiple shaders using members of a built-in
> > > block
> > > belonging
> > > +       *     to the same interface are linked together in the
> > > same
> > > program,
> > > +       *     they must all redeclare the built-in block in the
> > > same
> > > way, as
> > > +       *     described in section 4.3.9 “Interface Blocks” for
> > > interface-block
> > > +       *     matching, or a link-time error will result."
> > > +       */
> > > +      const glsl_type *consumer_iface =
> > > +         consumer->symbols->get_interface(var-
> > > >get_interface_type()-
> > > > name,
> > > 
> > > +                                          ir_var_shader_in);
> > > +
> > > +      const glsl_type *producer_iface = NULL;
> > > +      if (producer_def && producer_def->get_interface_type()) {
> > > +         producer_iface =
> > > +            producer->symbols->get_interface(producer_def-
> > > > get_interface_type()->name,
> > > 
> > > +                                             ir_var_shader_out);
> > > +      }
> > > +
> > > +      if (producer_iface && consumer_iface &&
> > > +          interstage_member_mismatch(prog, consumer_iface,
> > > producer_iface)) {
> > > +         linker_error(prog, "Incompatible or missing
> > > gl_PerVertex
> > > re-declaration"
> > > +                      "in consecutive shaders");
> > > +         return;
> > > +      }
> > > +
> > > +      if (var->data.mode != ir_var_shader_in)
> > > +         continue;
> > > +
> > >        /* The producer doesn't generate this input: fail to link.
> > > Skip built-in
> > >         * 'gl_in[]' since that may not be present if the producer
> > > does not
> > >         * write to any of the pre-defined outputs (e.g. if the
> > > vertex
> > > shader
> 
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev