diff --git a/MobileGL/MG_State/GLState/ProgramState/ShaderCompileTask.cpp b/MobileGL/MG_State/GLState/ProgramState/ShaderCompileTask.cpp index 70165f11..4eec41ba 100644 --- a/MobileGL/MG_State/GLState/ProgramState/ShaderCompileTask.cpp +++ b/MobileGL/MG_State/GLState/ProgramState/ShaderCompileTask.cpp @@ -8,6 +8,7 @@ #include "ShaderCompileTask.h" +#include #include #include #include @@ -15,6 +16,7 @@ #include +#include #include namespace { @@ -137,8 +139,21 @@ namespace { return std::nullopt; } + // What glGetIntegerv(GL_MAX_SHADER_STORAGE_BUFFER_BINDINGS) answers, recomputed rather than + // queried: the compile runs on a worker with no context, and the pname is not a plain backend + // parameter - the getter caps the backend's count by the state layer's fixed binding-point + // array (GL_Getter's GetIndexedBufferQueryPointCount). A shader must be judged against the + // number the application was told, not against either half of it. + static MobileGL::Int MaxShaderStorageBufferBindings( + const MobileGL::MG_Util::ShaderTranspiler::CompileEnv& env) { + const MobileGL::Int frontendPoints = + static_cast(MobileGL::MG_State::GLState::BufferBindingPointCount); + if (!env.HasBackend()) return frontendPoints; + return std::min(frontendPoints, std::max(env.params.MaxShaderStorageBufferBindings, 0)); + } + // The half of a compile that depends on nothing but the source text, the stage and the - // environment snapshot: preprocessing, the two lexical rejections, and the two lexical + // environment snapshot: preprocessing, the three lexical rejections, and the two lexical // side-channel extractions. Split out so P0b layer 2 can memoize exactly this and // nothing else - the glslang parse stays per-object because its TShader is consume-once. // Deliberately free of any per-object state so the memo is sound. @@ -172,6 +187,13 @@ namespace { return result; } + if (const std::optional bindingError = FindShaderStorageBindingViolation( + result.preprocessedSource, MaxShaderStorageBufferBindings(env))) { + result.outcome = ShaderPreprocessOutcome::ResourceBindingRejected; + result.infoLog = *bindingError; + 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 6a5c8416..238ff7ba 100644 --- a/MobileGL/MG_State/GLState/ProgramState/ShaderPreprocessCache.h +++ b/MobileGL/MG_State/GLState/ProgramState/ShaderPreprocessCache.h @@ -26,6 +26,9 @@ namespace MobileGL::MG_State::GLState { ComputeLocalSizeRejected, // FindReservedIdentifierViolation rejected it. ReservedIdentifierRejected, + // FindShaderStorageBindingViolation rejected it: a storage block declared a binding at or + // past GL_MAX_SHADER_STORAGE_BUFFER_BINDINGS. + ResourceBindingRejected, // 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 344d9d17..25b4ba30 100644 --- a/MobileGL/MG_Test/Program/ProgramUtilTest.cpp +++ b/MobileGL/MG_Test/Program/ProgramUtilTest.cpp @@ -3840,3 +3840,41 @@ TEST_F(ProgramUtilTest, EsslCoreImageFormatSetIsTheThirteenTheSpecLists) { EXPECT_FALSE(ShaderCompiler::GLInternalFormatIsCoreEsslImageFormat(0x8051 /*GL_RGB8*/)); EXPECT_FALSE(ShaderCompiler::GLInternalFormatIsCoreEsslImageFormat(0 /*GL_NONE*/)); } + +// KHR-GL43.shader_storage_buffer_object.negative-glsl-compileTime: a storage block declared at +// GL_MAX_SHADER_STORAGE_BUFFER_BINDINGS must fail to compile, and so must an arrayed one whose +// LAST element passes the ceiling. The relaxed Vulkan-rules parse enforces neither. +TEST_F(ProgramUtilTest, StorageBlockBindingCeilingIsCheckedAtItsExactBoundary) { + using namespace MG_Util::ShaderTranspiler; + + constexpr Int kMaxBindings = 36; + const auto violation = [](const String& body) { + return FindShaderStorageBindingViolation("#version 430 core\n" + body + "void main() {}\n", kMaxBindings); + }; + + // The boundary itself: max - 1 is the last legal point, max is one past it. + EXPECT_FALSE(violation("layout(binding = 35) buffer Buffer { int x; };\n").has_value()); + EXPECT_TRUE(violation("layout(binding = 36) buffer Buffer { int x; };\n").has_value()); + + // An instance array takes CONSECUTIVE points, so what has to fit is base + count - 1. + EXPECT_FALSE(violation("layout(binding = 32) buffer Buffer { int x; } g_array[4];\n").has_value()); + EXPECT_TRUE(violation("layout(binding = 34) buffer Buffer { int x; } g_array[4];\n").has_value()); + + // Qualifiers and a second layout list may sit between the binding and the keyword. + EXPECT_TRUE(violation("layout(std430) layout(binding = 36) coherent restrict buffer B { int x; };\n") + .has_value()); + + // Things the scanner must NOT judge: a uniform block (a different ceiling), a storage block + // with no explicit binding, the bare default-qualifier form, and an instance array whose size + // is not a literal. + EXPECT_FALSE(violation("layout(binding = 40) uniform Block { int x; };\n" + "layout(binding = 0) buffer Buffer { int y; };\n") + .has_value()); + EXPECT_FALSE(violation("buffer Buffer { int x; };\nconst int binding = 40;\n").has_value()); + EXPECT_FALSE(violation("layout(binding = 1) buffer;\nbuffer Buffer { int x; };\n").has_value()); + EXPECT_FALSE(violation("const int kCount = 4;\nlayout(binding = 34) buffer B { int x; } g[kCount];\n") + .has_value()); + + // A backend that advertises no binding points has no ceiling to enforce. + EXPECT_FALSE(FindShaderStorageBindingViolation("layout(binding = 36) buffer B { int x; };\n", 0).has_value()); +} diff --git a/MobileGL/MG_Util/ShaderTranspiler/ShaderSourceProcessor.cpp b/MobileGL/MG_Util/ShaderTranspiler/ShaderSourceProcessor.cpp index c0353917..3a5027d7 100644 --- a/MobileGL/MG_Util/ShaderTranspiler/ShaderSourceProcessor.cpp +++ b/MobileGL/MG_Util/ShaderTranspiler/ShaderSourceProcessor.cpp @@ -1332,6 +1332,97 @@ namespace MobileGL { return bindings; } + namespace { + // Binding points a storage-block declaration starting at `bufferPos` occupies. + // One for a scalar instance (and for the "layout(...) buffer;" default-qualifier + // form, which declares no block at all); the element count for an instance array, + // whose elements take base, base+1, ... (GLSL 4.30 4.4.5). -1 means "the grammar + // here is outside this scanner's narrow subset", i.e. do not judge this one. + long long StorageBlockBindingPointCount(const Vector& tokens, SizeT bufferPos, + SizeT count) { + SizeT k = bufferPos + 1; + if (k < count && IsIdentifierToken(tokens[k])) ++k; // block type name + if (k >= count || tokens[k].text != "{") return 1; + + MobileGL::Int braceDepth = 0; + while (k < count) { + if (tokens[k].text == "{") { + ++braceDepth; + } else if (tokens[k].text == "}") { + --braceDepth; + if (braceDepth == 0) { + ++k; + break; + } + } + ++k; + } + if (braceDepth != 0) return -1; // unterminated block: not this scanner's business + + if (k < count && IsIdentifierToken(tokens[k])) ++k; // instance name + if (k >= count || tokens[k].text != "[") return 1; + if (k + 2 < count && IsDecimalIntegerToken(tokens[k + 1].text) && tokens[k + 2].text == "]") { + return std::max(1, std::strtoll(tokens[k + 1].text.c_str(), nullptr, 10)); + } + return -1; // sized by an expression, or unsized + } + } // namespace + + std::optional FindShaderStorageBindingViolation(const String& source, Int maxBindings) { + // A backend that advertises nothing has no ceiling to enforce. + if (maxBindings <= 0) return std::nullopt; + // Fast path: no storage block, nothing to check. Both keywords are required for a + // violation to exist, and the pair is absent from almost every shader-pack source. + if (source.find("buffer") == String::npos || source.find("binding") == String::npos) { + return std::nullopt; + } + + const Vector tokens = TokenizeCode(source); + const SizeT count = tokens.size(); + // The binding the qualifier run currently being scanned declared, -1 for none. + // Several layout(...) lists may precede one declaration and the later one wins, + // which is the same accumulate-then-consume shape the extractors above use. + long long binding = -1; + 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 == "binding" && j + 2 < count && + tokens[j + 1].text == "=" && IsDecimalIntegerToken(tokens[j + 2].text)) { + binding = std::min(std::strtoll(tokens[j + 2].text.c_str(), nullptr, 10), + static_cast(INT_MAX / 2)); + j += 2; + } + ++j; + } + pos = j - 1; + continue; + } + if (text == "buffer") { + const long long points = binding >= 0 ? StorageBlockBindingPointCount(tokens, pos, count) : -1; + if (points > 0 && binding + points > static_cast(maxBindings)) { + return "ERROR: invalid value " + std::to_string(binding) + + " for layout specifier 'binding': a shader storage block occupying " + + std::to_string(points) + " binding point(s) from there passes " + + "GL_MAX_SHADER_STORAGE_BUFFER_BINDINGS (" + std::to_string(maxBindings) + ")."; + } + binding = -1; + continue; + } + // Qualifiers may sit between the layout list and the `buffer` keyword; anything + // else ends the run, so a binding never leaks onto an unrelated declaration. + if (!IsNonLayoutQualifierKeyword(text)) binding = -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 d6e41c71..0a55cc78 100644 --- a/MobileGL/MG_Util/ShaderTranspiler/ShaderSourceProcessor.h +++ b/MobileGL/MG_Util/ShaderTranspiler/ShaderSourceProcessor.h @@ -64,6 +64,17 @@ namespace MobileGL { // mapIO can capture them, so they are recovered lexically (same narrow // grammar discipline as ExtractExplicitUniformLocations). UnorderedMap ExtractExplicitOpaqueBindings(const String& source); + + // A shader storage block whose layout(binding = N) reaches or passes + // GL_MAX_SHADER_STORAGE_BUFFER_BINDINGS is a compile-time error in GL 4.3 core 4.4.5, + // and an arrayed block instance takes CONSECUTIVE points, so the last element is what + // has to fit. glslang cannot raise it for MobileGL: every shader is parsed as a Vulkan + // client under relaxed rules, where the GL ceilings do not apply, and TBuiltInResource + // has no storage-buffer binding field to check against in the first place. Returns the + // compile-error text for the first violation, or nullopt for a clean source. + // `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); } // namespace ShaderTranspiler } // namespace MG_Util } // namespace MobileGL