From d6caed78229ada3944beb5ef7aa25750bc55dcfe Mon Sep 17 00:00:00 2001 From: BZLZHH Date: Sat, 8 Aug 2026 05:28:28 -0400 Subject: [PATCH] [Fix] (MG_Util, MG_State): five latent frontend bugs the async work made load-bearing - SpvcSession's move constructor and move assignment dropped the parsed metadata, so a moved-to session silently reported empty reflection. - ParseComputeLocalSize used std::stoull, whose std::out_of_range escaped glCompileShader on an oversized local_size literal; now std::from_chars saturating to UINT_MAX, pinned by a regression test that reproduced the escaping exception. - The compute local_size std::regex was rebuilt on every compile; hoisted. - LinkProgram dumped every shader's full source through MGLOG_D per link. - glslang::FinalizeProcess ran before the GL context tore down, leaving the context's live TShaders pointing at freed builtin symbol tables. --- MobileGL/Init.cpp | 7 +++++- .../GLState/ProgramState/ProgramObject.cpp | 5 ++-- .../GLState/ProgramState/ShaderObject.cpp | 25 ++++++++++++++++--- MobileGL/MG_Test/Program/ProgramTest.cpp | 23 +++++++++++++++++ .../MG_Util/ShaderTranspiler/SpvcSession.cpp | 4 +++ 5 files changed, 57 insertions(+), 7 deletions(-) diff --git a/MobileGL/Init.cpp b/MobileGL/Init.cpp index 80868a9f..51b1b519 100644 --- a/MobileGL/Init.cpp +++ b/MobileGL/Init.cpp @@ -37,7 +37,6 @@ namespace MobileGL { if (logLifecycle) { MGLOG_I("MobileGL closing..."); } - glslang::FinalizeProcess(); // GL syncs die with their contexts, and every context is gone by the // time full teardown runs: drain the live-sync registry while the // backend function table can still release the backend handles (and @@ -49,6 +48,12 @@ namespace MobileGL { MG_State::pEGLContext.reset(); MG_Impl::GLImpl::TextureImpl::pProxyTextureManager.reset(); MG_Impl::GLImpl::FramebufferImpl::pDefaultFramebufferInfo.reset(); + // Must run AFTER pGLContext.reset(). FinalizeProcess -> ShFinalize deletes + // glslang's process-wide pool allocator and every cached built-in symbol table, + // while the TShader/TProgram objects owned by the shader and program objects + // still reference levels adopted from those tables. Finalizing first left live + // glslang objects pointing at freed memory for the rest of the teardown. + glslang::FinalizeProcess(); MG_Backend::gBackendFunctionsTable = {}; g_isInitialized = false; if (logLifecycle) { diff --git a/MobileGL/MG_State/GLState/ProgramState/ProgramObject.cpp b/MobileGL/MG_State/GLState/ProgramState/ProgramObject.cpp index acff6e6f..9703e53b 100644 --- a/MobileGL/MG_State/GLState/ProgramState/ProgramObject.cpp +++ b/MobileGL/MG_State/GLState/ProgramState/ProgramObject.cpp @@ -579,10 +579,11 @@ namespace MobileGL::MG_State::GLState { MGLOG_E("ProgramObject %u: Link failed - %s", m_externalIndex, m_infoLog.c_str()); return; } + // Deliberately no full-source dump here: a shaderpack stage runs to ~100 KB, and + // one MGLOG line per shader per link is unreadable even single-threaded. Use the + // transpiler dump paths when a specific source is actually needed. MGLOG_D("ProgramObject %u: shader[%zu] compiled shader ptr %p, src len %zu", m_externalIndex, i, shaders[i].get(), m_shaders[i]->GetShaderSource().length()); - MGLOG_D("ProgramObject %u: shader[%zu] source:\n%s", m_externalIndex, i, - m_shaders[i]->GetShaderSource().c_str()); } // Merge the shaders' lexically extracted explicit uniform locations. The same diff --git a/MobileGL/MG_State/GLState/ProgramState/ShaderObject.cpp b/MobileGL/MG_State/GLState/ProgramState/ShaderObject.cpp index d58dfd83..ecd31b32 100644 --- a/MobileGL/MG_State/GLState/ProgramState/ShaderObject.cpp +++ b/MobileGL/MG_State/GLState/ProgramState/ShaderObject.cpp @@ -15,6 +15,8 @@ #include #include +#include + namespace { struct ComputeLocalSize { MobileGL::Uint x = 1; @@ -72,16 +74,31 @@ namespace { return result; } + // Hoisted out of ParseComputeLocalSize: constructing a std::regex costs far more than + // running it over a small source, and it was being rebuilt on every compute compile. A + // const regex carries no mutable state, so sharing one instance is safe. + static const std::regex kComputeLocalSizePattern(R"(local_size_([xyz])\s*=\s*([0-9]+))"); + static ComputeLocalSize ParseComputeLocalSize(const MobileGL::String& source) { ComputeLocalSize localSize; const MobileGL::String uncommentedSource = StripGlslComments(source); - const std::regex localSizePattern(R"(local_size_([xyz])\s*=\s*([0-9]+))"); - for (std::sregex_iterator it(uncommentedSource.begin(), uncommentedSource.end(), localSizePattern), end; + for (std::sregex_iterator it(uncommentedSource.begin(), uncommentedSource.end(), kComputeLocalSizePattern), + end; it != end; ++it) { const char axis = (*it)[1].str()[0]; - const auto value = static_cast(std::stoull((*it)[2].str())); - const MobileGL::Uint clampedValue = value > UINT_MAX ? UINT_MAX : static_cast(value); + // The [0-9]+ capture is unbounded, so `local_size_x = 99999999999999999999999` + // is a legal match. std::stoull would throw std::out_of_range on it and let the + // exception escape glCompileShader; std::from_chars reports the overflow instead. + // An overflowing literal saturates to UINT_MAX, which the device-limit check + // below rejects anyway - the same verdict a non-overflowing huge value gets. + const MobileGL::String digits = (*it)[2].str(); + unsigned long long value = 0; + const std::from_chars_result parsed = + std::from_chars(digits.data(), digits.data() + digits.size(), value); + const MobileGL::Uint clampedValue = (parsed.ec != std::errc() || value > UINT_MAX) + ? UINT_MAX + : static_cast(value); // TODO: Replace this literal layout scanner with parser/AST-backed validation so expressions and // specialization-id layouts are handled consistently with glslang. diff --git a/MobileGL/MG_Test/Program/ProgramTest.cpp b/MobileGL/MG_Test/Program/ProgramTest.cpp index ed830a2f..d5d0fb25 100644 --- a/MobileGL/MG_Test/Program/ProgramTest.cpp +++ b/MobileGL/MG_Test/Program/ProgramTest.cpp @@ -347,6 +347,29 @@ void main() { EXPECT_EQ(GetError(), GL_NO_ERROR); } +TEST_F(ProgramTest, OutOfRangeComputeLocalSizeLiteralFailsCompileInsteadOfThrowing) { + // The layout scanner's digit capture is unbounded, so a literal wider than 64 bits is a + // legal match. It must saturate and be rejected through COMPILE_STATUS; if the integer + // conversion throws instead, the exception escapes glCompileShader entirely. + char infoLog[1024] = ""; + const char* csSrc = R"(#version 460 core +layout(local_size_x = 99999999999999999999999) in; +void main() { +} +)"; + + GLuint cs = CreateShader(GL_COMPUTE_SHADER); + ShaderSource(cs, 1, &csSrc, nullptr); + CompileShader(cs); + + GLint csStatus = GL_TRUE; + GetShaderiv(cs, GL_COMPILE_STATUS, &csStatus); + EXPECT_EQ(csStatus, GL_FALSE); + GetShaderInfoLog(cs, sizeof(infoLog), nullptr, infoLog); + EXPECT_NE(String(infoLog).find("GL_MAX_COMPUTE_WORK_GROUP_SIZE"), String::npos) << infoLog; + EXPECT_EQ(GetError(), GL_NO_ERROR); +} + TEST_F(ProgramTest, DirectVulkanStorageBlockUsesShaderLayoutBinding) { char infoLog[1024] = ""; const char* csSrc = R"(#version 460 core diff --git a/MobileGL/MG_Util/ShaderTranspiler/SpvcSession.cpp b/MobileGL/MG_Util/ShaderTranspiler/SpvcSession.cpp index 32e3d831..0abe8f20 100644 --- a/MobileGL/MG_Util/ShaderTranspiler/SpvcSession.cpp +++ b/MobileGL/MG_Util/ShaderTranspiler/SpvcSession.cpp @@ -138,6 +138,9 @@ namespace MobileGL { std::swap(this->resources, that.resources); std::swap(this->reflectModule, that.reflectModule); std::swap(this->reflectModuleValid, that.reflectModuleValid); + // ParseMetaData() fills `metadata`, and GetMetadata() is read through the + // moved-to session: leaving it behind silently returns an empty reflection. + std::swap(this->metadata, that.metadata); } SpvcSession& SpvcSession::operator=(SpvcSession&& that) { @@ -149,6 +152,7 @@ namespace MobileGL { std::swap(this->resources, that.resources); std::swap(this->reflectModule, that.reflectModule); std::swap(this->reflectModuleValid, that.reflectModuleValid); + std::swap(this->metadata, that.metadata); return *this; }