From c129cdec2d65b76fd4f99cb846edb81588007229 Mon Sep 17 00:00:00 2001 From: Swung0x48 Date: Fri, 21 Aug 2026 00:10:53 -0400 Subject: [PATCH] [Fix, Test] (ShaderTranspiler, ProgramState): reject an out-of-range atomic-counter offset at compile --- .../ProgramState/ShaderCompileTask.cpp | 7 ++ .../ProgramState/ShaderPreprocessCache.h | 3 + MobileGL/MG_Test/Program/ProgramUtilTest.cpp | 43 +++++++++ .../ShaderSourceProcessor.cpp | 87 +++++++++++++++++++ .../ShaderTranspiler/ShaderSourceProcessor.h | 12 +++ 5 files changed, 152 insertions(+) diff --git a/MobileGL/MG_State/GLState/ProgramState/ShaderCompileTask.cpp b/MobileGL/MG_State/GLState/ProgramState/ShaderCompileTask.cpp index 41fdeb37..6e458c03 100644 --- a/MobileGL/MG_State/GLState/ProgramState/ShaderCompileTask.cpp +++ b/MobileGL/MG_State/GLState/ProgramState/ShaderCompileTask.cpp @@ -194,6 +194,13 @@ namespace { return result; } + if (const std::optional counterOffsetError = + FindAtomicCounterOffsetViolation(result.preprocessedSource)) { + result.outcome = ShaderPreprocessOutcome::AtomicCounterOffsetRejected; + result.infoLog = *counterOffsetError; + return result; + } + // The parse this feeds runs in the link-compatible configuration (Vulkan-client // env with relaxed rules): the TShader it produces is what glLinkProgram links and // what the backends' SPIR-V is generated from - there is no second, GL-client diff --git a/MobileGL/MG_State/GLState/ProgramState/ShaderPreprocessCache.h b/MobileGL/MG_State/GLState/ProgramState/ShaderPreprocessCache.h index 238ff7ba..55ec4465 100644 --- a/MobileGL/MG_State/GLState/ProgramState/ShaderPreprocessCache.h +++ b/MobileGL/MG_State/GLState/ProgramState/ShaderPreprocessCache.h @@ -29,6 +29,9 @@ namespace MobileGL::MG_State::GLState { // FindShaderStorageBindingViolation rejected it: a storage block declared a binding at or // past GL_MAX_SHADER_STORAGE_BUFFER_BINDINGS. ResourceBindingRejected, + // FindAtomicCounterOffsetViolation rejected it: an atomic counter declared a + // layout(offset =) that is misaligned or reaches past GL_MAX_ATOMIC_COUNTER_BUFFER_SIZE. + AtomicCounterOffsetRejected, // The source-only half was clean but glslang rejected the preprocessed source. // Memoizing this saves the parse itself on every later object with that source. ParseFailed, diff --git a/MobileGL/MG_Test/Program/ProgramUtilTest.cpp b/MobileGL/MG_Test/Program/ProgramUtilTest.cpp index 0505a03d..7478bce6 100644 --- a/MobileGL/MG_Test/Program/ProgramUtilTest.cpp +++ b/MobileGL/MG_Test/Program/ProgramUtilTest.cpp @@ -4064,6 +4064,49 @@ TEST_F(ProgramUtilTest, StorageBlockBindingCeilingIsCheckedAtItsExactBoundary) { EXPECT_FALSE(FindShaderStorageBindingViolation("layout(binding = 36) buffer B { int x; };\n", 0).has_value()); } +// KHR-GL43.shader_atomic_counters.negative-offset-1: an atomic counter whose layout(offset = N) +// puts its last byte past GL_MAX_ATOMIC_COUNTER_BUFFER_SIZE is a COMPILE-time error, and the CTS +// never links the shader at all. MobileGL only had the rule at link, because the Vulkan-relaxed +// parse never reaches glslang's fixOffset(). +TEST_F(ProgramUtilTest, AtomicCounterOffsetCeilingIsCheckedAtCompile) { + using namespace MG_Util::ShaderTranspiler; + + const auto violation = [](const String& body) { + return FindAtomicCounterOffsetViolation("#version 430 core\n" + body + "void main() {}\n"); + }; + const String maxSize = std::to_string(MAX_ATOMIC_COUNTER_BUFFER_SIZE); + const String lastLegal = std::to_string(MAX_ATOMIC_COUNTER_BUFFER_SIZE - 4); + + // The boundary itself: the last counter that still fits, and the first that does not. + EXPECT_FALSE(violation("layout(binding = 0, offset = " + lastLegal + ") uniform atomic_uint c;\n").has_value()); + EXPECT_TRUE(violation("layout(binding = 0, offset = " + maxSize + ") uniform atomic_uint c;\n").has_value()); + + // An array occupies one word per element, so what has to fit is the LAST one. + EXPECT_FALSE(violation("layout(offset = " + std::to_string(MAX_ATOMIC_COUNTER_BUFFER_SIZE - 16) + + ") uniform atomic_uint c[4];\n") + .has_value()); + EXPECT_TRUE(violation("layout(offset = " + std::to_string(MAX_ATOMIC_COUNTER_BUFFER_SIZE - 8) + + ") uniform atomic_uint c[4];\n") + .has_value()); + + // An offset that is not a multiple of 4 (GL 4.6 core 7.7), and one that is. + EXPECT_TRUE(violation("layout(offset = 2) uniform atomic_uint c;\n").has_value()); + EXPECT_FALSE(violation("layout(offset = 8) uniform atomic_uint c;\n").has_value()); + + // Things the scanner must NOT judge: a counter with no explicit offset, an `offset` that is + // an ordinary identifier rather than a layout qualifier, an array sized by an expression, + // and an offset qualifier that belongs to a different declaration. + EXPECT_FALSE(violation("uniform atomic_uint c;\nconst int offset = 99999;\n").has_value()); + EXPECT_FALSE(violation("const int kCount = 4;\nlayout(offset = " + maxSize + + ") uniform atomic_uint c[kCount];\n") + .has_value()); + EXPECT_FALSE(violation("layout(offset = " + maxSize + ") uniform Block { int x; };\n" + "uniform atomic_uint c;\n") + .has_value()); + // A source with no counter at all never pays for the scan and never reports one. + EXPECT_FALSE(FindAtomicCounterOffsetViolation("#version 430 core\nvoid main() {}\n").has_value()); +} + // KHR-GL43.explicit_uniform_location.uniform-loc-nondecimal: GLSL integer literals are C-style, so // layout(location = 0xA) is 10 and layout(location = 010) is OCTAL 8. The extractor used to accept // a base-10 digit run and nothing else: the hex spelling failed the test entirely and the diff --git a/MobileGL/MG_Util/ShaderTranspiler/ShaderSourceProcessor.cpp b/MobileGL/MG_Util/ShaderTranspiler/ShaderSourceProcessor.cpp index 1b15eb1e..6b1502f3 100644 --- a/MobileGL/MG_Util/ShaderTranspiler/ShaderSourceProcessor.cpp +++ b/MobileGL/MG_Util/ShaderTranspiler/ShaderSourceProcessor.cpp @@ -18,6 +18,7 @@ #include #include #include +#include #include "EsslBuiltinFunctionNames.h" @@ -1603,6 +1604,92 @@ namespace MobileGL { return std::nullopt; } + std::optional FindAtomicCounterOffsetViolation(const String& source) { + // Fast path: both keywords are required for a violation to exist, and the pair is + // absent from every shader-pack source. + if (source.find("atomic_uint") == String::npos || source.find("offset") == String::npos) { + return std::nullopt; + } + + constexpr long long kAtomicCounterSize = 4; // one 32-bit word per counter + const long long maxBufferSize = static_cast(MAX_ATOMIC_COUNTER_BUFFER_SIZE); + const Vector tokens = TokenizeCode(source); + const SizeT count = tokens.size(); + // The offset the qualifier run currently being scanned declared, -1 for none. + // Same accumulate-then-consume shape as the storage-binding scan above. + long long offset = -1; + long long literal = 0; + for (SizeT pos = 0; pos < count; ++pos) { + const String& text = tokens[pos].text; + if (text == "layout" && pos + 1 < count && tokens[pos + 1].text == "(") { + SizeT j = pos + 2; + Int parenDepth = 1; + while (j < count && parenDepth > 0) { + const String& layoutToken = tokens[j].text; + if (layoutToken == "(") { + ++parenDepth; + } else if (layoutToken == ")") { + --parenDepth; + } else if (parenDepth == 1 && layoutToken == "offset" && j + 2 < count && + tokens[j + 1].text == "=" && + ParseGlslIntegerLiteral(tokens[j + 2].text, literal)) { + offset = literal; + j += 2; + } + ++j; + } + pos = j - 1; + continue; + } + if (text == "atomic_uint") { + // How far the declaration reaches: `atomic_uint c[N]` occupies N words + // from the offset. An unparsable or absent declarator (an expression-sized + // array, or the "layout(...) uniform atomic_uint;" default-qualifier form, + // which declares no counter at all) is left alone rather than guessed at - + // over-rejection here would be a compile failure the application cannot + // work around. + long long elements = 1; + SizeT k = pos + 1; + if (k < count && IsIdentifierToken(tokens[k])) { + ++k; + if (k < count && tokens[k].text == "[") { + elements = (k + 2 < count && tokens[k + 2].text == "]" && + ParseGlslIntegerLiteral(tokens[k + 1].text, literal)) + ? std::max(1, literal) + : -1; + } + } else { + elements = -1; + } + // Clamped so the byte arithmetic below cannot overflow on an absurd + // literal; any element count at or past the ceiling already fails. + elements = std::min(elements, maxBufferSize); + + if (offset >= 0 && elements > 0) { + if (offset % kAtomicCounterSize != 0) { + return "ERROR: invalid value " + std::to_string(offset) + + " for layout specifier 'offset': an atomic counter offset must be a " + "multiple of 4."; + } + if (offset > maxBufferSize - elements * kAtomicCounterSize) { + return "ERROR: invalid value " + std::to_string(offset) + + " for layout specifier 'offset': an atomic counter ending at byte " + + std::to_string(offset + elements * kAtomicCounterSize) + + " passes GL_MAX_ATOMIC_COUNTER_BUFFER_SIZE (" + + std::to_string(maxBufferSize) + ")."; + } + } + offset = -1; + continue; + } + // `uniform` and the precision/auxiliary qualifiers may sit between the layout + // list and the type keyword; anything else ends the run, so an offset never + // leaks onto an unrelated declaration. + if (text != "uniform" && !IsNonLayoutQualifierKeyword(text)) offset = -1; + } + return std::nullopt; + } + UnorderedMap ExtractExplicitUniformLocations(const String& source) { UnorderedMap locations; // Fast path: without the qualifier keyword there is nothing to extract. diff --git a/MobileGL/MG_Util/ShaderTranspiler/ShaderSourceProcessor.h b/MobileGL/MG_Util/ShaderTranspiler/ShaderSourceProcessor.h index 0a55cc78..e7973a25 100644 --- a/MobileGL/MG_Util/ShaderTranspiler/ShaderSourceProcessor.h +++ b/MobileGL/MG_Util/ShaderTranspiler/ShaderSourceProcessor.h @@ -75,6 +75,18 @@ namespace MobileGL { // `maxBindings` is what glGetIntegerv answers for that pname; a non-positive value // means "nothing to check against" and every declaration passes. std::optional FindShaderStorageBindingViolation(const String& source, Int maxBindings); + + // GL 4.6 core 7.7 / ARB_shader_atomic_counters makes it a COMPILE-time error to + // declare an atomic counter at an offset that is not a multiple of 4, or whose last + // byte passes GL_MAX_ATOMIC_COUNTER_BUFFER_SIZE. glslang enforces both in fixOffset(), + // which the Vulkan-relaxed parse never reaches (vkRelaxedRemapUniformVariable folds + // the atomic_uint into a synthesized storage block and returns from declareVariable() + // first), so MobileGL only caught them at LINK - and + // KHR-GL43.shader_atomic_counters.negative-offset-1 never links at all. The + // cross-stage rule (two counters sharing a binding must not overlap) stays at link: + // a single-stage source cannot see it. Returns the compile-error text for the first + // violation, or nullopt for a clean source. + std::optional FindAtomicCounterOffsetViolation(const String& source); } // namespace ShaderTranspiler } // namespace MG_Util } // namespace MobileGL