[Fix, Test] (GLState): deliver the GL_MIN_MAP_BUFFER_ALIGNMENT that glGetIntegerv advertises

MobileGL/MG_Impl/GLImpl/Getter/GL_Getter.cpp

@@ -1664,7 +1664,11 @@ namespace MobileGL::MG_Impl::GLImpl {
             *params = MG_State::pGLContext->IsCapabilityEnabled(CapabilityInput::Multisample) ? GL_TRUE : GL_FALSE;
             return;
         case GL_MIN_MAP_BUFFER_ALIGNMENT:
-            *params = 64; // TODO
+            // The same constant the map paths align to (MG_State/GLState/BufferState/
+            // PipeResource.h), never a literal: this number is a PROMISE about the pointers
+            // glMapBuffer and glMapBufferRange return, and the two used to be unrelated - the
+            // query said 64 while the pointers came out of a std::vector aligned to 16.
+            *params = static_cast<GLint>(MG_State::GLState::MIN_MAP_BUFFER_ALIGNMENT);
             return;
         case GL_MAX_LABEL_LENGTH:
             *params = 256; // TODO

MobileGL/MG_State/GLState/BufferState/BufferObject.cpp

@@ -163,7 +163,7 @@ namespace MobileGL::MG_State::GLState {
             if (!m_resource.IsGpuResident() &&
                 !(m_mappingAccess & BufferMappingAccessBit::FlushExplicit)) { // if we didn't flush explicitly
                 if (!(m_mappingAccess & BufferMappingAccessBit::Persistent)) {
-                    Memcpy(m_resource.Bytes() + m_mappedRange.start, m_stagingData.data(),
+                    Memcpy(m_resource.Bytes() + m_mappedRange.start, m_stagingData.data() + m_stagingBias,
                            m_mappedRange.end - m_mappedRange.start);
                 }
                 NotifyFlushMappedRange(m_mappedRange, m_mappingAccess);
@@ -175,6 +175,7 @@ namespace MobileGL::MG_State::GLState {
         m_isMapped = false;
         m_mappingAccess = BufferMappingAccessBit::Null;
         m_mappedRange = {0, 0};
+        m_stagingBias = 0;
         m_ownsStagingData = false;
     }
 
@@ -193,7 +194,7 @@ namespace MobileGL::MG_State::GLState {
         // FLUSH_EXPLICIT maps are never GPU-resident (only coherent maps are adopted), so
         // the staged bytes must be copied into the shadow before the backend reads them.
         if (!(m_mappingAccess & BufferMappingAccessBit::Persistent)) {
-            Memcpy(m_resource.Bytes() + start, m_stagingData.data() + offset, length);
+            Memcpy(m_resource.Bytes() + start, m_stagingData.data() + m_stagingBias + offset, length);
         }
         NotifyFlushMappedRange({start, end}, m_mappingAccess);
     }
@@ -311,6 +312,9 @@ namespace MobileGL::MG_State::GLState {
             m_mappedRange = {0, m_size};
 
             if (m_mappingAccess & BufferMappingAccessBit::Write) {
+                // glMapBuffer maps from offset 0, so no bias: the allocation's own
+                // GL_MIN_MAP_BUFFER_ALIGNMENT-aligned base is what the application must get.
+                m_stagingBias = 0;
                 m_stagingData.resize(m_size);
                 m_ownsStagingData = true;
 
@@ -372,14 +376,21 @@ namespace MobileGL::MG_State::GLState {
         }
 
         if (access & BufferMappingAccessBit::Write) {
-            m_stagingData.resize(range.end - range.start);
+            // ARB_map_buffer_alignment constrains (returned pointer - offset), not the pointer:
+            // a map at offset 63 must hand back a pointer 63 bytes past the alignment grid, which
+            // is exactly what the read path below gets for free from shadowBase + offset. The
+            // staging store has to be biased by the same phase to match, so it over-allocates by
+            // it and the mapped bytes start at data() + m_stagingBias.
+            m_stagingBias = range.start % MIN_MAP_BUFFER_ALIGNMENT;
+            const SizeT mappedLength = range.end - range.start;
+            m_stagingData.resize(m_stagingBias + mappedLength);
             m_ownsStagingData = true;
 
             if (!(access & (BufferMappingAccessBit::InvalidateRange | BufferMappingAccessBit::InvalidateBuffer))) {
-                Memcpy(m_stagingData.data(), m_resource.Bytes() + range.start, m_stagingData.size());
+                Memcpy(m_stagingData.data() + m_stagingBias, m_resource.Bytes() + range.start, mappedLength);
             }
 
-            return m_stagingData.data();
+            return m_stagingData.data() + m_stagingBias;
         } else {
             m_ownsStagingData = false;
             return m_resource.Bytes() + range.start;
@@ -438,7 +449,7 @@ namespace MobileGL::MG_State::GLState {
             return const_cast<Uint8*>(m_resource.Bytes()) + m_mappedRange.start;
         }
         if (m_ownsStagingData) {
-            return const_cast<Uint8*>(m_stagingData.data());
+            return const_cast<Uint8*>(m_stagingData.data()) + m_stagingBias;
         }
         return const_cast<Uint8*>(m_resource.Bytes()) + m_mappedRange.start;
     }

MobileGL/MG_State/GLState/BufferState/BufferObject.h

@@ -239,7 +239,14 @@ namespace MobileGL {
             // Set by MarkGpuWritten, cleared by SyncGpuWrites once the shadow is refreshed.
             Bool m_gpuWritePending = false;
             Range1D m_mappedRange;
-            Vector<Uint8> m_stagingData;
+            // The write-map staging store. MapAlignedData because the application is handed a
+            // pointer into it, and biased by m_stagingBias because ARB_map_buffer_alignment
+            // requires (returned pointer - offset) to be aligned, not the pointer itself: a range
+            // map at offset 63 must hand back a pointer sitting 63 bytes past the alignment grid.
+            // The bias is the offset's phase, so the mapped bytes still start at
+            // m_stagingData.data() + m_stagingBias and the allocation is that much longer.
+            MapAlignedData m_stagingData;
+            SizeT m_stagingBias = 0;
             Bool m_ownsStagingData;
         };
     } // namespace MG_State::GLState

MobileGL/MG_State/GLState/BufferState/PipeResource.h

@@ -10,8 +10,56 @@
 #include <Includes.h>
 #include <MG_Util/Types.h>
 #include <bit>
+#include <new>
+#include <vector>
 
 namespace MobileGL::MG_State::GLState {
+    // GL_MIN_MAP_BUFFER_ALIGNMENT. GL 4.2 / ARB_map_buffer_alignment fix the minimum at 64 and
+    // MobileGL advertises exactly that (MG_Impl/GLImpl/Getter/GL_Getter.cpp reads this constant),
+    // so under-reporting is not available - the implementation has to be brought up to the number
+    // instead. The promise is about POINTERS, not just the query: glMapBuffer must return a
+    // 64-byte-aligned pointer, and glMapBufferRange must return one whose base - the returned
+    // pointer minus the offset the caller asked for - is. Every pointer the frontend hands out
+    // comes from the shadow below or from BufferObject's staging buffer, and std::vector only
+    // promises alignof(std::max_align_t) (16 on aarch64), so both allocations carry the alignment
+    // themselves. One constant for the getter and the allocator, because the two may never
+    // disagree - the same reason the atomic-counter limits are shared through
+    // MG_Util/ShaderTranspiler/Types.h.
+    inline constexpr SizeT MIN_MAP_BUFFER_ALIGNMENT = 64;
+
+    // Allocator that gives every allocation MIN_MAP_BUFFER_ALIGNMENT. Deliberately minimal: the
+    // vectors it backs hold raw bytes and are only ever sized, so allocate/deallocate plus the
+    // rebinding and equality boilerplate std::vector requires is the whole interface.
+    template <typename T>
+    struct MapAlignedAllocator {
+        using value_type = T;
+
+        MapAlignedAllocator() noexcept = default;
+        template <typename U>
+        MapAlignedAllocator(const MapAlignedAllocator<U>&) noexcept {}
+
+        T* allocate(SizeT count) {
+            if (count == 0) return nullptr;
+            return static_cast<T*>(
+                ::operator new(count * sizeof(T), std::align_val_t{MIN_MAP_BUFFER_ALIGNMENT}));
+        }
+        void deallocate(T* pointer, SizeT) noexcept {
+            ::operator delete(pointer, std::align_val_t{MIN_MAP_BUFFER_ALIGNMENT});
+        }
+
+        template <typename U>
+        Bool operator==(const MapAlignedAllocator<U>&) const noexcept {
+            return true;
+        }
+        template <typename U>
+        Bool operator!=(const MapAlignedAllocator<U>&) const noexcept {
+            return false;
+        }
+    };
+
+    // Byte store for anything the application may end up holding a mapped pointer into.
+    using MapAlignedData = std::vector<Uint8, MapAlignedAllocator<Uint8>>;
+
     // Opaque, refcounted handle to the backend's GPU storage for one buffer
     // (the driver-side resource). The active backend derives from it and attaches
     // its own payload (VkBufferResource / GLESBufferResource). Held by PipeResource.
@@ -57,8 +105,8 @@ namespace MobileGL::MG_State::GLState {
         }
         // Direct shadow access, used only by the backend's upload-from-shadow path,
         // which never runs for a GPU-resident (persistent) buffer.
-        Data& Shadow() { return *m_shadow; }
-        const Data& Shadow() const { return *m_shadow; }
+        MapAlignedData& Shadow() { return *m_shadow; }
+        const MapAlignedData& Shadow() const { return *m_shadow; }
 
         // Transition to persistent GPU residency: adopt the backend's coherent
         // mapped base as the source of truth and drop the CPU shadow. The caller
@@ -85,7 +133,10 @@ namespace MobileGL::MG_State::GLState {
         SharedPtr<BackendBufferResource> ReleaseBackend() { return std::move(m_backend); }
 
     private:
-        SharedPtr<Data> m_shadow = MakeShared<Data>();
+        // MapAlignedData, not Data: a read-only glMapBuffer hands the application this very
+        // pointer, and a range map hands it base + offset, so the base has to be on the
+        // GL_MIN_MAP_BUFFER_ALIGNMENT grid for either to satisfy ARB_map_buffer_alignment.
+        SharedPtr<MapAlignedData> m_shadow = MakeShared<MapAlignedData>();
         void* m_gpuMapped = nullptr;
         SharedPtr<BackendBufferResource> m_backend;
     };

MobileGL/MG_Test/Buffer/BufferTest.cpp

@@ -8,6 +8,7 @@
 
 #include <gtest/gtest.h>
 
+#include <cstdint>
 #include <limits>
 
 #include "Includes.h"
@@ -267,6 +268,116 @@ TEST_F(BufferTest, AcquireMemoryRangeWithExplicit) {
     ASSERT_EQ(actual, expected);
 }
 
+// GL_MIN_MAP_BUFFER_ALIGNMENT is a promise about POINTERS, and MobileGL used to keep only the
+// query half of it: glGetIntegerv answered 64 while every mapped pointer came out of a plain
+// std::vector, aligned to alignof(std::max_align_t) - 16 on aarch64. GL 4.2 /
+// ARB_map_buffer_alignment fix the minimum at 64, so under-reporting is not available and the
+// implementation has to be brought up to the number instead. Note the two different constraints:
+// glMapBuffer's pointer must be aligned outright, while glMapBufferRange's must be aligned AFTER
+// subtracting the offset the caller asked for - i.e. it sits at the offset's own alignment phase.
+// KHR-GLxx.map_buffer_alignment.functional asserts exactly these two, at offset 63, for 24
+// storage-flag combinations across 14 targets, and failed identically on both test devices.
+TEST_F(BufferTest, MappedPointersHonourTheAdvertisedMapBufferAlignment) {
+    GLint advertisedAlignment = 0;
+    MobileGL::MG_Impl::GLImpl::GetIntegerv(GL_MIN_MAP_BUFFER_ALIGNMENT, &advertisedAlignment);
+    ASSERT_EQ(advertisedAlignment, static_cast<GLint>(MobileGL::MG_State::GLState::MIN_MAP_BUFFER_ALIGNMENT))
+        << "the query and the allocator must read the same constant";
+    ASSERT_GE(advertisedAlignment, 64) << "GL 4.2 fixes the minimum at 64";
+    const SizeT alignment = static_cast<SizeT>(advertisedAlignment);
+
+    auto& slot = MobileGL::MG_State::pGLContext->GetBufferBindingSlot(BufferTarget::Uniform);
+    Vector<Uint> bufferNames;
+    MobileGL::MG_State::pGLContext->GenBufferNames(1, bufferNames);
+    auto bufObj = MobileGL::MG_State::pGLContext->CreateBufferObject(bufferNames[0]);
+    slot.Bind(bufObj);
+
+    // The conformance test's own shape: a buffer two alignments long, mapped from the last byte
+    // inside the first alignment - the offset most likely to expose a base-aligned-only fix.
+    const SizeT bufferSize = 2 * alignment;
+    const SizeT offset = alignment - 1;
+    bufObj->Resize(bufferSize);
+    Vector<Uint8> initData(bufferSize);
+    for (SizeT i = 0; i < bufferSize; ++i) initData[i] = static_cast<Uint8>(i);
+    bufObj->UploadData(DataPtr{.data = initData.data(), .size = bufferSize}, 0);
+
+    const auto addressOf = [](const void* pointer) { return reinterpret_cast<std::uintptr_t>(pointer); };
+
+    // glMapBuffer, read-only: the shadow base itself is handed out.
+    void* readMapped = bufObj->AcquireMemory(true, true, false);
+    ASSERT_NE(readMapped, nullptr);
+    EXPECT_EQ(addressOf(readMapped) % alignment, 0u) << "glMapBuffer(GL_READ_ONLY) returned an unaligned pointer";
+    bufObj->ReleaseMemory();
+
+    // glMapBuffer, write: the staging store is handed out instead.
+    void* writeMapped = bufObj->AcquireMemory(true, false, true);
+    ASSERT_NE(writeMapped, nullptr);
+    EXPECT_EQ(addressOf(writeMapped) % alignment, 0u) << "glMapBuffer(GL_WRITE_ONLY) returned an unaligned pointer";
+    EXPECT_EQ(bufObj->GetMappedPointer(), writeMapped)
+        << "GL_BUFFER_MAP_POINTER must report the pointer the map returned";
+    bufObj->ReleaseMemory();
+
+    // glMapBufferRange, read-only: shadow base + offset, so the phase falls out for free.
+    const Range1D mapRange{.start = offset, .end = bufferSize};
+    void* rangeRead = bufObj->AcquireMemoryRange(mapRange, BufferMappingAccessBit::Read);
+    ASSERT_NE(rangeRead, nullptr);
+    EXPECT_EQ((addressOf(rangeRead) - offset) % alignment, 0u)
+        << "glMapBufferRange(READ) returned a pointer whose base is unaligned";
+    bufObj->ReleaseMemory();
+
+    // glMapBufferRange, write: the staging store has to be biased to the same phase, and the
+    // write-back has to follow the bias or the bytes land at the wrong place in the shadow.
+    Uint8* rangeWrite = static_cast<Uint8*>(bufObj->AcquireMemoryRange(mapRange, BufferMappingAccessBit::Write));
+    ASSERT_NE(rangeWrite, nullptr);
+    EXPECT_EQ((addressOf(rangeWrite) - offset) % alignment, 0u)
+        << "glMapBufferRange(WRITE) returned a pointer whose base is unaligned";
+    EXPECT_EQ(bufObj->GetMappedPointer(), rangeWrite)
+        << "GL_BUFFER_MAP_POINTER must report the pointer the map returned";
+    // Seeded from the shadow, so the mapped view starts at the offset's byte.
+    EXPECT_EQ(rangeWrite[0], static_cast<Uint8>(offset));
+    rangeWrite[0] = 0xAB;
+    rangeWrite[bufferSize - offset - 1] = 0xCD;
+    bufObj->ReleaseMemory();
+
+    Vector<Uint8> readBack(bufferSize);
+    bufObj->DownloadSubData(readBack.data(), 0, bufferSize);
+    EXPECT_EQ(readBack[offset], 0xAB) << "the biased staging write-back landed at the wrong offset";
+    EXPECT_EQ(readBack[bufferSize - 1], 0xCD) << "the biased staging write-back landed at the wrong offset";
+    EXPECT_EQ(readBack[offset - 1], static_cast<Uint8>(offset - 1)) << "the write-back overran the mapped range";
+}
+
+// The explicit-flush path reads through the same bias, one flush offset further in: a flush of
+// [offset + 4, offset + 8) must copy the bytes the application wrote at rangeWrite[4..8), not the
+// ones sitting four bytes into the raw allocation.
+TEST_F(BufferTest, ExplicitFlushOfARangeMapFollowsTheAlignmentBias) {
+    auto& slot = MobileGL::MG_State::pGLContext->GetBufferBindingSlot(BufferTarget::Uniform);
+    Vector<Uint> bufferNames;
+    MobileGL::MG_State::pGLContext->GenBufferNames(1, bufferNames);
+    auto bufObj = MobileGL::MG_State::pGLContext->CreateBufferObject(bufferNames[0]);
+    slot.Bind(bufObj);
+
+    const SizeT alignment = MobileGL::MG_State::GLState::MIN_MAP_BUFFER_ALIGNMENT;
+    const SizeT bufferSize = 2 * alignment;
+    const SizeT offset = alignment - 1;
+    bufObj->Resize(bufferSize);
+    Vector<Uint8> initData(bufferSize, 0);
+    bufObj->UploadData(DataPtr{.data = initData.data(), .size = bufferSize}, 0);
+
+    const Range1D mapRange{.start = offset, .end = bufferSize};
+    Uint8* mapped = static_cast<Uint8*>(bufObj->AcquireMemoryRange(
+        mapRange, BufferMappingAccessBit::Write | BufferMappingAccessBit::FlushExplicit));
+    ASSERT_NE(mapped, nullptr);
+    mapped[4] = 0x5A;
+    mapped[5] = 0x5B;
+    bufObj->FlushMemoryRange(4, 2);
+    bufObj->ReleaseMemory();
+
+    Vector<Uint8> readBack(bufferSize);
+    bufObj->DownloadSubData(readBack.data(), 0, bufferSize);
+    EXPECT_EQ(readBack[offset + 4], 0x5A);
+    EXPECT_EQ(readBack[offset + 5], 0x5B);
+    EXPECT_EQ(readBack[offset + 3], 0x00) << "the explicit flush copied bytes outside the flushed range";
+}
+
 TEST_F(BufferTest, CopyBufferSubData) {
     auto& srcSlot = MobileGL::MG_State::pGLContext->GetBufferBindingSlot(BufferTarget::CopyRead);
     auto& dstSlot = MobileGL::MG_State::pGLContext->GetBufferBindingSlot(BufferTarget::CopyWrite);