From 26f02567d7790b300c5213d8d3982da33de88839 Mon Sep 17 00:00:00 2001 From: Swung0x48 Date: Thu, 20 Aug 2026 13:39:38 -0400 Subject: [PATCH] [Fix, Test] (GLState, GLImpl): reserve an inactive uniform's explicit location and pin the link to GL_MAX_UNIFORM_LOCATIONS --- MobileGL/MG_Impl/GLImpl/Getter/GL_Getter.cpp | 3 +- .../GLState/ProgramState/ProgramLinkTask.cpp | 111 ++++++++++++++++-- .../GLState/ProgramState/ProgramObject.h | 14 +++ MobileGL/MG_Test/Program/ProgramTest.cpp | 73 ++++++++++++ 4 files changed, 193 insertions(+), 8 deletions(-) diff --git a/MobileGL/MG_Impl/GLImpl/Getter/GL_Getter.cpp b/MobileGL/MG_Impl/GLImpl/Getter/GL_Getter.cpp index 4be84f38..692ead4e 100644 --- a/MobileGL/MG_Impl/GLImpl/Getter/GL_Getter.cpp +++ b/MobileGL/MG_Impl/GLImpl/Getter/GL_Getter.cpp @@ -1692,7 +1692,8 @@ namespace MobileGL::MG_Impl::GLImpl { *params = 15; // TODO return; case GL_MAX_UNIFORM_LOCATIONS: - *params = 1024 * 4; // TODO + // The same constant the link's location allocator enforces - see ProgramObject. + *params = MG_State::GLState::ProgramObject::MAX_UNIFORM_LOCATIONS; return; case GL_MAX_VARYING_COMPONENTS: *params = kFrontendMaxVaryingComponents; diff --git a/MobileGL/MG_State/GLState/ProgramState/ProgramLinkTask.cpp b/MobileGL/MG_State/GLState/ProgramState/ProgramLinkTask.cpp index c541a62a..071a4846 100644 --- a/MobileGL/MG_State/GLState/ProgramState/ProgramLinkTask.cpp +++ b/MobileGL/MG_State/GLState/ProgramState/ProgramLinkTask.cpp @@ -848,7 +848,16 @@ namespace MobileGL::MG_State::GLState { // MGL_GLOBAL_UBO, so reflection cannot provide them ("source-explicit"); // - glslang's layoutLocation() for opaque uniforms, where the qualifier // survives the relaxed parse (and mapIO auto-assigns the rest). - constexpr Uint kNoLocation = glslang::TQualifier::layoutLocationEnd; + // + // "no effective location yet". Deliberately OUTSIDE the location space rather than + // glslang::TQualifier::layoutLocationEnd, which is the first location past the pool and + // therefore only one off a legal one - a sentinel that sits at the boundary it guards has + // to be re-proved safe every time the ceiling moves, and glslang uses that same value for + // "this opaque uniform has no location" as well. + constexpr Uint kNoLocation = ~static_cast(0); + // The ceiling glGetIntegerv(GL_MAX_UNIFORM_LOCATIONS) advertises, which is what the + // allocator below has to honour: locations 0..kMaxUniformLocations-1 and no others. + constexpr Uint kMaxUniformLocations = static_cast(ProgramObject::MAX_UNIFORM_LOCATIONS); Vector effectiveLocation(tProgramUniformCount, kNoLocation); Vector locationIsSourceExplicit(tProgramUniformCount, false); UnorderedMap structExplicitCursor; // declared root -> next member location @@ -885,13 +894,19 @@ namespace MobileGL::MG_State::GLState { cursor->second += static_cast(GetUniformLocationSpan(uniform)); } } - if (effectiveLocation[i] == kNoLocation && type != nullptr && type->isOpaque()) { + // glslang parks "no location" at layoutLocationEnd, which is a real location in this + // table's numbering - test for it explicitly rather than letting it through as one. + if (effectiveLocation[i] == kNoLocation && type != nullptr && type->isOpaque() && + uniform.layoutLocation() != glslang::TQualifier::layoutLocationEnd) { effectiveLocation[i] = uniform.layoutLocation(); } if (locationIsSourceExplicit[i] && - effectiveLocation[i] + static_cast(GetUniformLocationSpan(uniform)) > kNoLocation) { + effectiveLocation[i] + static_cast(GetUniformLocationSpan(uniform)) > kMaxUniformLocations) { // Config A rejected out-of-range explicit locations at parse; keep them - // from growing the location table unboundedly. + // from growing the location table unboundedly. Stated against the advertised + // GL_MAX_UNIFORM_LOCATIONS, because that is the rule being enforced (GL 4.6 core + // 7.6.1): an array whose LAST element passes the ceiling is a link error even + // though its base compiled fine. artifacts.infoLog = std::format("Uniform '{}' explicit location {} is out of range.", uniform.name, effectiveLocation[i]); ProgramObject::ResetLinkArtifacts(artifacts); @@ -899,12 +914,55 @@ namespace MobileGL::MG_State::GLState { } } - Int requiredUniformLocations = 0; + // ARB_explicit_uniform_location / GL 4.6 core 7.6.1: an explicit location is RESERVED + // whether or not the uniform turned out to be active. The dead default-block uniforms + // filtered out of glUniformIndexToTProgram above are invisible to every GL query - which + // is correct - but their locations must still be kept out of the implicit allocator's + // reach, or an implicit uniform is handed a location the source already claimed. + // + // Deliberately NOT written into artifacts.uniformLocations or uniformIndexInTProgram: + // glGetUniformLocation must keep answering -1 for a dead uniform, and a location no + // application can legally obtain must not become writable through glUniform*. The + // occupancy therefore lives in its own bitset, built once the table has been sized. + Vector> deadExplicitReservations; + Int deadReservedLocationCount = 0; + for (Int i = 0; i < tProgramUniformCount; i++) { + if (artifacts.tProgramUniformIndexToGl[i] >= 0) continue; // GL-visible: handled above + const auto& uniform = artifacts.program->getUniform(i); + if (!isGlobalUboMember(uniform) || uniform.stages != 0) continue; + const Int* explicitLocation = findExplicitLocation(uniform.name); + if (explicitLocation == nullptr) continue; + + const Uint location = static_cast(*explicitLocation); + const Int locationSpan = GetUniformLocationSpan(uniform); + if (location + static_cast(locationSpan) > kMaxUniformLocations) { + artifacts.infoLog = std::format("Uniform '{}' explicit location {} is out of range.", uniform.name, + location); + ProgramObject::ResetLinkArtifacts(artifacts); + return false; + } + deadExplicitReservations.emplace_back(location, locationSpan); + deadReservedLocationCount += locationSpan; + artifacts.maxUniformLocation = std::max(artifacts.maxUniformLocation, location + locationSpan - 1); + MGLOG_D("ProgramObject %u: Reflection - inactive uniform '%s' reserves locations %u..%u without " + "becoming GL-visible", + in.externalIndex, uniform.name.c_str(), location, location + locationSpan - 1); + } + + Int requiredUniformLocations = deadReservedLocationCount; + // The same count restricted to DEFAULT-BLOCK uniforms, which is the only thing + // GL_MAX_UNIFORM_LOCATIONS bounds. requiredUniformLocations cannot serve: it also carries + // named-block members, which take a slot in this allocator's table (an implementation + // detail) but consume no GL uniform location at all, so a big UBO array would otherwise + // fail a link the spec allows. + Int defaultBlockLocationDemand = deadReservedLocationCount; for (const Int i : artifacts.glUniformIndexToTProgram) { auto& uniform = artifacts.program->getUniform(i); const Uint location = effectiveLocation[i]; const Int locationSpan = GetUniformLocationSpan(uniform); requiredUniformLocations += locationSpan; + const Bool inNamedBlock = uniform.index >= 0 && !isGlobalUboMember(uniform); + if (!inNamedBlock) defaultBlockLocationDemand += locationSpan; if (location != kNoLocation) { artifacts.maxUniformLocation = std::max(artifacts.maxUniformLocation, location + locationSpan - 1); } @@ -917,6 +975,22 @@ namespace MobileGL::MG_State::GLState { MGLOG_D("ProgramObject %u: Reflection - computed maxUniformLocation=%u uniformNameMaxLength=%d", in.externalIndex, artifacts.maxUniformLocation, artifacts.uniformNameMaxLength); + // GL 4.6 core 7.6.1: explicit, implicit and reserved-but-inactive default-block uniforms + // all draw from the one GL_MAX_UNIFORM_LOCATIONS pool, and a program asking for more than + // the implementation advertises FAILS TO LINK + // (KHR-GL43.explicit_uniform_location.uniform-loc-negative-link-max-num-of-locations). + // A single uniform whose own span passes the ceiling was already rejected above; this is + // the aggregate half of the same rule. + if (defaultBlockLocationDemand > static_cast(kMaxUniformLocations)) { + artifacts.infoLog = + std::format("Uniform locations exhausted: the default-block uniforms need {} locations but " + "GL_MAX_UNIFORM_LOCATIONS is {}.", + defaultBlockLocationDemand, kMaxUniformLocations); + DeferLog(std::format("ProgramObject {}: Link failed - {}", in.externalIndex, artifacts.infoLog)); + ProgramObject::ResetLinkArtifacts(artifacts); + return false; + } + if (artifacts.maxUniformLocation + 1 < requiredUniformLocations) { MGLOG_D("ProgramObject %u: Reflection - maxUniformLocation+1 (%u) < requiredUniformLocations (%d), " "adjusting", @@ -931,6 +1005,27 @@ namespace MobileGL::MG_State::GLState { glslang::TQualifier::layoutLocationEnd); artifacts.uniformSamplerOrImageUnitIndex.resize(artifacts.maxUniformLocation + 1, -1); + // Occupancy for the inactive explicit uniforms collected above: a set bit means "the + // source claimed this location", which is enough to keep the two implicit passes off it + // without making the location reachable through any GL entry point. A location the + // fallback grow path mints later is past this bitset by construction (every reservation + // was folded into maxUniformLocation before the table was sized), so the lookup treats + // out-of-range as free rather than resizing in lockstep. + // Left empty - and unallocated - when nothing reserved anything, which is every program in + // the shader-pack corpus; the lookup below reads an empty bitset as "nothing is reserved". + Vector reservedLocation; + if (!deadExplicitReservations.empty()) { + reservedLocation.assign(artifacts.maxUniformLocation + 1, false); + for (const auto& [reservedBase, reservedSpan] : deadExplicitReservations) { + for (Int element = 0; element < reservedSpan; ++element) { + reservedLocation[reservedBase + element] = true; + } + } + } + const auto locationIsReserved = [&reservedLocation](SizeT location) { + return location < reservedLocation.size() && reservedLocation[location]; + }; + Vector unallocatedUniformIndex; // Pass 1: source-explicit locations. These are API contract @@ -975,7 +1070,8 @@ namespace MobileGL::MG_State::GLState { Bool spanIsFree = location + locationSpan - 1 <= artifacts.maxUniformLocation; for (Int element = 0; spanIsFree && element < locationSpan; ++element) { spanIsFree = - artifacts.uniformIndexInTProgram[location + element] == glslang::TQualifier::layoutLocationEnd; + artifacts.uniformIndexInTProgram[location + element] == glslang::TQualifier::layoutLocationEnd && + !locationIsReserved(location + element); } if (!spanIsFree) { artifacts.uniformLocations[uniform.name] = kNoLocation; @@ -1007,7 +1103,8 @@ namespace MobileGL::MG_State::GLState { bool hasRoom = locNeedle + locationSpan - 1 <= artifacts.maxUniformLocation; for (Int element = 0; hasRoom && element < locationSpan; ++element) { hasRoom = artifacts.uniformIndexInTProgram[locNeedle + element] == - glslang::TQualifier::layoutLocationEnd; + glslang::TQualifier::layoutLocationEnd && + !locationIsReserved(locNeedle + element); } if (!hasRoom) continue; // Found a vacant location at locNeedle diff --git a/MobileGL/MG_State/GLState/ProgramState/ProgramObject.h b/MobileGL/MG_State/GLState/ProgramState/ProgramObject.h index b8c2bdca..2271b8b4 100644 --- a/MobileGL/MG_State/GLState/ProgramState/ProgramObject.h +++ b/MobileGL/MG_State/GLState/ProgramState/ProgramObject.h @@ -24,6 +24,20 @@ namespace MobileGL::MG_State::GLState { class ProgramObject { public: + // GL_MAX_UNIFORM_LOCATIONS: locations 0 .. MAX_UNIFORM_LOCATIONS-1 are the whole legal + // range (GL 4.6 core 7.6.1 / ARB_explicit_uniform_location). Shared with GL_Getter rather + // than spelled twice, because the link and the query must agree exactly - the CTS declares + // a uniform at the advertised value minus one and expects it to link + // (KHR-GL43.explicit_uniform_location.uniform-loc-max). + // + // Tied to glslang's own ceiling and NOT raisable past it: ParseHelper rejects + // `layout(location = N)` for N >= TQualifier::layoutLocationEnd at COMPILE time, so + // layoutLocationEnd - 1 is the largest location any shader in this stack can declare - + // which makes exactly layoutLocationEnd locations, 0 .. layoutLocationEnd - 1, the pool. + // Advertising more would promise a location no shader could name. Comfortably above the + // 1024 GL 4.3 requires. + static constexpr Int MAX_UNIFORM_LOCATIONS = static_cast(glslang::TQualifier::layoutLocationEnd); + ProgramObject(Uint externalIndex) : m_externalIndex(externalIndex), m_lifetimeId(AllocateLifetimeId()) {} // Cancel-not-join, exactly like ~ShaderObject: the link job owns its inputs, so an // in-flight link whose program just went away is safe to abandon where it stands. diff --git a/MobileGL/MG_Test/Program/ProgramTest.cpp b/MobileGL/MG_Test/Program/ProgramTest.cpp index eeb37f11..faf5c84b 100644 --- a/MobileGL/MG_Test/Program/ProgramTest.cpp +++ b/MobileGL/MG_Test/Program/ProgramTest.cpp @@ -3239,3 +3239,76 @@ TEST_F(ProgramTest, CreateShaderAndCreateShaderProgramvReportTheRightErrorClasse EXPECT_NE(program, 0u); EXPECT_EQ(GetError(), GL_NO_ERROR); } + +// ARB_explicit_uniform_location / GL 4.6 core 7.6.1: a `layout(location = N)` uniform reserves N +// EVEN WHEN IT IS INACTIVE. Dead default-block uniforms are correctly filtered off the GL surface +// (glGetUniformLocation must answer -1 for them), but the implicit allocator used to walk straight +// over the location they claimed and hand it to a uniform that never asked for it +// (KHR-GL43.explicit_uniform_location.uniform-loc-mix-with-implicit3). +TEST_F(ProgramTest, InactiveExplicitUniformLocationIsStillReserved) { + const char* vsSource = R"(#version 430 core +layout(location = 2) uniform vec4 uDeadAtTwo; +uniform vec4 uA; +uniform vec4 uB; +uniform vec4 uC; +uniform vec4 uD; +void main() { gl_Position = uA + uB + uC + uD; } +)"; + const char* fsSource = R"(#version 430 core +out vec4 fragColor; +void main() { fragColor = vec4(1.0); } +)"; + const GLuint vs = CompileShaderChecked(GL_VERTEX_SHADER, vsSource); + const GLuint fs = CompileShaderChecked(GL_FRAGMENT_SHADER, fsSource); + const GLuint program = LinkVsFs(vs, fs, GL_TRUE); + + // Reserving a location must not resurrect the uniform: it is still inactive to GL. + EXPECT_EQ(GetUniformLocation(program, "uDeadAtTwo"), -1); + + for (const char* name : {"uA", "uB", "uC", "uD"}) { + const GLint location = GetUniformLocation(program, name); + EXPECT_GE(location, 0) << name << " lost its implicit location"; + EXPECT_NE(location, 2) << name << " was handed the location uDeadAtTwo reserved"; + } + EXPECT_EQ(GetError(), GL_NO_ERROR); +} + +// The GL_MAX_UNIFORM_LOCATIONS boundary, from both sides. MAX_UNIFORM_LOCATIONS - 1 is the LAST +// LEGAL location: it has to link and read back verbatim +// (KHR-GL43.explicit_uniform_location.uniform-loc-max), which is only true while the advertised +// value and what the link accepts are the SAME number - the getter used to advertise one more +// location than any shader could name. +// +// The over-the-ceiling half is asserted through an ARRAY, because that is the only spelling the +// link gets to judge: a bare `layout(location = MAX)` is already a compile error inside glslang +// ("location is too large"), while an array's base compiles fine and only its last element passes +// the ceiling (...uniform-loc-negative-link-max-num-of-locations). +TEST_F(ProgramTest, ExplicitUniformLocationsHonourMaxUniformLocations) { + GLint maxLocations = 0; + GetIntegerv(GL_MAX_UNIFORM_LOCATIONS, &maxLocations); + ASSERT_GE(maxLocations, 1024) << "GL 4.3 requires at least 1024 uniform locations"; + + const char* fsSource = R"(#version 430 core +out vec4 fragColor; +void main() { fragColor = vec4(1.0); } +)"; + const GLuint fs = CompileShaderChecked(GL_FRAGMENT_SHADER, fsSource); + + { + const String source = String("#version 430 core\nlayout(location = ") + + std::to_string(maxLocations - 1) + + ") uniform vec4 uAtLimit;\nvoid main() { gl_Position = uAtLimit; }\n"; + const GLuint vs = CompileShaderChecked(GL_VERTEX_SHADER, source.c_str()); + const GLuint program = LinkVsFs(vs, fs, GL_TRUE); + EXPECT_EQ(GetUniformLocation(program, "uAtLimit"), maxLocations - 1) + << "the last location in the pool is legal and must come back verbatim"; + } + { + const String source = String("#version 430 core\nlayout(location = ") + + std::to_string(maxLocations - 4) + + ") uniform vec4 uSpill[8];\nvoid main() { gl_Position = uSpill[0]; }\n"; + const GLuint vs = CompileShaderChecked(GL_VERTEX_SHADER, source.c_str()); + (void)LinkVsFs(vs, fs, GL_FALSE); + } + EXPECT_EQ(GetError(), GL_NO_ERROR); +}