[Mesa-dev] [PATCH] glsl: validate output blocks against input blocks

[Nicolai Hähnle]

On 04.11.2016 13:37, Iago Toral Quiroga wrote:
> Until now were validating in/out blocks by listing the inputs in the
> consumer stage and then, for each output of the producer, we checked that
> it was a match if it was consumed. This method does not catch the case
> where the consumer has an input that is not present as an output in the
> producer stage, because it only generates link errors for outputs present
> in the producer stage that don't match the inputs in the consumer stage.
> The current method does catch the case were an output from the producer
> stage is not consumed, which is irrelevant and is ignored.
>
> By reversing the way we do this, we can detect this situation, so this
> patch lists the outputs of the producer stage and then validates inputs
> of the consumer stage against them. If we see an input in the consumer
> for which there is no associated output in the producer, we produce a
> link error.
>
> The only exception to this is the special built-in input block gl_in[],
> since this is implicitly generated for geometry and tessellation stages,
> but we don't generate it if the producer stage does not write to any of
> the pre-defined outputs (for example, if the vertex shader does not write
> to gl_Position, etc). Since writing to these is not mandatory, do not
> produce a link error in that case. There is a CTS tessellation test
> (GL45-CTS.tessellation_shader.program_object_properties) that has an
> empty vertex shader (so it does not produce gl_in[]) and would fail to
> link if we don't do this.
>
> This fixes the following dEQP test:
> dEQP-GLES31.functional.shaders.linkage.io_block.missing_output_block
>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=98245
> ---
>
> There is a single regression for this patch in Piglit, for
> tests/spec/glsl-1.50/compiler/interface-blocks-containing-unsized-arrays.geom
> (CTS and dEQP don't have regressions). The affected piglit test now produces
> a link error, which is actually the correct result. I have sent a patch to
> Piglit to avoid this.

Makes sense to me.

Reviewed-by: Nicolai Hähnle <nicolai.haehnle at amd.com>

>
>  src/compiler/glsl/link_interface_blocks.cpp | 41 +++++++++++++++++++++--------
>  1 file changed, 30 insertions(+), 11 deletions(-)
>
> diff --git a/src/compiler/glsl/link_interface_blocks.cpp b/src/compiler/glsl/link_interface_blocks.cpp
> index a176c3b..4e91abc 100644
> --- a/src/compiler/glsl/link_interface_blocks.cpp
> +++ b/src/compiler/glsl/link_interface_blocks.cpp
> @@ -344,6 +344,15 @@ validate_intrastage_interface_blocks(struct gl_shader_program *prog,
>     }
>  }
>
> +static bool
> +is_builtin_gl_in_block(ir_variable *var, int consumer_stage)
> +{
> +   return !strcmp(var->name, "gl_in") &&
> +          (consumer_stage == MESA_SHADER_TESS_CTRL ||
> +           consumer_stage == MESA_SHADER_TESS_EVAL ||
> +           consumer_stage == MESA_SHADER_GEOMETRY);
> +}
> +
>  void
>  validate_interstage_inout_blocks(struct gl_shader_program *prog,
>                                   const gl_linked_shader *producer,
> @@ -355,28 +364,38 @@ validate_interstage_inout_blocks(struct gl_shader_program *prog,
>                                     consumer->Stage != MESA_SHADER_FRAGMENT) ||
>                                    consumer->Stage == MESA_SHADER_GEOMETRY;
>
> -   /* Add input interfaces from the consumer to the symbol table. */
> -   foreach_in_list(ir_instruction, node, consumer->ir) {
> +   /* Add output interfaces from the producer to the symbol table. */
> +   foreach_in_list(ir_instruction, node, producer->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() || var->data.mode != ir_var_shader_out)
>           continue;
>
>        definitions.store(var);
>     }
>
> -   /* Verify that the producer's output interfaces match. */
> -   foreach_in_list(ir_instruction, node, producer->ir) {
> +   /* 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_out)
> +      if (!var || !var->get_interface_type() || var->data.mode != ir_var_shader_in)
>           continue;
>
> -      ir_variable *consumer_def = definitions.lookup(var);
> +      ir_variable *producer_def = definitions.lookup(var);
>
> -      /* The consumer doesn't use this output block.  Ignore it. */
> -      if (consumer_def == NULL)
> -         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
> +       * does not write to gl_Position, etc), which is allowed and results in
> +       * undefined behavior.
> +       */
> +      if (producer_def == NULL &&
> +          !is_builtin_gl_in_block(var, consumer->Stage)) {
> +         linker_error(prog, "Input block `%s' is not an output of "
> +                      "the previous stage\n", var->get_interface_type()->name);
> +         return;
> +      }
>
> -      if (!interstage_match(prog, var, consumer_def, extra_array_level)) {
> +      if (producer_def &&
> +          !interstage_match(prog, producer_def, var, extra_array_level)) {
>           linker_error(prog, "definitions of interface block `%s' do not "
>                        "match\n", var->get_interface_type()->name);
>           return;
>