[Fix, Test] (GLImpl, DirectVulkan, MG_IntegrationTest): record glVertexAttribLFormat's state and drop the array at draw

MobileGL/MG_Backend/DirectGLES/Managers.cpp

@@ -1718,14 +1718,16 @@ namespace MobileGL::MG_Backend::DirectGLES {
                                                                m_syncedAttributeVersions[attribIndex].BufferVersion;
                 if (!needsSyncFormat && !needsSyncBuffer && !needsSyncBaseInstance) continue;
 
-                // Defence in depth. The frontend already declines glVertexAttribLFormat on this
+                // This is where a 64-bit array actually stops. glVertexAttribLFormat is a legal call
-                // backend (SupportsFloat64VertexAttributes is false - ES has no GL_DOUBLE vertex
+                // in a GL 4.3 context and the frontend RECORDS its format (the state queries have to
-                // format and ESSL has no fp64 type), so IsLong should never arrive here; if it ever
+                // answer), so IsLong does arrive here - what this backend cannot do is FEED it:
-                // did, passing GL_DOUBLE to glVertexAttribPointer would only raise GL_INVALID_ENUM on
+                // SupportsFloat64VertexAttributes is false because ES has no GL_DOUBLE vertex format
-                // the real driver. Disabling rather than merely skipping matters: becoming long bumps
+                // and ESSL has no fp64 type, and passing GL_DOUBLE to glVertexAttribPointer would
-                // FormatVersion, not SwitchVersion, so the enable/disable block above will not run
+                // only raise GL_INVALID_ENUM on the real driver. Disabling rather than merely
-                // again and an already-enabled array would stay enabled with no pointer and no
+                // skipping matters: becoming long bumps FormatVersion, not SwitchVersion, so the
-                // ARRAY_BUFFER binding - which ES 3.1+ makes an INVALID_OPERATION at draw.
+                // enable/disable block above will not run again and an already-enabled array would
+                // stay enabled with no pointer and no ARRAY_BUFFER binding - which ES 3.1+ makes an
+                // INVALID_OPERATION at draw.
                 //
                 // IsLong is not the only way a 64-bit array gets here: glVertexAttribFormat
                 // with GL_DOUBLE asks for doubles in memory CONVERTED to float, so it is not

MobileGL/MG_Backend/DirectVulkan/Renderer/VertexInputStateFactory.cpp

@@ -8,6 +8,7 @@
 
 #include "VertexInputStateFactory.h"
 #include "MG_Util/Converters/MGToStr/DataTypeConverter.h"
+#include <MG_Backend/BackendObjects.h>
 #include <utility>
 
 namespace MobileGL::MG_Backend::DirectVulkan {
@@ -330,6 +331,20 @@ namespace MobileGL::MG_Backend::DirectVulkan {
             // for every R64 float format, so a native 64-bit vertex fetch is simply unavailable there
             // while shaderFloat64 is not. Both halves key off nothing but the attribute being long,
             // so they always agree without extra plumbing.
+            //
+            // ... as long as the shader half still runs. It does not when the backend has declared
+            // no 64-bit vertex attribute support: DemoteFloat64Pass has already narrowed every
+            // `dvec` input to a `vec` by then, so PackDoubleVertexInputsPass finds nothing to pack
+            // and a UINT-formatted attribute would be fed to a float input - garbage with no
+            // diagnostic anywhere. Declining here drops the array instead (the caller skips
+            // UNDEFINED attributes and reports them through unsupportedAttribMask), which is what
+            // DirectGLES does for the same state. The frontend RECORDS the format either way, so
+            // this gate is the only thing standing between a legal glVertexAttribLFormat and a
+            // mismatched pipeline.
+            if (MG_Backend::pActiveBackendObject == nullptr ||
+                !MG_Backend::pActiveBackendObject->GetDynamicParameters().SupportsFloat64VertexAttributes) {
+                return VK_FORMAT_UNDEFINED;
+            }
             if (!isLong || isInteger || normalized) return VK_FORMAT_UNDEFINED;
             switch (size) {
             case 1: return VK_FORMAT_R32G32_UINT;

MobileGL/MG_Impl/GLImpl/VertexArray/GL_VertexArray.cpp

@@ -514,10 +514,15 @@ namespace MobileGL::MG_Impl::GLImpl {
     // recorded DataType is always Float64 - what IsLong adds is that this is the *unconverted* form,
     // as opposed to VertexAttribFormat(GL_DOUBLE), which asks for a float conversion.
     //
-    // Whether the backend can feed it is detected, not assumed: DirectVulkan needs shaderFloat64,
+    // Whether the backend can FEED it is detected, not assumed: DirectVulkan needs shaderFloat64,
-    // and DirectGLES can never have it at all. A backend without it declines here, loudly - GL error
+    // and DirectGLES can never have it at all. What that costs is the ARRAY, not the call: GL 4.6
-    // plus a log line naming the reason - rather than accepting state no draw could honour and
+    // core 10.3.2 defines no error for a well-formed glVertexAttribLFormat, and a GL 4.3 context
-    // rendering garbage. The matching startup POST row is in MG_Util/SelfTest/DriverPost.cpp.
+    // has 64-bit attributes in core, so declining the call would be non-conformant and would make
+    // the four pure state queries (VERTEX_ATTRIB_ARRAY_SIZE / _TYPE / _LONG / _RELATIVE_OFFSET)
+    // unanswerable (KHR-GL43.vertex_attrib_binding.basic-state1/3). The format is therefore
+    // RECORDED here and the enabled array is dropped at draw instead - loudly, once, naming the
+    // reason. The matching startup POST row is in MG_Util/SelfTest/DriverPost.cpp; the draw-side
+    // drop is DirectGLES/Managers.cpp and, on DirectVulkan, VertexInputStateFactory's Float64 case.
     static void VertexAttribLFormatSeparate_State(const SharedPtr<MG_State::GLState::VertexArrayObject>& vao,
                                                   GLuint attribindex, GLint size, GLenum type,
                                                   GLuint relativeoffset) {
@@ -528,14 +533,11 @@ namespace MobileGL::MG_Impl::GLImpl {
         if (!MG_Backend::pActiveBackendObject ||
             !MG_Backend::pActiveBackendObject->GetDynamicParameters().SupportsFloat64VertexAttributes) {
             MGLOG_W_ONCE("VertexAttribLFormat: attribute %u asked for a 64-bit (GL_DOUBLE) format, but this "
-                    "backend has no double-precision vertex attribute support - see the "
+                    "backend has no double-precision vertex attribute support - the format is recorded "
-                    "\"64-bit vertex attributes\" / \"shaderFloat64\" POST row for what that costs",
+                    "and queryable, but the array will be DROPPED at draw and the attribute will read "
+                    "its generic current value; see the \"64-bit vertex attributes\" / \"shaderFloat64\" "
+                    "POST row for what that costs",
                     attribindex);
-            MG_State::pGLContext->RecordError(
-                ErrorCode::InvalidOperation,
-                MakeUnique<GenericErrorInfo>("MG_Impl/GLImpl", "VertexAttribLFormat",
-                                             "64-bit vertex attributes are not supported by this backend."));
-            return;
         }
 
         vao->SetAttributeFormatSeparate(attribindex, size, MG_Util::ConvertGLEnumToDataType(type),

MobileGL/MG_IntegrationTest/Scenarios/DoublePrecisionScenario.cpp

@@ -697,24 +697,127 @@ void main() {
             EXPECT_EQ(glGetError(), static_cast<GLenum>(GL_NO_ERROR));
         }
 
-        TEST_F(DoublePrecisionScenario, A64BitVertexFormatIsDeclinedOnEveryBackend) {
+        TEST_F(DoublePrecisionScenario, A64BitVertexFormatIsRecordedAndItsArrayIsDroppedAtDraw) {
             if (!Ready()) return;
             // The demotion leaves no 64-bit shader input to feed, so there is nothing a 64-bit
             // vertex FETCH could be fetched into - on either backend, and no longer only on the
-            // ones whose device lacks shaderFloat64. Declined loudly rather than accepted and
+            // ones whose device lacks shaderFloat64.
-            // drawn as garbage; the matching POST row says the same thing at startup.
+            //
+            // What that costs is the ARRAY, not the CALL. GL 4.6 core 10.3.2 defines no error for
+            // a well-formed glVertexAttribLFormat and 64-bit attributes are core in the GL 4.3
+            // context MobileGL advertises, so refusing the call would be non-conformant and would
+            // leave four pure state queries unanswerable
+            // (KHR-GL43.vertex_attrib_binding.basic-state1/3). The format is therefore recorded and
+            // queryable; the enabled array is what gets dropped, and the attribute then reads its
+            // generic current value. The matching POST row says exactly that at startup.
             GLuint vao = 0;
             glGenVertexArrays(1, &vao);
             glBindVertexArray(vao);
             while (glGetError() != GL_NO_ERROR) {}
 
-            glVertexAttribLFormat(0, 3, GL_DOUBLE, 0);
+            glVertexAttribLFormat(1, 3, GL_DOUBLE, 8);
-            EXPECT_EQ(glGetError(), static_cast<GLenum>(GL_INVALID_OPERATION));
+            EXPECT_EQ(glGetError(), static_cast<GLenum>(GL_NO_ERROR))
+                << "glVertexAttribLFormat is a legal call in a GL 4.3 context";
+
+            GLint attribSize = 0;
+            GLint attribType = 0;
+            GLint attribIsLong = 0;
+            GLint attribRelativeOffset = 0;
+            glGetVertexAttribiv(1, GL_VERTEX_ATTRIB_ARRAY_SIZE, &attribSize);
+            glGetVertexAttribiv(1, GL_VERTEX_ATTRIB_ARRAY_TYPE, &attribType);
+            glGetVertexAttribiv(1, GL_VERTEX_ATTRIB_ARRAY_LONG, &attribIsLong);
+            glGetVertexAttribiv(1, GL_VERTEX_ATTRIB_RELATIVE_OFFSET, &attribRelativeOffset);
+            EXPECT_EQ(attribSize, 3);
+            EXPECT_EQ(attribType, static_cast<GLint>(GL_DOUBLE));
+            EXPECT_EQ(attribIsLong, GL_TRUE) << "GL_VERTEX_ATTRIB_ARRAY_LONG is what makes this the "
+                                                "unconverted form; without it the state is a lie";
+            EXPECT_EQ(attribRelativeOffset, 8);
+            EXPECT_EQ(glGetError(), static_cast<GLenum>(GL_NO_ERROR));
 
             glBindVertexArray(0);
             glDeleteVertexArrays(1, &vao);
             while (glGetError() != GL_NO_ERROR) {}
         }
 
+        // The consequence of recording the state rather than refusing the call: a 64-bit array can
+        // now be ENABLED in a VAO that a draw uses, which it never could before. That must not
+        // take the draw down. Leaving such an array enabled with no pointer behind it is exactly
+        // the documented Adreno null-deref (SIGSEGV inside the next glDraw*), so DirectGLES
+        // disables it before glVertexAttribPointer can ever see GL_DOUBLE, and DirectVulkan maps
+        // the format to VK_FORMAT_UNDEFINED so it never enters the pipeline's vertex input state.
+        //
+        // The shader deliberately does NOT read location 1: that keeps the two backends on the
+        // same path (DirectVulkan declines a draw whose SHADER reads an unsupported enabled array,
+        // by design and loudly, which is a different assertion from this one) and it is the shape
+        // the crash needed - an enabled array nothing set a pointer for.
+        TEST_F(DoublePrecisionScenario, AnEnabledLongArrayDoesNotBreakADrawThatIgnoresIt) {
+            if (!Ready()) return;
+
+            constexpr const char* kVs = R"(#version 430 core
+layout(location = 0) in vec2 aPos;
+void main() { gl_Position = vec4(aPos, 0.0, 1.0); }
+)";
+            constexpr const char* kFs = R"(#version 430 core
+out vec4 o_color;
+void main() { o_color = vec4(0.0, 1.0, 0.0, 1.0); }
+)";
+            std::string error;
+            const unsigned int program = CompileProgram(kVs, kFs, &error);
+            ASSERT_NE(program, 0u) << error;
+
+            ColorFbo target = MakeColorFbo(32, 32);
+            ASSERT_NE(target.fbo, 0u) << "could not create the render target";
+            BindFbo(target);
+
+            const float positions[8] = {-1.0f, -1.0f, 1.0f, -1.0f, -1.0f, 1.0f, 1.0f, 1.0f};
+            const double doubles[4] = {1.0, 2.0, 3.0, 4.0};
+
+            GLuint vao = 0;
+            GLuint positionBuffer = 0;
+            GLuint doubleBuffer = 0;
+            glGenVertexArrays(1, &vao);
+            glBindVertexArray(vao);
+            glGenBuffers(1, &positionBuffer);
+            glBindBuffer(GL_ARRAY_BUFFER, positionBuffer);
+            glBufferData(GL_ARRAY_BUFFER, sizeof(positions), positions, GL_STATIC_DRAW);
+            glGenBuffers(1, &doubleBuffer);
+            glBindBuffer(GL_ARRAY_BUFFER, doubleBuffer);
+            glBufferData(GL_ARRAY_BUFFER, sizeof(doubles), doubles, GL_STATIC_DRAW);
+            glBindBuffer(GL_ARRAY_BUFFER, 0);
+
+            glVertexAttribFormat(0, 2, GL_FLOAT, GL_FALSE, 0);
+            glVertexAttribBinding(0, 0);
+            glBindVertexBuffer(0, positionBuffer, 0, static_cast<GLsizei>(2 * sizeof(float)));
+            glEnableVertexAttribArray(0);
+
+            glVertexAttribLFormat(1, 1, GL_DOUBLE, 0);
+            glVertexAttribBinding(1, 1);
+            glBindVertexBuffer(1, doubleBuffer, 0, static_cast<GLsizei>(sizeof(double)));
+            glEnableVertexAttribArray(1);
+            EXPECT_EQ(FirstGLError(), 0u) << "setting up the 64-bit array was refused";
+
+            ClearTo(0.0f, 0.0f, 0.0f, 1.0f);
+            glUseProgram(program);
+            glDrawArrays(GL_TRIANGLE_STRIP, 0, 4);
+            EXPECT_EQ(FirstGLError(), 0u) << "a draw with an enabled 64-bit array must not raise an error";
+
+            const Image image = ReadPixels(target.width, target.height);
+            ASSERT_FALSE(image.Empty());
+            EXPECT_GT(image.At(target.width / 2, target.height / 2).g, 200)
+                << "the draw did not happen; the enabled 64-bit array must be dropped, not fatal";
+
+            glDisableVertexAttribArray(0);
+            glDisableVertexAttribArray(1);
+            glBindVertexArray(0);
+            glDeleteVertexArrays(1, &vao);
+            glDeleteBuffers(1, &positionBuffer);
+            glDeleteBuffers(1, &doubleBuffer);
+            BindDefaultFramebuffer();
+            DestroyColorFbo(target);
+            glUseProgram(0);
+            glDeleteProgram(program);
+            EXPECT_EQ(FirstGLError(), 0u);
+        }
+
     } // namespace
 } // namespace MGITest

MobileGL/MG_Util/SelfTest/DriverPost.cpp

@@ -521,9 +521,11 @@ namespace MobileGL::MG_Util::SelfTest {
             builder.Warn("64-bit vertex attributes",
                          "not supported (ES has no GL_DOUBLE vertex format, and after the fp64 demotion "
                          "above there is no 64-bit shader input left to feed either); "
-                         "glVertexAttribLFormat / glVertexArrayAttribLFormat report "
+                         "glVertexAttribLFormat / glVertexArrayAttribLFormat succeed and their state is "
-                         "GL_INVALID_OPERATION - feed the attribute with glVertexAttribPointer(GL_FLOAT), "
+                         "queryable, but an ENABLED 64-bit array is DROPPED at draw and the attribute "
-                         "which a demoted dvec input reads correctly");
+                         "reads its generic current value - feed the attribute with "
+                         "glVertexAttribPointer(GL_FLOAT) instead, which a demoted dvec input reads "
+                         "correctly");
             if (glesFuncs.glPatchParameteri != nullptr) {
                 builder.Pass("Tessellation patch parameters",
                              "glPatchParameteri present (GL_PATCH_VERTICES reaches the driver)");
@@ -2336,8 +2338,10 @@ namespace MobileGL::MG_Util::SelfTest {
         builder.Warn("64-bit vertex attributes",
                      "not supported; there is no 64-bit shader input left to feed after the fp64 demotion "
                      "above, and no VK_FORMAT_R64*_SFLOAT vertex fetch to feed it with on most devices "
-                     "anyway. glVertexAttribLFormat reports GL_INVALID_OPERATION - feed the attribute with "
+                     "anyway. glVertexAttribLFormat succeeds and its state is queryable, but an ENABLED "
-                     "glVertexAttribPointer(GL_FLOAT), which a demoted dvec input reads correctly");
+                     "64-bit array is DROPPED at pipeline build and the attribute reads its generic "
+                     "current value - feed the attribute with glVertexAttribPointer(GL_FLOAT) instead, "
+                     "which a demoted dvec input reads correctly");
 
         Bool shaderDrawParameters = false;
         if (vkGetPhysicalDeviceFeatures2Fn != nullptr && properties.apiVersion >= VK_API_VERSION_1_1) {