From 35f11e85cbe82b4bb77535e84e5515a5c49f67a6 Mon Sep 17 00:00:00 2001 From: Anuj Phogat Date: Wed, 5 Feb 2014 15:01:58 -0800 Subject: [PATCH] glsl: Link error if fs defines conflicting qualifiers for gl_FragCoord 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. V2: Restructure the code and add conditions to correctly handle the following case: fragment shader 1: layout(origin_upper_left) in vec4 gl_FragCoord; void main() { foo(); gl_FragColor = gl_FragData; } fragment shader 2: layout(pixel_center_integer) in vec4 gl_FragCoord; void foo() { } V3: Allow linking in the following case: fragment shader 1: void main() { foo(); gl_FragColor = gl_FragCoord; } fragment shader 2: in vec4 gl_FragCoord; void foo() { ... } Signed-off-by: Anuj Phogat Cc: Reviewed-by: Ian Romanick --- src/glsl/ast_to_hir.cpp | 5 +++ src/glsl/glsl_parser_extras.cpp | 21 +++++++++ src/glsl/glsl_parser_extras.h | 1 + src/glsl/linker.cpp | 77 +++++++++++++++++++++++++++++++++ src/mesa/main/mtypes.h | 8 ++++ 5 files changed, 112 insertions(+) diff --git a/src/glsl/ast_to_hir.cpp b/src/glsl/ast_to_hir.cpp index f257a248680..7516c33e1df 100644 --- a/src/glsl/ast_to_hir.cpp +++ b/src/glsl/ast_to_hir.cpp @@ -123,6 +123,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 03c2a972a6e..600658d9788 100644 --- a/src/glsl/glsl_parser_extras.cpp +++ b/src/glsl/glsl_parser_extras.cpp @@ -197,6 +197,12 @@ _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; + 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(); @@ -1356,6 +1362,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; @@ -1389,6 +1403,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 e3db877be49..2e105740346 100644 --- a/src/glsl/glsl_parser_extras.h +++ b/src/glsl/glsl_parser_extras.h @@ -423,6 +423,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 a2e43073e53..9a018774f80 100644 --- a/src/glsl/linker.cpp +++ b/src/glsl/linker.cpp @@ -1194,6 +1194,82 @@ private: hash_table *unnamed_interfaces; }; +/** + * 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->redeclares_gl_fragcoord = false; + 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; + + for (unsigned i = 0; i < num_shaders; i++) { + struct gl_shader *shader = shader_list[i]; + /* 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." + * + * Exclude the case when one of the 'linked_shader' or 'shader' redeclares + * gl_FragCoord with no layout qualifiers but the other one doesn't + * redeclare it. If we strictly follow GLSL 1.50 spec's language, it + * should be a link error. But, generating link error for this case will + * be a wrong behaviour which spec didn't intend to do and it could also + * break some applications. + */ + if ((linked_shader->redeclares_gl_fragcoord + && !shader->redeclares_gl_fragcoord + && shader->uses_gl_fragcoord + && (linked_shader->origin_upper_left + || linked_shader->pixel_center_integer)) + || (shader->redeclares_gl_fragcoord + && !linked_shader->redeclares_gl_fragcoord + && linked_shader->uses_gl_fragcoord + && (shader->origin_upper_left + || shader->pixel_center_integer))) { + linker_error(prog, "fragment shader defined with conflicting " + "layout qualifiers for gl_FragCoord\n"); + } + + /* 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)) { + linker_error(prog, "fragment shader defined with conflicting " + "layout qualifiers for gl_FragCoord\n"); + } + + /* 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 || shader->uses_gl_fragcoord) { + linked_shader->redeclares_gl_fragcoord = + 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 geometry shader max_vertices and * primitive type layout qualifiers for the attached geometry shaders, @@ -1471,6 +1547,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 5fbfffe98e0..d9fb297a966 100644 --- a/src/mesa/main/mtypes.h +++ b/src/mesa/main/mtypes.h @@ -2432,6 +2432,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. -- 2.30.2