[Mesa-dev] [PATCH V2 05/12] glsl: add layout qualifier validation for the shader outside the parser

[Emil Velikov]

Hi Tim,

On 8 November 2015 at 22:34, Timothy Arceri <t_arceri at yahoo.com.au> wrote:
> From: Timothy Arceri <timothy.arceri at 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