From 36f62a0f0356aad8ff191cee4802562c8e3d9273 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Vladim=C3=ADr=20Vondru=C5=A1?= <mosra@centrum.cz>
Date: Sun, 13 Apr 2025 12:40:08 +0200
Subject: [PATCH] GL: still use DSA for buffer creation w/
 "nv-broken-buffer-dsa".

Otherwise there's a really bad interaction with VAO DSA. Adding
explanatory comments, a repro case is in previous commit.
---
 src/Magnum/GL/Implementation/BufferState.cpp | 47 +++++++++++++++-----
 src/Magnum/GL/Mesh.cpp                       | 20 +++++++++
 2 files changed, 55 insertions(+), 12 deletions(-)

diff --git a/src/Magnum/GL/Implementation/BufferState.cpp b/src/Magnum/GL/Implementation/BufferState.cpp
index 9859f0a90..4d9f1db4c 100644
--- a/src/Magnum/GL/Implementation/BufferState.cpp
+++ b/src/Magnum/GL/Implementation/BufferState.cpp
@@ -100,23 +100,46 @@ BufferState::BufferState(Context& context, Containers::StaticArrayView<Implement
         && (!(context.detectedDriver() & Context::DetectedDriver::IntelWindows) ||
         context.isDriverWorkaroundDisabled("intel-windows-crazy-broken-buffer-dsa"_s))
         #endif
-        && (!(context.detectedDriver() & Context::DetectedDriver::NVidia) ||
-        context.isDriverWorkaroundDisabled("nv-broken-buffer-dsa"_s))
     ) {
         extensions[Extensions::ARB::direct_state_access::Index] =
                    Extensions::ARB::direct_state_access::string();
 
+        /* Using the glCreateBuffer() API even with the "nv-broken-buffer-dsa"
+           workaround as otherwise the mesh VAO + DSA code paths can cause GL
+           errors or even crashes if attempting to attach empty index or vertex
+           buffers -- i.e., glGenBuffers() doesn't actually create the buffer,
+           so attempting to call glVertexArrayElementBuffer() with such a
+           buffer leads to a GL error, and then (once filling the buffer)
+           drawing such a mesh leads to a crash because it's trying to fetch
+           the indices from CPU-side null pointer.
+
+           See the MeshGLTest::addEmptyBuffer() test for a repro case. */
         createImplementation = &Buffer::createImplementationDSA;
-        copyImplementation = &Buffer::copyImplementationDSA;
-        storageImplementation = &Buffer::storageImplementationDSA;
-        getParameterImplementation = &Buffer::getParameterImplementationDSA;
-        getSubDataImplementation = &Buffer::getSubDataImplementationDSA;
-        dataImplementation = &Buffer::dataImplementationDSA;
-        subDataImplementation = &Buffer::subDataImplementationDSA;
-        mapImplementation = &Buffer::mapImplementationDSA;
-        mapRangeImplementation = &Buffer::mapRangeImplementationDSA;
-        flushMappedRangeImplementation = &Buffer::flushMappedRangeImplementationDSA;
-        unmapImplementation = &Buffer::unmapImplementationDSA;
+
+        if(!(context.detectedDriver() & Context::DetectedDriver::NVidia) ||
+        context.isDriverWorkaroundDisabled("nv-broken-buffer-dsa"_s)) {
+            copyImplementation = &Buffer::copyImplementationDSA;
+            storageImplementation = &Buffer::storageImplementationDSA;
+            getParameterImplementation = &Buffer::getParameterImplementationDSA;
+            getSubDataImplementation = &Buffer::getSubDataImplementationDSA;
+            dataImplementation = &Buffer::dataImplementationDSA;
+            subDataImplementation = &Buffer::subDataImplementationDSA;
+            mapImplementation = &Buffer::mapImplementationDSA;
+            mapRangeImplementation = &Buffer::mapRangeImplementationDSA;
+            flushMappedRangeImplementation = &Buffer::flushMappedRangeImplementationDSA;
+            unmapImplementation = &Buffer::unmapImplementationDSA;
+        } else {
+            copyImplementation = &Buffer::copyImplementationDefault;
+            storageImplementation = &Buffer::storageImplementationDefault;
+            getParameterImplementation = &Buffer::getParameterImplementationDefault;
+            getSubDataImplementation = &Buffer::getSubDataImplementationDefault;
+            dataImplementation = &Buffer::dataImplementationDefault;
+            subDataImplementation = &Buffer::subDataImplementationDefault;
+            mapImplementation = &Buffer::mapImplementationDefault;
+            mapRangeImplementation = &Buffer::mapRangeImplementationDefault;
+            flushMappedRangeImplementation = &Buffer::flushMappedRangeImplementationDefault;
+            unmapImplementation = &Buffer::unmapImplementationDefault;
+        }
     } else
     #endif
     {
diff --git a/src/Magnum/GL/Mesh.cpp b/src/Magnum/GL/Mesh.cpp
index 8e802430f..95f366143 100644
--- a/src/Magnum/GL/Mesh.cpp
+++ b/src/Magnum/GL/Mesh.cpp
@@ -1177,6 +1177,16 @@ void Mesh::attributePointerImplementationVAODSA(Mesh& self, AttributeLayout&& at
 
     glVertexArrayAttribBinding(self._id, attribute.location, attribute.location);
     CORRADE_INTERNAL_ASSERT(attributeStride != 0);
+    /* Note that this assumes the buffer is already created (glCreateBuffers(),
+       not glBindBuffer()), if empty, otherwise a GL error or a crash later at
+       draw may occur. See MeshGLTest::addEmptyBuffer() for a repro case,
+       especially important in relation to workarounds that disable DSA for
+       buffers but not meshes. In case of "nv-broken-buffer-dsa" DSA is still
+       used for buffer creation so that's fine, in case of
+       "intel-windows-crazy-broken-buffer-dsa" there's also an accompanying
+       "intel-windows-crazy-broken-vao-dsa" that disables VAO DSA code paths as
+       well so that's fine too. Similar case is with
+       glVertexArrayElementBuffer() below. */
     glVertexArrayVertexBuffer(self._id, attribute.location, attribute.buffer.id(), attributeOffset, attributeStride);
 
     if(attribute.divisor)
@@ -1294,6 +1304,16 @@ void Mesh::bindIndexBufferImplementationVAO(Mesh& self, Buffer& buffer) {
 
 #ifndef MAGNUM_TARGET_GLES
 void Mesh::bindIndexBufferImplementationVAODSA(Mesh& self, Buffer& buffer) {
+    /* Note that this assumes the buffer is already created (glCreateBuffers(),
+       not glBindBuffer()), if empty, otherwise a GL error or a crash later at
+       draw may occur. See MeshGLTest::addEmptyBuffer() for a repro case,
+       especially important in relation to workarounds that disable DSA for
+       buffers but not meshes. In case of "nv-broken-buffer-dsa" DSA is still
+       used for buffer creation so that's fine, in case of
+       "intel-windows-crazy-broken-buffer-dsa" there's also an accompanying
+       "intel-windows-crazy-broken-vao-dsa" that disables VAO DSA code paths as
+       well so that's fine too. Similar case is with
+       glVertexArrayVertexBuffer() above. */
     glVertexArrayElementBuffer(self._id, buffer.id());
 }
 #endif