From c9d7365937fa702c9a96f8a79e3d20b9f7e243cb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Vladim=C3=ADr=20Vondru=C5=A1?= Date: Tue, 6 Sep 2022 17:56:42 +0200 Subject: [PATCH] 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 | 3 +- src/Magnum/GL/AbstractShaderProgram.cpp | 11 ++- src/Magnum/GL/AbstractShaderProgram.h | 37 +++++++--- src/Magnum/GL/Shader.h | 34 +++++---- .../GL/Test/AbstractShaderProgramGLTest.cpp | 69 ++++++++++++++++++- src/Magnum/Shaders/DistanceFieldVectorGL.cpp | 3 +- src/Magnum/Shaders/FlatGL.cpp | 3 +- src/Magnum/Shaders/MeshVisualizerGL.cpp | 11 ++- src/Magnum/Shaders/PhongGL.cpp | 3 +- src/Magnum/Shaders/VectorGL.cpp | 3 +- src/Magnum/Shaders/VertexColorGL.cpp | 3 +- 11 files changed, 142 insertions(+), 38 deletions(-) diff --git a/doc/snippets/MagnumGL.cpp b/doc/snippets/MagnumGL.cpp index 06adbcf01..98755e21a 100644 --- a/doc/snippets/MagnumGL.cpp +++ b/doc/snippets/MagnumGL.cpp @@ -25,6 +25,7 @@ #include /* for std::tie() :( */ #include +#include #include #include @@ -300,7 +301,7 @@ MyShader::MyShader(NoInitT) {} MyShader::MyShader(CompileState&& state): MyShader{static_cast(std::move(state))} { - CORRADE_INTERNAL_ASSERT_OUTPUT(checkLink()); + CORRADE_INTERNAL_ASSERT_OUTPUT(checkLink({state._vert, state._frag})); DOXYGEN_ELLIPSIS() } diff --git a/src/Magnum/GL/AbstractShaderProgram.cpp b/src/Magnum/GL/AbstractShaderProgram.cpp index 4b5e53be7..c26f0cab9 100644 --- a/src/Magnum/GL/AbstractShaderProgram.cpp +++ b/src/Magnum/GL/AbstractShaderProgram.cpp @@ -27,6 +27,7 @@ #include "AbstractShaderProgram.h" #include +#include #include #ifndef MAGNUM_TARGET_WEBGL #include @@ -591,7 +592,13 @@ void AbstractShaderProgram::submitLink() { glLinkProgram(_id); } -bool AbstractShaderProgram::checkLink() { +bool AbstractShaderProgram::checkLink(const Containers::Iterable 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> 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; } diff --git a/src/Magnum/GL/AbstractShaderProgram.h b/src/Magnum/GL/AbstractShaderProgram.h index abba41a1e..10b804461 100644 --- a/src/Magnum/GL/AbstractShaderProgram.h +++ b/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 Shader::compile() and @ref link(). Calling the submit functions will -trigger a (potentially async) compilation and linking, calling the check +@ref checkLink() (which optionally delegates to @ref Shader::checkCompile()), +instead of @ref Shader::compile() and @ref link(). Calling the submit functions +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() - to provide more context) and then performs any remaining post-link steps - such as uniform setup. + it calls @ref checkLink(), passing all input shaders to it for a complete + context in case of an error, and finally performs any remaining post-link + 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 - * 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. + * If linking failed, it first goes through @p shaders and calls + * @ref Shader::checkCompile() on each until a failure is reached. If + * no compilation failed, a linker message is printed to error output. + * 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 shaders); /** * @brief Get uniform location diff --git a/src/Magnum/GL/Shader.h b/src/Magnum/GL/Shader.h index 1ae65dcfc..5850e0ba6 100644 --- a/src/Magnum/GL/Shader.h +++ b/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 - * then continuing with @relativeref{AbstractShaderProgram,isLinkFinished()} - * or @relativeref{AbstractShaderProgram,checkLink()} on the final - * program --- if compilation would fail, subsequent linking will as - * well. See @ref GL-AbstractShaderProgram-async for more information. + * @relativeref{AbstractShaderProgram,submitLink()}, then optionally + * continue with @relativeref{AbstractShaderProgram,isLinkFinished()} + * and pass all input shaders to + * @relativeref{AbstractShaderProgram,checkLink()} on the final program + * --- 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 - * most cases it's enough to defer this check to after + * Has to be called only if @ref submitCompile() was called before. + * It's however recommended to instead immediately call * @ref AbstractShaderProgram::attachShader() and - * @relativeref{AbstractShaderProgram,submitLink()} were called, and - * then continuing with @relativeref{AbstractShaderProgram,isLinkFinished()} - * or @relativeref{AbstractShaderProgram,checkLink()} on the final - * program --- if compilation would fail, subsequent linking will as - * well. See @ref GL-AbstractShaderProgram-async for more information. + * @relativeref{AbstractShaderProgram,submitLink()}, then optionally + * continue with @relativeref{AbstractShaderProgram,isLinkFinished()} + * and pass all input shaders to + * @relativeref{AbstractShaderProgram,checkLink()} on the final program + * --- 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 diff --git a/src/Magnum/GL/Test/AbstractShaderProgramGLTest.cpp b/src/Magnum/GL/Test/AbstractShaderProgramGLTest.cpp index 20753b820..9b90f75da 100644 --- a/src/Magnum/GL/Test/AbstractShaderProgramGLTest.cpp +++ b/src/Magnum/GL/Test/AbstractShaderProgramGLTest.cpp @@ -24,6 +24,7 @@ */ #include +#include #include #include /** @todo remove when Shader is -free */ #include @@ -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; diff --git a/src/Magnum/Shaders/DistanceFieldVectorGL.cpp b/src/Magnum/Shaders/DistanceFieldVectorGL.cpp index be4dd12f5..648330122 100644 --- a/src/Magnum/Shaders/DistanceFieldVectorGL.cpp +++ b/src/Magnum/Shaders/DistanceFieldVectorGL.cpp @@ -27,6 +27,7 @@ #include "DistanceFieldVectorGL.h" #include +#include #include #include @@ -170,7 +171,7 @@ template DistanceFieldVectorGL::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; diff --git a/src/Magnum/Shaders/FlatGL.cpp b/src/Magnum/Shaders/FlatGL.cpp index af12286a3..1d61fed6c 100644 --- a/src/Magnum/Shaders/FlatGL.cpp +++ b/src/Magnum/Shaders/FlatGL.cpp @@ -27,6 +27,7 @@ #include "FlatGL.h" #include +#include #include #include @@ -245,7 +246,7 @@ template FlatGL::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; diff --git a/src/Magnum/Shaders/MeshVisualizerGL.cpp b/src/Magnum/Shaders/MeshVisualizerGL.cpp index 80cde4f35..6c079655b 100644 --- a/src/Magnum/Shaders/MeshVisualizerGL.cpp +++ b/src/Magnum/Shaders/MeshVisualizerGL.cpp @@ -27,6 +27,7 @@ #include "MeshVisualizerGL.h" #include +#include #include #include #include @@ -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; diff --git a/src/Magnum/Shaders/PhongGL.cpp b/src/Magnum/Shaders/PhongGL.cpp index f68258dc8..6e511a0ba 100644 --- a/src/Magnum/Shaders/PhongGL.cpp +++ b/src/Magnum/Shaders/PhongGL.cpp @@ -30,6 +30,7 @@ #include #endif #include +#include #include #include #include @@ -355,7 +356,7 @@ PhongGL::PhongGL(CompileState&& state): PhongGL{static_cast(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; diff --git a/src/Magnum/Shaders/VectorGL.cpp b/src/Magnum/Shaders/VectorGL.cpp index b9f1b597b..89b5353e3 100644 --- a/src/Magnum/Shaders/VectorGL.cpp +++ b/src/Magnum/Shaders/VectorGL.cpp @@ -27,6 +27,7 @@ #include "VectorGL.h" #include +#include #include #include @@ -172,7 +173,7 @@ template VectorGL::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; diff --git a/src/Magnum/Shaders/VertexColorGL.cpp b/src/Magnum/Shaders/VertexColorGL.cpp index 0853e52f2..048e95653 100644 --- a/src/Magnum/Shaders/VertexColorGL.cpp +++ b/src/Magnum/Shaders/VertexColorGL.cpp @@ -27,6 +27,7 @@ #include "VertexColorGL.h" #include +#include #include #include @@ -150,7 +151,7 @@ template VertexColorGL::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;