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.

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 ?

ср, 3 окт. 2018 г. в 13:59, Iago Toral <ito...@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.shovkopl...@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;
>
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Reply via email to