From 0eb5d54bb8f3461f3ae4ca97311c00e175b7d79c Mon Sep 17 00:00:00 2001 From: Swung0x48 Date: Sat, 22 Aug 2026 12:25:52 -0400 Subject: [PATCH] [Fix, Test] (GLState, ShaderTranspiler): report GL's default binding of zero for an unqualified uniform block --- .../GLState/ProgramState/ProgramLinkTask.cpp | 23 ++++++++- .../GLState/ProgramState/ProgramObject.cpp | 1 + .../GLState/ProgramState/ProgramObject.h | 11 ++++- .../ShaderTranspiler/GlslangCaptureTest.cpp | 48 +++++++++++++++++-- .../ShaderTranspiler/ShaderCompiler.cpp | 3 +- MobileGL/MG_Util/ShaderTranspiler/Types.h | 1 + .../glslang/TMglGlslIoResolver.cpp | 11 +++++ .../glslang/TMglGlslIoResolver.h | 18 +++++-- 8 files changed, 103 insertions(+), 13 deletions(-) diff --git a/MobileGL/MG_State/GLState/ProgramState/ProgramLinkTask.cpp b/MobileGL/MG_State/GLState/ProgramState/ProgramLinkTask.cpp index 1580e63a..8572e2f4 100644 --- a/MobileGL/MG_State/GLState/ProgramState/ProgramLinkTask.cpp +++ b/MobileGL/MG_State/GLState/ProgramState/ProgramLinkTask.cpp @@ -627,7 +627,8 @@ namespace MobileGL::MG_State::GLState { .explicitFragmentOutLocations = in.explicitFragDataLocation, .explicitFragmentOutIndices = in.explicitFragDataIndex, .explicitOpaqueUniformBindings = &artifacts.explicitOpaqueUniformBindings, - .storageBlocksWithoutBinding = &artifacts.storageBlocksWithoutBinding}; + .storageBlocksWithoutBinding = &artifacts.storageBlocksWithoutBinding, + .uniformBlocksWithoutBinding = &artifacts.uniformBlocksWithoutBinding}; MGLOG_D("ProgramObject %u: Calling ShaderCompiler::LinkProgram", in.externalIndex); auto result = ShaderCompiler::LinkProgram(attrib); @@ -1573,7 +1574,25 @@ namespace MobileGL::MG_State::GLState { // (DirectGLES.cpp / UniformManager.cpp), all 14 elements also read the same // buffer. This is the rule the storage-block path in ProgramInterface.cpp // already applies, and whose comment there claims uniform blocks follow. - const Int declaredBinding = ubo.getBinding(); + // + // "Declared" cannot be read back off the reflection, though. MobileGL asks glslang + // to auto-map bindings, so mapIO writes an invented one into every block's + // qualifier before reflection ever runs and ubo.getBinding() is never negative; + // worse, glslang packs uniform blocks into the SAME slot space as samplers and + // images (setEnvClient(EShClientVulkan) leaves spvVersion.openGl at 0, so + // TDefaultGlslIoResolver::resolveBinding keys every resource kind on set 0), so a + // block declared after an unbound image gets 1. GL 4.6 core 7.6.2 says an + // unqualified block reports ZERO. The set below is the shader's own answer, + // captured during mapIO while the qualifier still meant it - the same mechanism + // SeedDefaultStorageBlockBindings uses for storage blocks, and the aliasing at 0 + // that results is GL's, not a bug: unqualified blocks collide there until the + // application rebinds them. + // + // Only this GL-visible binding POINT changes. The backends' descriptor lookups run + // off glslang's assignment through uniformBlockIndexByBinding, which is untouched. + const String blockTypeName = StripArrayElementSuffix(ubo.name); + const Int declaredBinding = + artifacts.uniformBlocksWithoutBinding.contains(blockTypeName) ? 0 : ubo.getBinding(); artifacts.uniformBlockBinding[i] = declaredBinding < 0 ? declaredBinding : declaredBinding + BlockArrayElement(ubo.name); MGLOG_D("ProgramObject %u: Reflection - UBO[%d] name='%s' size=%u binding=%d", in.externalIndex, i, diff --git a/MobileGL/MG_State/GLState/ProgramState/ProgramObject.cpp b/MobileGL/MG_State/GLState/ProgramState/ProgramObject.cpp index c3cdbc25..85e27db1 100644 --- a/MobileGL/MG_State/GLState/ProgramState/ProgramObject.cpp +++ b/MobileGL/MG_State/GLState/ProgramState/ProgramObject.cpp @@ -369,6 +369,7 @@ namespace MobileGL::MG_State::GLState { // link, so a stale set would otherwise default a block the new sources do declare a // binding for. artifacts.storageBlocksWithoutBinding.clear(); + artifacts.uniformBlocksWithoutBinding.clear(); artifacts.attribs.clear(); artifacts.attribTypes.clear(); artifacts.activeUniformCount = 0; diff --git a/MobileGL/MG_State/GLState/ProgramState/ProgramObject.h b/MobileGL/MG_State/GLState/ProgramState/ProgramObject.h index 7104ab61..f73cd7e6 100644 --- a/MobileGL/MG_State/GLState/ProgramState/ProgramObject.h +++ b/MobileGL/MG_State/GLState/ProgramState/ProgramObject.h @@ -1024,8 +1024,10 @@ namespace MobileGL::MG_State::GLState { Uint32 GetBlockBindingVersion() const { return m_blockBindingVersion; } // Set by glUniformBlockBinding. The vector is seeded at link with each block's DECLARED - // binding (layout(binding=N), else -1), so an untouched program already reports what its - // shaders asked for. + // binding (layout(binding=N)), and with GL's default of 0 for a block that declared none + // - which the reflection cannot tell apart on its own, so the seeder consults + // uniformBlocksWithoutBinding. Either way an untouched program already reports what GL + // says it should. void SetUniformBlockBinding(Uint index, Uint binding) { if (index >= Artifacts().uniformBlockBinding.size() || Artifacts().uniformBlockBinding[index] == static_cast(binding)) { return; @@ -1287,6 +1289,11 @@ namespace MobileGL::MG_State::GLState { // binding from an invented one - and, unlike the per-shader lexer this replaced, // sees the declaration with its macros expanded. std::set storageBlocksWithoutBinding; + // The same list for UNIFORM blocks, and it is needed for the same reason: glslang's + // auto-mapper assigns every uniform block a binding whether or not the shader asked + // for one, so uniformBlockBinding below cannot tell "declared 1" from "invented 1". + // GL 4.6 core 7.6.2 requires an unqualified block to report ZERO. + std::set uniformBlocksWithoutBinding; Uint activeUniformCount = 0; Uint maxUniformLocation = 0; diff --git a/MobileGL/MG_Test/ShaderTranspiler/GlslangCaptureTest.cpp b/MobileGL/MG_Test/ShaderTranspiler/GlslangCaptureTest.cpp index 4f061d6b..2dbc03f5 100644 --- a/MobileGL/MG_Test/ShaderTranspiler/GlslangCaptureTest.cpp +++ b/MobileGL/MG_Test/ShaderTranspiler/GlslangCaptureTest.cpp @@ -123,6 +123,7 @@ namespace { String log; UnorderedMap opaqueBindings; std::set storageBlocksWithoutBinding; + std::set uniformBlocksWithoutBinding; UnorderedMap uniformLocations; }; @@ -145,6 +146,7 @@ namespace { if (captureEnabled) { programAttrib.explicitOpaqueUniformBindings = &capture.opaqueBindings; programAttrib.storageBlocksWithoutBinding = &capture.storageBlocksWithoutBinding; + programAttrib.uniformBlocksWithoutBinding = &capture.uniformBlocksWithoutBinding; } auto programResult = ShaderCompiler::LinkProgram(programAttrib); @@ -561,11 +563,49 @@ void main() { << "a declared binding must never be defaulted away"; EXPECT_EQ(capture.storageBlocksWithoutBinding.count("BoundFirst"), 0u) << "the binding may appear anywhere in the layout list, not only last"; - // A UNIFORM block is a different binding space with its own glUniformBlockBinding path, and - // its default is already handled where uniformBlockBinding is seeded. Naming it here would - // make the seeder default a resource it does not own. + // A UNIFORM block is a different binding space with its own glUniformBlockBinding path, so it + // must not reach the storage-block seeder - it has a capture set of its own (see + // UnqualifiedUniformBlocksAreCapturedSeparatelyFromStorageBlocks below). EXPECT_EQ(capture.storageBlocksWithoutBinding.count("InputBuffer"), 0u) - << "uniform blocks are out of scope"; + << "uniform blocks belong to the other set"; +} + +// The uniform-block half of the same capture, and the reason it exists: glslang packs uniform +// blocks into the same auto-mapped slot space as samplers and images, so an unqualified block +// declared AFTER an unbound image comes back carrying binding 1 while GL 4.6 core 7.6.2 requires +// it to report 0. Reflection cannot tell the invented number from a declared one, so the shader's +// own answer has to be captured here, during mapIO, and applied at reflection time. +// KHR-GL4{2,3}.shading_language_420pack.binding_uniform_default is exactly this shader shape. +TEST_F(GlslangCaptureProbeTest, UnqualifiedUniformBlocksAreCapturedSeparatelyFromStorageBlocks) { + const String source = R"(#version 430 core +layout(local_size_x = 1) in; +writeonly uniform image2D uni_image; +layout(std140) uniform GOKU { vec4 gohan; vec4 goten; } goku; +layout(std140, binding = 3) uniform VEGETA { vec4 trunks; } vegeta; +layout(std430) buffer OutputBuffer { vec4 data0[]; } g_out_buffer; +void main() { + g_out_buffer.data0[0] = goku.gohan + goku.goten + vegeta.trunks; + imageStore(uni_image, ivec2(0), vec4(1.0)); +} +)"; + + const LinkCapture capture = CaptureFromCompute(source); + ASSERT_TRUE(capture.linked) << capture.log; + + EXPECT_EQ(capture.uniformBlocksWithoutBinding.count("GOKU"), 1u) + << "an unqualified uniform block declared after an unbound image is the regressing shape"; + EXPECT_EQ(capture.uniformBlocksWithoutBinding.count("VEGETA"), 0u) + << "a declared binding must never be defaulted away"; + EXPECT_EQ(capture.uniformBlocksWithoutBinding.count("OutputBuffer"), 0u) + << "storage blocks belong to the other set"; + EXPECT_EQ(capture.storageBlocksWithoutBinding.count("GOKU"), 0u) + << "the two sets must not cross-contaminate"; + + // The negative control every capture case here carries: with the OUT pointer left null the + // resolver must write nothing at all. + const LinkCapture off = CaptureFromCompute(source, /*captureEnabled=*/false); + ASSERT_TRUE(off.linked) << off.log; + EXPECT_TRUE(off.uniformBlocksWithoutBinding.empty()); } // The capture must not mistake a buffer-typed SAMPLER or a member qualifier for a block, and diff --git a/MobileGL/MG_Util/ShaderTranspiler/ShaderCompiler.cpp b/MobileGL/MG_Util/ShaderTranspiler/ShaderCompiler.cpp index 04418a84..f0f2455c 100644 --- a/MobileGL/MG_Util/ShaderTranspiler/ShaderCompiler.cpp +++ b/MobileGL/MG_Util/ShaderTranspiler/ShaderCompiler.cpp @@ -486,7 +486,8 @@ namespace MobileGL { attrib.explicitFragmentOutLocations, attrib.explicitFragmentOutIndices, attrib.explicitOpaqueUniformBindings, - attrib.storageBlocksWithoutBinding); + attrib.storageBlocksWithoutBinding, + attrib.uniformBlocksWithoutBinding); break; } auto ioMapper = UniquePtr(glslang::GetGlslIoMapper()); diff --git a/MobileGL/MG_Util/ShaderTranspiler/Types.h b/MobileGL/MG_Util/ShaderTranspiler/Types.h index 1269ce2e..13c628be 100644 --- a/MobileGL/MG_Util/ShaderTranspiler/Types.h +++ b/MobileGL/MG_Util/ShaderTranspiler/Types.h @@ -76,6 +76,7 @@ namespace MobileGL { // assigned - see the comment on TMglGlslIoResolver::reserverResourceSlot. UnorderedMap* explicitOpaqueUniformBindings = nullptr; std::set* storageBlocksWithoutBinding = nullptr; + std::set* uniformBlocksWithoutBinding = nullptr; }; struct ProgramBinaryAttrib { diff --git a/MobileGL/MG_Util/ShaderTranspiler/glslang/TMglGlslIoResolver.cpp b/MobileGL/MG_Util/ShaderTranspiler/glslang/TMglGlslIoResolver.cpp index 7febc3f0..0bfec581 100644 --- a/MobileGL/MG_Util/ShaderTranspiler/glslang/TMglGlslIoResolver.cpp +++ b/MobileGL/MG_Util/ShaderTranspiler/glslang/TMglGlslIoResolver.cpp @@ -196,6 +196,17 @@ namespace MobileGL { m_storageBlocksWithoutBinding->insert(name.c_str()); } + // A UNIFORM block that declared no binding. Same capture point and same union-across- + // stages reasoning as the storage-block set above, and the same reason it cannot be + // asked later: mapIO is about to write an auto-assigned binding into this very + // qualifier. MGL_GLOBAL_UBO is MobileGL's own synthesized block, not an application + // one - it never reaches the GL block space and must not be seeded here. + if (m_uniformBlocksWithoutBinding != nullptr && type.getBasicType() == glslang::EbtBlock && + qualifier.storage == glslang::EvqUniform && !qualifier.hasBinding() && + name.compare(MG_Util::ShaderTranspiler::GLOBAL_UBO_NAME) != 0) { + m_uniformBlocksWithoutBinding->insert(name.c_str()); + } + TDefaultGlslIoResolver::reserverResourceSlot(ent, infoSink); } diff --git a/MobileGL/MG_Util/ShaderTranspiler/glslang/TMglGlslIoResolver.h b/MobileGL/MG_Util/ShaderTranspiler/glslang/TMglGlslIoResolver.h index d4bd8dad..d8427327 100644 --- a/MobileGL/MG_Util/ShaderTranspiler/glslang/TMglGlslIoResolver.h +++ b/MobileGL/MG_Util/ShaderTranspiler/glslang/TMglGlslIoResolver.h @@ -29,16 +29,19 @@ namespace MobileGL { TMglGlslIoResolver(const glslang::TIntermediate& intermediate, const ExplicitVarSlotMap& vertexIns, const ExplicitVarSlotMap& fragOuts, const ExplicitVarSlotMap& fragOutIndices, ExplicitVarSlotMap* opaqueUniformBindings, - std::set* storageBlocksWithoutBinding = nullptr) + std::set* storageBlocksWithoutBinding = nullptr, + std::set* uniformBlocksWithoutBinding = nullptr) : TDefaultGlslIoResolver(intermediate), m_explicitVertexIns(vertexIns), m_explicitFragOuts(fragOuts), m_explicitFragOutIndices(fragOutIndices), m_explicitOpaqueUniformBindings(opaqueUniformBindings), - m_storageBlocksWithoutBinding(storageBlocksWithoutBinding) {} + m_storageBlocksWithoutBinding(storageBlocksWithoutBinding), + m_uniformBlocksWithoutBinding(uniformBlocksWithoutBinding) {} TMglGlslIoResolver(const glslang::TProgram& program, const EShLanguage stage, const ExplicitVarSlotMap& vertexIns, const ExplicitVarSlotMap& fragOuts, const ExplicitVarSlotMap& fragOutIndices, ExplicitVarSlotMap* opaqueUniformBindings, - std::set* storageBlocksWithoutBinding = nullptr) + std::set* storageBlocksWithoutBinding = nullptr, + std::set* uniformBlocksWithoutBinding = nullptr) : TMglGlslIoResolver(*program.getIntermediate(stage), vertexIns, fragOuts, fragOutIndices, - opaqueUniformBindings, storageBlocksWithoutBinding) {} + opaqueUniformBindings, storageBlocksWithoutBinding, uniformBlocksWithoutBinding) {} void reserverStorageSlot(glslang::TVarEntryInfo& ent, TInfoSink& infoSink) override; void reserverResourceSlot(glslang::TVarEntryInfo& ent, TInfoSink& infoSink) override; int resolveInOutLocation(EShLanguage stage, glslang::TVarEntryInfo& ent) override; @@ -62,6 +65,13 @@ namespace MobileGL { // layout(binding = N). GL 4.3 core 7.8 gives such a block binding ZERO; see // ProgramLinkTask::SeedDefaultStorageBlockBindings for what is done with them. std::set* m_storageBlocksWithoutBinding = nullptr; + // The same capture for UNIFORM blocks. GL 4.6 core 7.6.2 gives an unqualified uniform + // block binding ZERO, and glslang's auto-mapper does not: it packs uniform blocks into + // the same slot space as samplers and images (spvVersion.openGl is 0 under + // setEnvClient(EShClientVulkan), so TDefaultGlslIoResolver::resolveBinding keys every + // resource kind on set 0), so an unbound block declared after an unbound image lands on + // 1. See ProgramLinkTask's UBO reflection loop for what is done with them. + std::set* m_uniformBlocksWithoutBinding = nullptr; std::map m_plainUniformLocationSizeByName; std::map m_plainUniformLocationByName; bool m_plainUniformLocationsAssigned = false;