From b95d237fe6731055dad2ff3eaa59e4d6fc14bfff Mon Sep 17 00:00:00 2001 From: Paul Berry Date: Sat, 27 Jul 2013 11:08:31 -0700 Subject: [PATCH] glsl: Use a consistent technique for tracking link success/failure. This patch changes link_shaders() so that it sets prog->LinkStatus to true when it starts, and then relies on linker_error() to set it to false if a link failure occurs. Previously, link_shaders() would set prog->LinkStatus to true halfway through its execution; as a result, linker functions that executed during the first half of link_shaders() would have to do their own success/failure tracking; if they didn't, then calling linker_error() would add an error message to the log, but not cause the link to fail. Since it wasn't always obvious from looking at a linker function whether it was called before or after link_shaders() set prog->LinkStatus to true, this carried a high risk of bugs. Reviewed-by: Jordan Justen Reviewed-by: Ian Romanick --- src/glsl/link_interface_blocks.cpp | 19 +++-- src/glsl/link_varyings.cpp | 12 ++- src/glsl/link_varyings.h | 2 +- src/glsl/linker.cpp | 117 ++++++++++++++--------------- src/glsl/linker.h | 7 +- 5 files changed, 75 insertions(+), 82 deletions(-) diff --git a/src/glsl/link_interface_blocks.cpp b/src/glsl/link_interface_blocks.cpp index 4f67291d85f..ffb44530f5d 100644 --- a/src/glsl/link_interface_blocks.cpp +++ b/src/glsl/link_interface_blocks.cpp @@ -31,7 +31,7 @@ #include "linker.h" #include "main/macros.h" -bool +void validate_intrastage_interface_blocks(struct gl_shader_program *prog, const gl_shader **shader_list, unsigned num_shaders) @@ -65,16 +65,15 @@ validate_intrastage_interface_blocks(struct gl_shader_program *prog, } else if (old_iface_type != iface_type) { linker_error(prog, "definitions of interface block `%s' do not" " match\n", iface_type->name); - return false; + return; } } } - - return true; } -bool -validate_interstage_interface_blocks(const gl_shader *producer, +void +validate_interstage_interface_blocks(struct gl_shader_program *prog, + const gl_shader *producer, const gl_shader *consumer) { glsl_symbol_table interfaces; @@ -105,9 +104,9 @@ validate_interstage_interface_blocks(const gl_shader *producer, if (expected_type == NULL) continue; - if (var->interface_type != expected_type) - return false; + if (var->interface_type != expected_type) { + linker_error(prog, "interface block mismatch between shader stages\n"); + return; + } } - - return true; } diff --git a/src/glsl/link_varyings.cpp b/src/glsl/link_varyings.cpp index 51cbdaa0e6e..2c7e4514e15 100644 --- a/src/glsl/link_varyings.cpp +++ b/src/glsl/link_varyings.cpp @@ -43,7 +43,7 @@ /** * Validate that outputs from one stage match inputs of another */ -bool +void cross_validate_outputs_to_inputs(struct gl_shader_program *prog, gl_shader *producer, gl_shader *consumer) { @@ -106,7 +106,7 @@ cross_validate_outputs_to_inputs(struct gl_shader_program *prog, producer_stage, output->name, output->type->name, consumer_stage, input->type->name); - return false; + return; } } @@ -121,7 +121,7 @@ cross_validate_outputs_to_inputs(struct gl_shader_program *prog, (output->centroid) ? "has" : "lacks", consumer_stage, (input->centroid) ? "has" : "lacks"); - return false; + return; } if (input->invariant != output->invariant) { @@ -133,7 +133,7 @@ cross_validate_outputs_to_inputs(struct gl_shader_program *prog, (output->invariant) ? "has" : "lacks", consumer_stage, (input->invariant) ? "has" : "lacks"); - return false; + return; } if (input->interpolation != output->interpolation) { @@ -147,12 +147,10 @@ cross_validate_outputs_to_inputs(struct gl_shader_program *prog, output->interpolation_string(), consumer_stage, input->interpolation_string()); - return false; + return; } } } - - return true; } diff --git a/src/glsl/link_varyings.h b/src/glsl/link_varyings.h index 7f7be353b6d..cfc6e474f78 100644 --- a/src/glsl/link_varyings.h +++ b/src/glsl/link_varyings.h @@ -214,7 +214,7 @@ private: }; -bool +void cross_validate_outputs_to_inputs(struct gl_shader_program *prog, gl_shader *producer, gl_shader *consumer); diff --git a/src/glsl/linker.cpp b/src/glsl/linker.cpp index 3d9c59d5de3..942f9061596 100644 --- a/src/glsl/linker.cpp +++ b/src/glsl/linker.cpp @@ -340,12 +340,12 @@ count_attribute_slots(const glsl_type *t) * * \param shader Vertex shader executable to be verified */ -bool +void validate_vertex_shader_executable(struct gl_shader_program *prog, struct gl_shader *shader) { if (shader == NULL) - return true; + return; /* From the GLSL 1.10 spec, page 48: * @@ -378,7 +378,7 @@ validate_vertex_shader_executable(struct gl_shader_program *prog, find.run(shader->ir); if (!find.variable_found()) { linker_error(prog, "vertex shader does not write to `gl_Position'\n"); - return false; + return; } } @@ -402,7 +402,7 @@ validate_vertex_shader_executable(struct gl_shader_program *prog, if (clip_vertex.variable_found() && clip_distance.variable_found()) { linker_error(prog, "vertex shader writes to both `gl_ClipVertex' " "and `gl_ClipDistance'\n"); - return false; + return; } prog->Vert.UsesClipDistance = clip_distance.variable_found(); ir_variable *clip_distance_var = @@ -410,8 +410,6 @@ validate_vertex_shader_executable(struct gl_shader_program *prog, if (clip_distance_var) prog->Vert.ClipDistanceArraySize = clip_distance_var->type->length; } - - return true; } @@ -420,12 +418,12 @@ validate_vertex_shader_executable(struct gl_shader_program *prog, * * \param shader Fragment shader executable to be verified */ -bool +void validate_fragment_shader_executable(struct gl_shader_program *prog, struct gl_shader *shader) { if (shader == NULL) - return true; + return; find_assignment_visitor frag_color("gl_FragColor"); find_assignment_visitor frag_data("gl_FragData"); @@ -436,10 +434,7 @@ validate_fragment_shader_executable(struct gl_shader_program *prog, if (frag_color.variable_found() && frag_data.variable_found()) { linker_error(prog, "fragment shader writes to both " "`gl_FragColor' and `gl_FragData'\n"); - return false; } - - return true; } @@ -469,7 +464,7 @@ mode_string(const ir_variable *var) /** * Perform validation of global variables used across multiple shaders */ -bool +void cross_validate_globals(struct gl_shader_program *prog, struct gl_shader **shader_list, unsigned num_shaders, @@ -524,7 +519,7 @@ cross_validate_globals(struct gl_shader_program *prog, mode_string(var), var->name, var->type->name, existing->type->name); - return false; + return; } } @@ -534,7 +529,7 @@ cross_validate_globals(struct gl_shader_program *prog, linker_error(prog, "explicit locations for %s " "`%s' have differing values\n", mode_string(var), var->name); - return false; + return; } existing->location = var->location; @@ -553,7 +548,7 @@ cross_validate_globals(struct gl_shader_program *prog, linker_error(prog, "explicit bindings for %s " "`%s' have differing values\n", mode_string(var), var->name); - return false; + return; } existing->binding = var->binding; @@ -614,7 +609,7 @@ cross_validate_globals(struct gl_shader_program *prog, linker_error(prog, "initializers for %s " "`%s' have differing values\n", mode_string(var), var->name); - return false; + return; } } else { /* If the first-seen instance of a particular uniform did not @@ -643,7 +638,7 @@ cross_validate_globals(struct gl_shader_program *prog, "shared global variable `%s' has multiple " "non-constant initializers.\n", var->name); - return false; + return; } /* Some instance had an initializer, so keep track of that. In @@ -658,31 +653,29 @@ cross_validate_globals(struct gl_shader_program *prog, linker_error(prog, "declarations for %s `%s' have " "mismatching invariant qualifiers\n", mode_string(var), var->name); - return false; + return; } if (existing->centroid != var->centroid) { linker_error(prog, "declarations for %s `%s' have " "mismatching centroid qualifiers\n", mode_string(var), var->name); - return false; + return; } } else variables.add_variable(var); } } - - return true; } /** * Perform validation of uniforms used across multiple shader stages */ -bool +void cross_validate_uniforms(struct gl_shader_program *prog) { - return cross_validate_globals(prog, prog->_LinkedShaders, - MESA_SHADER_TYPES, true); + cross_validate_globals(prog, prog->_LinkedShaders, + MESA_SHADER_TYPES, true); } /** @@ -955,14 +948,15 @@ link_intrastage_shaders(void *mem_ctx, /* Check that global variables defined in multiple shaders are consistent. */ - if (!cross_validate_globals(prog, shader_list, num_shaders, false)) + cross_validate_globals(prog, shader_list, num_shaders, false); + if (!prog->LinkStatus) return NULL; /* Check that interface blocks defined in multiple shaders are consistent. */ - if (!validate_intrastage_interface_blocks(prog, - (const gl_shader **)shader_list, - num_shaders)) + validate_intrastage_interface_blocks(prog, (const gl_shader **)shader_list, + num_shaders); + if (!prog->LinkStatus) return NULL; /* Link up uniform blocks defined within this stage. */ @@ -1528,7 +1522,7 @@ store_fragdepth_layout(struct gl_shader_program *prog) /** * Validate the resources used by a program versus the implementation limits */ -static bool +static void check_resources(struct gl_context *ctx, struct gl_shader_program *prog) { static const char *const shader_names[MESA_SHADER_TYPES] = { @@ -1625,8 +1619,6 @@ check_resources(struct gl_context *ctx, struct gl_shader_program *prog) } } } - - return prog->LinkStatus; } void @@ -1637,7 +1629,7 @@ link_shaders(struct gl_context *ctx, struct gl_shader_program *prog) void *mem_ctx = ralloc_context(NULL); // temporary linker context - prog->LinkStatus = false; + prog->LinkStatus = true; /* All error paths will set this to false */ prog->Validated = false; prog->_Used = false; @@ -1723,10 +1715,11 @@ link_shaders(struct gl_context *ctx, struct gl_shader_program *prog) link_intrastage_shaders(mem_ctx, ctx, prog, vert_shader_list, num_vert_shaders); - if (sh == NULL) + if (!prog->LinkStatus) goto done; - if (!validate_vertex_shader_executable(prog, sh)) + validate_vertex_shader_executable(prog, sh); + if (!prog->LinkStatus) goto done; _mesa_reference_shader(ctx, &prog->_LinkedShaders[MESA_SHADER_VERTEX], @@ -1738,10 +1731,11 @@ link_shaders(struct gl_context *ctx, struct gl_shader_program *prog) link_intrastage_shaders(mem_ctx, ctx, prog, frag_shader_list, num_frag_shaders); - if (sh == NULL) + if (!prog->LinkStatus) goto done; - if (!validate_fragment_shader_executable(prog, sh)) + validate_fragment_shader_executable(prog, sh); + if (!prog->LinkStatus) goto done; _mesa_reference_shader(ctx, &prog->_LinkedShaders[MESA_SHADER_FRAGMENT], @@ -1752,36 +1746,36 @@ link_shaders(struct gl_context *ctx, struct gl_shader_program *prog) * performed, then locations are assigned for uniforms, attributes, and * varyings. */ - if (cross_validate_uniforms(prog)) { - unsigned prev; + cross_validate_uniforms(prog); + if (!prog->LinkStatus) + goto done; - for (prev = 0; prev < MESA_SHADER_TYPES; prev++) { - if (prog->_LinkedShaders[prev] != NULL) - break; - } + unsigned prev; - /* Validate the inputs of each stage with the output of the preceding - * stage. - */ - for (unsigned i = prev + 1; i < MESA_SHADER_TYPES; i++) { - if (prog->_LinkedShaders[i] == NULL) - continue; + for (prev = 0; prev < MESA_SHADER_TYPES; prev++) { + if (prog->_LinkedShaders[prev] != NULL) + break; + } - if (!validate_interstage_interface_blocks(prog->_LinkedShaders[prev], - prog->_LinkedShaders[i])) { - linker_error(prog, "interface block mismatch between shader stages\n"); - goto done; - } + /* Validate the inputs of each stage with the output of the preceding + * stage. + */ + for (unsigned i = prev + 1; i < MESA_SHADER_TYPES; i++) { + if (prog->_LinkedShaders[i] == NULL) + continue; - if (!cross_validate_outputs_to_inputs(prog, - prog->_LinkedShaders[prev], - prog->_LinkedShaders[i])) - goto done; + validate_interstage_interface_blocks(prog, prog->_LinkedShaders[prev], + prog->_LinkedShaders[i]); + if (!prog->LinkStatus) + goto done; - prev = i; - } + cross_validate_outputs_to_inputs(prog, + prog->_LinkedShaders[prev], + prog->_LinkedShaders[i]); + if (!prog->LinkStatus) + goto done; - prog->LinkStatus = true; + prev = i; } @@ -1970,7 +1964,8 @@ link_shaders(struct gl_context *ctx, struct gl_shader_program *prog) link_assign_uniform_locations(prog); store_fragdepth_layout(prog); - if (!check_resources(ctx, prog)) + check_resources(ctx, prog); + if (!prog->LinkStatus) goto done; /* OpenGL ES requires that a vertex shader and a fragment shader both be diff --git a/src/glsl/linker.h b/src/glsl/linker.h index 9f5deb5c08f..0ce747d6cb9 100644 --- a/src/glsl/linker.h +++ b/src/glsl/linker.h @@ -60,13 +60,14 @@ link_uniform_blocks(void *mem_ctx, unsigned num_shaders, struct gl_uniform_block **blocks_ret); -bool +void validate_intrastage_interface_blocks(struct gl_shader_program *prog, const gl_shader **shader_list, unsigned num_shaders); -bool -validate_interstage_interface_blocks(const gl_shader *producer, +void +validate_interstage_interface_blocks(struct gl_shader_program *prog, + const gl_shader *producer, const gl_shader *consumer); /** -- 2.30.2