From d40f7539832d7bcaa2088027d76b4c66c6e71789 Mon Sep 17 00:00:00 2001 From: Swung0x48 Date: Fri, 10 Jul 2026 11:23:16 -0400 Subject: [PATCH] [Fix] (MG_State, MG_Impl, MG_Backend): conformant current generic vertex attribute values GL 3.3 Core: a shader input whose generic attribute array is disabled reads that attribute's current value (per-context state, default (0,0,0,1)). Four defects made that path non-conformant, three of them silently. * Out-of-bounds current-value reads. m_currentVertexAttributes held 16 entries while the DirectVulkan draw path walked shader input locations 0..31 and GL_MAX_VERTEX_ATTRIBS was advertised straight from the device (commonly 32). The only guard was MOBILEGL_ASSERT, which expands to nothing outside debug builds. Grow the storage capacity to 32, advertise min(device limit, capacity), validate against that dynamic limit, and give the accessors real runtime bounds checks. Replace the literal 32 loops with the constant, and pin MAX_VERTEX_ATTRIBS to the Uint32 mask width and to vertexInputTypes' bound with static_asserts so the two can no longer drift apart -- that drift was the bug. * DirectGLES never fed current values to the driver. Values were stored in MG_State only, so a disabled attribute always rendered as the ES driver's own untouched (0,0,0,1) while DirectVulkan rendered it correctly: identical GL code, different pixels per backend. Add SyncCurrentVertexAttributeValues() to the draw prologue, and hoist the glType -> (base type, component count) dispatch into MG_State::GLState so both backends resolve the semantics from one place instead of it living inside VulkanRenderer. * Enabled arrays the backend could not map were silently demoted to the current value. ToVkVertexFormat had no DataType::Float16 case, so a GL_HALF_FLOAT array fell to VK_FORMAT_UNDEFINED, dropped out of the vertex input state, and became indistinguishable from a disabled array: the geometry rendered a constant colour with GL_NO_ERROR. Add the Float16 mapping, track an unsupportedAttribMask, and hard-fail the draw before pipeline creation so no synthetic attribute is baked into a cached VkPipeline. * glGetVertexAttrib{fv,iv,Iiv,Iuiv}(GL_CURRENT_VERTEX_ATTRIB) returned before any index validation, reading past the array instead of raising GL_INVALID_VALUE. Also resolve ProgramObject::DoReflection's "TODO: get from backend" 16-location clamp, which capped the new DirectGLES sync at locations 0..15; report GL_MAX_VERTEX_ATTRIBS through the same helper the validators use, so the clamp cannot be bypassed; and bound vertex binding indices by the same dynamic limit, since the default attribute -> binding mapping is the identity. Add a "Vertex attributes" driver POST row to both backends: FAIL below the GL 3.3 Core minimum of 16, WARN above MobileGL's storage capacity (clamped, extra attributes unusable), PASS in between -- making the driver/host mismatch that caused the out-of-bounds read visible instead of silently swallowed. Covered by 7 new regression tests (each verified to fail against the previous behaviour). --- .../DirectGLES/BackendObject_DirectGLES.cpp | 6 +- MobileGL/MG_Backend/DirectGLES/DirectGLES.cpp | 48 +++++ .../BackendObject_DirectVulkan.cpp | 6 +- .../Renderer/VertexInputStateFactory.cpp | 21 ++- .../Renderer/VertexInputStateFactory.h | 4 + .../DirectVulkan/Renderer/VulkanRenderer.cpp | 64 ++++++- MobileGL/MG_Impl/GLImpl/Getter/GL_Getter.cpp | 6 +- .../MG_Impl/GLImpl/Program/GL_Program.cpp | 3 +- .../GLImpl/VertexArray/GL_VertexArray.cpp | 11 +- .../MG_Impl/GLImpl/VertexArray/Validators.cpp | 16 +- .../MG_Impl/GLImpl/VertexArray/Validators.h | 4 + MobileGL/MG_State/GLState/Core.cpp | 46 ++++- MobileGL/MG_State/GLState/Core.h | 14 ++ .../GLState/ProgramState/ProgramObject.cpp | 20 +- .../VertexArrayState/VertexArrayObject.h | 22 ++- MobileGL/MG_Test/SanityTest.cpp | 50 +++++ .../MG_Test/VertexArray/VertexArrayTest.cpp | 176 ++++++++++++++++++ MobileGL/MG_Util/SelfTest/DriverPost.cpp | 52 ++++++ 18 files changed, 539 insertions(+), 30 deletions(-) diff --git a/MobileGL/MG_Backend/DirectGLES/BackendObject_DirectGLES.cpp b/MobileGL/MG_Backend/DirectGLES/BackendObject_DirectGLES.cpp index 053de746..a81d8f7a 100644 --- a/MobileGL/MG_Backend/DirectGLES/BackendObject_DirectGLES.cpp +++ b/MobileGL/MG_Backend/DirectGLES/BackendObject_DirectGLES.cpp @@ -981,7 +981,11 @@ namespace MobileGL::MG_Backend::DirectGLES { m_dynamicParameters.MaxVertexTextureImageUnits = m_GLESCapabilities.MaxVertexTextureImageUnits; m_dynamicParameters.MaxComputeTextureImageUnits = m_GLESCapabilities.MaxComputeTextureImageUnits; m_dynamicParameters.MaxCombinedTextureImageUnits = m_GLESCapabilities.MaxCombinedTextureImageUnits; - m_dynamicParameters.MaxVertexAttribs = m_GLESCapabilities.MaxVertexAttribs; + // Never advertise more attributes than the state layer can store: the current-value array and + // the Uint32 attribute masks the draw path passes around are both bounded by MAX_VERTEX_ATTRIBS. + m_dynamicParameters.MaxVertexAttribs = + std::min(m_GLESCapabilities.MaxVertexAttribs, + static_cast(MG_State::GLState::VertexArrayObject::MAX_VERTEX_ATTRIBS)); m_dynamicParameters.MaxComputeShaderStorageBlocks = m_GLESCapabilities.MaxComputeShaderStorageBlocks; m_dynamicParameters.MaxCombinedShaderStorageBlocks = m_GLESCapabilities.MaxCombinedShaderStorageBlocks; m_dynamicParameters.MaxComputeUniformBlocks = m_GLESCapabilities.MaxComputeUniformBlocks; diff --git a/MobileGL/MG_Backend/DirectGLES/DirectGLES.cpp b/MobileGL/MG_Backend/DirectGLES/DirectGLES.cpp index cbdc1da0..1588ca28 100644 --- a/MobileGL/MG_Backend/DirectGLES/DirectGLES.cpp +++ b/MobileGL/MG_Backend/DirectGLES/DirectGLES.cpp @@ -321,6 +321,52 @@ namespace MobileGL::MG_Backend::DirectGLES { } backendObj->SyncToBackend(currentVAOObject); } + + // GL: a shader input whose generic attribute array is DISABLED reads that attribute's *current + // value* (context state set by glVertexAttrib*, default (0,0,0,1)) rather than any buffer. + // MobileGL stores those values in MG_State only, so without this step the ES driver would feed + // the shader its own current values, which MobileGL never writes -- i.e. always (0,0,0,1). + // SyncToBackend has already issued glDisableVertexAttribArray for these locations, so the ES + // current value is what the shader will actually read. + void SyncCurrentVertexAttributeValues() { +#ifdef TRACY_ENABLE + ZoneScopedC(TRACY_ZONECOLOR_BACKEND); +#endif + const auto& program = MG_State::pGLContext->GetCurrentProgram(); + if (!program) return; + + const auto& vao = MG_State::pGLContext->GetBoundVertexArray(); + if (!vao) return; + + const Uint32 activeAttribMask = program->GetActiveAttributeLocationMask(); + if (activeAttribMask == 0) return; + + constexpr Uint32 maxVertexAttribs = + static_cast(MG_State::GLState::VertexArrayObject::MAX_VERTEX_ATTRIBS); + for (Uint32 location = 0; location < maxVertexAttribs; ++location) { + if ((activeAttribMask & (1u << location)) == 0) continue; + if (vao->GetAttribute(location).Enabled) continue; + + const auto& currentValue = MG_State::pGLContext->GetCurrentVertexAttribute(location); + const auto typeInfo = MG_State::GLState::ClassifyVertexAttribType(program->GetAttribType(location)); + switch (typeInfo.baseType) { + case MG_State::GLState::VertexAttribBaseType::Float: + g_GLESFuncs.glVertexAttrib4fv(location, currentValue.floatValue.data()); + break; + case MG_State::GLState::VertexAttribBaseType::Int: + g_GLESFuncs.glVertexAttribI4iv(location, currentValue.intValue.data()); + break; + case MG_State::GLState::VertexAttribBaseType::Uint: + g_GLESFuncs.glVertexAttribI4uiv(location, currentValue.uintValue.data()); + break; + case MG_State::GLState::VertexAttribBaseType::Unsupported: + MGLOG_E("SyncCurrentVertexAttributeValues: program=%u location=%u has no enabled array and its " + "shader input type 0x%x is not supported as a current generic vertex attribute", + program->GetExternalIndex(), location, program->GetAttribType(location)); + break; + } + } + } } // namespace VertexArrayImpl namespace TextureImpl { @@ -864,6 +910,8 @@ namespace MobileGL::MG_Backend::DirectGLES { } } + VertexArrayImpl::SyncCurrentVertexAttributeValues(); + BindCurrentTextures(); BindCurrentProgramWithResources(); } diff --git a/MobileGL/MG_Backend/DirectVulkan/BackendObject_DirectVulkan.cpp b/MobileGL/MG_Backend/DirectVulkan/BackendObject_DirectVulkan.cpp index baca8ade..3585eb6b 100644 --- a/MobileGL/MG_Backend/DirectVulkan/BackendObject_DirectVulkan.cpp +++ b/MobileGL/MG_Backend/DirectVulkan/BackendObject_DirectVulkan.cpp @@ -725,7 +725,11 @@ namespace MobileGL::MG_Backend::DirectVulkan { std::min(m_vulkanCaps.MaxComputeTextureImageUnits, maxSupportedTextureUnits); m_dynamicParameters.MaxCombinedTextureImageUnits = std::min(m_vulkanCaps.MaxCombinedTextureImageUnits, maxSupportedTextureUnits); - m_dynamicParameters.MaxVertexAttribs = m_vulkanCaps.MaxVertexAttribs; + // Never advertise more attributes than the state layer can store: the current-value array and + // the Uint32 attribute masks the draw path passes around are both bounded by MAX_VERTEX_ATTRIBS. + m_dynamicParameters.MaxVertexAttribs = + std::min(m_vulkanCaps.MaxVertexAttribs, + static_cast(MG_State::GLState::VertexArrayObject::MAX_VERTEX_ATTRIBS)); m_dynamicParameters.MaxComputeShaderStorageBlocks = m_vulkanCaps.MaxComputeShaderStorageBlocks; m_dynamicParameters.MaxCombinedShaderStorageBlocks = m_vulkanCaps.MaxCombinedShaderStorageBlocks; m_dynamicParameters.MaxComputeUniformBlocks = m_vulkanCaps.MaxComputeUniformBlocks; diff --git a/MobileGL/MG_Backend/DirectVulkan/Renderer/VertexInputStateFactory.cpp b/MobileGL/MG_Backend/DirectVulkan/Renderer/VertexInputStateFactory.cpp index 7e83a95e..048d0731 100644 --- a/MobileGL/MG_Backend/DirectVulkan/Renderer/VertexInputStateFactory.cpp +++ b/MobileGL/MG_Backend/DirectVulkan/Renderer/VertexInputStateFactory.cpp @@ -65,6 +65,7 @@ namespace MobileGL::MG_Backend::DirectVulkan { Vector bindingBaseOffsets; Vector bindingAttributeLocations; Vector bindingUsesClientMemory; + Uint32 unsupportedAttribMask = 0; for (Uint32 location = 0; location < MG_State::GLState::VertexArrayObject::MAX_VERTEX_ATTRIBS; ++location) { const auto& attr = vao.GetAttribute(location); @@ -74,15 +75,19 @@ namespace MobileGL::MG_Backend::DirectVulkan { const auto vkFormat = ToVkVertexFormat(attr.Type, attr.Size, attr.Normalized, attr.IsInteger); if (vkFormat == VK_FORMAT_UNDEFINED) { - MGLOG_D("Skipping unsupported vertex attribute layout (location=%u, type=%s, size=%d)", + MGLOG_E("Unsupported vertex attribute layout (location=%u, type=%s, size=%d): the array is " + "enabled but cannot be mapped to a VkFormat", location, MG_Util::ConvertDataTypeToString(attr.Type).c_str(), attr.Size); + unsupportedAttribMask |= (1u << location); continue; } const SizeT componentSize = GetComponentSize(attr.Type); if (componentSize == 0) { - MGLOG_D("Skipping vertex attribute with unknown component size (location=%u, type=%s)", + MGLOG_E("Vertex attribute with unknown component size (location=%u, type=%s): the array is " + "enabled but cannot be sized", location, MG_Util::ConvertDataTypeToString(attr.Type).c_str()); + unsupportedAttribMask |= (1u << location); continue; } @@ -112,6 +117,7 @@ namespace MobileGL::MG_Backend::DirectVulkan { entry.bindingBaseOffsets = std::move(bindingBaseOffsets); entry.bindingAttributeLocations = std::move(bindingAttributeLocations); entry.bindingUsesClientMemory = std::move(bindingUsesClientMemory); + entry.unsupportedAttribMask = unsupportedAttribMask; entry.state = state; entry.state.pVertexBindingDescriptions = entry.bindings.empty() ? nullptr : entry.bindings.data(); entry.state.pVertexAttributeDescriptions = entry.attributes.empty() ? nullptr : entry.attributes.data(); @@ -128,6 +134,17 @@ namespace MobileGL::MG_Backend::DirectVulkan { case 4: return VK_FORMAT_R32G32B32A32_SFLOAT; default: return VK_FORMAT_UNDEFINED; } + case DataType::Float16: + // GL_HALF_FLOAT is a floating-point array type: it is never an integer attribute, and + // GL_TRUE for `normalized` is ignored for float types rather than selecting a *NORM format. + if (isInteger) return VK_FORMAT_UNDEFINED; + switch (size) { + case 1: return VK_FORMAT_R16_SFLOAT; + case 2: return VK_FORMAT_R16G16_SFLOAT; + case 3: return VK_FORMAT_R16G16B16_SFLOAT; + case 4: return VK_FORMAT_R16G16B16A16_SFLOAT; + default: return VK_FORMAT_UNDEFINED; + } case DataType::Int32: if (!isInteger || normalized) return VK_FORMAT_UNDEFINED; switch (size) { diff --git a/MobileGL/MG_Backend/DirectVulkan/Renderer/VertexInputStateFactory.h b/MobileGL/MG_Backend/DirectVulkan/Renderer/VertexInputStateFactory.h index f1c108df..5661e8c4 100644 --- a/MobileGL/MG_Backend/DirectVulkan/Renderer/VertexInputStateFactory.h +++ b/MobileGL/MG_Backend/DirectVulkan/Renderer/VertexInputStateFactory.h @@ -27,6 +27,10 @@ namespace MobileGL::MG_Backend::DirectVulkan { Vector bindingBaseOffsets; Vector bindingAttributeLocations; Vector bindingUsesClientMemory; + // Locations whose array is ENABLED but whose GL format has no VkFormat mapping. They are + // absent from `attributes`, so without this mask the draw path cannot tell them apart from + // a genuinely disabled array and would silently feed the shader the current attribute value. + Uint32 unsupportedAttribMask = 0; VkPipelineVertexInputStateCreateInfo state{ VK_STRUCTURE_TYPE_PIPELINE_VERTEX_INPUT_STATE_CREATE_INFO }; diff --git a/MobileGL/MG_Backend/DirectVulkan/Renderer/VulkanRenderer.cpp b/MobileGL/MG_Backend/DirectVulkan/Renderer/VulkanRenderer.cpp index 04a67702..db5465c5 100644 --- a/MobileGL/MG_Backend/DirectVulkan/Renderer/VulkanRenderer.cpp +++ b/MobileGL/MG_Backend/DirectVulkan/Renderer/VulkanRenderer.cpp @@ -607,10 +607,20 @@ namespace MobileGL::MG_Backend::DirectVulkan { } } + // Vertex attribute locations are tracked in Uint32 bitmasks, so MAX_VERTEX_ATTRIBS is both the + // state-layer storage bound and the width of every mask below. Keep them in lockstep. + static constexpr Uint32 kMaxVertexAttribs = + static_cast(MG_State::GLState::VertexArrayObject::MAX_VERTEX_ATTRIBS); + static_assert(kMaxVertexAttribs <= 32, "Vertex attribute masks are Uint32"); + // The loops below walk locations [0, kMaxVertexAttribs) and index programObj.vertexInputTypes with + // each one, so that array must be at least as wide. + static_assert(kMaxVertexAttribs <= ProgramFactory::VkProgramObject::kMaxVertexInputLocations, + "vertexInputTypes is indexed by vertex attribute location"); + static Uint32 BuildVertexInputAttributeMask(const Vector& attributes) { Uint32 attributeMask = 0; for (const auto& attribute : attributes) { - if (attribute.location < 32) { + if (attribute.location < kMaxVertexAttribs) { attributeMask |= (1u << attribute.location); } } @@ -2194,7 +2204,7 @@ void main() { } SizeT syntheticBinding = vertexInputState.bindings.size(); - for (Uint32 location = 0; location < 32; ++location) { + for (Uint32 location = 0; location < kMaxVertexAttribs; ++location) { if ((missingAttribMask & (1u << location)) == 0) { continue; } @@ -2206,9 +2216,13 @@ void main() { VkDeviceSize sourceSize = 0; const Bool supported = TryGetCurrentVertexAttributeUploadPayload(currentValue, glType, format, sourceData, sourceSize); - MOBILEGL_ASSERT(supported, - "DirectVulkan does not support current generic vertex attribute type yet: program=%u location=%u type=0x%x", - program.GetExternalIndex(), location, glType); + if (!supported) { + // SetupDraw's pre-flight should have rejected this already; never upload a null payload. + MGLOG_E("UploadAndBindVertexStreams skipped: unsupported current generic vertex attribute type: " + "program=%u location=%u type=0x%x", + program.GetExternalIndex(), location, glType); + return false; + } BufferSlice slice{}; if (!m_bufferManager.UploadTransient(BufferKind::Vertex, m_frameContext.GetCurrentFrameIndex(), @@ -2877,7 +2891,7 @@ void main() { patchedAttributes.assign(vis.attributes.begin(), vis.attributes.end()); Bool hasPatchedVertexAttributes = false; for (auto& attribute : patchedAttributes) { - if (attribute.location >= 32 || (activeAttribMask & (1u << attribute.location)) == 0) { + if (attribute.location >= kMaxVertexAttribs || (activeAttribMask & (1u << attribute.location)) == 0) { continue; } @@ -2920,7 +2934,7 @@ void main() { } Uint32 syntheticBinding = static_cast(vis.bindings.size()); - for (Uint32 location = 0; location < 32; ++location) { + for (Uint32 location = 0; location < kMaxVertexAttribs; ++location) { if ((missingAttribMask & (1u << location)) == 0) { continue; } @@ -3297,6 +3311,42 @@ void main() { return false; } + // Vertex-input pre-flight, run before pipeline creation so that a bad attribute can never be + // baked into a cached VkPipeline. + { + const auto& vertexInputState = m_vertexInputStateFactory->GetOrCreateVertexInputState(vao); + const Uint32 activeAttribMask = programObj.activeVertexInputLocationMask; + + // An enabled array whose GL type has no VkFormat mapping never reaches the vertex input + // state, which makes it indistinguishable from a disabled array: the draw would treat it as + // "missing" and silently feed the shader the current attribute value instead of the app's + // vertex data. Fail loudly rather than render wrong pixels. + const Uint32 brokenAttribMask = vertexInputState.unsupportedAttribMask & activeAttribMask; + if (brokenAttribMask != 0) { + MGLOG_E("SetupDraw skipped: program=%u reads vertex attribute location mask 0x%x whose enabled " + "array has no supported vertex format", + program.GetExternalIndex(), brokenAttribMask); + return false; + } + + // Every genuinely disabled attribute the shader reads must have a current-value type we can + // synthesize a binding for; otherwise the upload below would push a null payload. + const Uint32 missingAttribMask = + activeAttribMask & ~BuildVertexInputAttributeMask(vertexInputState.attributes); + for (Uint32 location = 0; location < kMaxVertexAttribs; ++location) { + if ((missingAttribMask & (1u << location)) == 0) continue; + + const GLenum glType = programObj.vertexInputTypes[location]; + if (MG_State::GLState::ClassifyVertexAttribType(glType).baseType == + MG_State::GLState::VertexAttribBaseType::Unsupported) { + MGLOG_E("SetupDraw skipped: program=%u location=%u has no enabled array and its shader input " + "type 0x%x is not supported as a current generic vertex attribute", + program.GetExternalIndex(), location, glType); + return false; + } + } + } + auto pipeline = GetOrCreatePipeline(mode, program, programObj, transformFlags, vao, *renderPassEntry); activeRenderPass = VkRenderPassManager::GetActiveRenderPass(); diff --git a/MobileGL/MG_Impl/GLImpl/Getter/GL_Getter.cpp b/MobileGL/MG_Impl/GLImpl/Getter/GL_Getter.cpp index 00e56e17..8057347a 100644 --- a/MobileGL/MG_Impl/GLImpl/Getter/GL_Getter.cpp +++ b/MobileGL/MG_Impl/GLImpl/Getter/GL_Getter.cpp @@ -9,6 +9,7 @@ #include "GL_Getter.h" #include #include +#include #include #include #include @@ -1845,7 +1846,10 @@ namespace MobileGL::MG_Impl::GLImpl { *params = dynamicParameters.MaxUniformBlockSize; break; case GL_MAX_VERTEX_ATTRIBS: - *params = dynamicParameters.MaxVertexAttribs; + // Single source of truth with the validators: the value reported here is exactly the bound + // glVertexAttrib*/glGetVertexAttrib*/glBindAttribLocation enforce, and it never exceeds the + // state layer's current-value storage capacity. + *params = static_cast(VertexArrayImpl::GetMaxVertexAttribs()); break; case GL_MAX_VERTEX_TEXTURE_IMAGE_UNITS: *params = dynamicParameters.MaxVertexTextureImageUnits; diff --git a/MobileGL/MG_Impl/GLImpl/Program/GL_Program.cpp b/MobileGL/MG_Impl/GLImpl/Program/GL_Program.cpp index dc693582..2191c177 100644 --- a/MobileGL/MG_Impl/GLImpl/Program/GL_Program.cpp +++ b/MobileGL/MG_Impl/GLImpl/Program/GL_Program.cpp @@ -8,6 +8,7 @@ #include "GL_Program.h" #include "Config.h" +#include #include #include #include @@ -270,7 +271,7 @@ namespace MobileGL::MG_Impl::GLImpl { } void BindAttribLocation_State(GLuint program, GLuint index, const GLchar* name) { - if (index >= MG_State::GLState::VertexArrayObject::MAX_VERTEX_ATTRIBS) { + if (index >= VertexArrayImpl::GetMaxVertexAttribs()) { MG_State::pGLContext->RecordError( ErrorCode::InvalidValue, MakeUnique("MG_Impl/GLImpl", __func__, diff --git a/MobileGL/MG_Impl/GLImpl/VertexArray/GL_VertexArray.cpp b/MobileGL/MG_Impl/GLImpl/VertexArray/GL_VertexArray.cpp index 35261711..63ac57d8 100644 --- a/MobileGL/MG_Impl/GLImpl/VertexArray/GL_VertexArray.cpp +++ b/MobileGL/MG_Impl/GLImpl/VertexArray/GL_VertexArray.cpp @@ -48,7 +48,11 @@ namespace MobileGL::MG_Impl::GLImpl { } static bool ValidateVertexBindingIndex(GLuint bindingindex, const char* funcName) { - if (bindingindex >= MG_State::GLState::VertexArrayObject::MAX_VERTEX_ATTRIB_BINDINGS) { + // Bound by the same dynamic limit as attribute indices: the default attribute -> binding + // mapping is the identity, so a binding point the backend cannot address as an attribute + // would resolve into an attribute the backend must then reject on every draw. Real drivers + // likewise report MAX_VERTEX_ATTRIB_BINDINGS == MAX_VERTEX_ATTRIBS. + if (bindingindex >= VertexArrayImpl::GetMaxVertexAttribs()) { MG_State::pGLContext->RecordError( ErrorCode::InvalidValue, MakeUnique("MG_Impl/GLImpl", funcName, @@ -503,6 +507,9 @@ namespace MobileGL::MG_Impl::GLImpl { MakeUnique("MG_Impl/GLImpl", __func__, "params pointer cannot be null.")); return; } + // GL_CURRENT_VERTEX_ATTRIB is context state and returns before TryGetVertexAttribute, so the + // index bound has to be enforced up front or an out-of-range index reads past the array. + if (!VertexArrayImpl::ValidateVertexAttributeIndex(index)) return; if (!ValidateVertexAttribPname(pname)) return; if (IsCurrentVertexAttribQuery(pname)) { @@ -558,6 +565,7 @@ namespace MobileGL::MG_Impl::GLImpl { MakeUnique("MG_Impl/GLImpl", __func__, "params pointer cannot be null.")); return; } + if (!VertexArrayImpl::ValidateVertexAttributeIndex(index)) return; if (!ValidateVertexAttribPname(pname)) return; if (IsCurrentVertexAttribQuery(pname)) { @@ -645,6 +653,7 @@ namespace MobileGL::MG_Impl::GLImpl { MakeUnique("MG_Impl/GLImpl", __func__, "params pointer cannot be null.")); return; } + if (!VertexArrayImpl::ValidateVertexAttributeIndex(index)) return; if (!ValidateVertexAttribPname(pname)) return; if (IsCurrentVertexAttribQuery(pname)) { diff --git a/MobileGL/MG_Impl/GLImpl/VertexArray/Validators.cpp b/MobileGL/MG_Impl/GLImpl/VertexArray/Validators.cpp index 9adf089f..4842b175 100644 --- a/MobileGL/MG_Impl/GLImpl/VertexArray/Validators.cpp +++ b/MobileGL/MG_Impl/GLImpl/VertexArray/Validators.cpp @@ -7,12 +7,22 @@ // End of Source File Header #include "Validators.h" +#include #include #include #include #include namespace MobileGL::MG_Impl::GLImpl::VertexArrayImpl { + Uint GetMaxVertexAttribs() { + constexpr Uint capacity = static_cast(MG_State::GLState::VertexArrayObject::MAX_VERTEX_ATTRIBS); + if (!MG_Backend::pActiveBackendObject) return capacity; + + const Int backendLimit = MG_Backend::pActiveBackendObject->GetDynamicParameters().MaxVertexAttribs; + if (backendLimit <= 0) return capacity; + return std::min(static_cast(backendLimit), capacity); + } + Bool ValidateVertexArrayName(Uint index) { Bool isValid = MG_State::pGLContext->ValidateVertexArrayName(index); if (!isValid) { @@ -37,13 +47,13 @@ namespace MobileGL::MG_Impl::GLImpl::VertexArrayImpl { } Bool ValidateVertexAttributeIndex(Uint index) { - if (index >= MG_State::GLState::VertexArrayObject::MAX_VERTEX_ATTRIBS) { + const Uint maxVertexAttribs = GetMaxVertexAttribs(); + if (index >= maxVertexAttribs) { MG_State::pGLContext->RecordError( ErrorCode::InvalidValue, MakeUnique( "MG_Impl/GLImpl", "ValidateVertexAttributeIndex", - std::format("Attribute index {} exceeds maximum of {}.", index, - MG_State::GLState::VertexArrayObject::MAX_VERTEX_ATTRIBS - 1))); + std::format("Attribute index {} exceeds maximum of {}.", index, maxVertexAttribs - 1))); return false; } return true; diff --git a/MobileGL/MG_Impl/GLImpl/VertexArray/Validators.h b/MobileGL/MG_Impl/GLImpl/VertexArray/Validators.h index 198c70b6..dcf1728c 100644 --- a/MobileGL/MG_Impl/GLImpl/VertexArray/Validators.h +++ b/MobileGL/MG_Impl/GLImpl/VertexArray/Validators.h @@ -11,6 +11,10 @@ #include namespace MobileGL::MG_Impl::GLImpl::VertexArrayImpl { + // The GL-visible GL_MAX_VERTEX_ATTRIBS: min(active backend limit, VertexArrayObject storage + // capacity). Falls back to the capacity when no backend is active (unit tests). + Uint GetMaxVertexAttribs(); + Bool ValidateVertexArrayName(Uint index); Bool ValidateVertexArrayObject(Uint index); Bool ValidateVertexAttributeIndex(Uint index); diff --git a/MobileGL/MG_State/GLState/Core.cpp b/MobileGL/MG_State/GLState/Core.cpp index 30a7851e..39cecbd5 100644 --- a/MobileGL/MG_State/GLState/Core.cpp +++ b/MobileGL/MG_State/GLState/Core.cpp @@ -145,9 +145,32 @@ namespace MobileGL::MG_State { return m_vertexArrayState.GetBoundVertexArray(); } + VertexAttribTypeInfo ClassifyVertexAttribType(GLenum glType) { + switch (glType) { + case GL_FLOAT: return {VertexAttribBaseType::Float, 1}; + case GL_FLOAT_VEC2: return {VertexAttribBaseType::Float, 2}; + case GL_FLOAT_VEC3: return {VertexAttribBaseType::Float, 3}; + case GL_FLOAT_VEC4: return {VertexAttribBaseType::Float, 4}; + case GL_INT: return {VertexAttribBaseType::Int, 1}; + case GL_INT_VEC2: return {VertexAttribBaseType::Int, 2}; + case GL_INT_VEC3: return {VertexAttribBaseType::Int, 3}; + case GL_INT_VEC4: return {VertexAttribBaseType::Int, 4}; + case GL_UNSIGNED_INT: return {VertexAttribBaseType::Uint, 1}; + case GL_UNSIGNED_INT_VEC2: return {VertexAttribBaseType::Uint, 2}; + case GL_UNSIGNED_INT_VEC3: return {VertexAttribBaseType::Uint, 3}; + case GL_UNSIGNED_INT_VEC4: return {VertexAttribBaseType::Uint, 4}; + default: return {}; + } + } + + // The three accessors below are reachable from backend draw paths with a location taken from + // shader reflection, so the bound must be enforced at runtime rather than by MOBILEGL_ASSERT + // (which expands to nothing outside debug builds). void GLContext::SetCurrentVertexAttributeFloat(Uint index, const Array& value) { - MOBILEGL_ASSERT(index < m_currentVertexAttributes.size(), - "SetCurrentVertexAttributeFloat: index %u is out of range", index); + if (index >= m_currentVertexAttributes.size()) { + MGLOG_E("SetCurrentVertexAttributeFloat: index %u is out of range", index); + return; + } auto& current = m_currentVertexAttributes[index]; current.floatValue = value; @@ -158,8 +181,10 @@ namespace MobileGL::MG_State { } void GLContext::SetCurrentVertexAttributeInt(Uint index, const Array& value) { - MOBILEGL_ASSERT(index < m_currentVertexAttributes.size(), - "SetCurrentVertexAttributeInt: index %u is out of range", index); + if (index >= m_currentVertexAttributes.size()) { + MGLOG_E("SetCurrentVertexAttributeInt: index %u is out of range", index); + return; + } auto& current = m_currentVertexAttributes[index]; current.intValue = value; @@ -170,8 +195,10 @@ namespace MobileGL::MG_State { } void GLContext::SetCurrentVertexAttributeUint(Uint index, const Array& value) { - MOBILEGL_ASSERT(index < m_currentVertexAttributes.size(), - "SetCurrentVertexAttributeUint: index %u is out of range", index); + if (index >= m_currentVertexAttributes.size()) { + MGLOG_E("SetCurrentVertexAttributeUint: index %u is out of range", index); + return; + } auto& current = m_currentVertexAttributes[index]; current.uintValue = value; @@ -182,8 +209,11 @@ namespace MobileGL::MG_State { } const CurrentVertexAttributeValue& GLContext::GetCurrentVertexAttribute(Uint index) const { - MOBILEGL_ASSERT(index < m_currentVertexAttributes.size(), - "GetCurrentVertexAttribute: index %u is out of range", index); + static const CurrentVertexAttributeValue defaultValue{}; + if (index >= m_currentVertexAttributes.size()) { + MGLOG_E("GetCurrentVertexAttribute: index %u is out of range", index); + return defaultValue; + } return m_currentVertexAttributes[index]; } diff --git a/MobileGL/MG_State/GLState/Core.h b/MobileGL/MG_State/GLState/Core.h index e6df3ea7..291f2a44 100644 --- a/MobileGL/MG_State/GLState/Core.h +++ b/MobileGL/MG_State/GLState/Core.h @@ -31,6 +31,20 @@ namespace MobileGL { Array uintValue{0u, 0u, 0u, 1u}; }; + // Which of the three views above a shader input of a given GLSL type consumes. + enum class VertexAttribBaseType { Unsupported, Float, Int, Uint }; + + struct VertexAttribTypeInfo { + VertexAttribBaseType baseType = VertexAttribBaseType::Unsupported; + Uint componentCount = 0; + }; + + // Maps a shader vertex-input type (GL_FLOAT_VEC3, GL_INT_VEC2, ...) onto the current-value + // view that feeds it. Shared by every backend so that "a disabled array reads the current + // value" resolves identically regardless of which backend is active; each backend only + // translates the result into its own API call. + VertexAttribTypeInfo ClassifyVertexAttribType(GLenum glType); + class GLContext { public: GLContext() = default; diff --git a/MobileGL/MG_State/GLState/ProgramState/ProgramObject.cpp b/MobileGL/MG_State/GLState/ProgramState/ProgramObject.cpp index 56abafb1..63644c3a 100644 --- a/MobileGL/MG_State/GLState/ProgramState/ProgramObject.cpp +++ b/MobileGL/MG_State/GLState/ProgramState/ProgramObject.cpp @@ -7,6 +7,8 @@ // End of Source File Header #include "ProgramObject.h" +#include +#include #include #include #include @@ -20,6 +22,22 @@ void main() {} )"; namespace { + // How many vertex input locations reflection may record. Backends consume this through + // GetActiveAttributeLocationMask()/GetAttribType(), so a value below the advertised + // GL_MAX_VERTEX_ATTRIBS would make a legal attribute location invisible to them -- DirectGLES would + // then never feed the shader that attribute's current value. Bounded by the state layer's storage + // capacity, which is also the width of the Uint32 masks backends build from it. + static MobileGL::Int GetReflectionVertexAttribLimit() { + constexpr MobileGL::Int capacity = + static_cast(MobileGL::MG_State::GLState::VertexArrayObject::MAX_VERTEX_ATTRIBS); + if (!MobileGL::MG_Backend::pActiveBackendObject) return capacity; + + const MobileGL::Int backendLimit = + MobileGL::MG_Backend::pActiveBackendObject->GetDynamicParameters().MaxVertexAttribs; + if (backendLimit <= 0) return capacity; + return std::min(backendLimit, capacity); + } + static MobileGL::String StripArrayElementSuffix(const MobileGL::String& name) { const MobileGL::SizeT bracket = name.find('['); return bracket == MobileGL::String::npos ? name : name.substr(0, bracket); @@ -460,7 +478,7 @@ namespace MobileGL::MG_State::GLState { maxLoc = std::max(0, inCount - 1); } - GLint maxAttribs = 16; // TODO: get from backend + const GLint maxAttribs = GetReflectionVertexAttribLimit(); MGLOG_D("ProgramObject %u: Reflection - computed maxLoc=%d, using maxAttribs=%d", m_externalIndex, maxLoc, maxAttribs); diff --git a/MobileGL/MG_State/GLState/VertexArrayState/VertexArrayObject.h b/MobileGL/MG_State/GLState/VertexArrayState/VertexArrayObject.h index 9497dd76..a93227a1 100644 --- a/MobileGL/MG_State/GLState/VertexArrayState/VertexArrayObject.h +++ b/MobileGL/MG_State/GLState/VertexArrayState/VertexArrayObject.h @@ -44,8 +44,12 @@ namespace MobileGL { class VertexArrayObject { public: - static constexpr int MAX_VERTEX_ATTRIBS = 16; - static constexpr int MAX_VERTEX_ATTRIB_BINDINGS = 16; + // Storage capacity, not the GL-visible limit. GL_MAX_VERTEX_ATTRIBS is reported as + // min(backend limit, MAX_VERTEX_ATTRIBS) and validated against that dynamic value; + // 32 is the width of the Uint32 attribute masks the backends pass around, so it is + // also the hard ceiling. + static constexpr int MAX_VERTEX_ATTRIBS = 32; + static constexpr int MAX_VERTEX_ATTRIB_BINDINGS = 32; VertexArrayObject(Uint externIndex); @@ -104,14 +108,24 @@ namespace MobileGL { void BumpAttributeSwitchVersion(Uint index); void ResolveAttributeFromBinding(Uint attribIndex); + // The default mapping is attribute i -> binding point i. Keep it an iota over + // MAX_VERTEX_ATTRIBS rather than a literal list: a literal list silently leaves the + // tail mapped to binding point 0 whenever the limit grows. + static constexpr Array MakeIdentityAttributeBindings() { + Array mapping{}; + for (Uint index = 0; index < static_cast(MAX_VERTEX_ATTRIBS); ++index) { + mapping[index] = index; + } + return mapping; + } + const Uint m_externalIndex = 0; Array m_attributes; Array m_attributeVersions; BindingSlot m_indexBufferBindingSlot; Array m_bindingPoints; - Array m_attributeBindingIndex = {0, 1, 2, 3, 4, 5, 6, 7, - 8, 9, 10, 11, 12, 13, 14, 15}; + Array m_attributeBindingIndex = MakeIdentityAttributeBindings(); Array m_attributeRelativeOffset = {}; // Set once an attribute (or its binding point) is touched through the // ARB_vertex_attrib_binding API; only such attributes are re-resolved, so the diff --git a/MobileGL/MG_Test/SanityTest.cpp b/MobileGL/MG_Test/SanityTest.cpp index 498258f9..01fc4e45 100644 --- a/MobileGL/MG_Test/SanityTest.cpp +++ b/MobileGL/MG_Test/SanityTest.cpp @@ -19,6 +19,7 @@ #include #include #include +#include #include #include #include @@ -408,6 +409,55 @@ void main() { MG_Backend::pActiveBackendObject.reset(); } +// GL_MAX_VERTEX_ATTRIBS must follow the backend but never exceed the state layer's current-value +// storage: the DirectVulkan draw path indexes that array by shader input location, so advertising more +// than it can hold is an out-of-bounds read waiting to happen. +TEST(GetterSanity, ClampsMaxVertexAttribsToCurrentValueStorageCapacity) { + using namespace MobileGL; + constexpr GLint capacity = MG_State::GLState::VertexArrayObject::MAX_VERTEX_ATTRIBS; + + MG_State::pGLContext = MakeUnique(); + + // A driver reporting more attributes than MobileGL can store gets clamped. + { + MG_Backend::DynamicBackendParameters params; + params.MaxVertexAttribs = capacity * 2; + MG_Backend::pActiveBackendObject = MakeUnique(params); + + EXPECT_EQ(MG_Impl::GLImpl::VertexArrayImpl::GetMaxVertexAttribs(), static_cast(capacity)); + GLint reported = 0; + MG_Impl::GLImpl::GetIntegerv(GL_MAX_VERTEX_ATTRIBS, &reported); + EXPECT_EQ(reported, capacity); + MG_Backend::pActiveBackendObject.reset(); + } + + // A driver below the capacity is followed exactly, and validation enforces that same bound. + { + MG_Backend::DynamicBackendParameters params; + params.MaxVertexAttribs = 16; + MG_Backend::pActiveBackendObject = MakeUnique(params); + + EXPECT_EQ(MG_Impl::GLImpl::VertexArrayImpl::GetMaxVertexAttribs(), 16u); + GLint reported = 0; + MG_Impl::GLImpl::GetIntegerv(GL_MAX_VERTEX_ATTRIBS, &reported); + EXPECT_EQ(reported, 16); + + MG_State::pGLContext->ClearErrors(); + EXPECT_FALSE(MG_Impl::GLImpl::VertexArrayImpl::ValidateVertexAttributeIndex(16)); + EXPECT_TRUE(MG_State::pGLContext->HasGLError()); + MG_State::pGLContext->ClearErrors(); + EXPECT_TRUE(MG_Impl::GLImpl::VertexArrayImpl::ValidateVertexAttributeIndex(15)); + EXPECT_FALSE(MG_State::pGLContext->HasGLError()); + + MG_Backend::pActiveBackendObject.reset(); + } + + // With no active backend the storage capacity is the bound, and nothing dereferences a null backend. + EXPECT_EQ(MG_Impl::GLImpl::VertexArrayImpl::GetMaxVertexAttribs(), static_cast(capacity)); + + MG_State::pGLContext.reset(); +} + TEST(GetterSanity, ReportsKhrSubgroupDynamicParameters) { using namespace MobileGL; diff --git a/MobileGL/MG_Test/VertexArray/VertexArrayTest.cpp b/MobileGL/MG_Test/VertexArray/VertexArrayTest.cpp index 8f19fb2f..ea0e82b4 100644 --- a/MobileGL/MG_Test/VertexArray/VertexArrayTest.cpp +++ b/MobileGL/MG_Test/VertexArray/VertexArrayTest.cpp @@ -14,6 +14,7 @@ #include #include #include +#include #include using namespace MobileGL; @@ -225,6 +226,75 @@ TEST_F(VertexArrayTest, BoundVAOPreservesState) { ASSERT_FALSE(vao2->IsAttributeEnabled(0)); } +// The current-value array must cover the full attribute capacity. It used to be sized 16 while the +// DirectVulkan draw path indexed it with shader locations up to 31, reading past the end; the only +// guard was MOBILEGL_ASSERT, which expands to nothing outside debug builds. +TEST_F(VertexArrayTest, CurrentVertexAttributeStorageCoversFullCapacity) { + constexpr Uint capacity = static_cast(MG_State::GLState::VertexArrayObject::MAX_VERTEX_ATTRIBS); + const Uint highIndex = capacity - 1; + + MG_State::pGLContext->SetCurrentVertexAttributeFloat(highIndex, {1.0f, 2.0f, 3.0f, 4.0f}); + const auto& stored = MG_State::pGLContext->GetCurrentVertexAttribute(highIndex); + EXPECT_FLOAT_EQ(stored.floatValue[0], 1.0f); + EXPECT_FLOAT_EQ(stored.floatValue[3], 4.0f); + + // Neighbouring slots keep the GL default of (0, 0, 0, 1). + const auto& untouched = MG_State::pGLContext->GetCurrentVertexAttribute(highIndex - 1); + EXPECT_FLOAT_EQ(untouched.floatValue[0], 0.0f); + EXPECT_FLOAT_EQ(untouched.floatValue[3], 1.0f); + + // Out-of-range access must be bounded at runtime, not just asserted in debug builds. + const auto& outOfRange = MG_State::pGLContext->GetCurrentVertexAttribute(capacity); + EXPECT_FLOAT_EQ(outOfRange.floatValue[0], 0.0f); + EXPECT_FLOAT_EQ(outOfRange.floatValue[3], 1.0f); + + MG_State::pGLContext->SetCurrentVertexAttributeFloat(capacity, {9.0f, 9.0f, 9.0f, 9.0f}); + EXPECT_FLOAT_EQ(MG_State::pGLContext->GetCurrentVertexAttribute(highIndex).floatValue[0], 1.0f); +} + +// A binding point the backend cannot address as an attribute must be rejected: the default mapping +// is the identity, so accepting it would resolve into an attribute index the backend then rejects on +// every draw. +TEST_F(VertexArrayTest, VertexBindingIndexIsBoundedByTheAdvertisedAttribLimit) { + Vector vaoNames; + MG_State::pGLContext->GenVertexArrayNames(1, vaoNames); + MG_State::pGLContext->CreateVertexArrayObject(vaoNames[0]); + MG_State::pGLContext->BindVertexArray(vaoNames[0]); + MG_State::pGLContext->ClearErrors(); + + const GLuint outOfRange = MG_Impl::GLImpl::VertexArrayImpl::GetMaxVertexAttribs(); + MG_Impl::GLImpl::VertexAttribBinding(0, outOfRange); + EXPECT_TRUE(MG_State::pGLContext->HasGLError()); +} + +// The default attribute -> binding-point mapping is the identity. It used to be a 16-element literal +// list, so every attribute at or above 16 silently resolved against binding point 0 instead. +TEST_F(VertexArrayTest, DefaultAttributeBindingIsIdentityAcrossFullCapacity) { + Vector vaoNames; + MG_State::pGLContext->GenVertexArrayNames(1, vaoNames); + auto vao = MG_State::pGLContext->CreateVertexArrayObject(vaoNames[0]); + MG_State::pGLContext->BindVertexArray(vaoNames[0]); + + auto vbo = CreateTestVBO(); + + constexpr Uint kHighAttrib = 20; + static_assert(kHighAttrib >= 16, "must exceed the old 16-entry identity list"); + static_assert(kHighAttrib < MG_State::GLState::VertexArrayObject::MAX_VERTEX_ATTRIBS); + static_assert(kHighAttrib < MG_State::GLState::VertexArrayObject::MAX_VERTEX_ATTRIB_BINDINGS); + + // Binding point kHighAttrib must feed attribute kHighAttrib with no explicit SetAttributeBinding. + vao->SetAttributeFormatSeparate(kHighAttrib, 3, DataType::Float32, false, false, 12); + vao->SetBindingBuffer(kHighAttrib, vbo, 16, 24); + + const auto& attr = vao->GetAttribute(kHighAttrib); + EXPECT_EQ(attr.Buffer, vbo); + EXPECT_EQ(attr.Stride, 24); + EXPECT_EQ(attr.Offset, 28u); // binding offset (16) + attribute relative offset (12) + + // Attribute 0 must not have been dragged along by binding point kHighAttrib. + EXPECT_EQ(vao->GetAttribute(0).Buffer, nullptr); +} + using namespace MobileGL::MG_Impl::GLImpl; class GeneralVertexArrayTest : public ::testing::Test { @@ -674,3 +744,109 @@ TEST_F(GeneralVertexArrayTest, General_ElementBufferBindingPoint) { EXPECT_EQ(GetError(), GL_NO_ERROR); } + +// GL_CURRENT_VERTEX_ATTRIB is per-context state that exists for every index below +// GL_MAX_VERTEX_ATTRIBS, and defaults to (0, 0, 0, 1). +TEST_F(GeneralVertexArrayTest, General_CurrentVertexAttribRoundTripsAtHighestLegalIndex) { + CreateVAO(); + const GLuint highIndex = VertexArrayImpl::GetMaxVertexAttribs() - 1; + ASSERT_GT(highIndex, 0u); + + VertexAttrib4f(highIndex, 1.0f, 2.0f, 3.0f, 4.0f); + EXPECT_EQ(GetError(), GL_NO_ERROR); + + GLfloat values[4] = {-1.0f, -1.0f, -1.0f, -1.0f}; + GetVertexAttribfv(highIndex, GL_CURRENT_VERTEX_ATTRIB, values); + EXPECT_EQ(GetError(), GL_NO_ERROR); + EXPECT_FLOAT_EQ(values[0], 1.0f); + EXPECT_FLOAT_EQ(values[1], 2.0f); + EXPECT_FLOAT_EQ(values[2], 3.0f); + EXPECT_FLOAT_EQ(values[3], 4.0f); + + // Untouched attributes keep the GL default of (0, 0, 0, 1). + GLfloat defaults[4] = {-1.0f, -1.0f, -1.0f, -1.0f}; + GetVertexAttribfv(highIndex - 1, GL_CURRENT_VERTEX_ATTRIB, defaults); + EXPECT_EQ(GetError(), GL_NO_ERROR); + EXPECT_FLOAT_EQ(defaults[0], 0.0f); + EXPECT_FLOAT_EQ(defaults[1], 0.0f); + EXPECT_FLOAT_EQ(defaults[2], 0.0f); + EXPECT_FLOAT_EQ(defaults[3], 1.0f); +} + +// glVertexAttrib{1,2,3}f fill the components the caller omitted with (0, 0, 1). +TEST_F(GeneralVertexArrayTest, General_CurrentVertexAttribFillsOmittedComponents) { + CreateVAO(); + + VertexAttrib1f(1, 7.0f); + GLfloat one[4] = {}; + GetVertexAttribfv(1, GL_CURRENT_VERTEX_ATTRIB, one); + EXPECT_FLOAT_EQ(one[0], 7.0f); + EXPECT_FLOAT_EQ(one[1], 0.0f); + EXPECT_FLOAT_EQ(one[2], 0.0f); + EXPECT_FLOAT_EQ(one[3], 1.0f); + + VertexAttrib2f(2, 7.0f, 8.0f); + GLfloat two[4] = {}; + GetVertexAttribfv(2, GL_CURRENT_VERTEX_ATTRIB, two); + EXPECT_FLOAT_EQ(two[1], 8.0f); + EXPECT_FLOAT_EQ(two[2], 0.0f); + EXPECT_FLOAT_EQ(two[3], 1.0f); + + VertexAttrib3f(3, 7.0f, 8.0f, 9.0f); + GLfloat three[4] = {}; + GetVertexAttribfv(3, GL_CURRENT_VERTEX_ATTRIB, three); + EXPECT_FLOAT_EQ(three[2], 9.0f); + EXPECT_FLOAT_EQ(three[3], 1.0f); + + EXPECT_EQ(GetError(), GL_NO_ERROR); +} + +// The integer current-value views must survive a round trip without going through float. +TEST_F(GeneralVertexArrayTest, General_CurrentVertexAttribIntegerRoundTrip) { + CreateVAO(); + + VertexAttribI4i(1, -5, 6, -7, 8); + GLint signedValues[4] = {}; + GetVertexAttribIiv(1, GL_CURRENT_VERTEX_ATTRIB, signedValues); + EXPECT_EQ(signedValues[0], -5); + EXPECT_EQ(signedValues[1], 6); + EXPECT_EQ(signedValues[2], -7); + EXPECT_EQ(signedValues[3], 8); + + VertexAttribI4ui(2, 10u, 20u, 30u, 40u); + GLuint unsignedValues[4] = {}; + GetVertexAttribIuiv(2, GL_CURRENT_VERTEX_ATTRIB, unsignedValues); + EXPECT_EQ(unsignedValues[0], 10u); + EXPECT_EQ(unsignedValues[3], 40u); + + EXPECT_EQ(GetError(), GL_NO_ERROR); +} + +// The GL_CURRENT_VERTEX_ATTRIB branch returned before any index validation, so an out-of-range +// index silently read past the current-value array instead of raising GL_INVALID_VALUE. +TEST_F(GeneralVertexArrayTest, General_CurrentVertexAttribQueryRejectsOutOfRangeIndex) { + CreateVAO(); + const GLuint outOfRange = VertexArrayImpl::GetMaxVertexAttribs(); + + GLfloat floats[4] = {-1.0f, -2.0f, -3.0f, -4.0f}; + GetVertexAttribfv(outOfRange, GL_CURRENT_VERTEX_ATTRIB, floats); + EXPECT_EQ(GetError(), GL_INVALID_VALUE); + EXPECT_FLOAT_EQ(floats[0], -1.0f); + EXPECT_FLOAT_EQ(floats[3], -4.0f); + + GLint ints[4] = {-1, -2, -3, -4}; + GetVertexAttribiv(outOfRange, GL_CURRENT_VERTEX_ATTRIB, ints); + EXPECT_EQ(GetError(), GL_INVALID_VALUE); + EXPECT_EQ(ints[0], -1); + + GLint signedInts[4] = {-1, -2, -3, -4}; + GetVertexAttribIiv(outOfRange, GL_CURRENT_VERTEX_ATTRIB, signedInts); + EXPECT_EQ(GetError(), GL_INVALID_VALUE); + EXPECT_EQ(signedInts[0], -1); + + GLuint uints[4] = {1u, 2u, 3u, 4u}; + GetVertexAttribIuiv(outOfRange, GL_CURRENT_VERTEX_ATTRIB, uints); + EXPECT_EQ(GetError(), GL_INVALID_VALUE); + EXPECT_EQ(uints[0], 1u); +} + diff --git a/MobileGL/MG_Util/SelfTest/DriverPost.cpp b/MobileGL/MG_Util/SelfTest/DriverPost.cpp index 768eb83e..f42d6995 100644 --- a/MobileGL/MG_Util/SelfTest/DriverPost.cpp +++ b/MobileGL/MG_Util/SelfTest/DriverPost.cpp @@ -12,6 +12,9 @@ #include #include #include +// Only for the compile-time MAX_VERTEX_ATTRIBS constant asserted below. The POST still executes no +// MG_State code: it runs standalone, before MG_State::Init(). +#include #include #include #include @@ -167,6 +170,49 @@ namespace MobileGL::MG_Util::SelfTest { : ""; } + // ---- Vertex attribute limit -------------------------------------------- + // GL 3.3 Core mandates GL_MAX_VERTEX_ATTRIBS >= 16 (spec table 6.32); a driver below + // that cannot back a conformant core context at all. + constexpr Int kGL33MinVertexAttribs = 16; + + // The capacity of the per-context current-vertex-attribute array, which is also the width of + // the Uint32 attribute masks the backends pass around. Pinned to the state layer's constant so + // the two can never drift: a mismatch between them is precisely the defect this row guards. + constexpr Int kMobileGLMaxVertexAttribs = MG_State::GLState::VertexArrayObject::MAX_VERTEX_ATTRIBS; + static_assert(kMobileGLMaxVertexAttribs <= 32, "Vertex attribute masks are Uint32"); + static_assert(kMobileGLMaxVertexAttribs >= kGL33MinVertexAttribs, + "MobileGL cannot advertise a conformant GL 3.3 Core GL_MAX_VERTEX_ATTRIBS"); + + // Both backends index a fixed-size, per-context array of current generic vertex attribute + // values by shader input location, and both clamp the GL_MAX_VERTEX_ATTRIBS they advertise + // to that array's capacity. A driver reporting more attributes than the array can hold used + // to make the DirectVulkan draw path walk locations past the end of it -- an out-of-bounds + // read in release builds, and a MOBILEGL_ASSERT abort in debug builds -- as soon as a shader + // declared a vertex input at a high location whose array was disabled. The clamp closes that + // hole, so this row exists to make the underlying driver/host mismatch visible rather than + // silently swallowed. + void EvaluateVertexAttribLimit(ReportBuilder& builder, Int deviceLimit, const char* rowName, + const char* driverLimitName) { + if (deviceLimit < kGL33MinVertexAttribs) { + builder.Fail(rowName, + format("{} = {} (< {}); OpenGL 3.3 Core requires at least {} generic vertex " + "attributes, so this driver cannot back a conformant core context", + driverLimitName, deviceLimit, kGL33MinVertexAttribs, kGL33MinVertexAttribs)); + return; + } + if (deviceLimit > kMobileGLMaxVertexAttribs) { + builder.Warn(rowName, + format("{} = {} (> {}); MobileGL clamps GL_MAX_VERTEX_ATTRIBS to {} because its " + "current-vertex-attribute storage and its Uint32 attribute masks hold {} " + "locations, so the driver's extra attributes stay unusable", + driverLimitName, deviceLimit, kMobileGLMaxVertexAttribs, + kMobileGLMaxVertexAttribs, kMobileGLMaxVertexAttribs)); + return; + } + builder.Pass(rowName, format("{} = {}; MobileGL advertises GL_MAX_VERTEX_ATTRIBS = {}", + driverLimitName, deviceLimit, deviceLimit)); + } + void EvaluateGlesChecklist(ReportBuilder& builder, const MG_External::GLESCapabilities& caps, const MG_External::GLESFunctionsTable& glesFuncs) { const Int major = caps.GLESVersion.Major; @@ -185,6 +231,9 @@ namespace MobileGL::MG_Util::SelfTest { versionLabel + " (< 3.1: no compute shaders or native indirect draws)"); } + EvaluateVertexAttribLimit(builder, caps.MaxVertexAttribs, "Vertex attributes", + "GL_MAX_VERTEX_ATTRIBS"); + if (es31) { GLint maxVertexSsboBlocks = 0; glesFuncs.glGetIntegerv(GL_MAX_VERTEX_SHADER_STORAGE_BLOCKS, &maxVertexSsboBlocks); @@ -1006,6 +1055,9 @@ namespace MobileGL::MG_Util::SelfTest { VkApiVersionToString(properties.apiVersion))); } + EvaluateVertexAttribLimit(builder, static_cast(properties.limits.maxVertexInputAttributes), + "Vertex attributes", "maxVertexInputAttributes"); + Vector deviceExtensions; Uint32 deviceExtensionCount = 0; if (vkEnumerateDeviceExtensionPropertiesFn(physicalDevice, nullptr, &deviceExtensionCount, nullptr) ==