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 <ian.d.romanick@intel.com>
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=34687
Reviewed-by: Kenneth Graunke <kenneth@whitecape.org>
Acked-by: Paul Berry <stereotype441@gmail.com>

diff --git a/src/glsl/ast_to_hir.cpp b/src/glsl/ast_to_hir.cpp
index ebdbbc10e69ef16b7c8b1d4c77cf255c2bafffa3..ed6abdc7083563bfcdba506080b5a0f75f9ba6ee 100644 (file)
--- 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 ef7300e6535292946f4d1396905c526bcf5b8464..a5eca5a51a0dd15d0c58b42feebd98f3591e4972 100644 (file)
--- 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 abbf4556978d83adbbccee95162df91ade7b3d5f..5878c051b152a6857c65cf879c4d640cdb6e2dc2 100644 (file)
--- a/src/glsl/ir.h
+++ b/src/glsl/ir.h
@@ -364,6 +364,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.
     *
@@ -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 9adf47050d131935137e6576212db5e8592c0db6..e8ac9fbe456074560358d6ef0e9727c65b7fb133 100644 (file)
--- 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<ir_variable *>(this));
    }

diff --git a/src/glsl/ir_validate.cpp b/src/glsl/ir_validate.cpp
index c387ecbcafd7434f7abeb96fc2c74d81bee863f5..a3520120f2d9e3c466b4edb1d995b45f5e571cef 100644 (file)
--- 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 8fbcf1da8cbb8e6eecdc851ff926fcfc366d4fc9..3092507bc39349184d1de84f595f1b7a6500d8e0 100644 (file)
--- 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 a595c9c5f7a9327c88965e74dbead15a28151dd5..915d5bbcf7dd091ef47dc0c1c9c5a07a45a81d04 100644 (file)
--- 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) {