On Tue, 2015-11-10 at 12:29 +0000, Emil Velikov wrote:
> 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 ?
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
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
