From: Mikko Rasa <tdb@tdb.fi>
Date: Fri, 2 Apr 2021 20:05:06 +0000 (+0300)
Subject: Validate location overlap and type matching for GLSL interfaces
X-Git-Url: http://git.tdb.fi/?a=commitdiff_plain;h=20a86c5905e1f7527d3c9edc3f56f6b7679c268a;p=libs%2Fgl.git

Validate location overlap and type matching for GLSL interfaces
---

diff --git a/source/glsl/compiler.cpp b/source/glsl/compiler.cpp
index 5ff003e7..e0a67606 100644
--- a/source/glsl/compiler.cpp
+++ b/source/glsl/compiler.cpp
@@ -82,9 +82,13 @@ void Compiler::compile(Mode mode)
 		generate(*i);
 	ConstantIdAssigner().apply(*module, features);
 
+	for(list<Stage>::iterator i=module->stages.begin(); i!=module->stages.end(); ++i)
+		validate(*i);
+	GlobalInterfaceValidator().apply(*module);
+
 	bool valid = true;
 	for(list<Stage>::iterator i=module->stages.begin(); i!=module->stages.end(); ++i)
-		if(!validate(*i))
+		if(!check_errors(*i))
 			valid = false;
 	if(!valid)
 		throw invalid_shader_source(get_diagnostics());
@@ -297,13 +301,17 @@ void Compiler::resolve(Stage &stage, unsigned flags)
 	}
 }
 
-bool Compiler::validate(Stage &stage)
+void Compiler::validate(Stage &stage)
 {
 	DeclarationValidator().apply(stage);
 	IdentifierValidator().apply(stage);
 	ReferenceValidator().apply(stage);
 	ExpressionValidator().apply(stage);
+	StageInterfaceValidator().apply(stage);
+}
 
+bool Compiler::check_errors(Stage &stage)
+{
 	stable_sort(stage.diagnostics, &diagnostic_line_order);
 
 	for(vector<Diagnostic>::const_iterator i=stage.diagnostics.begin(); i!=stage.diagnostics.end(); ++i)
diff --git a/source/glsl/compiler.h b/source/glsl/compiler.h
index 674081a0..e30bd709 100644
--- a/source/glsl/compiler.h
+++ b/source/glsl/compiler.h
@@ -132,9 +132,13 @@ private:
 	aspects as necessary. */
 	void resolve(Stage &, unsigned = RESOLVE_ALL);
 
-	/** Checks the validity of the module.  If the return value is false, the
-	module's diagnostics list will contain additional information of errors. */
-	bool validate(Stage &);
+	/** Runs validators on a stage.  Diagnostic messages are recorded in the
+	stage for later inspection. */
+	void validate(Stage &);
+
+	/** Checks a stage's recorded diagnostics for errors.  If any are found,
+	returns true. */
+	bool check_errors(Stage &);
 
 	static bool diagnostic_line_order(const Diagnostic &, const Diagnostic &);
 
diff --git a/source/glsl/syntax.cpp b/source/glsl/syntax.cpp
index 32a7b12c..23b8f030 100644
--- a/source/glsl/syntax.cpp
+++ b/source/glsl/syntax.cpp
@@ -485,6 +485,66 @@ string get_unused_variable_name(const Block &block, const string &base)
 	}
 }
 
+bool is_same_type(const TypeDeclaration &type1, const TypeDeclaration &type2)
+{
+	if(const BasicTypeDeclaration *basic1 = dynamic_cast<const BasicTypeDeclaration *>(&type1))
+	{
+		const BasicTypeDeclaration *basic2 = dynamic_cast<const BasicTypeDeclaration *>(&type2);
+		if(!basic2)
+			return false;
+
+		if(basic1->kind!=basic2->kind || basic1->size!=basic2->size)
+			return false;
+
+		if(basic1->base_type && basic2->base_type)
+			return is_same_type(*basic1->base_type, *basic2->base_type);
+		else
+			return (!basic1->base_type && !basic2->base_type);
+	}
+	else if(const ImageTypeDeclaration *image1 = dynamic_cast<const ImageTypeDeclaration *>(&type1))
+	{
+		const ImageTypeDeclaration *image2 = dynamic_cast<const ImageTypeDeclaration *>(&type2);
+		if(!image2)
+			return false;
+
+		if(image1->dimensions!=image2->dimensions || image1->array!=image2->array)
+			return false;
+		if(image1->sampled!=image2->sampled || image1->shadow!=image2->shadow)
+			return false;
+
+		if(image1->base_type && image2->base_type)
+			return is_same_type(*image1->base_type, *image2->base_type);
+		else
+			return (!image1->base_type && !image2->base_type);
+	}
+	else if(const StructDeclaration *strct1 = dynamic_cast<const StructDeclaration *>(&type1))
+	{
+		const StructDeclaration *strct2 = dynamic_cast<const StructDeclaration *>(&type2);
+		if(!strct2)
+			return false;
+
+		NodeList<Statement>::const_iterator i = strct1->members.body.begin();
+		NodeList<Statement>::const_iterator j = strct2->members.body.begin();
+		for(; (i!=strct1->members.body.end() && j!=strct2->members.body.end()); ++i, ++j)
+		{
+			const VariableDeclaration *var1 = dynamic_cast<const VariableDeclaration *>(i->get());
+			const VariableDeclaration *var2 = dynamic_cast<const VariableDeclaration *>(j->get());
+			if(!var1 || !var1->type_declaration || !var2 || !var2->type_declaration)
+				return false;
+			if(!is_same_type(*var1->type_declaration, *var2->type_declaration))
+				return false;
+			if(var1->name!=var2->name || var1->array!=var2->array)
+				return false;
+			// TODO Compare array sizes
+			// TODO Compare layout qualifiers for interface block members
+		}
+
+		return (i==strct1->members.body.end() && j==strct2->members.body.end());
+	}
+	else
+		return false;
+}
+
 } // namespace SL
 } // namespace GL
 } // namespace Msp
diff --git a/source/glsl/syntax.h b/source/glsl/syntax.h
index 1caf4d95..d69922a7 100644
--- a/source/glsl/syntax.h
+++ b/source/glsl/syntax.h
@@ -551,6 +551,8 @@ struct Module
 
 std::string get_unused_variable_name(const Block &, const std::string &);
 
+bool is_same_type(const TypeDeclaration &, const TypeDeclaration &);
+
 } // namespace SL
 } // namespace GL
 } // namespace Msp
diff --git a/source/glsl/validate.cpp b/source/glsl/validate.cpp
index 154189aa..0096ff5e 100644
--- a/source/glsl/validate.cpp
+++ b/source/glsl/validate.cpp
@@ -609,6 +609,139 @@ void ExpressionValidator::visit(Return &ret)
 	TraversingVisitor::visit(ret);
 }
 
+
+int StageInterfaceValidator::get_location(const Layout &layout)
+{
+	for(vector<Layout::Qualifier>::const_iterator i=layout.qualifiers.begin(); i!=layout.qualifiers.end(); ++i)
+		if(i->name=="location")
+			return i->value;
+	return -1;
+}
+
+void StageInterfaceValidator::visit(VariableDeclaration &var)
+{
+	int location = (var.layout ? get_location(*var.layout) : -1);
+	if(var.interface=="in" && var.linked_declaration)
+	{
+		const Layout *linked_layout = var.linked_declaration->layout.get();
+		int linked_location = (linked_layout ? get_location(*linked_layout) : -1);
+		if(linked_location!=location)
+		{
+			error(var, format("Mismatched location %d for 'in %s'", location, var.name));
+			add_info(*var.linked_declaration, format("Linked to 'out %s' with location %d",
+				var.linked_declaration->name, linked_location));
+		}
+		if(var.type_declaration && var.linked_declaration->type_declaration)
+		{
+			const TypeDeclaration *type = var.type_declaration;
+			if(stage->type==Stage::GEOMETRY)
+			{
+				if(const BasicTypeDeclaration *basic = dynamic_cast<const BasicTypeDeclaration *>(type))
+					if(basic->kind==BasicTypeDeclaration::ARRAY && basic->base_type)
+						type = basic->base_type;
+			}
+			if(!is_same_type(*type, *var.linked_declaration->type_declaration))
+			{
+				error(var, format("Mismatched type '%s' for 'in %s'", type->name, var.name));
+				add_info(*var.linked_declaration, format("Linked to 'out %s' with type '%s'",
+					var.linked_declaration->name, var.linked_declaration->type_declaration->name));
+			}
+		}
+	}
+
+	if(location>=0 && !var.interface.empty())
+	{
+		map<unsigned, VariableDeclaration *> &used = used_locations[var.interface];
+
+		unsigned loc_count = 1;
+		if(var.array)
+			if(const Literal *literal = dynamic_cast<const Literal *>(var.array_size.get()))
+				if(literal->value.check_type<int>())
+					loc_count = literal->value.value<int>();
+
+		for(unsigned i=0; i<loc_count; ++i)
+		{
+			map<unsigned, VariableDeclaration *>::const_iterator j = used.find(location+i);
+			if(j!=used.end())
+			{
+				error(var, format("Overlapping location %d for '%s %s'", location+i, var.interface, var.name));
+				add_info(*j->second, format("Previously used here for '%s %s'", j->second->interface, j->second->name));
+			}
+			else
+				used[location+i] = &var;
+		}
+	}
+}
+
+
+void GlobalInterfaceValidator::apply(Module &module)
+{
+	for(list<Stage>::iterator i=module.stages.begin(); i!=module.stages.end(); ++i)
+	{
+		stage = &*i;
+		i->content.visit(*this);
+	}
+}
+
+void GlobalInterfaceValidator::get_binding(const Layout &layout, unsigned &desc_set, int &binding)
+{
+	for(vector<Layout::Qualifier>::const_iterator i=layout.qualifiers.begin(); i!=layout.qualifiers.end(); ++i)
+	{
+		if(i->name=="set")
+			desc_set = i->value;
+		else if(i->name=="binding")
+			binding = i->value;
+	}
+}
+
+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;
+
+	map<unsigned, Binding> &used = used_bindings[desc_set];
+	map<unsigned, Binding>::const_iterator i = used.find(bind_point);
+	if(i!=used.end())
+	{
+		if(i->second.name!=binding.name)
+		{
+			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));
+		}
+		if(i->second.type && binding.type)
+		{
+			if(!is_same_type(*i->second.type, *binding.type))
+			{
+				string type_name = (dynamic_cast<const StructDeclaration *>(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<const StructDeclaration *>(i->second.type))
+					message += format(" with type '%s'", i->second.type->name);
+				add_info(*i->second.node, message);
+			}
+		}
+	}
+	else
+		used.insert(make_pair(bind_point, binding));
+}
+
+void GlobalInterfaceValidator::visit(VariableDeclaration &var)
+{
+	if(var.interface=="uniform" && var.layout)
+		check_binding(*var.layout, var);
+}
+
+void GlobalInterfaceValidator::visit(InterfaceBlock &iface)
+{
+	if(iface.interface=="uniform" && iface.layout)
+		check_binding(*iface.layout, iface);
+}
+
 } // namespace SL
 } // namespace GL
 } // namespace Msp
diff --git a/source/glsl/validate.h b/source/glsl/validate.h
index f1e3f512..63af3610 100644
--- a/source/glsl/validate.h
+++ b/source/glsl/validate.h
@@ -127,6 +127,48 @@ private:
 	virtual void visit(Return &);
 };
 
+class StageInterfaceValidator: private Validator
+{
+private:
+	std::map<std::string, std::map<unsigned, VariableDeclaration *> > used_locations;
+
+public:
+	void apply(Stage &s) { stage = &s; s.content.visit(*this); }
+
+private:
+	int get_location(const Layout &);
+
+	virtual void visit(VariableDeclaration &);
+	virtual void visit(FunctionDeclaration &) { }
+};
+
+class GlobalInterfaceValidator: private Validator
+{
+private:
+	struct Binding
+	{
+		Node *node;
+		TypeDeclaration *type;
+		std::string name;
+
+		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) { }
+	};
+
+	std::map<unsigned, std::map<unsigned, Binding> > used_bindings;
+
+public:
+	void apply(Module &);
+
+private:
+	void get_binding(const Layout &, unsigned &, int &);
+	void check_binding(const Layout &, const Binding &);
+
+	virtual void visit(VariableDeclaration &);
+	virtual void visit(InterfaceBlock &);
+	virtual void visit(FunctionDeclaration &) { }
+};
+
 } // namespace SL
 } // namespace GL
 } // namespace Msp
diff --git a/tests/glsl/binding_mismatch.glsl b/tests/glsl/binding_mismatch.glsl
new file mode 100644
index 00000000..fa88bf1a
--- /dev/null
+++ b/tests/glsl/binding_mismatch.glsl
@@ -0,0 +1,43 @@
+#pragma MSP stage(vertex)
+layout(binding=0) uniform Transform
+{
+	mat4 model;
+	mat4 vp;
+};
+layout(binding=1) uniform Lighting
+{
+	vec3 position;
+} light;
+layout(location=0) in vec4 position;
+layout(location=1) in vec3 normal;
+void main()
+{
+	vec4 world_pos = model*position;
+	gl_Position = vp*world_pos;
+	out vec3 light_dir = world_pos.xyz-light.position;
+	passthrough;
+}
+
+#pragma MSP stage(fragment)
+layout(binding=1) uniform Lighting
+{
+	vec4 color;
+} light;
+layout(binding=0) uniform Material
+{
+	vec4 color;
+};
+layout(location=0) out vec4 frag_color;
+void main()
+{
+	frag_color = color*vec4(vec3(max(dot(normalize(normal), normalize(light_dir)), 0.0)), 1.0);
+}
+
+/* Expected error:
+<test>:23: Mismatched struct type for binding 1 'Lighting'
+<test>:8: Previously used here
+<test>:27: Overlapping binding 0 for 'Material'
+<test>:3: Previously used here for 'Transform'
+<test>:27: Mismatched struct type for binding 0 'Material'
+<test>:3: Previously used here
+*/
diff --git a/tests/glsl/linked_interface_mismatch.glsl b/tests/glsl/linked_interface_mismatch.glsl
new file mode 100644
index 00000000..30a70d52
--- /dev/null
+++ b/tests/glsl/linked_interface_mismatch.glsl
@@ -0,0 +1,30 @@
+uniform sampler2D tex;
+
+#pragma MSP stage(vertex)
+layout(location=0) in vec4 position;
+layout(location=1) in vec4 color;
+layout(location=2) in vec4 texcoord;
+layout(location=0) out vec4 color_out;
+layout(location=2) out vec4 tc_out;
+void main()
+{
+	gl_Position = position;
+	color_out = color;
+	tc_out = texcoord;
+}
+
+#pragma MSP stage(fragment)
+layout(location=1) in vec4 color_out;
+layout(location=2) in vec2 tc_out;
+layout(location=0) out vec4 frag_color;
+void main()
+{
+	frag_color = color_out*texture(tex, tc_out);
+}
+
+/* Expected error:
+<test>:17: Mismatched location 1 for 'in color_out'
+<test>:7: Linked to 'out color_out' with location 0
+<test>:18: Mismatched type 'vec2' for 'in tc_out'
+<test>:8: Linked to 'out tc_out' with type 'vec4'
+*/
diff --git a/tests/glsl/location_overlap.glsl b/tests/glsl/location_overlap.glsl
new file mode 100644
index 00000000..df0f0314
--- /dev/null
+++ b/tests/glsl/location_overlap.glsl
@@ -0,0 +1,15 @@
+#pragma MSP stage(vertex)
+layout(location=0) in vec4 position;
+layout(location=0) in vec2 texcoords[3];
+layout(location=2) in vec4 color;
+void main()
+{
+	gl_Position = position;
+}
+
+/* Expected error:
+<test>:3: Overlapping location 0 for 'in texcoords'
+<test>:2: Previously used here for 'in position'
+<test>:4: Overlapping location 2 for 'in color'
+<test>:3: Previously used here for 'in texcoords'
+*/