<html><head></head><body><div>Jenkins is good, I have pushed the patch with a small style fix and a v2 tag in the commit log.</div><div><br></div><div>Iago</div><div><br></div><div>On Thu, 2018-10-04 at 10:15 +0300, Vadim Shovkoplias wrote:</div><blockquote type="cite" style="margin:0 0 0 .8ex; border-left:2px #729fcf solid;padding-left:1ex"><div dir="ltr">Thanks, I'll appreciate if you will push the patch once test will be finished.</div><br><div class="gmail_quote"><div dir="ltr">чт, 4 окт. 2018 г. в 9:52, Iago Toral <<a href="mailto:itoral@igalia.com">itoral@igalia.com</a>>:<br></div><blockquote type="cite" style="margin:0 0 0 .8ex; border-left:2px #729fcf solid;padding-left:1ex"><div><div>On Wed, 2018-10-03 at 16:24 +0300, Vadim Shovkoplias wrote:</div><blockquote type="cite" style="margin:0 0 0 .8ex; border-left:2px #729fcf solid;padding-left:1ex"><div dir="ltr"><div dir="ltr">Hi Iago,<div><br></div><div>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).</div><div><div>In GLSL spec mentioned: "A program will fail to compile or link if any <b>shader or stage</b> contains two or more functions with the same name if the name is </div><div>associated with a subroutine type".</div></div><div>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 </div><div>two fragment shader objects. One object contains function foo() which is associated with subroutine type and second shader object has some regular foo() function </div><div>with the same name. I suppose compiler fix won't be able to detect this.</div></div></div></blockquote><div><br></div><div>Yes, that is correct, good point.</div><div><br></div><blockquote type="cite" style="margin:0 0 0 .8ex; border-left:2px #729fcf solid;padding-left:1ex"><div dir="ltr"><div dir="ltr"><div>IMHO the best way to fix this is to implement 2 patches:</div><div>1) One patch for linker to implement restriction for the shader stages (already done in this patch)</div><div>2) Another one for compiler to check the restriction for distinct shader objects.</div><div><br></div><div>What do you think ?</div></div></div></blockquote><div><br></div><div>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)</div><div><br></div><div>Once you have addressed Tapani's comment feel free to add my:</div><div><br></div><div>Reviewed-by: Iago Toral Quiroga <<a>itoral@igalia.com></a></div><div><br></div><div>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.</div><div><br></div><div>Iago</div><div><br></div><blockquote type="cite" style="margin:0 0 0 .8ex; border-left:2px #729fcf solid;padding-left:1ex"><div class="gmail_quote"><div dir="ltr">ср, 3 окт. 2018 г. в 13:59, Iago Toral <<a href="mailto:itoral@igalia.com" target="_blank">itoral@igalia.com</a>>:<br></div><blockquote type="cite" style="margin:0 0 0 .8ex; border-left:2px #729fcf solid;padding-left:1ex">Hi Vadym,<br>
<br>
I think this looks correct, but I was wondering if you considered<br>
implementing this check in ir_reader::read_function_sig (ir_reader.cpp)<br>
instead, which runs at compile time. My rationale is that given the<br>
option, I think it is preferable to push work to compile time rather<br>
than link time.<br>
<br>
Iago<br>
<br>
On Wed, 2018-10-03 at 11:39 +0300, Vadym Shovkoplias wrote:<br>
> From Section 6.1.2 (Subroutines) of the GLSL 4.00 specification<br>
> <br>
> "A program will fail to compile or link if any shader<br>
> or stage contains two or more functions with the same<br>
> name if the name is associated with a subroutine type."<br>
> <br>
> Fixes:<br>
> * no-overloads.vert<br>
> <br>
> Bugzilla: <a href="https://bugs.freedesktop.org/show_bug.cgi?id=108109" rel="noreferrer" target="_blank">https://bugs.freedesktop.org/show_bug.cgi?id=108109</a><br>
> Signed-off-by: Vadym Shovkoplias <<a href="mailto:vadym.shovkoplias@globallogic.com" target="_blank">vadym.shovkoplias@globallogic.com</a>><br>
> ---<br>
> src/compiler/glsl/linker.cpp | 40<br>
> ++++++++++++++++++++++++++++++++++++<br>
> 1 file changed, 40 insertions(+)<br>
> <br>
> diff --git a/src/compiler/glsl/linker.cpp<br>
> b/src/compiler/glsl/linker.cpp<br>
> index 3fde7e78d3..aca5488a1e 100644<br>
> --- a/src/compiler/glsl/linker.cpp<br>
> +++ b/src/compiler/glsl/linker.cpp<br>
> @@ -4639,6 +4639,45 @@ link_assign_subroutine_types(struct<br>
> gl_shader_program *prog)<br>
> }<br>
> }<br>
> <br>
> +static void<br>
> +verify_subroutine_associated_funcs(struct gl_shader_program *prog)<br>
> +{<br>
> + unsigned mask = prog->data->linked_stages;<br>
> + while (mask) {<br>
> + const int i = u_bit_scan(&mask);<br>
> + gl_program *p = prog->_LinkedShaders[i]->Program;<br>
> + glsl_symbol_table *symbols = prog->_LinkedShaders[i]->symbols;<br>
> +<br>
> + /*<br>
> + * From OpenGL ES Shading Language 4.00 specification<br>
> + * (6.1.2 Subroutines):<br>
> + * "A program will fail to compile or link if any shader<br>
> + * or stage contains two or more functions with the same<br>
> + * name if the name is associated with a subroutine type."<br>
> + */<br>
> + for (unsigned j = 0; j < p->sh.NumSubroutineFunctions; j++) {<br>
> + unsigned definitions = 0;<br>
> + char *name = p->sh.SubroutineFunctions[j].name;<br>
> + ir_function *fn = symbols->get_function(name);<br>
> +<br>
> + /* Calculate number of function definitions with the same<br>
> name */<br>
> + foreach_in_list(ir_function_signature, sig, &fn-<br>
> >signatures) {<br>
> + if (sig->is_defined) {<br>
> + if (++definitions > 1) {<br>
> + linker_error(prog, "%s shader contains two or more<br>
> function "<br>
> + "definitions with name `%s', which is "<br>
> + "associated with a subroutine type.\n",<br>
> + _mesa_shader_stage_to_string(i),<br>
> + fn->name);<br>
> + return;<br>
> + }<br>
> + }<br>
> + }<br>
> + }<br>
> + }<br>
> +}<br>
> +<br>
> +<br>
> static void<br>
> set_always_active_io(exec_list *ir, ir_variable_mode io_mode)<br>
> {<br>
> @@ -5024,6 +5063,7 @@ link_shaders(struct gl_context *ctx, struct<br>
> gl_shader_program *prog)<br>
> <br>
> check_explicit_uniform_locations(ctx, prog);<br>
> link_assign_subroutine_types(prog);<br>
> + verify_subroutine_associated_funcs(prog);<br>
> <br>
> if (!prog->data->LinkStatus)<br>
> goto done;<br>
</blockquote></div>
</blockquote></div></blockquote></div>
</blockquote></body></html>