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

[Fix] (MG_State, MG_Impl, MG_Backend): conformant current generic vertex attribute values

Browse Source 
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).
This commit is contained in:
swung0x48
2026-07-10 11:23:16 -04:00
parent eb090c6170
commit d40f753983
18 changed files with 539 additions and 30 deletions
Download Patch File Download Diff File Expand all files Collapse all files
MobileGL/MG_State/GLState/Core.cpp
+38 -8
View File
@@ -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<Float, 4>& 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<Int32, 4>& 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<Uint32, 4>& 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];
}
MobileGL/MG_State/GLState/Core.h
+14
View File
@@ -31,6 +31,20 @@ namespace MobileGL {
Array<Uint32, 4> 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;
MobileGL/MG_State/GLState/ProgramState/ProgramObject.cpp
+19 -1
View File
@@ -7,6 +7,8 @@
// End of Source File Header
#include "ProgramObject.h"
#include <MG_Backend/BackendObjects.h>
#include <MG_State/GLState/VertexArrayState/VertexArrayObject.h>
#include <MG_Util/Converters/GLToStr/GLEnumConverter.h>
#include <MG_Util/ShaderTranspiler/Types.h>
#include <MG_Util/ShaderTranspiler/ShaderCompiler.h>
@@ -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::Int>(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);
MobileGL/MG_State/GLState/VertexArrayState/VertexArrayObject.h
+18 -4
View File
@@ -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<Uint, MAX_VERTEX_ATTRIBS> MakeIdentityAttributeBindings() {
Array<Uint, MAX_VERTEX_ATTRIBS> mapping{};
for (Uint index = 0; index < static_cast<Uint>(MAX_VERTEX_ATTRIBS); ++index) {
mapping[index] = index;
}
return mapping;
}
const Uint m_externalIndex = 0;
Array<VertexAttribute, MAX_VERTEX_ATTRIBS> m_attributes;
Array<VertexAttributeVersion, MAX_VERTEX_ATTRIBS> m_attributeVersions;
BindingSlot<BufferObject> m_indexBufferBindingSlot;
Array<VertexBufferBindingPoint, MAX_VERTEX_ATTRIB_BINDINGS> m_bindingPoints;
Array<Uint, MAX_VERTEX_ATTRIBS> m_attributeBindingIndex = {0, 1, 2, 3, 4, 5, 6, 7,
8, 9, 10, 11, 12, 13, 14, 15};
Array<Uint, MAX_VERTEX_ATTRIBS> m_attributeBindingIndex = MakeIdentityAttributeBindings();
Array<Uint, MAX_VERTEX_ATTRIBS> 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