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

[Vadim Shovkoplias]

Thanks, I'll appreciate if you will push the patch once test will be
finished.

чт, 4 окт. 2018 г. в 9:52, Iago Toral <itoral at igalia.com>:

> On Wed, 2018-10-03 at 16:24 +0300, Vadim Shovkoplias wrote:
>
> Hi Iago,
>
> I also think that compiler is the best place for the fix. But from my
> understanding compiler fix will be limited to distinct shader objects (not
> the shader stage).
> In GLSL spec mentioned: "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".
> So if I understand the spec correctly such restriction for the shader
> stage can be fixed only in linker. E.g. consider the use case when fragment
> shader is linked from
> two fragment shader objects. One object contains function foo() which is
> associated with subroutine type and second shader object has some regular
> foo() function
> with the same name.  I suppose compiler fix won't be able to detect this.
>
>
> Yes, that is correct, good point.
>
> IMHO the best way to fix this is to implement 2 patches:
> 1) One patch for linker to implement restriction for the shader stages
> (already done in this patch)
> 2) Another one for compiler to check the restriction for distinct shader
> objects.
>
> What do you think ?
>
>
> Since we need to keep the link-time check, any compile-time checks we add
> would be redundant with that, so I think we should just go with the
> link-time check only (your original patch)
>
> Once you have addressed Tapani's comment feel free to add my:
>
> Reviewed-by: Iago Toral Quiroga <itoral at igalia.com>
>
> I have sent the patch to Jenkins for testing just in case, if that comes
> back clean as expected let me know if you need me to push the patch for you.
>
> Iago
>
> ср, 3 окт. 2018 г. в 13:59, Iago Toral <itoral at igalia.com>:
>
> Hi Vadym,
>
> I think this looks correct, but I was wondering if you considered
> implementing this check in ir_reader::read_function_sig (ir_reader.cpp)
> instead, which runs at compile time. My rationale is that given the
> option, I think it is preferable to push work to compile time rather
> than link time.
>
> Iago
>
> On Wed, 2018-10-03 at 11:39 +0300, 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>
> > ---
> >  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..aca5488a1e 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) {
> > +               if (++definitions > 1) {
> > +                  linker_error(prog, "%s shader contains two or more
> > function "
> > +                        "definitions with name `%s', which is "
> > +                        "associated with a subroutine type.\n",
> > +                        _mesa_shader_stage_to_string(i),
> > +                        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;
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20181004/70fc647c/attachment-0001.html>