[Mesa-dev] [PATCH] glsl: Ignore redundant prototypes after a function's been defined.

[Ian Romanick]

On 04/30/2013 01:52 AM, Kenneth Graunke wrote:
> Consider the following shader:
>
>      vec4 f(vec4 v) { return v; }
>      vec4 f(vec4 v);
>
> The prototype exactly matches the signature of the earlier definition,
> so there's absolutely no point in it.  However, it doesn't appear to
> be illegal.  The GLSL 4.30 specification offers two relevant quotes:
>
> "If a function name is declared twice with the same parameter types,
>   then the return types and all qualifiers must also match, and it is the
>   same function being declared."
>
> "User-defined functions can have multiple declarations, but only one
>   definition."
>
> In this case the same function was declared twice, and there's only one
> definition, which fits both pieces of text.  There doesn't appear to be
> any text saying late prototypes are illegal, so presumably it's valid.
>
> Unfortunately, it currently triggers an assertion failure:
> ir_dereference_variable @ <p1> specifies undeclared variable `v' @ <p2>

OMG.  I wonder if this is the bug with Second Life.

https://bugs.freedesktop.org/show_bug.cgi?id=39251
https://bugs.freedesktop.org/show_bug.cgi?id=61773

> When we process the second line, we look for an existing exact match so
> we can enforce the one-definition rule.  We then leave sig set to that
> existing function, and hit sig->replace_parameters(&hir_parameters),
> unfortunately nuking our existing definition's parameters (which have
> actual dereferences) with the prototype's bogus unused parameters.
>
> Simply bailing out and ignoring such late prototypes is the safest
> thing to do.
>
> Fixes Piglit's late-proto.vert as well as 3DMark/Ice Storm for Android.
>
> NOTE: This is a candidate for stable branches.
> Cc: Tapani Pälli <tapani.palli at intel.com>
> Cc: Ian Romanick <idr at freedesktop.org>

Reviewed-by: Ian Romanick <ian.d.romanick at intel.com>

> Signed-off-by: Kenneth Graunke <kenneth at whitecape.org>
> ---
>   src/glsl/ast_to_hir.cpp | 15 +++++++++++----
>   1 file changed, 11 insertions(+), 4 deletions(-)
>
> diff --git a/src/glsl/ast_to_hir.cpp b/src/glsl/ast_to_hir.cpp
> index 2638411..e595110 100644
> --- a/src/glsl/ast_to_hir.cpp
> +++ b/src/glsl/ast_to_hir.cpp
> @@ -3205,10 +3205,17 @@ ast_function::hir(exec_list *instructions,
>   			     "match prototype", name);
>   	 }
>
> -	 if (is_definition && sig->is_defined) {
> -	    YYLTYPE loc = this->get_location();
> -
> -	    _mesa_glsl_error(& loc, state, "function `%s' redefined", name);
> +         if (sig->is_defined) {
> +            if (is_definition) {
> +               YYLTYPE loc = this->get_location();
> +               _mesa_glsl_error(& loc, state, "function `%s' redefined", name);
> +            } else {
> +               /* We just encountered a prototype that exactly matches a
> +                * function that's already been defined.  This is redundant,
> +                * and we should ignore it.
> +                */
> +               return NULL;
> +            }
>   	 }
>         }
>      } else {
>