[Fix, Test] (MG_Impl): multi-bind name rejection is per element, and only transform feedback constrains the range size to a multiple of four

MobileGL/MG_Impl/GLImpl/Buffer/GL_Buffer.cpp

@@ -1527,19 +1527,27 @@ namespace MobileGL::MG_Impl::GLImpl {
                 return false;
             }
         }
-        // A transform feedback capture binding is addressed in 32-bit components, so BOTH the
+        // GL 4.6 core 6.1.1 constrains the OFFSET to a multiple of four for both
-        // offset and the size must be multiples of 4. An atomic counter binding is addressed in
+        // TRANSFORM_FEEDBACK_BUFFER and ATOMIC_COUNTER_BUFFER (the atomic-counter one has no
-        // 32-bit counters and constrains its offset the same way (GL 4.6 core 6.1.1) - that one
+        // queryable alignment pname, which is why it was missing here), and the SIZE only for
-        // has no queryable alignment pname, which is why it was missing here.
+        // transform feedback, whose capture is written in whole 32-bit components. Extending the
-        const Bool isFourByteAddressed =
+        // size rule to atomic counters as well breaks a legal bind: the conformance suite splits
-            target == GL_TRANSFORM_FEEDBACK_BUFFER || target == GL_ATOMIC_COUNTER_BUFFER;
+        // MAX_ATOMIC_COUNTER_BUFFER_SIZE evenly across the binding points and that quotient is
-        if (isFourByteAddressed && ((offset % 4) != 0 || (hasBuffer && (size % 4) != 0))) {
+        // not required to land on four.
+        if ((target == GL_TRANSFORM_FEEDBACK_BUFFER || target == GL_ATOMIC_COUNTER_BUFFER) && (offset % 4) != 0) {
+            MG_State::pGLContext->RecordError(
+                ErrorCode::InvalidValue,
+                MakeUnique<GenericErrorInfo>("MG_Impl/GLImpl", funcName,
+                                             std::format("offset ({}) must be a multiple of 4 for {}.", offset,
+                                                         MG_Util::ConvertGLEnumToString(target))));
+            return false;
+        }
+        if (target == GL_TRANSFORM_FEEDBACK_BUFFER && hasBuffer && (size % 4) != 0) {
             MG_State::pGLContext->RecordError(
                 ErrorCode::InvalidValue,
                 MakeUnique<GenericErrorInfo>(
                     "MG_Impl/GLImpl", funcName,
-                    std::format("offset ({}) and size ({}) must both be multiples of 4 for {}.", offset, size,
+                    std::format("size ({}) must be a multiple of 4 for GL_TRANSFORM_FEEDBACK_BUFFER.", size)));
-                                MG_Util::ConvertGLEnumToString(target))));
             return false;
         }
         return true;
@@ -1742,29 +1750,28 @@ namespace MobileGL::MG_Impl::GLImpl {
 
     // ARB_multi_bind states the equivalence to a loop of single binds "except that ... buffers
     // will not be created if they do not exist": glBindBuffer instantiates a name glGenBuffers
-    // merely reserved, glBindBuffers* must refuse it instead. That is INVALID_OPERATION, and it
+    // merely reserved, glBindBuffers* must refuse it and raise INVALID_OPERATION instead
-    // is an all-or-nothing check - one bad name leaves every binding point in the range alone
     // (KHR-GL44.multi_bind.errors_bind_buffers).
-    static Bool ValidateMultiBindBufferNames(const GLuint* buffers, GLsizei count, const char* funcName) {
+    //
-        if (buffers == nullptr) return true;
+    // Deliberately PER ELEMENT, not all-or-nothing: the equivalence the extension defines is a
-        for (GLsizei i = 0; i < count; ++i) {
+    // loop, so a bad entry costs its own binding point and nothing else. Rejecting the whole
-            if (buffers[i] == 0) continue;
+    // call instead cost multi_bind.functional_bind_buffers_base its bindings.
-            if (MG_State::pGLContext->ValidateBufferObject(buffers[i])) continue;
+    static Bool IsExistingBufferForMultiBind(GLuint buffer, GLsizei index, const char* funcName) {
+        if (buffer == 0 || MG_State::pGLContext->ValidateBufferObject(buffer)) return true;
         MG_State::pGLContext->RecordError(
             ErrorCode::InvalidOperation,
             MakeUnique<GenericErrorInfo>(
                 "MG_Impl/GLImpl", funcName,
-                    std::format("buffers[{}] ({}) is not the name of an existing buffer object.", i, buffers[i])));
+                std::format("buffers[{}] ({}) is not the name of an existing buffer object.", index, buffer)));
         return false;
     }
-        return true;
-    }
 
     void BindBuffersBase(GLenum target, GLuint first, GLsizei count, const GLuint* buffers) {
         if (!ValidateMultiBindBufferRange(target, first, count, __func__)) return;
-        if (!ValidateMultiBindBufferNames(buffers, count, __func__)) return;
         for (GLsizei i = 0; i < count; ++i) {
-            BindBufferBase_State(target, first + i, buffers ? buffers[i] : 0);
+            const GLuint buffer = buffers ? buffers[i] : 0;
+            if (!IsExistingBufferForMultiBind(buffer, i, __func__)) continue;
+            BindBufferBase_State(target, first + i, buffer);
         }
     }
 
@@ -1777,8 +1784,8 @@ namespace MobileGL::MG_Impl::GLImpl {
     void BindBuffersRange(GLenum target, GLuint first, GLsizei count, const GLuint* buffers, const GLintptr* offsets,
                           const GLsizeiptr* sizes) {
         if (!ValidateMultiBindBufferRange(target, first, count, __func__)) return;
-        if (!ValidateMultiBindBufferNames(buffers, count, __func__)) return;
         for (GLsizei i = 0; i < count; ++i) {
+            if (buffers && !IsExistingBufferForMultiBind(buffers[i], i, __func__)) continue;
             if (!buffers || buffers[i] == 0) {
                 BindBufferBase_State(target, first + i, 0);
             } else {

MobileGL/MG_Impl/GLImpl/Sampler/GL_Sampler.cpp

@@ -336,27 +336,22 @@ namespace MobileGL::MG_Impl::GLImpl {
             return;
         }
 
-        // ...and the names are checked up front for the same reason, with the extra rule that
+        // ARB_multi_bind adds one rule the single-bind path does not have: "samplers will not be
-        // ARB_multi_bind spells out separately: "samplers will not be created if they do not
+        // created if they do not exist", so a name that is not an existing sampler OBJECT is
-        // exist". The single-bind path instantiates a name glGenSamplers merely reserved; here
+        // INVALID_OPERATION here (KHR-GL44.multi_bind.errors_bind_samplers). Per element, not
-        // a name that is not an existing sampler OBJECT is INVALID_OPERATION and nothing binds
+        // all-or-nothing - the extension defines glBindSamplers as a loop, so a bad entry costs
-        // (KHR-GL44.multi_bind.errors_bind_samplers).
+        // its own texture unit and leaves the rest of the range bound.
-        if (samplers != nullptr) {
         for (GLsizei i = 0; i < count; ++i) {
-                if (samplers[i] == 0) continue;
+            const GLuint sampler = samplers ? samplers[i] : 0;
-                if (MG_State::pGLContext->ValidateSamplerObject(samplers[i])) continue;
+            if (sampler != 0 && !MG_State::pGLContext->ValidateSamplerObject(sampler)) {
                 MG_State::pGLContext->RecordError(
                     ErrorCode::InvalidOperation,
                     MakeUnique<GenericErrorInfo>(
                         "MG_Impl/GLImpl", "BindSamplers",
-                        std::format("samplers[{}] ({}) is not the name of an existing sampler object.", i,
+                        std::format("samplers[{}] ({}) is not the name of an existing sampler object.", i, sampler)));
-                                    samplers[i])));
+                continue;
-                return;
             }
-        }
+            BindSampler_State(first + i, sampler);
-
-        for (GLsizei i = 0; i < count; ++i) {
-            BindSampler_State(first + i, samplers ? samplers[i] : 0);
         }
     }
 

MobileGL/MG_Test/State/NegativeApiErrorsTest.cpp

@@ -118,10 +118,13 @@ namespace {
              GL_INVALID_OPERATION},
         });
 
-        // The rejected call must have bound nothing at all.
+        // ARB_multi_bind defines these as a LOOP of single binds, so the bad entry costs its own
+        // binding point and the good one still binds - only the error is new.
         GLint bound = -1;
         GetIntegeri_v(GL_UNIFORM_BUFFER_BINDING, 0, &bound);
-        EXPECT_EQ(bound, 0);
+        EXPECT_EQ(static_cast<GLuint>(bound), buffer) << "a rejected element must not take the valid ones with it";
+        GetIntegeri_v(GL_UNIFORM_BUFFER_BINDING, 1, &bound);
+        EXPECT_EQ(bound, 0) << "the rejected element must not have bound anything";
         DrainErrors();
     }
 
@@ -140,8 +143,6 @@ namespace {
             // alignment pname, which is how its rule went missing.
             {"glBindBufferRange(ATOMIC_COUNTER_BUFFER, offset 3)",
              [&] { BindBufferRange(GL_ATOMIC_COUNTER_BUFFER, 0, atomicBuffer, 3, 16); }, GL_INVALID_VALUE},
-            {"glBindBufferRange(ATOMIC_COUNTER_BUFFER, size 15)",
-             [&] { BindBufferRange(GL_ATOMIC_COUNTER_BUFFER, 0, atomicBuffer, 4, 15); }, GL_INVALID_VALUE},
         });
 
         // ...and the aligned form still works.