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

[Samuel Pitoiset]

Well, I have just tested it with mesa git-ca7d2025a7 (just before 
be8aa76afd - glsl: remove unecessary flags.q.subroutine_def) and it also 
fails. While git-be8aa76afd introduces a compile-time error:

"Failed to compile vertex shader 
/home/hakzsam/programming/piglit/tests/spec/arb_shader_subroutine/linker/no-overloads.vert: 
0:19(7): error: syntax error, unexpected '(', expecting ',' or ';'"

This doesn't look like a regression, it just "restores" the previous 
behavior.

Though, piglit.spec.arb_shader_subroutine.linker.no-overloads_vert 
should be fixed in a separate patch.

Mark, can you confirm?
Thanks!

On 03/02/2017 06:56 PM, Mark Janes wrote:
> 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