[Mesa-dev] [PATCH] glsl: fix subroutine mismatch between declarations/definitions

[Mark Janes]

This patch regresses:
piglit.spec.arb_shader_subroutine.linker.no-overloads_vert

The program in the test should fail to link, but does not.



Samuel Pitoiset <samuel.pitoiset at gmail.com> writes:

> Previously, when q.subroutine was set to 1, a new subroutine
> declaration was added to the AST, while 0 meant a subroutine
> definition has been detected by the parser.
>
> Thus, setting the q.subroutine flag in both situations is
> obviously wrong because a new type identifier is added instead
> of trying to match the declaration. To fix it up, introduce
> ast_type_qualifier::is_subroutine_decl() to differentiate
> declarations and definitions easily.
>
> This fixes a regression with:
> arb_shader_subroutine/compiler/direct-call.vert
>
> Cc: Mark Janes <mark.a.janes at intel.com>
> Fixes: be8aa76afd ("glsl: remove unecessary flags.q.subroutine_def")
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=100026
> Signed-off-by: Samuel Pitoiset <samuel.pitoiset at gmail.com>
> ---
>  src/compiler/glsl/ast.h                  |  5 +++++
>  src/compiler/glsl/ast_to_hir.cpp         | 12 ++++++------
>  src/compiler/glsl/ast_type.cpp           |  5 +++++
>  src/compiler/glsl/glsl_parser.yy         |  2 +-
>  src/compiler/glsl/glsl_parser_extras.cpp |  2 +-
>  5 files changed, 18 insertions(+), 8 deletions(-)
>
> diff --git a/src/compiler/glsl/ast.h b/src/compiler/glsl/ast.h
> index d27b940744..55cc5df8f3 100644
> --- a/src/compiler/glsl/ast.h
> +++ b/src/compiler/glsl/ast.h
> @@ -770,6 +770,11 @@ struct ast_type_qualifier {
>      */
>     bool has_memory() const;
>  
> +   /**
> +    * Return true if the qualifier is a subroutine declaration.
> +    */
> +   bool is_subroutine_decl() const;
> +
>     bool merge_qualifier(YYLTYPE *loc,
>  			_mesa_glsl_parse_state *state,
>                          const ast_type_qualifier &q,
> diff --git a/src/compiler/glsl/ast_to_hir.cpp b/src/compiler/glsl/ast_to_hir.cpp
> index a90813033f..59d03c9c96 100644
> --- a/src/compiler/glsl/ast_to_hir.cpp
> +++ b/src/compiler/glsl/ast_to_hir.cpp
> @@ -3256,7 +3256,7 @@ apply_explicit_location(const struct ast_type_qualifier *qual,
>        }
>  
>        /* Check if index was set for the uniform instead of the function */
> -      if (qual->flags.q.explicit_index && qual->flags.q.subroutine) {
> +      if (qual->flags.q.explicit_index && qual->is_subroutine_decl()) {
>           _mesa_glsl_error(loc, state, "an index qualifier can only be "
>                            "used with subroutine functions");
>           return;
> @@ -3742,7 +3742,7 @@ apply_type_qualifier_to_variable(const struct ast_type_qualifier *qual,
>        }
>     }
>  
> -   if (qual->flags.q.subroutine && !qual->flags.q.uniform) {
> +   if (qual->is_subroutine_decl() && !qual->flags.q.uniform) {
>        _mesa_glsl_error(loc, state,
>                         "`subroutine' may only be applied to uniforms, "
>                         "subroutine type declarations, or function definitions");
> @@ -4780,7 +4780,7 @@ ast_declarator_list::hir(exec_list *instructions,
>           continue;
>        }
>  
> -      if (this->type->qualifier.flags.q.subroutine) {
> +      if (this->type->qualifier.is_subroutine_decl()) {
>           const glsl_type *t;
>           const char *name;
>  
> @@ -4879,7 +4879,7 @@ ast_declarator_list::hir(exec_list *instructions,
>            */
>           if (this->type->qualifier.flags.q.attribute) {
>              mode = "attribute";
> -         } else if (this->type->qualifier.flags.q.subroutine) {
> +         } else if (this->type->qualifier.is_subroutine_decl()) {
>              mode = "subroutine uniform";
>           } else if (this->type->qualifier.flags.q.uniform) {
>              mode = "uniform";
> @@ -5629,7 +5629,7 @@ ast_function::hir(exec_list *instructions,
>     f = state->symbols->get_function(name);
>     if (f == NULL) {
>        f = new(ctx) ir_function(name);
> -      if (!this->return_type->qualifier.flags.q.subroutine) {
> +      if (!this->return_type->qualifier.is_subroutine_decl()) {
>           if (!state->symbols->add_function(f)) {
>              /* This function name shadows a non-function use of the same name. */
>              YYLTYPE loc = this->get_location();
> @@ -5787,7 +5787,7 @@ ast_function::hir(exec_list *instructions,
>  
>     }
>  
> -   if (this->return_type->qualifier.flags.q.subroutine) {
> +   if (this->return_type->qualifier.is_subroutine_decl()) {
>        if (!state->symbols->add_type(this->identifier, glsl_type::get_subroutine_instance(this->identifier))) {
>           _mesa_glsl_error(& loc, state, "type '%s' previously defined", this->identifier);
>           return NULL;
> diff --git a/src/compiler/glsl/ast_type.cpp b/src/compiler/glsl/ast_type.cpp
> index 5f868a81f2..d302fc45fd 100644
> --- a/src/compiler/glsl/ast_type.cpp
> +++ b/src/compiler/glsl/ast_type.cpp
> @@ -112,6 +112,11 @@ bool ast_type_qualifier::has_memory() const
>            || this->flags.q.write_only;
>  }
>  
> +bool ast_type_qualifier::is_subroutine_decl() const
> +{
> +   return this->flags.q.subroutine && !this->subroutine_list;
> +}
> +
>  static bool
>  validate_prim_type(YYLTYPE *loc,
>                     _mesa_glsl_parse_state *state,
> diff --git a/src/compiler/glsl/glsl_parser.yy b/src/compiler/glsl/glsl_parser.yy
> index 59453d72f6..e703073c9c 100644
> --- a/src/compiler/glsl/glsl_parser.yy
> +++ b/src/compiler/glsl/glsl_parser.yy
> @@ -900,7 +900,7 @@ function_header:
>        $$->return_type = $1;
>        $$->identifier = $2;
>  
> -      if ($1->qualifier.flags.q.subroutine) {
> +      if ($1->qualifier.is_subroutine_decl()) {
>           /* add type for IDENTIFIER search */
>           state->symbols->add_type($2, glsl_type::get_subroutine_instance($2));
>        } else
> diff --git a/src/compiler/glsl/glsl_parser_extras.cpp b/src/compiler/glsl/glsl_parser_extras.cpp
> index e88dd071b3..44fb46ab83 100644
> --- a/src/compiler/glsl/glsl_parser_extras.cpp
> +++ b/src/compiler/glsl/glsl_parser_extras.cpp
> @@ -1072,7 +1072,7 @@ _mesa_ast_process_interface_block(YYLTYPE *locp,
>  void
>  _mesa_ast_type_qualifier_print(const struct ast_type_qualifier *q)
>  {
> -   if (q->flags.q.subroutine)
> +   if (q->is_subroutine_decl())
>        printf("subroutine ");
>  
>     if (q->subroutine_list) {
> -- 
> 2.12.0