[Mesa-dev] [PATCH] glsl/linker: Check the subroutine associated functions names

[Tapani Pälli]

On 10/2/18 7:38 PM, Vadym Shovkoplias wrote:
> Hi Tapani,
> 
> Thanks for the review!
> 
> Completely agree with the first comment, I'll change that and resend the 
> patch.
> Regarding second comment. I'm not sure if it is possible to do this 
> check after the optimization loop. From my observations compiler inlines 
> everything
> and only after that it removes dead functions (actually all funcs except 
> "main"). After the optimization I don't see any possible way how to
> implement this subroutine functions check because all functions and 
> functions signatures are removed at that point.

Yeah I was considering it could be done by storing some data but it 
seems this is probably the most straightforward version.

> On Tue, Oct 2, 2018 at 10:02 AM Tapani Pälli <tapani.palli at intel.com 
> <mailto:tapani.palli at intel.com>> wrote:
> 
> 
>     On 10/1/18 5:03 PM, Vadym Shovkoplias wrote:
>      >  From Section 6.1.2 (Subroutines) of the GLSL 4.00 specification
>      >
>      >      "A program will fail to compile or link if any shader
>      >       or stage contains two or more functions with the same
>      >       name if the name is associated with a subroutine type."
>      >
>      > Fixes:
>      >      * no-overloads.vert
>      >
>      > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=108109
>      > Signed-off-by: Vadym Shovkoplias
>     <vadym.shovkoplias at globallogic.com
>     <mailto:vadym.shovkoplias at globallogic.com>>
>      > ---
>      >   src/compiler/glsl/linker.cpp | 40
>     ++++++++++++++++++++++++++++++++++++
>      >   1 file changed, 40 insertions(+)
>      >
>      > diff --git a/src/compiler/glsl/linker.cpp
>     b/src/compiler/glsl/linker.cpp
>      > index 3fde7e78d3..d0d017c7ff 100644
>      > --- a/src/compiler/glsl/linker.cpp
>      > +++ b/src/compiler/glsl/linker.cpp
>      > @@ -4639,6 +4639,45 @@ link_assign_subroutine_types(struct
>     gl_shader_program *prog)
>      >      }
>      >   }
>      >
>      > +static void
>      > +verify_subroutine_associated_funcs(struct gl_shader_program *prog)
>      > +{
>      > +   unsigned mask = prog->data->linked_stages;
>      > +   while (mask) {
>      > +      const int i = u_bit_scan(&mask);
>      > +      gl_program *p = prog->_LinkedShaders[i]->Program;
>      > +      glsl_symbol_table *symbols = prog->_LinkedShaders[i]->symbols;
>      > +
>      > +      /*
>      > +       * From OpenGL ES Shading Language 4.00 specification
>      > +       * (6.1.2 Subroutines):
>      > +       *     "A program will fail to compile or link if any shader
>      > +       *     or stage contains two or more functions with the same
>      > +       *     name if the name is associated with a subroutine type."
>      > +       */
>      > +      for (unsigned j = 0; j < p->sh.NumSubroutineFunctions; j++) {
>      > +         unsigned definitions = 0;
>      > +         char *name = p->sh.SubroutineFunctions[j].name;
>      > +         ir_function *fn = symbols->get_function(name);
>      > +
>      > +         /* Calculate number of function definitions with the
>     same name */
>      > +         foreach_in_list(ir_function_signature, sig,
>     &fn->signatures) {
>      > +            if (sig->is_defined)
>      > +               definitions++;
> 
>     You can just error out here, no need to calculate further.
> 
>     I'm wondering a bit though should we fail here even if that function
>     was
>     not used at all (optimized out)? I can see that the Piglit test does
>     not
>     have a call to the function defined.
> 
> 
>      > +         }
>      > +
>      > +         if (definitions > 1) {
>      > +            linker_error(prog, "%s shader contains %u function "
>      > +                  "definitions with name `%s', which is
>     associated with"
>      > +                  " a subroutine type.\n",
>      > +                  _mesa_shader_stage_to_string(i), definitions,
>     fn->name);
>      > +            return;
>      > +         }
>      > +      }
>      > +   }
>      > +}
>      > +
>      > +
>      >   static void
>      >   set_always_active_io(exec_list *ir, ir_variable_mode io_mode)
>      >   {
>      > @@ -5024,6 +5063,7 @@ link_shaders(struct gl_context *ctx, struct
>     gl_shader_program *prog)
>      >
>      >      check_explicit_uniform_locations(ctx, prog);
>      >      link_assign_subroutine_types(prog);
>      > +   verify_subroutine_associated_funcs(prog);
>      >
>      >      if (!prog->data->LinkStatus)
>      >         goto done;
>      >
>     _______________________________________________
>     mesa-dev mailing list
>     mesa-dev at lists.freedesktop.org <mailto:mesa-dev at lists.freedesktop.org>
>     https://lists.freedesktop.org/mailman/listinfo/mesa-dev
> 
> 
> 
> -- 
> 
> Vadym Shovkoplias | Senior Software Engineer
> GlobalLogic
> P +380.57.766.7667  M +3.8050.931.7304  S vadym.shovkoplias
> www.globallogic.com <http://www.globallogic.com/>
> <http://www.globallogic.com/>
> http://www.globallogic.com/email_disclaimer.txt