[Mesa-dev] [PATCH V3 1/4] glsl: Compile error if fs defines conflicting qualifiers for gl_FragCoord

[Anuj Phogat]

Identified 2 bugs in patches 1/4 and 3/4. Will send out V4 of the series
 with  fixes.

On Fri, Feb 28, 2014 at 11:53 AM, Anuj Phogat <anuj.phogat at gmail.com> wrote:
> GLSL 1.50 spec says:
>    "If gl_FragCoord is redeclared in any fragment shader in a program,
>     it must be redeclared in all the fragment shaders in that
>     program that have a static use gl_FragCoord. All redeclarations of
>     gl_FragCoord in all fragment shaders in a single program must
>     have the same set of qualifiers."
>
> This patch makes the glsl compiler to generate an error if we have a
> fragment shader defined with conflicting layout qualifier declarations
> for gl_FragCoord. For example:
>
> layout(origin_upper_left, pixel_center_integer) in vec4 gl_FragCoord;
> layout(pixel_center_integer) in vec4 gl_FragCoord;
>
> void main()
> {
> }
>
> V2: Some code refactoring for better readability.
>     Add compiler error conditions for redeclarations like:
>
> layout(origin_upper_left) in vec4 gl_FragCoord;
> layout(origin_upper_left, pixel_center_integer) in vec4 gl_FragCoord;
>
> and
>
> in vec4 gl_FragCoord;
> layout(origin_upper_left, pixel_center_integer) in vec4 gl_FragCoord;
>
> V3: Simplify the conditions to check for conflicting gl_FragCoord
> redeclarations.
>
> Signed-off-by: Anuj Phogat <anuj.phogat at gmail.com>
> Cc: <mesa-stable at lists.freedesktop.org>
> ---
>  src/glsl/ast_to_hir.cpp         | 45 +++++++++++++++++++++++++++++++++++++++++
>  src/glsl/glsl_parser_extras.cpp |  5 +++++
>  src/glsl/glsl_parser_extras.h   | 12 +++++++++++
>  3 files changed, 62 insertions(+)
>
> diff --git a/src/glsl/ast_to_hir.cpp b/src/glsl/ast_to_hir.cpp
> index 8f6e901..7b1cd1b 100644
> --- a/src/glsl/ast_to_hir.cpp
> +++ b/src/glsl/ast_to_hir.cpp
> @@ -2298,6 +2298,19 @@ apply_image_qualifier_to_variable(const struct ast_type_qualifier *qual,
>     }
>  }
>
> +static inline const char*
> +get_layout_qualifier_string(bool origin_upper_left, bool pixel_center_integer)
> +{
> +   if (origin_upper_left && pixel_center_integer)
> +      return "origin_upper_left, pixel_center_integer";
> +   else if (origin_upper_left)
> +      return "origin_upper_left";
> +   else if (pixel_center_integer)
> +      return "pixel_center_integer";
> +   else
> +      return " ";
> +}
> +
>  static void
>  apply_type_qualifier_to_variable(const struct ast_type_qualifier *qual,
>                                  ir_variable *var,
> @@ -2462,6 +2475,38 @@ apply_type_qualifier_to_variable(const struct ast_type_qualifier *qual,
>                        qual_string);
>     }
>
> +   if (strcmp(var->name, "gl_FragCoord") == 0) {
> +
> +      /* If gl_FragCoord was previously declared, and the qualifiers were
> +       * different in any way, generate an error.
> +       */
> +      if (state->fs_redeclares_gl_fragcoord
> +          && (state->fs_pixel_center_integer != qual->flags.q.pixel_center_integer
> +              || state->fs_origin_upper_left != qual->flags.q.origin_upper_left)) {
> +         const char *const qual_string =
> +            get_layout_qualifier_string(qual->flags.q.origin_upper_left,
> +                                        qual->flags.q.pixel_center_integer);
> +
> +         const char *const state_string =
> +            get_layout_qualifier_string(state->fs_origin_upper_left,
> +                                        state->fs_pixel_center_integer);
> +
> +         _mesa_glsl_error(loc, state,
> +                          "gl_FragCoord redeclared with different layout "
> +                          "qualifiers (%s) and (%s) ",
> +                          state_string,
> +                          qual_string);
> +      }
> +      state->fs_origin_upper_left = qual->flags.q.origin_upper_left;
> +      state->fs_pixel_center_integer = qual->flags.q.pixel_center_integer;
> +      state->fs_redeclares_gl_fragcoord_with_no_layout_qualifiers =
> +         !qual->flags.q.origin_upper_left && !qual->flags.q.pixel_center_integer;
> +      state->fs_redeclares_gl_fragcoord =
> +         state->fs_origin_upper_left ||
> +         state->fs_pixel_center_integer ||
> +         state->fs_redeclares_gl_fragcoord_with_no_layout_qualifiers;
> +   }
> +
>     if (qual->flags.q.explicit_location) {
>        validate_explicit_location(qual, var, state, loc);
>     } else if (qual->flags.q.explicit_index) {
> diff --git a/src/glsl/glsl_parser_extras.cpp b/src/glsl/glsl_parser_extras.cpp
> index d7f5202..fcbbf44 100644
> --- a/src/glsl/glsl_parser_extras.cpp
> +++ b/src/glsl/glsl_parser_extras.cpp
> @@ -201,6 +201,11 @@ _mesa_glsl_parse_state::_mesa_glsl_parse_state(struct gl_context *_ctx,
>     this->default_uniform_qualifier->flags.q.shared = 1;
>     this->default_uniform_qualifier->flags.q.column_major = 1;
>
> +   this->fs_redeclares_gl_fragcoord = false;
> +   this->fs_origin_upper_left = false;
> +   this->fs_pixel_center_integer = false;
> +   this->fs_redeclares_gl_fragcoord_with_no_layout_qualifiers = false;
> +
>     this->gs_input_prim_type_specified = false;
>     this->gs_input_size = 0;
>     this->in_qualifier = new(this) ast_type_qualifier();
> diff --git a/src/glsl/glsl_parser_extras.h b/src/glsl/glsl_parser_extras.h
> index 300d900..2017913 100644
> --- a/src/glsl/glsl_parser_extras.h
> +++ b/src/glsl/glsl_parser_extras.h
> @@ -204,6 +204,18 @@ struct _mesa_glsl_parse_state {
>     struct ast_type_qualifier *default_uniform_qualifier;
>
>     /**
> +    * Variables to track different cases if a fragment shader redeclares
> +    * built-in variable gl_FragCoord.
> +    *
> +    * Note: These values are computed at ast_to_hir time rather than at parse
> +    * time.
> +    */
> +   bool fs_redeclares_gl_fragcoord;
> +   bool fs_origin_upper_left;
> +   bool fs_pixel_center_integer;
> +   bool fs_redeclares_gl_fragcoord_with_no_layout_qualifiers;
> +
> +   /**
>      * True if a geometry shader input primitive type was specified using a
>      * layout directive.
>      *
> --
> 1.8.3.1
>