GL: better handle a case of async link failures.

The new async APIs were just checking the link status, and printing the
linker error. Because drivers commonly do all that in a single step,
without really separating compilation from linking (or at least that's
what I thought?), I assumed the linker error would contain *also* the
compilation error, if any.

But on a quick check with Mesa that's not the case, I only get "error:
linking with uncompiled/unspecialized shader", which is very useless.
Which means, to get proper error output, the checkLink() function now
explicitly takes a list of the input shaders. It will unconditionally go
through them at the beginning and call checkCompile() on each.

To further encourage the shaders to be passed, there's no default
argument -- so if the application calls checkCompile() on its own for
some reason, it has to pass an empty list to checkLink().

doc/snippets/MagnumGL.cpp

@@ -25,6 +25,7 @@
 
 #include <tuple> /* for std::tie() :( */
 #include <Corrade/Containers/ArrayViewStl.h>
+#include <Corrade/Containers/Iterable.h>
 #include <Corrade/Containers/Reference.h>
 #include <Corrade/TestSuite/Tester.h>
 
@@ -300,7 +301,7 @@ MyShader::MyShader(NoInitT) {}
 MyShader::MyShader(CompileState&& state):
     MyShader{static_cast<MyShader&&>(std::move(state))}
 {
-    CORRADE_INTERNAL_ASSERT_OUTPUT(checkLink());
+    CORRADE_INTERNAL_ASSERT_OUTPUT(checkLink({state._vert, state._frag}));
     DOXYGEN_ELLIPSIS()
 }
 

src/Magnum/GL/AbstractShaderProgram.cpp

@@ -27,6 +27,7 @@
 #include "AbstractShaderProgram.h"
 
 #include <Corrade/Containers/Array.h>
+#include <Corrade/Containers/Iterable.h>
 #include <Corrade/Containers/StridedArrayView.h>
 #ifndef MAGNUM_TARGET_WEBGL
 #include <Corrade/Containers/String.h>
@@ -591,7 +592,13 @@ void AbstractShaderProgram::submitLink() {
     glLinkProgram(_id);
 }
 
-bool AbstractShaderProgram::checkLink() {
+bool AbstractShaderProgram::checkLink(const Containers::Iterable<Shader> shaders) {
+    /* If any compilation failed, abort without even checking the link status.
+       The checkCompile() API is called always, to print also compilation
+       warnings even in case everything still manages to link well. */
+    for(Shader& shader: shaders)
+        if(!shader.checkCompile()) return false;
+
     GLint success, logLength;
     glGetProgramiv(_id, GL_LINK_STATUS, &success);
     glGetProgramiv(_id, GL_INFO_LOG_LENGTH, &logLength);
@@ -633,7 +640,7 @@ bool AbstractShaderProgram::checkLink() {
 bool AbstractShaderProgram::link(std::initializer_list<Containers::Reference<AbstractShaderProgram>> shaders) {
     for(AbstractShaderProgram& shader: shaders) shader.submitLink();
     bool allSuccess = true;
-    for(AbstractShaderProgram& shader: shaders) allSuccess = allSuccess && shader.checkLink();
+    for(AbstractShaderProgram& shader: shaders) allSuccess = allSuccess && shader.checkLink({});
     return allSuccess;
 }
 

src/Magnum/GL/AbstractShaderProgram.h

@@ -446,9 +446,9 @@ the application to schedule other work in the meantime.
 
 Async compilation and linking can be implemented by using
 @ref Shader::submitCompile() and @ref submitLink(), followed by
-@ref checkLink() (and optionally @ref Shader::checkCompile()), instead of
+@ref checkLink() (which optionally delegates to @ref Shader::checkCompile()),
-@ref Shader::compile() and @ref link(). Calling the submit functions will
+instead of @ref Shader::compile() and @ref link(). Calling the submit functions
-trigger a (potentially async) compilation and linking, calling the check
+will trigger a (potentially async) compilation and linking, calling the check
 functions will check the operation result, potentially stalling if the async
 operation isn't finished yet.
 
@@ -476,9 +476,9 @@ creation capability while keeping also the simple constructor is the following:
     @cpp CompileState @ce instance.
 4.  A @cpp MyShader(CompileState&&) @ce constructor then takes over the base
     of @cpp CompileState @ce by delegating it into the move constructor. Then
-    it calls @ref checkLink() (and if that fails also @ref Shader::checkCompile()
+    it calls @ref checkLink(), passing all input shaders to it for a complete
-    to provide more context) and then performs any remaining post-link steps
+    context in case of an error, and finally performs any remaining post-link
-    such as uniform setup.
+    steps such as uniform setup.
 5.  The original @cpp MyShader(…) @ce constructor now only passes the result of
     @cpp compile() @ce to @cpp MyShader(CompileState&&) @ce.
 
@@ -1551,16 +1551,31 @@ class MAGNUM_GL_EXPORT AbstractShaderProgram: public AbstractObject {
          * @m_since_latest
          *
          * Has to be called only if @ref submitLink() was called before.
+         *
+         * If @p shaders are not empty, first calls @ref Shader::checkCompile()
+         * on each. If a compilation failure is reached, returns @cpp false @ce
+         * without even checking link status. To have error messages with full
+         * context in case of a failed shader compilation or linking, an
+         * application is encouraged to pass all input @ref Shader instances to
+         * this function or, if not possible, explicitly call
+         * @ref Shader::checkCompile() on each.
+         *
+         * Then, link status is checked and a message (if any) is printed
          * Returns @cpp false @ce if linking failed, @cpp true @ce on success.
-         * Linker message (if any) is printed to error output. The function
+         * If linking failed, it first goes through @p shaders and calls
-         * will stall until a (potentially async) linking operation finishes,
+         * @ref Shader::checkCompile() on each until a failure is reached. If
-         * you can use @ref isLinkFinished() to check the status instead. See
+         * no compilation failed, a linker message is printed to error output.
-         * @ref GL-AbstractShaderProgram-async for more information.
+         * The function will stall until a (potentially async) linking
+         * operation finishes, you can use @ref isLinkFinished() to check the
+         * status instead. See @ref GL-AbstractShaderProgram-async for more
+         * information.
          * @see @ref Shader::checkCompile(), @fn_gl_keyword{GetProgram} with
          *      @def_gl{LINK_STATUS} and @def_gl{INFO_LOG_LENGTH},
          *      @fn_gl_keyword{GetProgramInfoLog}
          */
-        bool checkLink();
+        /* No default argument is provided in order to *really* encourage apps
+           to pass the shaders here */
+        bool checkLink(Containers::Iterable<Shader> shaders);
 
         /**
          * @brief Get uniform location

src/Magnum/GL/Shader.h

@@ -645,13 +645,16 @@ class MAGNUM_GL_EXPORT Shader: public AbstractObject {
          * @m_since_latest
          *
          * You can call @ref isCompileFinished() or @ref checkCompile() after,
-         * but in most cases it's enough to defer that to after
+         * but it's recommended to instead immediately call
          * @ref AbstractShaderProgram::attachShader() and
-         * @relativeref{AbstractShaderProgram,submitLink()} were called, and
+         * @relativeref{AbstractShaderProgram,submitLink()}, then optionally
-         * then continuing with @relativeref{AbstractShaderProgram,isLinkFinished()}
+         * continue with @relativeref{AbstractShaderProgram,isLinkFinished()}
-         * or @relativeref{AbstractShaderProgram,checkLink()} on the final
+         * and pass all input shaders to
-         * program --- if compilation would fail, subsequent linking will as
+         * @relativeref{AbstractShaderProgram,checkLink()} on the final program
-         * well. See @ref GL-AbstractShaderProgram-async for more information.
+         * --- if compilation would fail, subsequent linking will as well, and
+         * @relativeref{AbstractShaderProgram,checkLink()} will print the
+         * compilation error if linking failed due to that. See
+         * @ref GL-AbstractShaderProgram-async for more information.
          * @see @fn_gl_keyword{ShaderSource}, @fn_gl_keyword{CompileShader}
          */
         void submitCompile();
@@ -660,14 +663,17 @@ class MAGNUM_GL_EXPORT Shader: public AbstractObject {
          * @brief Check shader compilation status and await completion
          * @m_since_latest
          *
-         * Has to be called only if @ref submitCompile() was called before. In
+         * Has to be called only if @ref submitCompile() was called before.
-         * most cases it's enough to defer this check to after
+         * It's however recommended to instead immediately call
          * @ref AbstractShaderProgram::attachShader() and
-         * @relativeref{AbstractShaderProgram,submitLink()} were called, and
+         * @relativeref{AbstractShaderProgram,submitLink()}, then optionally
-         * then continuing with @relativeref{AbstractShaderProgram,isLinkFinished()}
+         * continue with @relativeref{AbstractShaderProgram,isLinkFinished()}
-         * or @relativeref{AbstractShaderProgram,checkLink()} on the final
+         * and pass all input shaders to
-         * program --- if compilation would fail, subsequent linking will as
+         * @relativeref{AbstractShaderProgram,checkLink()} on the final program
-         * well. See @ref GL-AbstractShaderProgram-async for more information.
+         * --- if compilation would fail, subsequent linking will as well, and
+         * @relativeref{AbstractShaderProgram,checkLink()} will print the
+         * compilation error if linking failed due to that. See
+         * @ref GL-AbstractShaderProgram-async for more information.
          * @see @fn_gl_keyword{GetShader} with @def_gl{COMPILE_STATUS} and
          *      @def_gl{INFO_LOG_LENGTH}, @fn_gl_keyword{GetShaderInfoLog}
          */
@@ -684,7 +690,7 @@ class MAGNUM_GL_EXPORT Shader: public AbstractObject {
          * available, the function always returns @cpp true @ce --- i.e., as if
          * the compilation was done synchronously.
          *
-         * In most cases it's enough to only wait for the final link to finish,
+         * It's however recommended to wait only for the final link to finish,
          * and not for particular compilations --- i.e., right after
          * @ref submitCompile() continue with
          * @ref AbstractShaderProgram::attachShader() and

src/Magnum/GL/Test/AbstractShaderProgramGLTest.cpp

@@ -24,6 +24,7 @@
 */
 
 #include <sstream>
+#include <Corrade/Containers/Iterable.h>
 #include <Corrade/Containers/Reference.h>
 #include <Corrade/Containers/StringStl.h> /** @todo remove when Shader is <string>-free */
 #include <Corrade/TestSuite/Compare/Container.h>
@@ -74,6 +75,7 @@ struct AbstractShaderProgramGLTest: OpenGLTester {
 
     void linkFailure();
     void linkFailureAsync();
+    void linkFailureAsyncShaderList();
 
     void uniformNotFound();
 
@@ -116,6 +118,7 @@ AbstractShaderProgramGLTest::AbstractShaderProgramGLTest() {
 
               &AbstractShaderProgramGLTest::linkFailure,
               &AbstractShaderProgramGLTest::linkFailureAsync,
+              &AbstractShaderProgramGLTest::linkFailureAsyncShaderList,
               &AbstractShaderProgramGLTest::uniformNotFound,
 
               &AbstractShaderProgramGLTest::uniform,
@@ -335,7 +338,7 @@ void AbstractShaderProgramGLTest::createAsync() {
     while(!program.isLinkFinished())
         Utility::System::sleep(100);
 
-    CORRADE_VERIFY(program.checkLink());
+    CORRADE_VERIFY(program.checkLink({vert, frag}));
     CORRADE_VERIFY(program.isLinkFinished());
     const bool valid = program.validate().first;
 
@@ -544,16 +547,76 @@ void AbstractShaderProgramGLTest::linkFailureAsync() {
     CORRADE_VERIFY(out.str().empty());
 
     /* ... only the final check should. In this case it's "error: linking with
-       uncompiled/unspecialized shader" as well. */
+       uncompiled/unspecialized shader" as well, but if the shaders would be
+       supplied like in linkFailureAsyncShaderList() below, it'd print the
+       shader failure instead. */
     {
         Error redirectError{&out};
-        CORRADE_VERIFY(!program.checkLink());
+        CORRADE_VERIFY(!program.checkLink({}));
     }
     CORRADE_VERIFY(program.isLinkFinished());
     CORRADE_COMPARE_AS(out.str(), "GL::AbstractShaderProgram::link(): linking failed with the following message:",
         TestSuite::Compare::StringHasPrefix);
 }
 
+void AbstractShaderProgramGLTest::linkFailureAsyncShaderList() {
+    Shader vert(
+        #ifndef MAGNUM_TARGET_GLES
+        #ifndef CORRADE_TARGET_APPLE
+        Version::GL210
+        #else
+        Version::GL310
+        #endif
+        #else
+        Version::GLES200
+        #endif
+        , Shader::Type::Vertex);
+    vert.addSource("void main() {}\n");
+
+    Shader frag(
+        #ifndef MAGNUM_TARGET_GLES
+        #ifndef CORRADE_TARGET_APPLE
+        Version::GL210
+        #else
+        Version::GL310
+        #endif
+        #else
+        Version::GLES200
+        #endif
+        , Shader::Type::Fragment);
+    frag.addSource("[fu] bleh error #:! stuff\n");
+
+    vert.submitCompile();
+    frag.submitCompile();
+
+    MyPublicShader program;
+    program.attachShaders({vert, frag});
+
+    /* The link submission should not print anything ... */
+    {
+        std::ostringstream out;
+        Error redirectError{&out};
+        program.submitLink();
+        CORRADE_VERIFY(out.str().empty());
+    }
+
+    /* ... only the final check should. Vertex shader should be fine, but
+       fragment should fail. */
+    std::ostringstream out;
+    {
+        Error redirectError{&out};
+        CORRADE_VERIFY(!program.checkLink({vert, frag}));
+    }
+    CORRADE_COMPARE_AS(out.str(), "GL::Shader::compile(): compilation of fragment shader failed with the following message:",
+        TestSuite::Compare::StringHasPrefix);
+
+    /* The linker error (which would most probably say something like "error:
+       linking with uncompiled/unspecialized shader") should not be even
+       printed */
+    CORRADE_COMPARE_AS(out.str(), "GL::AbstractShaderProgram::link(): linking failed with the following message:",
+        TestSuite::Compare::StringNotContains);
+}
+
 void AbstractShaderProgramGLTest::uniformNotFound() {
     MyPublicShader program;
 

src/Magnum/Shaders/DistanceFieldVectorGL.cpp

@@ -27,6 +27,7 @@
 #include "DistanceFieldVectorGL.h"
 
 #include <Corrade/Containers/EnumSet.hpp>
+#include <Corrade/Containers/Iterable.h>
 #include <Corrade/Containers/Reference.h>
 #include <Corrade/Utility/Resource.h>
 
@@ -170,7 +171,7 @@ template<UnsignedInt dimensions> DistanceFieldVectorGL<dimensions>::DistanceFiel
     if(!id()) return;
     #endif
 
-    CORRADE_INTERNAL_ASSERT_OUTPUT(checkLink());
+    CORRADE_INTERNAL_ASSERT_OUTPUT(checkLink({state._vert, state._frag}));
 
     const GL::Context& context = GL::Context::current();
     const GL::Version version = state._version;

src/Magnum/Shaders/FlatGL.cpp

@@ -27,6 +27,7 @@
 #include "FlatGL.h"
 
 #include <Corrade/Containers/EnumSet.hpp>
+#include <Corrade/Containers/Iterable.h>
 #include <Corrade/Containers/Reference.h>
 #include <Corrade/Utility/Resource.h>
 
@@ -245,7 +246,7 @@ template<UnsignedInt dimensions> FlatGL<dimensions>::FlatGL(CompileState&& state
     if(!id()) return;
     #endif
 
-    CORRADE_INTERNAL_ASSERT_OUTPUT(checkLink());
+    CORRADE_INTERNAL_ASSERT_OUTPUT(checkLink({state._vert, state._frag}));
 
     const GL::Context& context = GL::Context::current();
     const GL::Version version = state._version;

src/Magnum/Shaders/MeshVisualizerGL.cpp

@@ -27,6 +27,7 @@
 #include "MeshVisualizerGL.h"
 
 #include <Corrade/Containers/EnumSet.hpp>
+#include <Corrade/Containers/Iterable.h>
 #include <Corrade/Containers/Reference.h>
 #include <Corrade/Utility/FormatStl.h>
 #include <Corrade/Utility/Resource.h>
@@ -536,7 +537,10 @@ MeshVisualizerGL2D::MeshVisualizerGL2D(CompileState&& state): MeshVisualizerGL2D
     if(!id()) return;
     #endif
 
-    CORRADE_INTERNAL_ASSERT_OUTPUT(checkLink());
+    if(state._geom)
+        CORRADE_INTERNAL_ASSERT_OUTPUT(checkLink({state._vert, state._frag, *state._geom}));
+    else
+        CORRADE_INTERNAL_ASSERT_OUTPUT(checkLink({state._vert, state._frag}));
 
     const GL::Context& context = GL::Context::current();
     const GL::Version version = state._version;
@@ -912,7 +916,10 @@ MeshVisualizerGL3D::MeshVisualizerGL3D(CompileState&& state): MeshVisualizerGL3D
     if(!id()) return;
     #endif
 
-    CORRADE_INTERNAL_ASSERT_OUTPUT(checkLink());
+    if(state._geom)
+        CORRADE_INTERNAL_ASSERT_OUTPUT(checkLink({state._vert, state._frag, *state._geom}));
+    else
+        CORRADE_INTERNAL_ASSERT_OUTPUT(checkLink({state._vert, state._frag}));
 
     const GL::Context& context = GL::Context::current();
     const GL::Version version = state._version;

src/Magnum/Shaders/PhongGL.cpp

@@ -30,6 +30,7 @@
 #include <Corrade/Containers/Array.h>
 #endif
 #include <Corrade/Containers/EnumSet.hpp>
+#include <Corrade/Containers/Iterable.h>
 #include <Corrade/Containers/Reference.h>
 #include <Corrade/Containers/StringView.h>
 #include <Corrade/Containers/StringStl.h>
@@ -355,7 +356,7 @@ PhongGL::PhongGL(CompileState&& state): PhongGL{static_cast<PhongGL&&>(std::move
     if(!id()) return;
     #endif
 
-    CORRADE_INTERNAL_ASSERT_OUTPUT(checkLink());
+    CORRADE_INTERNAL_ASSERT_OUTPUT(checkLink({state._vert, state._frag}));
 
     const GL::Context& context = GL::Context::current();
     const GL::Version version = state._version;

src/Magnum/Shaders/VectorGL.cpp

@@ -27,6 +27,7 @@
 #include "VectorGL.h"
 
 #include <Corrade/Containers/EnumSet.hpp>
+#include <Corrade/Containers/Iterable.h>
 #include <Corrade/Containers/Reference.h>
 #include <Corrade/Utility/Resource.h>
 
@@ -172,7 +173,7 @@ template<UnsignedInt dimensions> VectorGL<dimensions>::VectorGL(CompileState&& s
     if(!id()) return;
     #endif
 
-    CORRADE_INTERNAL_ASSERT_OUTPUT(checkLink());
+    CORRADE_INTERNAL_ASSERT_OUTPUT(checkLink({state._vert, state._frag}));
 
     const GL::Context& context = GL::Context::current();
     const GL::Version version = state._version;

src/Magnum/Shaders/VertexColorGL.cpp

@@ -27,6 +27,7 @@
 #include "VertexColorGL.h"
 
 #include <Corrade/Containers/EnumSet.hpp>
+#include <Corrade/Containers/Iterable.h>
 #include <Corrade/Containers/Reference.h>
 #include <Corrade/Utility/Resource.h>
 
@@ -150,7 +151,7 @@ template<UnsignedInt dimensions> VertexColorGL<dimensions>::VertexColorGL(Compil
     if(!id()) return;
     #endif
 
-    CORRADE_INTERNAL_ASSERT_OUTPUT(checkLink());
+    CORRADE_INTERNAL_ASSERT_OUTPUT(checkLink({state._vert, state._frag}));
 
     const GL::Context& context = GL::Context::current();
     const GL::Version version = state._version;