[Mesa-dev] [PATCH] glsl: Fix interstage uniform interface block link error detection.

[Jordan Justen]

On Fri, Nov 15, 2013 at 2:56 PM, Paul Berry <stereotype441 at gmail.com> wrote:
> Previously, we checked for interstage uniform interface block link
> errors in validate_interstage_interface_blocks(), which is only called
> on pairs of adjacent shader stages.  Therefore, we failed to detect
> uniform interface block mismatches between non-adjacent shader stages.
>
> Before the introduction of geometry shaders, this wasn't a problem,
> because the only supported shader stages were vertex and fragment
> shaders, therefore they were always adjacent.  However, now that we
> allow a program to contain vertex, geometry, and fragment shaders,
> that is no longer the case.
>
> Fixes piglit test "skip-stage-uniform-block-array-size-mismatch".
>
> Cc: "10.0" <mesa-stable at lists.freedesktop.org>
> ---
>  src/glsl/link_interface_blocks.cpp | 61 +++++++++++++++++++++++++-------------
>  src/glsl/linker.cpp                |  5 ++++
>  src/glsl/linker.h                  |  4 +++
>  3 files changed, 50 insertions(+), 20 deletions(-)
>
> diff --git a/src/glsl/link_interface_blocks.cpp b/src/glsl/link_interface_blocks.cpp
> index a7fceb9..31269f6 100644
> --- a/src/glsl/link_interface_blocks.cpp
> +++ b/src/glsl/link_interface_blocks.cpp
> @@ -293,8 +293,7 @@ validate_interstage_interface_blocks(struct gl_shader_program *prog,
>                                       const gl_shader *producer,
>                                       const gl_shader *consumer)
>  {
> -   interface_block_definitions inout_interfaces;
> -   interface_block_definitions uniform_interfaces;
> +   interface_block_definitions definitions;
>     const bool extra_array_level = consumer->Type == GL_GEOMETRY_SHADER;
>
>     /* Add non-output interfaces from the consumer to the symbol table. */
> @@ -303,9 +302,7 @@ validate_interstage_interface_blocks(struct gl_shader_program *prog,
>        if (!var || !var->get_interface_type() || var->mode == ir_var_shader_out)
>           continue;
>
> -      interface_block_definitions *definitions = var->mode == ir_var_uniform ?
> -         &uniform_interfaces : &inout_interfaces;
> -      definitions->store(interface_block_definition(var));
> +      definitions.store(interface_block_definition(var));
>     }
>
>     /* Verify that the producer's interfaces match. */
> @@ -314,32 +311,56 @@ validate_interstage_interface_blocks(struct gl_shader_program *prog,
>        if (!var || !var->get_interface_type() || var->mode == ir_var_shader_in)
>           continue;
>
> -      interface_block_definitions *definitions = var->mode == ir_var_uniform ?
> -         &uniform_interfaces : &inout_interfaces;
>        interface_block_definition *consumer_def =
> -         definitions->lookup(var->get_interface_type()->name);
> +         definitions.lookup(var->get_interface_type()->name);
>
>        /* The consumer doesn't use this output block.  Ignore it. */
>        if (consumer_def == NULL)
>           continue;
>
>        const interface_block_definition producer_def(var);
> -      bool match;
> -      if (var->mode == ir_var_uniform) {
> -         /* Uniform matching rules are the same for interstage and intrastage
> -          * linking.
> -          */
> -         match = intrastage_match(consumer_def, &producer_def,
> -                                  (ir_variable_mode) var->mode);
> -      } else {
> -         match = interstage_match(&producer_def, consumer_def,
> -                                  extra_array_level);
> -      }
>
> -      if (!match) {
> +      if (!interstage_match(&producer_def, consumer_def, extra_array_level)) {
>           linker_error(prog, "definitions of interface block `%s' do not "
>                        "match\n", var->get_interface_type()->name);
>           return;
>        }
>     }
>  }
> +
> +
> +void
> +validate_interstage_uniform_blocks(struct gl_shader_program *prog,
> +                                   gl_shader **stages, int num_stages)
> +{
> +   interface_block_definitions definitions;
> +
> +   for (int i = 0; i < num_stages; i++) {
> +      if (stages[i] == NULL)
> +         continue;
> +
> +      const gl_shader *stage = stages[i];
> +      foreach_list(node, stage->ir) {
> +         ir_variable *var = ((ir_instruction *) node)->as_variable();
> +         if (!var || !var->get_interface_type() || var->mode != ir_var_uniform)
> +            continue;
> +
> +         interface_block_definition *old_def =
> +            definitions.lookup(var->get_interface_type()->name);
> +         const interface_block_definition new_def(var);
> +         if (old_def == NULL) {
> +            definitions.store(new_def);
> +         } else {
> +            /* Interstage uniform matching rules are the same as intrastage
> +             * uniform matchin rules (for uniforms, it is as though all
> +             * shaders are in the same shader stage).
> +             */
> +            if (!intrastage_match(old_def, &new_def, ir_var_uniform)) {
> +               linker_error(prog, "definitions of interface block `%s' do not "
> +                            "match\n", var->get_interface_type()->name);
> +               return;
> +            }
> +         }
> +      }
> +   }
> +}
> diff --git a/src/glsl/linker.cpp b/src/glsl/linker.cpp
> index 1d53b65..01b278c 100644
> --- a/src/glsl/linker.cpp
> +++ b/src/glsl/linker.cpp
> @@ -2168,6 +2168,11 @@ link_shaders(struct gl_context *ctx, struct gl_shader_program *prog)
>        prev = i;
>     }
>
> +   /* Cross-validate uniform blocks between shader stages */
> +   validate_interstage_uniform_blocks(prog, prog->_LinkedShaders,
> +                                      MESA_SHADER_TYPES);
> +   if (!prog->LinkStatus)
> +      goto done;
>
>     for (unsigned int i = 0; i < MESA_SHADER_TYPES; i++) {
>        if (prog->_LinkedShaders[i] != NULL)
> diff --git a/src/glsl/linker.h b/src/glsl/linker.h
> index 7b1f6f9..b285877 100644
> --- a/src/glsl/linker.h
> +++ b/src/glsl/linker.h
> @@ -69,6 +69,10 @@ validate_interstage_interface_blocks(struct gl_shader_program *prog,

It kind of seems like we should now have 'inout' in this function name.

Reviewed-by: Jordan Justen <jordan.l.justen at intel.com>

>                                       const gl_shader *producer,
>                                       const gl_shader *consumer);
>
> +void
> +validate_interstage_uniform_blocks(struct gl_shader_program *prog,
> +                                   gl_shader **stages, int num_stages);
> +
>  extern void
>  link_assign_atomic_counter_resources(struct gl_context *ctx,
>                                       struct gl_shader_program *prog);
> --
> 1.8.4.2
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev