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 <ito...@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 <ito...@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 <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.shovkoplias@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; > > > >
_______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
