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

[Timothy Arceri]

On Tue, 2015-11-10 at 12:29 +0000, Emil Velikov wrote:
> 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 ?

I have to disagree here. I've left the name the same in v3 as I don't
see enything wrong with having the validation mixed in with the copy,
in fact I prefer it that way.

Functions prefixed with set are usually assosiated with mutators and
one could fully expect a function with this prefix do both validation
and setting of values. I know this isn't OO code here but the
assumptions about a function prefixed with set_ still apply.


> 
> -Emil