<div dir="ltr">This patch was sent out for review before the Mesa 10.0 branch but didn't land until after the branch was made. It should be cherry-picked to stable, since it's a bug fix to functionality that is new in Mesa 10.0. <div> <div class="gmail_quote">---------- Forwarded message ---------- From: Paul Berry <<a href="mailto:stereotype441@gmail.com">stereotype441@gmail.com</a>> Date: 30 October 2013 16:33 Subject: [PATCH] glsl: Rework interface block linking. To: <a href="mailto:mesa-dev@lists.freedesktop.org">mesa-dev@lists.freedesktop.org</a> Previously, when doing intrastage and interstage interface block linking, we only checked the interface type; this prevented us from catching some link errors. We now check the following additional constraints: - For intrastage linking, the presence/absence of interface names must match. - For shader ins/outs, the interface names themselves must match when doing intrastage linking (note: it's not clear from the spec whether this is necessary, but Mesa's implementation currently relies on it). - Array vs. nonarray must be consistent, taking into account the special rules for vertex-geometry linkage. - Array sizes must be consistent (exception: during intrastage linking, an unsized array matches a sized array). Note: validate_interstage_interface_blocks currently handles both uniforms and in/out variables. As a result, if all three shader types are present (VS, GS, and FS), and a uniform interface block is mentioned in the VS and FS but not the GS, it won't be validated. I plan to address this in later patches. Fixes the following piglit tests in spec/glsl-1.50/linker: - interface-blocks-vs-fs-array-size-mismatch - interface-vs-array-to-fs-unnamed - interface-vs-unnamed-to-fs-array - intrastage-interface-unnamed-array --- src/glsl/link_interface_blocks.cpp | 280 ++++++++++++++++++++++++++++++++++--- 1 file changed, 260 insertions(+), 20 deletions(-) diff --git a/src/glsl/link_interface_blocks.cpp b/src/glsl/link_interface_blocks.cpp index 4f1c9d3..58c9538 100644 --- a/src/glsl/link_interface_blocks.cpp +++ b/src/glsl/link_interface_blocks.cpp @@ -30,13 +30,219 @@ #include "glsl_symbol_table.h" #include "linker.h" #include "main/macros.h" +#include "program/hash_table.h" + + +namespace { + +/** + * Information about a single interface block definition that we need to keep + * track of in order to check linkage rules. + * + * Note: this class is expected to be short lived, so it doesn't make copies + * of the strings it references; it simply borrows the pointers from the + * ir_variable class. + */ +struct interface_block_definition +{ + /** + * Extract an interface block definition from an ir_variable that + * represents either the interface instance (for named interfaces), or a + * member of the interface (for unnamed interfaces). + */ + explicit interface_block_definition(const ir_variable *var) + : type(var->get_interface_type()), + instance_name(NULL), + array_size(-1) + { + if (var->is_interface_instance()) { + instance_name = var->name; + if (var->type->is_array()) + array_size = var->type->length; + } + } + + /** + * Interface block type + */ + const glsl_type *type; + + /** + * For a named interface block, the instance name. Otherwise NULL. + */ + const char *instance_name; + + /** + * For an interface block array, the array size (or 0 if unsized). + * Otherwise -1. + */ + int array_size; +}; + + +/** + * Check if two interfaces match, according to intrastage interface matching + * rules. If they do, and the first interface uses an unsized array, it will + * be updated to reflect the array size declared in the second interface. + */ +bool +intrastage_match(interface_block_definition *a, + const interface_block_definition *b, + ir_variable_mode mode) +{ + /* Types must match. */ + if (a->type != b->type) + return false; + + /* Presence/absence of interface names must match. */ + if ((a->instance_name == NULL) != (b->instance_name == NULL)) + return false; + + /* For uniforms, instance names need not match. For shader ins/outs, + * it's not clear from the spec whether they need to match, but + * Mesa's implementation relies on them matching. + */ + if (a->instance_name != NULL && mode != ir_var_uniform && + strcmp(a->instance_name, b->instance_name) != 0) { + return false; + } + + /* Array vs. nonarray must be consistent, and sizes must be + * consistent, with the exception that unsized arrays match sized + * arrays. + */ + if (a->array_size == -1) { + if (b->array_size != -1) + return false; + } else if (a->array_size == 0) { + if (b->array_size == -1) + return false; + else if (b->array_size != 0) { + /* An unsized array matches a sized array. Record the size to + * match against future declarations. + */ + a->array_size = b->array_size; + } + } else { + /* An unsized array matches a sized array. */ + if (b->array_size != 0 && a->array_size != b->array_size) + return false; + } + return true; +} + + +/** + * Check if two interfaces match, according to interstage (in/out) interface + * matching rules. + * + * If \c extra_array_level is true, then vertex-to-geometry shader matching + * rules are enforced (i.e. a successful match requires the consumer interface + * to be an array and the producer interface to be a non-array). + */ +bool +interstage_match(const interface_block_definition *producer, + const interface_block_definition *consumer, + bool extra_array_level) +{ + /* Unsized arrays should not occur during interstage linking. They + * should have all been assigned a size by link_intrastage_shaders. + */ + assert(consumer->array_size != 0); + assert(producer->array_size != 0); + + /* Types must match. */ + if (consumer->type != producer->type) + return false; + if (extra_array_level) { + /* Consumer must be an array, and producer must not. */ + if (consumer->array_size == -1) + return false; + if (producer->array_size != -1) + return false; + } else { + /* Array vs. nonarray must be consistent, and sizes must be consistent. + * Since unsized arrays have been ruled out, we can check this by just + * making sure the sizes are equal. + */ + if (consumer->array_size != producer->array_size) + return false; + } + return true; +} + + +/** + * This class keeps track of a mapping from an interface block name to the + * necessary information about that interface block to determine whether to + * generate a link error. + * + * Note: this class is expected to be short lived, so it doesn't make copies + * of the strings it references; it simply borrows the pointers from the + * ir_variable class. + */ +class interface_block_definitions +{ +public: + interface_block_definitions() + : mem_ctx(ralloc_context(NULL)), + ht(hash_table_ctor(0, hash_table_string_hash, + hash_table_string_compare)) + { + } + + ~interface_block_definitions() + { + hash_table_dtor(ht); + ralloc_free(mem_ctx); + } + + /** + * Lookup the interface definition having the given block name. Return + * NULL if none is found. + */ + interface_block_definition *lookup(const char *block_name) + { + return (interface_block_definition *) hash_table_find(ht, block_name); + } + + /** + * Add a new interface definition. + */ + void store(const interface_block_definition &def) + { + interface_block_definition *hash_entry = + rzalloc(mem_ctx, interface_block_definition); + *hash_entry = def; + hash_table_insert(ht, hash_entry, def.type->name); + } + +private: + /** + * Ralloc context for data structures allocated by this class. + */ + void *mem_ctx; + + /** + * Hash table mapping interface block name to an \c + * interface_block_definition struct. interface_block_definition structs + * are allocated using \c mem_ctx. + */ + hash_table *ht; +}; + + +}; /* anonymous namespace */ + void validate_intrastage_interface_blocks(struct gl_shader_program *prog, const gl_shader **shader_list, unsigned num_shaders) { - glsl_symbol_table interfaces; + interface_block_definitions in_interfaces; + interface_block_definitions out_interfaces; + interface_block_definitions uniform_interfaces; for (unsigned int i = 0; i < num_shaders; i++) { if (shader_list[i] == NULL) @@ -52,17 +258,36 @@ validate_intrastage_interface_blocks(struct gl_shader_program *prog, if (iface_type == NULL) continue; - const glsl_type *old_iface_type = - interfaces.get_interface(iface_type->name, - (enum ir_variable_mode) var->mode); + interface_block_definitions *definitions; + switch (var->mode) { + case ir_var_shader_in: + definitions = &in_interfaces; + break; + case ir_var_shader_out: + definitions = &out_interfaces; + break; + case ir_var_uniform: + definitions = &uniform_interfaces; + break; + default: + /* Only in, out, and uniform interfaces are legal, so we should + * never get here. + */ + assert(!"illegal interface type"); + continue; + } - if (old_iface_type == NULL) { + const interface_block_definition def(var); + interface_block_definition *prev_def = + definitions->lookup(iface_type->name); + + if (prev_def == NULL) { /* This is the first time we've seen the interface, so save - * it into our symbol table. + * it into the appropriate data structure. */ - interfaces.add_interface(iface_type->name, iface_type, - (enum ir_variable_mode) var->mode); - } else if (old_iface_type != iface_type) { + definitions->store(def); + } else if (!intrastage_match(prev_def, &def, + (ir_variable_mode) var->mode)) { linker_error(prog, "definitions of interface block `%s' do not" " match\n", iface_type->name); return; @@ -76,7 +301,9 @@ validate_interstage_interface_blocks(struct gl_shader_program *prog, const gl_shader *producer, const gl_shader *consumer) { - glsl_symbol_table interfaces; + interface_block_definitions inout_interfaces; + interface_block_definitions uniform_interfaces; + bool extra_array_level = consumer->Type == GL_GEOMETRY_SHADER; /* Add non-output interfaces from the consumer to the symbol table. */ foreach_list(node, consumer->ir) { @@ -84,9 +311,10 @@ validate_interstage_interface_blocks(struct gl_shader_program *prog, if (!var || !var->get_interface_type() || var->mode == ir_var_shader_out) continue; - interfaces.add_interface(var->get_interface_type()->name, - var->get_interface_type(), - (enum ir_variable_mode) var->mode); + interface_block_definitions *definitions = var->mode == ir_var_uniform ? + &uniform_interfaces : &inout_interfaces; + const interface_block_definition def(var); + definitions->store(def); } /* Verify that the producer's interfaces match. */ @@ -95,17 +323,29 @@ validate_interstage_interface_blocks(struct gl_shader_program *prog, if (!var || !var->get_interface_type() || var->mode == ir_var_shader_in) continue; - enum ir_variable_mode consumer_mode = - var->mode == ir_var_uniform ? ir_var_uniform : ir_var_shader_in; - const glsl_type *expected_type = - interfaces.get_interface(var->get_interface_type()->name, - consumer_mode); + 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); /* The consumer doesn't use this output block. Ignore it. */ - if (expected_type == NULL) + if (consumer_def == NULL) continue; - if (var->get_interface_type() != expected_type) { + 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) { linker_error(prog, "definitions of interface block `%s' do not " "match\n", var->get_interface_type()->name); return; -- 1.8.4.2 </div> </div></div>