[Fix] (GL): a detached shader leaves the GL-visible attach list at once, and re-attaching it before the next link is legal.

MobileGL/MG_Impl/GLImpl/Program/GL_Program.cpp

@@ -833,7 +833,8 @@ namespace MobileGL::MG_Impl::GLImpl {
         }
         auto& programObject = TryToGetProgramObject(program);
         if (!programObject) return;
-        const auto& s = programObject->GetAttachedShaders();
+        // GL-visible: a shader detached since the last link is no longer attached (GL 4.6 §7.3).
+        const auto s = programObject->GetGLVisibleAttachedShaders();
         GLsizei c = std::min((GLsizei)s.size(), maxCount);
         if (count) *count = c;
         for (GLsizei i = 0; i < c; ++i) {
@@ -873,7 +874,7 @@ namespace MobileGL::MG_Impl::GLImpl {
             break;
         }
         case GL_ATTACHED_SHADERS: {
-            const auto& attachedShaders = programObject->GetAttachedShaders();
+            const auto attachedShaders = programObject->GetGLVisibleAttachedShaders();
             *params = (GLint)attachedShaders.size();
             MGLOG_D("%s: %s = %d", __func__, MG_Util::ConvertGLEnumToString(pname).c_str(), *params);
             break;

MobileGL/MG_IntegrationTest/Scenarios/ArbShaderObjectsScenario.cpp

@@ -225,6 +225,53 @@ void main() { oColor = uColor; }
             EXPECT_EQ(FirstGLError(), 0u);
         }
 
+        // A detach takes effect for every query at once (GL 4.6 ¡ì7.3), even though the program keeps
+        // the shader for its current executable until the next link: a second detach is
+        // INVALID_OPERATION, and attaching it again before that link is legal and lists it once.
+        TEST_F(ArbShaderObjectsScenario, ADetachIsVisibleAtOnceAndAReattachWithdrawsIt) {
+            if (!Ready()) return;
+            const GLuint vs = ArbShader(GL_VERTEX_SHADER, kVertexSource);
+            const GLuint fs = ArbShader(GL_FRAGMENT_SHADER, kFragmentSource);
+            const GLuint program = glCreateProgramObjectARB();
+            glAttachObjectARB(program, vs);
+            glAttachObjectARB(program, fs);
+            glLinkProgramARB(program);
+            GLint value = 0;
+            glDetachObjectARB(program, fs);
+            glGetProgramiv(program, GL_ATTACHED_SHADERS, &value);
+            EXPECT_EQ(value, 1) << "a detached shader is still counted";
+            GLuint attached[4] = {};
+            GLsizei count = 0;
+            glGetAttachedShaders(program, 4, &count, attached);
+            ASSERT_EQ(count, 1);
+            EXPECT_EQ(attached[0], vs);
+            glGetProgramiv(program, GL_LINK_STATUS, &value);
+            EXPECT_EQ(value, GL_TRUE) << "a detach must not touch the current link";
+            EXPECT_EQ(FirstGLError(), 0u);
+            glDetachShader(program, fs);
+            EXPECT_EQ(FirstGLError(), static_cast<unsigned>(GL_INVALID_OPERATION)) << "detaching a detached shader";
+            glAttachShader(program, fs);
+            EXPECT_EQ(FirstGLError(), 0u) << "re-attaching a shader detached since the last link";
+            glGetAttachedObjectsARB(program, 4, &count, attached);
+            EXPECT_EQ(count, 2) << "the re-attached shader is listed exactly once";
+            glAttachShader(program, fs);
+            EXPECT_EQ(FirstGLError(), static_cast<unsigned>(GL_INVALID_OPERATION)) << "attaching an attached shader";
+            // The relink sees both shaders again and draws.
+            glLinkProgram(program);
+            glGetProgramiv(program, GL_LINK_STATUS, &value);
+            ASSERT_EQ(value, GL_TRUE);
+            glUseProgram(program);
+            glUniform4f(glGetUniformLocation(program, "uColor"), 1.0f, 0.0f, 0.0f, 1.0f);
+            DrawInto();
+            const Image red = ReadPixels(kEdge, kEdge);
+            EXPECT_TRUE(RegionIsMostly(red, 0, kEdge - 1, 0, kEdge - 1, "red", 0.0, "relinked after detach + reattach"));
+            glUseProgram(0);
+            glDeleteObjectARB(program);
+            glDeleteObjectARB(vs);
+            glDeleteObjectARB(fs);
+            EXPECT_EQ(FirstGLError(), 0u);
+        }
+
         // A program built through the extension, mixed with core calls on the same objects, draws.
         TEST_F(ArbShaderObjectsScenario, AnArbProgramMixedWithCoreCallsDraws) {
             if (!Ready()) return;

MobileGL/MG_State/GLState/ProgramState/ProgramObject.cpp

@@ -436,10 +436,18 @@ namespace MobileGL::MG_State::GLState {
     // is specified as link-then-detach and would discard its own link before anyone read it.
     bool ProgramObject::AttachShader(const SharedPtr<ShaderObject>& shader) {
         MGLOG_D("ProgramObject %u: AttachShader called for shader %p", m_externalIndex, shader.get());
-        if (ShaderIsAttached(shader)) {
+        if (ShaderIsAttachedGLVisible(shader)) {
             MGLOG_D("ProgramObject %u: AttachShader - shader already attached, skipping", m_externalIndex);
             return false;
         }
+        if (ShaderIsAttached(shader)) {
+            // Detached since the last link, and attached again before the next one: GL sees it
+            // attached from here on, so the pending detach is withdrawn rather than the shader
+            // listed twice.
+            std::erase_if(m_detachedShaders,
+                          [&shader](const SharedPtr<ShaderObject>& s) { return s.get() == shader.get(); });
+            return true;
+        }
         m_shaders.emplace_back(shader);
         MGLOG_D("ProgramObject %u: AttachShader - attached successfully, total shaders now %zu", m_externalIndex,
                 m_shaders.size());
@@ -456,7 +464,9 @@ namespace MobileGL::MG_State::GLState {
 
     SizeT ProgramObject::DetachShader(const SharedPtr<ShaderObject>& shader) {
         MGLOG_D("DetachShader called for shader %p from ProgramObject %u", shader.get(), m_externalIndex);
-        if (!ShaderIsAttached(shader)) {
+        // GL-visible: a shader already detached (pending the next link) is not attached, and a
+        // second glDetachShader of it is INVALID_OPERATION like any other unattached shader.
+        if (!ShaderIsAttachedGLVisible(shader)) {
             MGLOG_D("Shader %p is not attached to ProgramObject %u, cannot detach.", shader.get(), m_externalIndex);
             return 0;
         }
@@ -665,6 +675,15 @@ namespace MobileGL::MG_State::GLState {
         MGLOG_D("ProgramObject %u: MarkAsDeleted - now marked deleted", m_externalIndex);
     }
 
+    Vector<SharedPtr<ShaderObject>> ProgramObject::GetGLVisibleAttachedShaders() const {
+        Vector<SharedPtr<ShaderObject>> visible;
+        visible.reserve(m_shaders.size());
+        for (const auto& shader : m_shaders) {
+            if (ShaderIsAttachedGLVisible(shader)) visible.push_back(shader);
+        }
+        return visible;
+    }
+
     Vector<SharedPtr<ShaderObject>>& ProgramObject::GetAttachedShaders() {
         MGLOG_D("ProgramObject %u: GetAttachedShaders called, returning %zu shaders", m_externalIndex,
                 m_shaders.size());

MobileGL/MG_State/GLState/ProgramState/ProgramObject.h

@@ -73,6 +73,10 @@ namespace MobileGL::MG_State::GLState {
             if (std::none_of(m_shaders.begin(), m_shaders.end(), matches)) return false;
             return std::none_of(m_detachedShaders.begin(), m_detachedShaders.end(), matches);
         }
+        // The attach list as GL sees it: the attach list minus the shaders pending detach. What
+        // glGetAttachedShaders and GL_ATTACHED_SHADERS answer from - the raw list keeps a detached
+        // shader until the next link consumes the detach.
+        Vector<SharedPtr<ShaderObject>> GetGLVisibleAttachedShaders() const;
         bool AttachShader(const SharedPtr<ShaderObject>& shader);
         SizeT DetachShader(const SharedPtr<ShaderObject>& shader);
         SizeT RemoveShader(const SharedPtr<ShaderObject>& shader);