Mesa (staging/21.1): glsl: relax rule on varying matching for shaders older than 4.20

[GitLab Mirror]

Module: Mesa
Branch: staging/21.1
Commit: 10556f735c4219d8d1d1fe51c3905a9392b18017
URL:    http://cgit.freedesktop.org/mesa/mesa/commit/?id=10556f735c4219d8d1d1fe51c3905a9392b18017

Author: Timothy Arceri <tarceri at itsqueeze.com>
Date:   Wed Jul 14 17:29:46 2021 +1000

glsl: relax rule on varying matching for shaders older than 4.20

This expands on commit c54c42321ea5. See the code comment for full
justifications. At the time of the previous commit Ian wanted to
limit the relaxing of the rule to GLSL 3.30 as that was the highest
version of shaders seen in the wild that were having trouble with
the stricter rules.

However since then I've found that the long standing issue with tess
shaders failing to compile in the game 'Layers Of Fear' is due to
this same issue. The game uses 4.10 shaders and also makes use of
explicit varying locations, so here we relax the rule to 4.20 and
make sure to apply the restriction to shaders using varyings with
explicit locations also.

Fixes: c54c42321ea5 ("glsl: relax rule on varying matching for shaders older than 4.00")

Reviewed-by: Samuel Iglesias Gonsálvez <siglesias at igalia.com>
Reviewed-by: Matt Turner <mattst88 at gmail.com>
Part-of: <https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/11873>
(cherry picked from commit 0e0633ca49425dbc869521cede6a82d2d91c8042)

---

 .pick_status.json                   |  2 +-
 src/compiler/glsl/link_varyings.cpp | 71 +++++++++++++++++++++----------------
 2 files changed, 41 insertions(+), 32 deletions(-)

diff --git a/.pick_status.json b/.pick_status.json
index e5a1bce8ce0..1e707b22c2d 100644
--- a/.pick_status.json
+++ b/.pick_status.json
@@ -328,7 +328,7 @@
         "description": "glsl: relax rule on varying matching for shaders older than 4.20",
         "nominated": true,
         "nomination_type": 1,
-        "resolution": 0,
+        "resolution": 1,
         "main_sha": null,
         "because_sha": "c54c42321ea5a3d9a09bbe89c00346f8c26b9300"
     },
diff --git a/src/compiler/glsl/link_varyings.cpp b/src/compiler/glsl/link_varyings.cpp
index dc5cb65612a..120682bb527 100644
--- a/src/compiler/glsl/link_varyings.cpp
+++ b/src/compiler/glsl/link_varyings.cpp
@@ -739,6 +739,44 @@ validate_first_and_last_interface_explicit_locations(struct gl_context *ctx,
    }
 }
 
+/**
+ * Check if we should force input / output matching between shader
+ * interfaces.
+ *
+ * Section 4.3.4 (Inputs) of the GLSL 4.10 specifications say:
+ *
+ *   "Only the input variables that are actually read need to be
+ *    written by the previous stage; it is allowed to have
+ *    superfluous declarations of input variables."
+ *
+ * However it's not defined anywhere as to how we should handle
+ * inputs that are not written in the previous stage and it's not
+ * clear what "actually read" means.
+ *
+ * The GLSL 4.20 spec however is much clearer:
+ *
+ *    "Only the input variables that are statically read need to
+ *     be written by the previous stage; it is allowed to have
+ *     superfluous declarations of input variables."
+ *
+ * It also has a table that states it is an error to statically
+ * read an input that is not defined in the previous stage. While
+ * it is not an error to not statically write to the output (it
+ * just needs to be defined to not be an error).
+ *
+ * The text in the GLSL 4.20 spec was an attempt to clarify the
+ * previous spec iterations. However given the difference in spec
+ * and that some applications seem to depend on not erroring when
+ * the input is not actually read in control flow we only apply
+ * this rule to GLSL 4.20 and higher. GLSL 4.10 shaders have been
+ * seen in the wild that depend on the less strict interpretation.
+ */
+static bool
+static_input_output_matching(struct gl_shader_program *prog)
+{
+   return prog->data->Version >= (prog->IsES ? 0 : 420);
+}
+
 /**
  * Validate that outputs from one stage match inputs of another
  */
@@ -847,7 +885,7 @@ cross_validate_outputs_to_inputs(struct gl_context *ctx,
                    * output declaration and there is Static Use of the
                    * declared input.
                    */
-                  if (input->data.used) {
+                  if (input->data.used && static_input_output_matching(prog)) {
                      linker_error(prog,
                                   "%s shader input `%s' with explicit location "
                                   "has no matching output\n",
@@ -882,40 +920,11 @@ cross_validate_outputs_to_inputs(struct gl_context *ctx,
             /* Check for input vars with unmatched output vars in prev stage
              * taking into account that interface blocks could have a matching
              * output but with different name, so we ignore them.
-             *
-             * Section 4.3.4 (Inputs) of the GLSL 4.10 specifications say:
-             *
-             *   "Only the input variables that are actually read need to be
-             *    written by the previous stage; it is allowed to have
-             *    superfluous declarations of input variables."
-             *
-             * However it's not defined anywhere as to how we should handle
-             * inputs that are not written in the previous stage and it's not
-             * clear what "actually read" means.
-             *
-             * The GLSL 4.20 spec however is much clearer:
-             *
-             *    "Only the input variables that are statically read need to
-             *     be written by the previous stage; it is allowed to have
-             *     superfluous declarations of input variables."
-             *
-             * It also has a table that states it is an error to statically
-             * read an input that is not defined in the previous stage. While
-             * it is not an error to not statically write to the output (it
-             * just needs to be defined to not be an error).
-             *
-             * The text in the GLSL 4.20 spec was an attempt to clarify the
-             * previous spec iterations. However given the difference in spec
-             * and that some applications seem to depend on not erroring when
-             * the input is not actually read in control flow we only apply
-             * this rule to GLSL 4.00 and higher. GLSL 4.00 was chosen as
-             * a 3.30 shader is the highest version of GLSL we have seen in
-             * the wild dependant on the less strict interpretation.
              */
             assert(!input->data.assigned);
             if (input->data.used && !input->get_interface_type() &&
                 !input->data.explicit_location &&
-                (prog->data->Version >= (prog->IsES ? 0 : 400)))
+                static_input_output_matching(prog))
                linker_error(prog,
                             "%s shader input `%s' "
                             "has no matching output in the previous stage\n",