swung0x48/MobileGL
1
0
Fork 0
mirror of https://github.com/MobileGL-Dev/MobileGL synced 2026-09-09 12:48:32 +09:00
Code Issues Packages Projects Releases Wiki Activity

[Fix, Test] (ShaderTranspiler): size a non-final unsized storage-block member so the members after it stop aliasing it

Browse Source
This commit is contained in:
swung0x48
2026-08-20 13:05:50 -04:00
parent 1740a8a41a
commit e2923a239f
2 changed files with 183 additions and 0 deletions
Download Patch File Download Diff File Expand all files Collapse all files
MobileGL/MG_Test/Program/ProgramUtilTest.cpp
+68
View File
@@ -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.
MobileGL/MG_Util/ShaderTranspiler/ShaderSourceProcessor.cpp
+115
View File
@@ -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<CodeToken> 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<String, long long> 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<SizeEdit> 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<std::pair<SizeT, SizeT>> 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);