From 8f5f54a9e165dae424e5b0bb8e488c3d01849bf6 Mon Sep 17 00:00:00 2001 From: Mikko Rasa Date: Sat, 3 Apr 2021 11:32:46 +0300 Subject: [PATCH] Even more validation for uniform mismatches --- source/glsl/reflect.cpp | 46 +++++++ source/glsl/reflect.h | 31 +++++ source/glsl/syntax.cpp | 8 ++ source/glsl/syntax.h | 1 + source/glsl/validate.cpp | 128 ++++++++++++------ source/glsl/validate.h | 17 ++- tests/glsl/location_overlap.glsl | 7 +- ...match.glsl => uniform_block_mismatch.glsl} | 6 +- tests/glsl/uniform_mismatch.glsl | 28 ++++ 9 files changed, 217 insertions(+), 55 deletions(-) create mode 100644 source/glsl/reflect.cpp create mode 100644 source/glsl/reflect.h rename tests/glsl/{binding_mismatch.glsl => uniform_block_mismatch.glsl} (81%) create mode 100644 tests/glsl/uniform_mismatch.glsl diff --git a/source/glsl/reflect.cpp b/source/glsl/reflect.cpp new file mode 100644 index 00000000..a3e36be8 --- /dev/null +++ b/source/glsl/reflect.cpp @@ -0,0 +1,46 @@ +#include "reflect.h" + +namespace Msp { +namespace GL { +namespace SL { + +LocationCounter::LocationCounter(): + r_count(0) +{ } + +void LocationCounter::visit(BasicTypeDeclaration &basic) +{ + r_count = basic.kind==BasicTypeDeclaration::MATRIX ? basic.size>>16 : 1; +} + +void LocationCounter::visit(ImageTypeDeclaration &) +{ + r_count = 1; +} + +void LocationCounter::visit(StructDeclaration &strct) +{ + unsigned total = 0; + for(NodeList::const_iterator i=strct.members.body.begin(); i!=strct.members.body.end(); ++i) + { + r_count = 1; + (*i)->visit(*this); + total += r_count; + } + r_count = total; +} + +void LocationCounter::visit(VariableDeclaration &var) +{ + r_count = 1; + if(var.type_declaration) + var.type_declaration->visit(*this); + if(var.array) + if(const Literal *literal = dynamic_cast(var.array_size.get())) + if(literal->value.check_type()) + r_count *= literal->value.value(); +} + +} // namespace SL +} // namespace GL +} // namespace Msp diff --git a/source/glsl/reflect.h b/source/glsl/reflect.h new file mode 100644 index 00000000..c03a5ccb --- /dev/null +++ b/source/glsl/reflect.h @@ -0,0 +1,31 @@ +#ifndef MSP_GL_SL_REFLECT_H_ +#define MSP_GL_SL_REFLECT_H_ + +#include "visitor.h" + +namespace Msp { +namespace GL { +namespace SL { + +class LocationCounter: private NodeVisitor +{ +private: + unsigned r_count; + +public: + LocationCounter(); + + unsigned apply(VariableDeclaration &v) { v.visit(*this); return r_count; } + +private: + virtual void visit(BasicTypeDeclaration &); + virtual void visit(ImageTypeDeclaration &); + virtual void visit(StructDeclaration &); + virtual void visit(VariableDeclaration &); +}; + +} // namespace SL +} // namespace GL +} // namespace Msp + +#endif diff --git a/source/glsl/syntax.cpp b/source/glsl/syntax.cpp index 23b8f030..d0273909 100644 --- a/source/glsl/syntax.cpp +++ b/source/glsl/syntax.cpp @@ -545,6 +545,14 @@ bool is_same_type(const TypeDeclaration &type1, const TypeDeclaration &type2) return false; } +int get_layout_value(const Layout &layout, const string &name, int def_value) +{ + for(vector::const_iterator i=layout.qualifiers.begin(); i!=layout.qualifiers.end(); ++i) + if(i->name==name) + return i->value; + return def_value; +} + } // namespace SL } // namespace GL } // namespace Msp diff --git a/source/glsl/syntax.h b/source/glsl/syntax.h index d69922a7..597a48e4 100644 --- a/source/glsl/syntax.h +++ b/source/glsl/syntax.h @@ -552,6 +552,7 @@ struct Module std::string get_unused_variable_name(const Block &, const std::string &); bool is_same_type(const TypeDeclaration &, const TypeDeclaration &); +int get_layout_value(const Layout &, const std::string &, int = -1); } // namespace SL } // namespace GL diff --git a/source/glsl/validate.cpp b/source/glsl/validate.cpp index 0096ff5e..889eee22 100644 --- a/source/glsl/validate.cpp +++ b/source/glsl/validate.cpp @@ -3,6 +3,7 @@ #include #include #include +#include "reflect.h" #include "validate.h" using namespace std; @@ -653,12 +654,7 @@ void StageInterfaceValidator::visit(VariableDeclaration &var) { map &used = used_locations[var.interface]; - unsigned loc_count = 1; - if(var.array) - if(const Literal *literal = dynamic_cast(var.array_size.get())) - if(literal->value.check_type()) - loc_count = literal->value.value(); - + unsigned loc_count = LocationCounter().apply(var); for(unsigned i=0; i::const_iterator j = used.find(location+i); @@ -683,63 +679,109 @@ void GlobalInterfaceValidator::apply(Module &module) } } -void GlobalInterfaceValidator::get_binding(const Layout &layout, unsigned &desc_set, int &binding) +void GlobalInterfaceValidator::check_uniform(const Uniform &uni) { - for(vector::const_iterator i=layout.qualifiers.begin(); i!=layout.qualifiers.end(); ++i) + map::const_iterator i = used_names.find(uni.name); + if(i!=used_names.end()) { - if(i->name=="set") - desc_set = i->value; - else if(i->name=="binding") - binding = i->value; + if(uni.location>=0 && i->second->location>=0 && i->second->location!=uni.location) + { + error(*uni.node, format("Mismatched location %d for uniform '%s'", uni.location, uni.name)); + add_info(*i->second->node, format("Previously declared here with location %d", i->second->location)); + } + if(uni.bind_point>=0 && i->second->bind_point>=0 && i->second->bind_point!=uni.bind_point) + { + error(*uni.node, format("Mismatched binding %d for uniform '%s'", uni.bind_point, uni.name)); + add_info(*i->second->node, format("Previously declared here with binding %d", i->second->bind_point)); + } + if(uni.type && i->second->type && !is_same_type(*uni.type, *i->second->type)) + { + string type_name = (dynamic_cast(uni.type) ? + "structure" : format("type '%s'", uni.type->name)); + error(*uni.node, format("Mismatched %s for uniform '%s'", type_name, uni.name)); + + string message = "Previously declared here"; + if(!dynamic_cast(i->second->type)) + message += format(" with type '%s'", i->second->type->name); + add_info(*i->second->node, message); + } } -} - -void GlobalInterfaceValidator::check_binding(const Layout &layout, const Binding &binding) -{ - unsigned desc_set = 0; - int bind_point = -1; - get_binding(layout, desc_set, bind_point); - if(bind_point<0) - return; + else + used_names.insert(make_pair(uni.name, &uni)); - map &used = used_bindings[desc_set]; - map::const_iterator i = used.find(bind_point); - if(i!=used.end()) + if(uni.location>=0) { - if(i->second.name!=binding.name) + map::const_iterator j = used_locations.find(uni.location); + if(j!=used_locations.end()) + { + if(j->second->name!=uni.name) + { + error(*uni.node, format("Overlapping location %d for '%s'", uni.location, uni.name)); + add_info(*j->second->node, format("Previously used here for '%s'", j->second->name)); + } + } + else { - error(*binding.node, format("Overlapping binding %d for '%s'", bind_point, binding.name)); - add_info(*i->second.node, format("Previously used here for '%s'", i->second.name)); + for(unsigned k=0; ksecond.type && binding.type) + } + + if(uni.bind_point>=0) + { + map &used = used_bindings[uni.desc_set]; + map::const_iterator j = used.find(uni.bind_point); + if(j!=used.end()) { - if(!is_same_type(*i->second.type, *binding.type)) + if(j->second->name!=uni.name) { - string type_name = (dynamic_cast(binding.type) ? "struct type" : - format("type '%s'", binding.type->name)); - error(*binding.node, format("Mismatched %s for binding %d '%s'", type_name, bind_point, binding.name)); - - string message = "Previously used here"; - if(!dynamic_cast(i->second.type)) - message += format(" with type '%s'", i->second.type->name); - add_info(*i->second.node, message); + error(*uni.node, format("Overlapping binding %d for '%s'", uni.bind_point, uni.name)); + add_info(*j->second->node, format("Previously used here for '%s'", j->second->name)); } } + else + used.insert(make_pair(uni.bind_point, &uni)); } - else - used.insert(make_pair(bind_point, binding)); } void GlobalInterfaceValidator::visit(VariableDeclaration &var) { - if(var.interface=="uniform" && var.layout) - check_binding(*var.layout, var); + if(var.interface=="uniform") + { + Uniform uni; + uni.node = &var; + uni.type = var.type_declaration; + uni.name = var.name; + if(var.layout) + { + uni.location = get_layout_value(*var.layout, "location"); + uni.loc_count = LocationCounter().apply(var); + uni.desc_set = get_layout_value(*var.layout, "set", 0); + uni.bind_point = get_layout_value(*var.layout, "binding"); + } + + uniforms.push_back(uni); + check_uniform(uniforms.back()); + } } void GlobalInterfaceValidator::visit(InterfaceBlock &iface) { - if(iface.interface=="uniform" && iface.layout) - check_binding(*iface.layout, iface); + if(iface.interface=="uniform") + { + Uniform uni; + uni.node = &iface; + uni.type = iface.struct_declaration; + uni.name = iface.block_name; + if(iface.layout) + { + uni.desc_set = get_layout_value(*iface.layout, "set", 0); + uni.bind_point = get_layout_value(*iface.layout, "binding"); + } + + uniforms.push_back(uni); + check_uniform(uniforms.back()); + } } } // namespace SL diff --git a/source/glsl/validate.h b/source/glsl/validate.h index 63af3610..acb5b4e4 100644 --- a/source/glsl/validate.h +++ b/source/glsl/validate.h @@ -145,24 +145,29 @@ private: class GlobalInterfaceValidator: private Validator { private: - struct Binding + struct Uniform { Node *node; TypeDeclaration *type; std::string name; + int location; + unsigned loc_count; + int desc_set; + int bind_point; - Binding(VariableDeclaration &v): node(&v), type(v.type_declaration), name(v.name) { } - Binding(InterfaceBlock &i): node(&i), type(i.struct_declaration), name(i.block_name) { } + Uniform(): node(0), type(0), location(-1), loc_count(1), desc_set(0), bind_point(-1) { } }; - std::map > used_bindings; + std::list uniforms; + std::map used_names; + std::map used_locations; + std::map > used_bindings; public: void apply(Module &); private: - void get_binding(const Layout &, unsigned &, int &); - void check_binding(const Layout &, const Binding &); + void check_uniform(const Uniform &); virtual void visit(VariableDeclaration &); virtual void visit(InterfaceBlock &); diff --git a/tests/glsl/location_overlap.glsl b/tests/glsl/location_overlap.glsl index df0f0314..d3cf5f9d 100644 --- a/tests/glsl/location_overlap.glsl +++ b/tests/glsl/location_overlap.glsl @@ -1,7 +1,8 @@ #pragma MSP stage(vertex) layout(location=0) in vec4 position; layout(location=0) in vec2 texcoords[3]; -layout(location=2) in vec4 color; +layout(location=2) in mat4 instance_transform; +layout(location=4) in vec4 color; void main() { gl_Position = position; @@ -10,6 +11,8 @@ void main() /* Expected error: :3: Overlapping location 0 for 'in texcoords' :2: Previously used here for 'in position' -:4: Overlapping location 2 for 'in color' +:4: Overlapping location 2 for 'in instance_transform' :3: Previously used here for 'in texcoords' +:5: Overlapping location 4 for 'in color' +:4: Previously used here for 'in instance_transform' */ diff --git a/tests/glsl/binding_mismatch.glsl b/tests/glsl/uniform_block_mismatch.glsl similarity index 81% rename from tests/glsl/binding_mismatch.glsl rename to tests/glsl/uniform_block_mismatch.glsl index fa88bf1a..6c896938 100644 --- a/tests/glsl/binding_mismatch.glsl +++ b/tests/glsl/uniform_block_mismatch.glsl @@ -34,10 +34,8 @@ void main() } /* Expected error: -:23: Mismatched struct type for binding 1 'Lighting' -:8: Previously used here +:23: Mismatched structure for uniform 'Lighting' +:8: Previously declared here :27: Overlapping binding 0 for 'Material' :3: Previously used here for 'Transform' -:27: Mismatched struct type for binding 0 'Material' -:3: Previously used here */ diff --git a/tests/glsl/uniform_mismatch.glsl b/tests/glsl/uniform_mismatch.glsl new file mode 100644 index 00000000..de5c08b2 --- /dev/null +++ b/tests/glsl/uniform_mismatch.glsl @@ -0,0 +1,28 @@ +#pragma MSP stage(vertex) +layout(binding=0) uniform sampler2D heightmap; +layout(location=0) uniform mat4 mvp; +layout(location=0) in vec2 position; +layout(location=1) in vec3 texcoord; +void main() +{ + gl_Position = mvp*vec4(position, texture(heightmap, texcoord.xy).r, 1.0); + passthrough; +} + +#pragma MSP stage(fragment) +layout(binding=1) uniform sampler2DArray heightmap; +layout(location=3) uniform vec4 color; +layout(location=0) out vec4 frag_color; +void main() +{ + frag_color = color*texture(heightmap, texcoord).r; +} + +/* Expected error: +:13: Mismatched binding 1 for uniform 'heightmap' +:2: Previously declared here with binding 0 +:13: Mismatched type 'sampler2DArray' for uniform 'heightmap' +:2: Previously declared here with type 'sampler2D' +:14: Overlapping location 3 for 'color' +:3: Previously used here for 'mvp' +*/ -- 2.43.0