From f37b1ad937dd2c420f4c9fd9aa5887942bd31f3f Mon Sep 17 00:00:00 2001 From: Ian Romanick Date: Mon, 31 Oct 2011 14:31:07 -0700 Subject: [PATCH] linker: Check that initializers for global variables match This requires tracking a couple extra fields in ir_variable: * A flag to indicate that a variable had an initializer. * For non-const variables, a field to track the constant value of the variable's initializer. For variables non-constant initalizers, ir_variable::has_initializer will be true, but ir_variable::constant_initializer will be NULL. The linker can use the values of these fields to check adherence to the GLSL 4.20 rules for shared global variables: "If a shared global has multiple initializers, the initializers must all be constant expressions, and they must all have the same value. Otherwise, a link error will result. (A shared global having only one initializer does not require that initializer to be a constant expression.)" Previous to 4.20 the GLSL spec simply said that initializers must have the same value. In this case of non-constant initializers, this was impossible to determine. As a result, no vendor actually implemented that behavior. The 4.20 behavior matches the behavior of NVIDIA's shipping implementations. NOTE: This is candidate for the 7.11 branch. This patch also needs the preceding patch "glsl: Refactor generate_ARB_draw_buffers_variables to use add_builtin_constant" Signed-off-by: Ian Romanick Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=34687 Reviewed-by: Kenneth Graunke Acked-by: Paul Berry --- src/glsl/ast_to_hir.cpp | 3 +++ src/glsl/ir.cpp | 5 +++++ src/glsl/ir.h | 18 ++++++++++++++++++ src/glsl/ir_clone.cpp | 5 +++++ src/glsl/ir_validate.cpp | 7 +++++++ src/glsl/ir_variable.cpp | 2 ++ src/glsl/linker.cpp | 48 +++++++++++++++++++++++++++++++++++++++++------- 7 files changed, 81 insertions(+), 7 deletions(-) diff --git a/src/glsl/ast_to_hir.cpp b/src/glsl/ast_to_hir.cpp index ebdbbc1..ed6abdc 100644 --- a/src/glsl/ast_to_hir.cpp +++ b/src/glsl/ast_to_hir.cpp @@ -2367,6 +2367,9 @@ process_initializer(ir_variable *var, ast_declaration *decl, } else initializer_type = rhs->type; + var->constant_initializer = rhs->constant_expression_value(); + var->has_initializer = true; + /* If the declared variable is an unsized array, it must inherrit * its full type from the initializer. A declaration such as * diff --git a/src/glsl/ir.cpp b/src/glsl/ir.cpp index ef7300e..a5eca5a 100644 --- a/src/glsl/ir.cpp +++ b/src/glsl/ir.cpp @@ -1326,9 +1326,11 @@ ir_variable::ir_variable(const struct glsl_type *type, const char *name, this->type = type; this->name = ralloc_strdup(this, name); this->explicit_location = false; + this->has_initializer = false; this->location = -1; this->warn_extension = NULL; this->constant_value = NULL; + this->constant_initializer = NULL; this->origin_upper_left = false; this->pixel_center_integer = false; this->depth_layout = ir_depth_layout_none; @@ -1489,6 +1491,9 @@ steal_memory(ir_instruction *ir, void *new_ctx) if (var != NULL && var->constant_value != NULL) steal_memory(var->constant_value, ir); + if (var != NULL && var->constant_initializer != NULL) + steal_memory(var->constant_initializer, ir); + /* The components of aggregate constants are not visited by the normal * visitor, so steal their values by hand. */ diff --git a/src/glsl/ir.h b/src/glsl/ir.h index abbf455..5878c05 100644 --- a/src/glsl/ir.h +++ b/src/glsl/ir.h @@ -365,6 +365,14 @@ public: unsigned explicit_location:1; /** + * Does this variable have an initializer? + * + * This is used by the linker to cross-validiate initializers of global + * variables. + */ + unsigned has_initializer:1; + + /** * \brief Layout qualifier for gl_FragDepth. * * This is not equal to \c ir_depth_layout_none if and only if this @@ -414,6 +422,16 @@ public: * Value assigned in the initializer of a variable declared "const" */ ir_constant *constant_value; + + /** + * Constant expression assigned in the initializer of the variable + * + * \warning + * This field and \c ::constant_value are distinct. Even if the two fields + * refer to constants with the same value, they must point to separate + * objects. + */ + ir_constant *constant_initializer; }; diff --git a/src/glsl/ir_clone.cpp b/src/glsl/ir_clone.cpp index 9adf470..e8ac9fb 100644 --- a/src/glsl/ir_clone.cpp +++ b/src/glsl/ir_clone.cpp @@ -50,6 +50,7 @@ ir_variable::clone(void *mem_ctx, struct hash_table *ht) const var->origin_upper_left = this->origin_upper_left; var->pixel_center_integer = this->pixel_center_integer; var->explicit_location = this->explicit_location; + var->has_initializer = this->has_initializer; var->num_state_slots = this->num_state_slots; if (this->state_slots) { @@ -68,6 +69,10 @@ ir_variable::clone(void *mem_ctx, struct hash_table *ht) const if (this->constant_value) var->constant_value = this->constant_value->clone(mem_ctx, ht); + if (this->constant_initializer) + var->constant_initializer = + this->constant_initializer->clone(mem_ctx, ht); + if (ht) { hash_table_insert(ht, var, (void *)const_cast(this)); } diff --git a/src/glsl/ir_validate.cpp b/src/glsl/ir_validate.cpp index c387ecb..a352012 100644 --- a/src/glsl/ir_validate.cpp +++ b/src/glsl/ir_validate.cpp @@ -496,6 +496,13 @@ ir_validate::visit(ir_variable *ir) } } + if (ir->constant_initializer != NULL && !ir->has_initializer) { + printf("ir_variable didn't have an initializer, but has a constant " + "initializer value.\n"); + ir->print(); + abort(); + } + return visit_continue; } diff --git a/src/glsl/ir_variable.cpp b/src/glsl/ir_variable.cpp index 8fbcf1d..3092507 100644 --- a/src/glsl/ir_variable.cpp +++ b/src/glsl/ir_variable.cpp @@ -408,6 +408,8 @@ add_builtin_constant(exec_list *instructions, glsl_symbol_table *symtab, name, glsl_type::int_type, ir_var_auto, -1); var->constant_value = new(var) ir_constant(value); + var->constant_initializer = new(var) ir_constant(value); + var->has_initializer = true; return var; } diff --git a/src/glsl/linker.cpp b/src/glsl/linker.cpp index a595c9c..915d5bb 100644 --- a/src/glsl/linker.cpp +++ b/src/glsl/linker.cpp @@ -448,17 +448,30 @@ cross_validate_globals(struct gl_shader_program *prog, } } - /* FINISHME: Handle non-constant initializers. + /* Page 35 (page 41 of the PDF) of the GLSL 4.20 spec says: + * + * "If a shared global has multiple initializers, the + * initializers must all be constant expressions, and they + * must all have the same value. Otherwise, a link error will + * result. (A shared global having only one initializer does + * not require that initializer to be a constant expression.)" + * + * Previous to 4.20 the GLSL spec simply said that initializers + * must have the same value. In this case of non-constant + * initializers, this was impossible to determine. As a result, + * no vendor actually implemented that behavior. The 4.20 + * behavior matches the implemented behavior of at least one other + * vendor, so we'll implement that for all GLSL versions. */ - if (var->constant_value != NULL) { - if (existing->constant_value != NULL) { - if (!var->constant_value->has_value(existing->constant_value)) { + if (var->constant_initializer != NULL) { + if (existing->constant_initializer != NULL) { + if (!var->constant_initializer->has_value(existing->constant_initializer)) { linker_error(prog, "initializers for %s " "`%s' have differing values\n", mode_string(var), var->name); return false; } - } else + } else { /* If the first-seen instance of a particular uniform did not * have an initializer but a later instance does, copy the * initializer to the version stored in the symbol table. @@ -471,8 +484,29 @@ cross_validate_globals(struct gl_shader_program *prog, * FINISHME: modify the shader, and linking with the second * FINISHME: will fail. */ - existing->constant_value = - var->constant_value->clone(ralloc_parent(existing), NULL); + existing->constant_initializer = + var->constant_initializer->clone(ralloc_parent(existing), + NULL); + } + } + + if (var->has_initializer) { + if (existing->has_initializer + && (var->constant_initializer == NULL + || existing->constant_initializer == NULL)) { + linker_error(prog, + "shared global variable `%s' has multiple " + "non-constant initializers.\n", + var->name); + return false; + } + + /* Some instance had an initializer, so keep track of that. In + * this location, all sorts of initializers (constant or + * otherwise) will propagate the existence to the variable + * stored in the symbol table. + */ + existing->has_initializer = true; } if (existing->invariant != var->invariant) { -- 2.7.4