[Mesa-stable] [PATCH V2 3/4] glsl: Link error if fs defines conflicting qualifiers for gl_FragCoord

Ian Romanick idr at freedesktop.org
Tue Feb 25 09:14:24 PST 2014


On 02/24/2014 05:34 PM, Anuj Phogat 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 causes the shader link to fail if we have multiple fragment
> shaders with conflicting layout qualifiers for gl_FragCoord. For example:
> 
> fragment shader 1:
> layout(origin_upper_left, pixel_center_integer) in vec4 gl_FragCoord;
> 
> void foo();
> void main()
> {
>     gl_FragColor = vec4(gl_FragCoord.xyz, 1.0);
>     foo();
> }
> 
> fragment shader 2:
> layout(origin_upper_left) in vec4 gl_FragCoord;
> void foo()
> {
>     gl_FragColor.a = gl_FragCoord.z;
> }
> 
> Signed-off-by: Anuj Phogat <anuj.phogat at gmail.com>
> Cc: <mesa-stable at lists.freedesktop.org>
> ---
>   src/glsl/ast_to_hir.cpp         |  5 ++++
>   src/glsl/glsl_parser_extras.cpp | 16 +++++++++++
>   src/glsl/glsl_parser_extras.h   |  1 +
>   src/glsl/linker.cpp             | 60 +++++++++++++++++++++++++++++++++++++++++
>   src/mesa/main/mtypes.h          |  8 ++++++
>   5 files changed, 90 insertions(+)
> 
> diff --git a/src/glsl/ast_to_hir.cpp b/src/glsl/ast_to_hir.cpp
> index f5dacfd..b639f98 100644
> --- a/src/glsl/ast_to_hir.cpp
> +++ b/src/glsl/ast_to_hir.cpp
> @@ -120,6 +120,11 @@ _mesa_ast_to_hir(exec_list *instructions, struct _mesa_glsl_parse_state *state)
>         instructions->push_head(var);
>      }
>   
> +   /* Figure out if gl_FragCoord is actually used in fragment shader */
> +   ir_variable *const var = state->symbols->get_variable("gl_FragCoord");
> +   if (var != NULL)
> +      state->fs_uses_gl_fragcoord = var->data.used;
> +
>      /* From section 7.1 (Built-In Language Variables) of the GLSL 4.10 spec:
>       *
>       *     If multiple shaders using members of a built-in block belonging to
> diff --git a/src/glsl/glsl_parser_extras.cpp b/src/glsl/glsl_parser_extras.cpp
> index fcbbf44..f953713 100644
> --- a/src/glsl/glsl_parser_extras.cpp
> +++ b/src/glsl/glsl_parser_extras.cpp
> @@ -201,6 +201,7 @@ _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_uses_gl_fragcoord = false;
>      this->fs_redeclares_gl_fragcoord = false;
>      this->fs_origin_upper_left = false;
>      this->fs_pixel_center_integer = false;
> @@ -1363,6 +1364,14 @@ set_shader_inout_layout(struct gl_shader *shader,
>         assert(!state->cs_input_local_size_specified);
>      }
>   
> +   if (shader->Stage != MESA_SHADER_FRAGMENT) {
> +      /* Should have been prevented by the parser. */
> +      assert(!state->fs_uses_gl_fragcoord);
> +      assert(!state->fs_redeclares_gl_fragcoord);
> +      assert(!state->fs_pixel_center_integer);
> +      assert(!state->fs_origin_upper_left);
> +   }
> +
>      switch (shader->Stage) {
>      case MESA_SHADER_GEOMETRY:
>         shader->Geom.VerticesOut = 0;
> @@ -1396,6 +1405,13 @@ set_shader_inout_layout(struct gl_shader *shader,
>         }
>         break;
>   
> +   case MESA_SHADER_FRAGMENT:
> +      shader->redeclares_gl_fragcoord = state->fs_redeclares_gl_fragcoord;
> +      shader->uses_gl_fragcoord = state->fs_uses_gl_fragcoord;
> +      shader->pixel_center_integer = state->fs_pixel_center_integer;
> +      shader->origin_upper_left = state->fs_origin_upper_left;
> +      break;
> +
>      default:
>         /* Nothing to do. */
>         break;
> diff --git a/src/glsl/glsl_parser_extras.h b/src/glsl/glsl_parser_extras.h
> index 2017913..511ca48 100644
> --- a/src/glsl/glsl_parser_extras.h
> +++ b/src/glsl/glsl_parser_extras.h
> @@ -427,6 +427,7 @@ struct _mesa_glsl_parse_state {
>      const struct gl_extensions *extensions;
>   
>      bool uses_builtin_functions;
> +   bool fs_uses_gl_fragcoord;
>   
>      /**
>       * For geometry shaders, size of the most recently seen input declaration
> diff --git a/src/glsl/linker.cpp b/src/glsl/linker.cpp
> index f6b2661..1268aef 100644
> --- a/src/glsl/linker.cpp
> +++ b/src/glsl/linker.cpp
> @@ -1194,6 +1194,65 @@ private:
>      hash_table *unnamed_interfaces;
>   };
>   
> +static bool
> +fs_uses_conflicting_layout_qualifiers(struct gl_shader *shader,
> +                                      struct gl_shader *linked_shader)
> +{
> +   bool qual_absent_in_shader_but_present_in_linked_shader =
> +      (!shader->origin_upper_left && linked_shader->origin_upper_left) ||
> +      (!shader->pixel_center_integer && linked_shader->pixel_center_integer);
> +
> +   bool qual_present_in_shader_but_absent_in_linked_shader =
> +      (shader->origin_upper_left && !linked_shader->origin_upper_left) ||
> +      (shader->pixel_center_integer && !linked_shader->pixel_center_integer);
> +
> +   return ((qual_absent_in_shader_but_present_in_linked_shader &&
> +            shader->uses_gl_fragcoord) ||
> +           (qual_present_in_shader_but_absent_in_linked_shader &&
> +            linked_shader->uses_gl_fragcoord));

I don't think this logic catches the redefined-but-not-used cases.

layout(origin_upper_left) in vec4 gl_FragCoord;
void main()
{
    foo();
    gl_FragColor = gl_FragData;
}

---

layout(pixel_center_integer) in vec4 gl_FragCoord;
void foo()
{
}

I think in the loop in link_fs_input_layout_qualifiers you want:

   /* From the GLSL 1.50 spec, page 39:
    *
    *    "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.
    */
   if ((linked_shader->redeclares_gl_fragcoord
        && !shader->redeclares_gl_fragcoord
        && shader->uses_gl_fragcoord)
       || (shader->redeclares_gl_fragcoord
           && !linked_shader->redeclares_gl_fragcoord
           && linked_shader->uses_gl_fragcoord)) {
      // error
   }

   /* From the GLSL 1.50 spec, page 39:
    *
    *     "All redeclarations of gl_FragCoord in all fragment shaders in a
    *     single program must have the same set of qualifiers."
    */
   if (linked_shader->redeclares_gl_fragcoord && shader->redeclares_gl_fragcoord
       && (shader->origin_upper_left != linked_shader->origin_upper_left
           || shader->pixel_center_integer != linked_shader->pixel_center_integer)){
      // error
   }

   /* Update the linked shader state.  Note that uses_gl_fragcoord should
    * accumulate the results.  The other values should replace.  If there
    * are multiple redeclarations, all the fields except uses_gl_fragcoord
    * are already known to be the same.
    */
   if (shader->redeclares_gl_fragcoord) {
      linked_shader->uses_gl_fragcoord = linked_shader->uses_gl_fragcoord
         || shader->uses_gl_fragcoord;
      linked_shader->origin_upper_left = shader->origin_upper_left;
      linked_shader->pixel_center_integer = shader->pixel_center_integer;
   }

> +}
> +
> +/**
> + * Performs the cross-validation of layout qualifiers specified in
> + * redeclaration of gl_FragCoord for the attached fragment shaders,
> + * and propagates them to the linked FS and linked shader program.
> + */
> +static void
> +link_fs_input_layout_qualifiers(struct gl_shader_program *prog,
> +	                        struct gl_shader *linked_shader,
> +	                        struct gl_shader **shader_list,
> +	                        unsigned num_shaders)
> +{
> +   linked_shader->uses_gl_fragcoord = false;
> +   linked_shader->origin_upper_left = false;
> +   linked_shader->pixel_center_integer = false;
> +
> +   if (linked_shader->Stage != MESA_SHADER_FRAGMENT || prog->Version < 150)
> +      return;
> +
> +   /* From the GLSL 1.50 spec, page 39:
> +    *    "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."
> +    */
> +
> +   for (unsigned i = 0; i < num_shaders; i++) {
> +      struct gl_shader *shader = shader_list[i];
> +
> +      if (fs_uses_conflicting_layout_qualifiers(shader, linked_shader)) {
> +         linker_error(prog, "fragment shader defined with conflicting "
> +                      "layout qualifiers for gl_FragCoord\n");
> +         return;
> +      } else if (shader->redeclares_gl_fragcoord || shader->uses_gl_fragcoord) {
> +         linked_shader->uses_gl_fragcoord = shader->uses_gl_fragcoord;
> +         linked_shader->origin_upper_left = shader->origin_upper_left;
> +         linked_shader->pixel_center_integer = shader->pixel_center_integer;
> +      }
> +   }
> +}
> +
>   /**
>    * Performs the cross-validation of geometry shader max_vertices and
>    * primitive type layout qualifiers for the attached geometry shaders,
> @@ -1471,6 +1530,7 @@ link_intrastage_shaders(void *mem_ctx,
>      linked->NumUniformBlocks = num_uniform_blocks;
>      ralloc_steal(linked, linked->UniformBlocks);
>   
> +   link_fs_input_layout_qualifiers(prog, linked, shader_list, num_shaders);
>      link_gs_inout_layout_qualifiers(prog, linked, shader_list, num_shaders);
>      link_cs_input_layout_qualifiers(prog, linked, shader_list, num_shaders);
>   
> diff --git a/src/mesa/main/mtypes.h b/src/mesa/main/mtypes.h
> index fbbca55..dd2ee82 100644
> --- a/src/mesa/main/mtypes.h
> +++ b/src/mesa/main/mtypes.h
> @@ -2414,6 +2414,14 @@ struct gl_shader
>      struct glsl_symbol_table *symbols;
>   
>      bool uses_builtin_functions;
> +   bool uses_gl_fragcoord;
> +   bool redeclares_gl_fragcoord;
> +
> +   /**
> +    * Fragment shader state from GLSL 1.50 layout qualifiers.
> +    */
> +   bool origin_upper_left;
> +   bool pixel_center_integer;
>   
>      /**
>       * Geometry shader state from GLSL 1.50 layout qualifiers.
> 



More information about the mesa-stable mailing list