Re: [Mesa-dev] [PATCH] glsl/linker: check same name is not used in block and outside
On Wed, 2018-01-31 at 01:38 +0100, Matteo Bruni wrote: > 2018-01-30 16:11 GMT+01:00 Juan A. Suarez Romero: > > According with OpenGL GLSL 3.20 spec, section 4.3.9: > > > > "It is a link-time error if any particular shader interface > >contains: > > - two different blocks, each having no instance name, and each > >having a member of the same name, or > > - a variable outside a block, and a block with no instance name, > >where the variable has the same name as a member in the block." > > > > This fixes a previous commit 9b894c8 ("glsl/linker: link-error using the > > same name in unnamed block and outside") that covered this case, but > > did not take in account that precision qualifiers are ignored when > > comparing blocks with no instance name. > > Thanks! This also fixes Wine, although the reason is slightly > different: Wine might end up linking a program with a vertex shader > containing "#extension GL_ARB_cull_distance : enable" together with a > vertex shader without it. I guess this makes the gl_PerVertex builtin > defined in the two shader objects differ and that triggered the old > check but not the new one from this patch. > > I don't know if you want to change the commit message to account for > that (maybe make it more general?) but in any case, and FWIW: > > Tested-by: Matteo Bruni > > I also have a nitpick which you can entirely ignore, below. > > > + if (var->get_interface_type() != existing->get_interface_type()) { > > +if (!var->get_interface_type() || > > !existing->get_interface_type()) { > > + linker_error(prog, "declarations for %s `%s` are inside > > block " > > +"`%s` and outside a block", > > +mode_string(var), var->name, > > +var->get_interface_type() > > + ? var->get_interface_type()->name > > + : existing->get_interface_type()->name); > > + return; > > +} else if (strcmp(var->get_interface_type()->name, > > + existing->get_interface_type()->name) != 0) { > > + linker_error(prog, "declarations for %s `%s` are inside > > blocks " > > +"`%s` and `%s`", > > +mode_string(var), var->name, > > +existing->get_interface_type()->name, > > +var->get_interface_type()->name); > > What about declaring a couple of variables for the interface types and > using them through? Like: > > const glsl_type var_itype = var->get_interface_type(); > const glsl_type existing_itype = existing->get_interface_type(); > Yes, I'll use a couple of variables. Thanks. J.A. > (maybe with different names, I don't know what's Mesa's preferred style) > ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] glsl/linker: check same name is not used in block and outside
Reviewed-by: Tapani PälliOn 30.01.2018 17:11, Juan A. Suarez Romero wrote: According with OpenGL GLSL 3.20 spec, section 4.3.9: "It is a link-time error if any particular shader interface contains: - two different blocks, each having no instance name, and each having a member of the same name, or - a variable outside a block, and a block with no instance name, where the variable has the same name as a member in the block." This fixes a previous commit 9b894c8 ("glsl/linker: link-error using the same name in unnamed block and outside") that covered this case, but did not take in account that precision qualifiers are ignored when comparing blocks with no instance name. With this commit, the original tests KHR-GL*.shaders.uniform_block.common.name_matching keep fixed, and also dEQP-GLES31.functional.shaders.linkage.uniform.block.differing_precision regression is fixed, which was broken by previous commit. Fixes: 9b894c8 ("glsl/linker: link-error using the same name in unnamed block and outside") Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=104668 Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=104777 CC: Mark Janes CC: "18.0" Signed-off-by: Juan A. Suarez Romero --- src/compiler/glsl/linker.cpp | 54 +--- 1 file changed, 31 insertions(+), 23 deletions(-) diff --git a/src/compiler/glsl/linker.cpp b/src/compiler/glsl/linker.cpp index ce101935b01..65b22fdba8a 100644 --- a/src/compiler/glsl/linker.cpp +++ b/src/compiler/glsl/linker.cpp @@ -,29 +,6 @@ cross_validate_globals(struct gl_shader_program *prog, return; } - /* In OpenGL GLSL 4.20 spec, section 4.3.9, page 57: - * - * "It is a link-time error if any particular shader interface - *contains: - * - *- two different blocks, each having no instance name, and each - * having a member of the same name, or - * - *- a variable outside a block, and a block with no instance name, - * where the variable has the same name as a member in the block." - */ - if (var->data.mode == existing->data.mode && - var->get_interface_type() != existing->get_interface_type()) { -linker_error(prog, "declarations for %s `%s` are in " - "%s and %s\n", - mode_string(var), var->name, - existing->get_interface_type() ? - existing->get_interface_type()->name : "outside a block", - var->get_interface_type() ? - var->get_interface_type()->name : "outside a block"); - -return; - } /* Only in GLSL ES 3.10, the precision qualifier should not match * between block members defined in matched block names within a * shader interface. @@ -1155,6 +1132,37 @@ cross_validate_globals(struct gl_shader_program *prog, mode_string(var), var->name); } } + + /* In OpenGL GLSL 3.20 spec, section 4.3.9: + * + * "It is a link-time error if any particular shader interface + *contains: + * + *- two different blocks, each having no instance name, and each + * having a member of the same name, or + * + *- a variable outside a block, and a block with no instance name, + * where the variable has the same name as a member in the block." + */ + if (var->get_interface_type() != existing->get_interface_type()) { +if (!var->get_interface_type() || !existing->get_interface_type()) { + linker_error(prog, "declarations for %s `%s` are inside block " +"`%s` and outside a block", +mode_string(var), var->name, +var->get_interface_type() + ? var->get_interface_type()->name + : existing->get_interface_type()->name); + return; +} else if (strcmp(var->get_interface_type()->name, + existing->get_interface_type()->name) != 0) { + linker_error(prog, "declarations for %s `%s` are inside blocks " +"`%s` and `%s`", +mode_string(var), var->name, +existing->get_interface_type()->name, +var->get_interface_type()->name); + return; +} + } } else variables->add_variable(var); } ___ mesa-dev mailing list
Re: [Mesa-dev] [PATCH] glsl/linker: check same name is not used in block and outside
2018-01-30 16:11 GMT+01:00 Juan A. Suarez Romero: > According with OpenGL GLSL 3.20 spec, section 4.3.9: > > "It is a link-time error if any particular shader interface >contains: > - two different blocks, each having no instance name, and each >having a member of the same name, or > - a variable outside a block, and a block with no instance name, >where the variable has the same name as a member in the block." > > This fixes a previous commit 9b894c8 ("glsl/linker: link-error using the > same name in unnamed block and outside") that covered this case, but > did not take in account that precision qualifiers are ignored when > comparing blocks with no instance name. Thanks! This also fixes Wine, although the reason is slightly different: Wine might end up linking a program with a vertex shader containing "#extension GL_ARB_cull_distance : enable" together with a vertex shader without it. I guess this makes the gl_PerVertex builtin defined in the two shader objects differ and that triggered the old check but not the new one from this patch. I don't know if you want to change the commit message to account for that (maybe make it more general?) but in any case, and FWIW: Tested-by: Matteo Bruni I also have a nitpick which you can entirely ignore, below. > + if (var->get_interface_type() != existing->get_interface_type()) { > +if (!var->get_interface_type() || > !existing->get_interface_type()) { > + linker_error(prog, "declarations for %s `%s` are inside block > " > +"`%s` and outside a block", > +mode_string(var), var->name, > +var->get_interface_type() > + ? var->get_interface_type()->name > + : existing->get_interface_type()->name); > + return; > +} else if (strcmp(var->get_interface_type()->name, > + existing->get_interface_type()->name) != 0) { > + linker_error(prog, "declarations for %s `%s` are inside > blocks " > +"`%s` and `%s`", > +mode_string(var), var->name, > +existing->get_interface_type()->name, > +var->get_interface_type()->name); What about declaring a couple of variables for the interface types and using them through? Like: const glsl_type var_itype = var->get_interface_type(); const glsl_type existing_itype = existing->get_interface_type(); (maybe with different names, I don't know what's Mesa's preferred style) ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev