Hi Tim,
On 8 November 2015 at 22:34, Timothy Arceri <t_arc...@yahoo.com.au> wrote:
> From: Timothy Arceri <timothy.arc...@collabora.com>
>
> This is in preparation for compile-time constant support, a later patch
> will remove the validation from the shader.
>
> The global shader layout qualifiers will now mostly be validated in
> glsl_parser_extras.cpp.
>
> In order to do validation at the later stage in glsl_parser_extras.cpp we
> need to temporarily add a field in ast_type_qualifier to keep track of the
> parser location, this will be removed in a following patch when we
> introduce a new type for storing the comiple-time qualifiers.
>
> Also as the set_shader_inout_layout() function in glsl parser extras is
> normally called after all validation is done we need to move the code that
> sets CompileStatus and InfoLog otherwise the newly add error messages would
> be ignored.
> ---
> src/glsl/ast_to_hir.cpp | 14 ++++++++++++--
> src/glsl/ast_type.cpp | 2 ++
> src/glsl/glsl_parser_extras.cpp | 37 ++++++++++++++++++++++++++++++++-----
> 3 files changed, 46 insertions(+), 7 deletions(-)
>
> diff --git a/src/glsl/ast_to_hir.cpp b/src/glsl/ast_to_hir.cpp
> index 0cea607..5643c86 100644
> --- a/src/glsl/ast_to_hir.cpp
> +++ b/src/glsl/ast_to_hir.cpp
> @@ -3544,10 +3544,19 @@ static void
> handle_tess_ctrl_shader_output_decl(struct _mesa_glsl_parse_state *state,
> YYLTYPE loc, ir_variable *var)
> {
> - unsigned num_vertices = 0;
> + int num_vertices = 0;
>
> if (state->tcs_output_vertices_specified) {
> num_vertices = state->out_qualifier->vertices;
> + if (num_vertices <= 0) {
> + _mesa_glsl_error(&loc, state, "invalid vertices (%d) specified",
> + num_vertices);
> + return;
> + } else if ((unsigned) num_vertices > state->Const.MaxPatchVertices) {
> + _mesa_glsl_error(&loc, state, "vertices (%d) exceeds "
> + "GL_MAX_PATCH_VERTICES", num_vertices);
> + return;
> + }
> }
>
> if (!var->type->is_array() && !var->data.patch) {
> @@ -3561,7 +3570,8 @@ handle_tess_ctrl_shader_output_decl(struct
> _mesa_glsl_parse_state *state,
> if (var->data.patch)
> return;
>
> - validate_layout_qualifier_vertex_count(state, loc, var, num_vertices,
> + validate_layout_qualifier_vertex_count(state, loc, var,
> + (unsigned) num_vertices,
> &state->tcs_output_size,
> "tessellation control shader
> output");
> }
> diff --git a/src/glsl/ast_type.cpp b/src/glsl/ast_type.cpp
> index 08a4504..53d1023 100644
> --- a/src/glsl/ast_type.cpp
> +++ b/src/glsl/ast_type.cpp
> @@ -310,6 +310,7 @@ ast_type_qualifier::merge_out_qualifier(YYLTYPE *loc,
> {
> void *mem_ctx = state;
> const bool r = this->merge_qualifier(loc, state, q);
> + this->loc = loc;
>
> if (state->stage == MESA_SHADER_TESS_CTRL) {
> node = new(mem_ctx) ast_tcs_output_layout(*loc, q.vertices);
> @@ -329,6 +330,7 @@ ast_type_qualifier::merge_in_qualifier(YYLTYPE *loc,
> bool create_cs_ast = false;
> ast_type_qualifier valid_in_mask;
> valid_in_mask.flags.i = 0;
> + this->loc = loc;
>
> switch (state->stage) {
> case MESA_SHADER_TESS_EVAL:
> diff --git a/src/glsl/glsl_parser_extras.cpp b/src/glsl/glsl_parser_extras.cpp
> index 2dba7d9..7d7f45c 100644
> --- a/src/glsl/glsl_parser_extras.cpp
> +++ b/src/glsl/glsl_parser_extras.cpp
> @@ -947,6 +947,14 @@ _mesa_ast_process_interface_block(YYLTYPE *locp,
>
> if (state->stage == MESA_SHADER_GEOMETRY &&
> state->has_explicit_attrib_stream()) {
> +
> + if (state->out_qualifier->flags.q.explicit_stream) {
> + if (state->out_qualifier->stream < 0) {
> + _mesa_glsl_error(locp, state, "invalid stream %d specified",
> + state->out_qualifier->stream);
> + }
> + }
> +
> /* Assign global layout's stream value. */
> block->layout.flags.q.stream = 1;
> block->layout.flags.q.explicit_stream = 0;
> @@ -1615,7 +1623,7 @@ void ast_subroutine_list::print(void) const
>
> static void
> set_shader_inout_layout(struct gl_shader *shader,
> - struct _mesa_glsl_parse_state *state)
> + struct _mesa_glsl_parse_state *state)
> {
You seems to me mixing the "validate" and "copy validated values"
functions into one. This invalidates the already (not too descriptive)
function name, and requires you to move CompileStatus,InfoLog
assignment. I am thinking that keeping the validation into a separate
function would be better. Perhaps even rename set_shader_inout_layout
to {copy,set}_shader_inout_layout_from_state ?
-Emil
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
