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

[Eduardo Lima Mitev]

On 03/08/2017 08:29 PM, Jordan Justen wrote:
> On 2017-03-05 11:28:43, 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."
> 
> We try to make sure that the interface matches for input variables in
> the code just below where you added the additional check. What case is
> it missing? Is there not an input variable being read in the test case
> to trigger the interface match check?
> 

Indeed, there isn't an input variable available to match consumer and
producer interfaces. A variable was optimized out, but we saved its
block interface in the symbols table (with the previous patches).

The current check gets the interface blocks from a consumer variable,
and tries to find a variable in the producer that has the same interface
block, then it tries to match them. It needs the two variables to exist
in the symbols table.

The new check implemented here, gets also the interface block from a
consumer variable, and tries to find *that interface block by name* in
the symbols table of the producer. The variable was still optimized out.
For example,

if we have in the producer:

out gl_PerVertex
{
    vec4 gl_Position;
};

and in the consumer:

in gl_PerVertex
{
    float gl_ClipDistance[];
} gl_in[];

But the consumer block is never used (gl_in.gl_ClipDistance is not used
anywhere in the shader), the original check will fail to validate the
two blocks because gl_in doesn't exist. The new check see gl_Position,
gets its block (gl_PerVertex), and look for a gl_PerVertex as input in
the consumer, then try to match them.

Now, by doing the exercise of explaining this here :), I just realized
that if both the consumer and the producer variables are unused, it will
never be able to match them. So this patch is incomplete. It will only
cover the case where only one of the shaders has an unused interface
block, as is the case with the CTS test.

I will give it some thought and come back with a more general solution.

Thanks a lot for looking into it, Jordan!

Eduardo

> -Jordan
> 
>> Fixes:
>> * GL45-CTS.CommonBugs.CommonBug_PerVertexValidation
>> ---
>>  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
>> -- 
>> 2.11.0
>>
>> _______________________________________________
>> mesa-dev mailing list
>> mesa-dev at lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>