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

[Eduardo Lima Mitev]

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.

> 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. So I'm open to drop the series if we still think we should not
be this strict. 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
>