Commit 8326b26a by Shahbaz Youssefi Committed by Commit Bot

Fix link validation with ambiguous instanceless interface blocks

The following interface blocks should fail link: VS: layout(binding=0) buffer BufferBlockNameA { mediump float variable; }; FS: layout(binding=1) buffer BufferBlockNameB { mediump float variable; }; Because `variable` is ambiguous. Bug: angleproject:3580 Change-Id: I29576a6f152780819af0e9fb63249dbee7d9f2fc Reviewed-on: https://chromium-review.googlesource.com/c/angle/angle/+/2587450Reviewed-by: 's avatarJamie Madill <jmadill@chromium.org> Reviewed-by: 's avatarTim Van Patten <timvp@google.com> Commit-Queue: Shahbaz Youssefi <syoussefi@chromium.org>
parent d9318acc
...@@ -1339,7 +1339,6 @@ void TOutputGLSLBase::declareInterfaceBlockLayout(const TType &type) ...@@ -1339,7 +1339,6 @@ void TOutputGLSLBase::declareInterfaceBlockLayout(const TType &type)
case EbsUnspecified: case EbsUnspecified:
case EbsShared: case EbsShared:
// Default block storage is shared. // Default block storage is shared.
fprintf(stderr, "\n\n\nHERE\n\n\n");
out << "shared"; out << "shared";
break; break;
......
...@@ -609,12 +609,75 @@ bool ValidateGraphicsInterfaceBlocksPerShader( ...@@ -609,12 +609,75 @@ bool ValidateGraphicsInterfaceBlocksPerShader(
return true; return true;
} }
void LogAmbiguousFieldLinkMismatch(InfoLog &infoLog,
const std::string &blockName1,
const std::string &blockName2,
const std::string &fieldName,
ShaderType shaderType1,
ShaderType shaderType2)
{
infoLog << "Ambiguous field '" << fieldName << "' in blocks '" << blockName1 << "' ("
<< GetShaderTypeString(shaderType1) << " shader) and '" << blockName2 << "' ("
<< GetShaderTypeString(shaderType2) << " shader) which don't have instance names.";
}
bool ValidateInstancelessGraphicsInterfaceBlocksPerShader(
const std::vector<sh::InterfaceBlock> &interfaceBlocks,
ShaderType shaderType,
InterfaceBlockMap *instancelessBlocksFields,
InfoLog &infoLog)
{
ASSERT(instancelessBlocksFields);
for (const sh::InterfaceBlock &block : interfaceBlocks)
{
if (!block.instanceName.empty())
{
continue;
}
for (const sh::ShaderVariable &field : block.fields)
{
const auto &entry = instancelessBlocksFields->find(field.name);
if (entry != instancelessBlocksFields->end())
{
const sh::InterfaceBlock &linkedBlock = *(entry->second.second);
if (block.name != linkedBlock.name)
{
LogAmbiguousFieldLinkMismatch(infoLog, block.name, linkedBlock.name, field.name,
entry->second.first, shaderType);
return false;
}
}
else
{
(*instancelessBlocksFields)[field.name] = std::make_pair(shaderType, &block);
}
}
}
return true;
}
bool ValidateInterfaceBlocksMatch( bool ValidateInterfaceBlocksMatch(
GLuint numShadersHasInterfaceBlocks, GLuint numShadersHasInterfaceBlocks,
const ShaderMap<const std::vector<sh::InterfaceBlock> *> &shaderInterfaceBlocks, const ShaderMap<const std::vector<sh::InterfaceBlock> *> &shaderInterfaceBlocks,
InfoLog &infoLog, InfoLog &infoLog,
bool webglCompatibility) bool webglCompatibility,
InterfaceBlockMap *instancelessInterfaceBlocksFields)
{ {
for (ShaderType shaderType : kAllGraphicsShaderTypes)
{
// Validate that instanceless blocks of different names don't have fields of the same name.
if (shaderInterfaceBlocks[shaderType] &&
!ValidateInstancelessGraphicsInterfaceBlocksPerShader(
*shaderInterfaceBlocks[shaderType], shaderType, instancelessInterfaceBlocksFields,
infoLog))
{
return false;
}
}
if (numShadersHasInterfaceBlocks < 2u) if (numShadersHasInterfaceBlocks < 2u)
{ {
return true; return true;
...@@ -4036,6 +4099,8 @@ bool Program::linkInterfaceBlocks(const Caps &caps, ...@@ -4036,6 +4099,8 @@ bool Program::linkInterfaceBlocks(const Caps &caps,
GLuint combinedUniformBlocksCount = 0u; GLuint combinedUniformBlocksCount = 0u;
GLuint numShadersHasUniformBlocks = 0u; GLuint numShadersHasUniformBlocks = 0u;
ShaderMap<const std::vector<sh::InterfaceBlock> *> allShaderUniformBlocks = {}; ShaderMap<const std::vector<sh::InterfaceBlock> *> allShaderUniformBlocks = {};
InterfaceBlockMap instancelessInterfaceBlocksFields;
for (ShaderType shaderType : AllShaderTypes()) for (ShaderType shaderType : AllShaderTypes())
{ {
Shader *shader = mState.mAttachedShaders[shaderType]; Shader *shader = mState.mAttachedShaders[shaderType];
...@@ -4068,7 +4133,7 @@ bool Program::linkInterfaceBlocks(const Caps &caps, ...@@ -4068,7 +4133,7 @@ bool Program::linkInterfaceBlocks(const Caps &caps,
} }
if (!ValidateInterfaceBlocksMatch(numShadersHasUniformBlocks, allShaderUniformBlocks, infoLog, if (!ValidateInterfaceBlocksMatch(numShadersHasUniformBlocks, allShaderUniformBlocks, infoLog,
webglCompatibility)) webglCompatibility, &instancelessInterfaceBlocksFields))
{ {
return false; return false;
} }
...@@ -4112,7 +4177,8 @@ bool Program::linkInterfaceBlocks(const Caps &caps, ...@@ -4112,7 +4177,8 @@ bool Program::linkInterfaceBlocks(const Caps &caps,
} }
if (!ValidateInterfaceBlocksMatch(numShadersHasShaderStorageBlocks, allShaderStorageBlocks, if (!ValidateInterfaceBlocksMatch(numShadersHasShaderStorageBlocks, allShaderStorageBlocks,
infoLog, webglCompatibility)) infoLog, webglCompatibility,
&instancelessInterfaceBlocksFields))
{ {
return false; return false;
} }
......
...@@ -192,8 +192,6 @@ ...@@ -192,8 +192,6 @@
3571 VULKAN : dEQP-GLES31.functional.program_interface_query.transform_feedback_varying.*geo* = SKIP 3571 VULKAN : dEQP-GLES31.functional.program_interface_query.transform_feedback_varying.*geo* = SKIP
// Shader I/O blocks: // Shader I/O blocks:
// Missing matching of block names with unnamed SSBOs with the same member variable
3580 VULKAN : dEQP-GLES31.functional.shaders.linkage.es31.shader_storage_block.ambiguous_variable_name_3 = FAIL
// Missing matching of struct name in member fields of matching nameless I/O blocks // Missing matching of struct name in member fields of matching nameless I/O blocks
3580 VULKAN : dEQP-GLES31.functional.separate_shader.validation.es31.io_blocks.mismatch_different_member_struct_names = FAIL 3580 VULKAN : dEQP-GLES31.functional.separate_shader.validation.es31.io_blocks.mismatch_different_member_struct_names = FAIL
......
...@@ -8779,6 +8779,34 @@ void main() ...@@ -8779,6 +8779,34 @@ void main()
EXPECT_EQ(0u, program); EXPECT_EQ(0u, program);
} }
// Validate that link fails when two instanceless interface blocks with different block names but
// same field names are present.
TEST_P(GLSLTest_ES31, AmbiguousInstancelessInterfaceBlockFields)
{
ANGLE_SKIP_TEST_IF(!IsGLExtensionEnabled("GL_EXT_shader_io_blocks"));
constexpr char kVS[] = R"(#version 310 es
in highp vec4 position;
layout(binding = 0) buffer BlockA { mediump float a; };
void main()
{
a = 0.0;
gl_Position = position;
})";
constexpr char kFS[] = R"(#version 310 es
precision mediump float;
layout(location = 0) out mediump vec4 color;
uniform BlockB { float a; };
void main()
{
color = vec4(a, a, a, 1.0);
})";
GLuint program = CompileProgram(kVS, kFS);
EXPECT_EQ(0u, program);
}
// Verify I/O block array locations // Verify I/O block array locations
TEST_P(GLSLTest_ES31, IOBlockLocations) TEST_P(GLSLTest_ES31, IOBlockLocations)
{ {
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment