From e2923a239f7edeb5683a5785737e5c18c716d134 Mon Sep 17 00:00:00 2001 From: Swung0x48 Date: Thu, 20 Aug 2026 13:05:50 -0400 Subject: [PATCH] [Fix, Test] (ShaderTranspiler): size a non-final unsized storage-block member so the members after it stop aliasing it --- MobileGL/MG_Test/Program/ProgramUtilTest.cpp | 68 +++++++++++ .../ShaderSourceProcessor.cpp | 115 ++++++++++++++++++ 2 files changed, 183 insertions(+) diff --git a/MobileGL/MG_Test/Program/ProgramUtilTest.cpp b/MobileGL/MG_Test/Program/ProgramUtilTest.cpp index 25b4ba30..34092c20 100644 --- a/MobileGL/MG_Test/Program/ProgramUtilTest.cpp +++ b/MobileGL/MG_Test/Program/ProgramUtilTest.cpp @@ -3841,6 +3841,74 @@ TEST_F(ProgramUtilTest, EsslCoreImageFormatSetIsTheThirteenTheSpecLists) { EXPECT_FALSE(ShaderCompiler::GLInternalFormatIsCoreEsslImageFormat(0 /*GL_NONE*/)); } +// KHR-GL43.shader_storage_buffer_object.basic-syntax iteration 6. glslang assigns a block's member +// offsets at DECLARATION time, where a member array that is still unsized contributes zero bytes - +// so `vec4 position01[]; vec4 position2;` put both members at offset 0 and the shader read +// position01[0] where it asked for position2. The preprocessor sizes the non-final member from the +// largest constant index the source uses, which is what the language says it means. +TEST_F(ProgramUtilTest, ANonFinalUnsizedBufferBlockMemberIsSizedFromItsLargestConstantIndex) { + using namespace MG_Util::ShaderTranspiler; + + String source = R"(#version 430 core +layout(packed) coherent buffer Buffer { + vec4 position01[]; + vec4 position2; +} g_buffer; +void main() { + if (gl_VertexID == 0) gl_Position = g_buffer.position01[0]; + else if (gl_VertexID == 1) gl_Position = g_buffer.position01[1]; + else if (gl_VertexID == 2) gl_Position = g_buffer.position2; +} +)"; + PreprocessShaderSource(ShaderStage::Vertex, source); + EXPECT_NE(source.find("vec4 position01[2];"), String::npos) << source; + EXPECT_EQ(source.find("position01[];"), String::npos) << source; + + // The LAST member of a storage block is a run-time sized array, which is legal and already + // laid out correctly - sizing it would be a wire-format change, not a repair. + String lastMember = R"(#version 430 core +buffer Buffer { + vec4 head; + vec4 tail[]; +} g_buffer; +void main() { + gl_Position = g_buffer.tail[0] + g_buffer.tail[3]; +} +)"; + PreprocessShaderSource(ShaderStage::Vertex, lastMember); + EXPECT_NE(lastMember.find("vec4 tail[];"), String::npos) << lastMember; + + // A member the shader subscripts with anything but a literal cannot be sized from the source, + // so it is left exactly as it was. + String dynamicIndex = R"(#version 430 core +buffer Buffer { + vec4 head[]; + vec4 tail; +} g_buffer; +uniform int g_index; +void main() { + gl_Position = g_buffer.head[g_index] + g_buffer.tail; +} +)"; + PreprocessShaderSource(ShaderStage::Vertex, dynamicIndex); + EXPECT_NE(dynamicIndex.find("vec4 head[];"), String::npos) << dynamicIndex; + + // `buffer` is also a member memory qualifier; a declaration that uses it must not be mistaken + // for a block header. + String memberQualifier = R"(#version 430 core +coherent buffer Buffer { + buffer vec4 position0; + vec4 position1[]; + vec4 position2; +} g_buffer; +void main() { + gl_Position = g_buffer.position0 + g_buffer.position1[2] + g_buffer.position2; +} +)"; + PreprocessShaderSource(ShaderStage::Vertex, memberQualifier); + EXPECT_NE(memberQualifier.find("vec4 position1[3];"), String::npos) << memberQualifier; +} + // 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. diff --git a/MobileGL/MG_Util/ShaderTranspiler/ShaderSourceProcessor.cpp b/MobileGL/MG_Util/ShaderTranspiler/ShaderSourceProcessor.cpp index 3a5027d7..87fdedd8 100644 --- a/MobileGL/MG_Util/ShaderTranspiler/ShaderSourceProcessor.cpp +++ b/MobileGL/MG_Util/ShaderTranspiler/ShaderSourceProcessor.cpp @@ -818,6 +818,116 @@ namespace { ReplaceIdentifier(source, "GL_ARB_gpu_shader_int64", "MG_DISABLED_GL_ARB_gpu_shader_int64"); } + // GLSL 4.30 4.1.9 allows an interface-block member array to be left unsized when it is NOT the + // last member; it is then implicitly sized by the largest constant index the shader uses. + // glslang implements the SIZING - adoptImplicitArraySizes, at link - but computes the block's + // member OFFSETS at DECLARATION time (fixBlockUniformOffsets), where the array is still + // unsized and so contributes zero bytes. Every member after it is therefore laid out on top of + // it: `vec4 a[]; vec4 b;` puts BOTH at offset 0, and a shader reading `b` gets `a[0]` + // (KHR-GL43.shader_storage_buffer_object.basic-syntax iteration 6, whose degenerate triangle + // rasterizes nothing at all). + // + // The source level is the only place the two can be reconciled, because the offset pass runs + // before a single statement has been parsed. Deliberately narrow: it fires only on a `buffer` + // block (no other block kind may hold an unsized member at all), only on a member that is not + // the last one, and only when every subscript of that member's name in the source is a decimal + // literal. Anything outside that shape is left exactly as it was - and the shape itself has no + // correct behaviour today, so the rewrite cannot take a working case away. + void SizeNonFinalUnsizedBufferBlockMembers(MobileGL::String& source) { + // Both tokens must be present for the shape to exist, and "[]" is absent from essentially + // every real shader source, so this is the whole cost for them. + if (source.find("[]") == MobileGL::String::npos || source.find("buffer") == MobileGL::String::npos) { + return; + } + + const auto isDecimalInteger = [](const String& text) { + return !text.empty() && std::all_of(text.begin(), text.end(), [](char ch) { + return ch >= '0' && ch <= '9'; + }); + }; + + const Vector tokens = TokenizeCode(source); + const SizeT count = tokens.size(); + + // Pass 1: for every identifier, the largest literal index it is subscripted with (as a + // count, i.e. index + 1), or -1 once it is subscripted with anything that is not a literal. + // The declaration's own empty `[]` is neither. + MobileGL::UnorderedMap subscriptExtent; + for (SizeT i = 1; i < count; ++i) { + if (tokens[i].text != "[" || !IsIdentifierToken(tokens[i - 1])) continue; + if (i + 1 < count && tokens[i + 1].text == "]") continue; // the unsized declarator itself + long long& extent = subscriptExtent[tokens[i - 1].text]; + if (i + 2 < count && isDecimalInteger(tokens[i + 1].text) && tokens[i + 2].text == "]") { + if (extent >= 0) { + extent = std::max(extent, std::strtoll(tokens[i + 1].text.c_str(), nullptr, 10) + 1); + } + } else { + extent = -1; + } + } + + // Pass 2: one edit per repairable member, applied back to front so earlier offsets stand. + struct SizeEdit { + SizeT pos; + String text; + }; + Vector edits; + for (SizeT i = 0; i < count; ++i) { + if (tokens[i].text != "buffer") continue; + SizeT cursor = i + 1; + // `buffer` is also a member MEMORY qualifier ("buffer vec4 position0;"), which is why + // the block body has to be found rather than assumed. + if (cursor < count && IsIdentifierToken(tokens[cursor])) ++cursor; + if (cursor >= count || tokens[cursor].text != "{") continue; + + const SizeT bodyBegin = cursor + 1; + SizeT bodyEnd = bodyBegin; + int depth = 1; + while (bodyEnd < count) { + if (tokens[bodyEnd].text == "{") { + ++depth; + } else if (tokens[bodyEnd].text == "}") { + --depth; + if (depth == 0) break; + } + ++bodyEnd; + } + if (depth != 0) continue; // unterminated; glslang will have the last word + + Vector> members; // [begin, end) of each member, ';' excluded + SizeT memberBegin = bodyBegin; + for (SizeT m = bodyBegin; m < bodyEnd; ++m) { + if (tokens[m].text != ";") continue; + members.emplace_back(memberBegin, m); + memberBegin = m + 1; + } + + // The LAST member is deliberately untouched: an unsized array there is a run-time + // sized array, which is both legal and correctly laid out already. + for (SizeT index = 0; index + 1 < members.size(); ++index) { + const SizeT begin = members[index].first; + const SizeT end = members[index].second; + if (end < begin + 3) continue; + if (tokens[end - 1].text != "]" || tokens[end - 2].text != "[") continue; + if (!IsIdentifierToken(tokens[end - 3])) continue; + // A multi-declarator member would need one size per declarator; out of scope. + bool multipleDeclarators = false; + for (SizeT t = begin; t < end; ++t) { + if (tokens[t].text == ",") multipleDeclarators = true; + } + if (multipleDeclarators) continue; + const auto known = subscriptExtent.find(tokens[end - 3].text); + if (known == subscriptExtent.end() || known->second <= 0) continue; + edits.push_back({tokens[end - 1].begin, std::to_string(known->second)}); + } + i = bodyEnd; + } + + for (auto it = edits.rbegin(); it != edits.rend(); ++it) { + source.insert(it->pos, it->text); + } + } + // Rewrite the `packed` / `shared` block-packing qualifiers inside layout(...) declarations to // `std140`. Desktop GL leaves the memory layout of such blocks to the implementation and the // app must query member offsets; MobileGL's SPIR-V pipeline always lays uniform blocks out as @@ -962,6 +1072,11 @@ namespace MobileGL { FilterUnsupportedGpuShaderInt64(env, source); CoerceUniformBlockPackingToStd140(source); + // After the packing coercion: that one rewrites `packed`/`shared` in place and so + // cannot move an offset this pass depends on, and reading the block declarations + // once both qualifiers are normalized keeps the two passes' notions of a block + // declaration identical. + SizeNonFinalUnsizedBufferBlockMembers(source); RenameBuiltinShadowingFunctions(source);