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

[Iago Toral]

Jenkins is good, I have pushed the patch with a small style fix and a
v2 tag in the commit log.
Iago
On Thu, 2018-10-04 at 10:15 +0300, Vadim Shovkoplias wrote:
> 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 globallog
> > > > ic.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/a15098b9/attachment-0001.html>