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

[Samuel Pitoiset]

On 03/02/2017 08:36 PM, Mark Janes wrote:
> Samuel Pitoiset <samuel.pitoiset at gmail.com> writes:
>
>> 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?
>
> Yes, you are correct.  Thanks for figuring out what happened.

Cool, thanks.

>
>> 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