From e9acbdb677ad2905cd4ccad9a98a51b6dc5fc9da Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Vladim=C3=ADr=20Vondru=C5=A1?= Date: Sun, 15 May 2022 15:47:48 +0200 Subject: [PATCH] GL,DebugTools: "fix" global symbol duplication between test libraries. If a GL test library would link to MagnumOpenGLTesterTestLib (which has CORRADE_GRACEFUL_ASSERT enabled to be able to verify assertions) and then to MagnumDebugTools for DebugTools::bufferData(), it'd mean there's one GL::Context global from MagnumGLTestLib and one from MagnumGL. Then, depending on whatever random order the linker uses, different parts of the library would see a different global, ultimately leading to a dreaded GL::Context::current(): no current context assertion. Right now this only manifested on the macOS static CI build, but depending on a phase of the moon could happen for any platform in any circumstance. First attempt was to switch to linking to MagnumDebugToolsTestLib and then making MagnumDebugToolsTestLib depend on MagnumGLTestLib instead of MagnumGL, HOWEVER because DebugTools also depend on Primitives and Shaders and whatnot for some features, it just moved the conflict between MagnumGL and MagnumGLTestLib elsewhere -- and ASan started loudly complaining about GL::defaultFramebuffer being duplicated. So instead there's now a dedicated subset of DebugTools just for the GL test themselves, containing currently just DebugTools::bufferData(), as nothing else is needed ATM. It may grow further when needed, such as with textureImage(), or CompareImage, etc. But of course that wouldn't be enough -- MagnumOpenGLTesterTestLib actually still links to MagnumGL for Other Reasons, meaning we have to pass it last to prefer symbols from MagnumGLTestLib which have graceful asserts enabled. Hopefully this works well enough, otherwise I'd have to figure out yet another variant of the fix. --- src/Magnum/DebugTools/CMakeLists.txt | 26 ++++++++++++++++++++++++++ src/Magnum/GL/Test/CMakeLists.txt | 21 ++++++++++++++++++--- 2 files changed, 44 insertions(+), 3 deletions(-) diff --git a/src/Magnum/DebugTools/CMakeLists.txt b/src/Magnum/DebugTools/CMakeLists.txt index 2618fbab7..e369aa67a 100644 --- a/src/Magnum/DebugTools/CMakeLists.txt +++ b/src/Magnum/DebugTools/CMakeLists.txt @@ -181,6 +181,32 @@ if(BUILD_TESTS) endif() endif() + # A subset of DebugTools used by GL's own tests that link to + # MagnumGLTestLib (or MagnumOpenGLTesterTestLib), linking also to + # MagnumGLTestLibMagnumGL. We can't link those to MagnumDebugToolsTestLib + # because it depends on MagnumGL instead, which would lead to the + # GL::Context global to be duplicated, causing the dreaded + # + # GL::Context::current(): no current context + # + # assertion. We however also can't link MagnumDebugToolsTestLib to + # MagnumGLTestLib, because the other libraries it depends on (MeshTools, + # Shaders...) link to MagnumGL and so the same problem would just reappear + # elsewhere. + if(TARGET_GL AND BUILD_GL_TESTS AND NOT (MAGNUM_TARGET_WEBGL AND MAGNUM_TARGET_GLES2)) + add_library(MagnumDebugToolsGLTestLibSubset ${SHARED_OR_STATIC} + # Add more files if needed + BufferData.cpp + BufferData.h) + set_target_properties(MagnumDebugToolsGLTestLibSubset PROPERTIES DEBUG_POSTFIX "-d") + target_compile_definitions(MagnumDebugToolsGLTestLibSubset PRIVATE + "CORRADE_GRACEFUL_ASSERT" "MagnumDebugTools_EXPORTS") + if(BUILD_STATIC_PIC) + set_target_properties(MagnumDebugToolsGLTestLibSubset PROPERTIES POSITION_INDEPENDENT_CODE ON) + endif() + target_link_libraries(MagnumDebugToolsGLTestLibSubset PUBLIC Magnum MagnumGLTestLib) + endif() + add_subdirectory(Test) endif() diff --git a/src/Magnum/GL/Test/CMakeLists.txt b/src/Magnum/GL/Test/CMakeLists.txt index 651848fb5..a51ee2fda 100644 --- a/src/Magnum/GL/Test/CMakeLists.txt +++ b/src/Magnum/GL/Test/CMakeLists.txt @@ -92,9 +92,17 @@ if(BUILD_GL_TESTS) target_link_libraries(GLBufferGLTest PRIVATE MagnumDebugTools) endif() - corrade_add_test(GLFramebufferGLTest FramebufferGLTest.cpp LIBRARIES MagnumOpenGLTesterTestLib) + corrade_add_test(GLFramebufferGLTest FramebufferGLTest.cpp) + # See DebugTools/CMakeLists.txt for details about + # MagnumDebugToolsGLTestLibSubset. It also has to be before + # MagnumOpenGLTesterTestLib to pick GL symbols with CORRADE_GRACEFUL_ASSERT + # over ones without... because MagnumOpenGLTesterTestLib is actually + # linking to MagnumGL on Linux and macOS. + # TODO fix MagnumOpenGLTesterTestLib to link to MagnumGLTestLib always if(NOT MAGNUM_TARGET_GLES2) - target_link_libraries(GLFramebufferGLTest PRIVATE MagnumDebugTools) + target_link_libraries(GLFramebufferGLTest PRIVATE MagnumDebugToolsGLTestLibSubset MagnumOpenGLTesterTestLib) + else() + target_link_libraries(GLFramebufferGLTest PRIVATE MagnumOpenGLTesterTestLib) endif() corrade_add_test(GLContextGLTest ContextGLTest.cpp LIBRARIES MagnumOpenGLTester) @@ -170,7 +178,14 @@ if(BUILD_GL_TESTS) endif() if(NOT MAGNUM_TARGET_GLES2) - corrade_add_test(GLBufferImageGLTest BufferImageGLTest.cpp LIBRARIES MagnumOpenGLTesterTestLib MagnumDebugTools) + # See DebugTools/CMakeLists.txt for details about + # MagnumDebugToolsGLTestLibSubset. It also has to be before + # MagnumOpenGLTesterTestLib to pick GL symbols with + # CORRADE_GRACEFUL_ASSERT over ones without... because + # MagnumOpenGLTesterTestLib is actually linking to MagnumGL on Linux + # and macOS. + # TODO fix MagnumOpenGLTesterTestLib to link to MagnumGLTestLib always + corrade_add_test(GLBufferImageGLTest BufferImageGLTest.cpp LIBRARIES MagnumDebugToolsGLTestLibSubset MagnumOpenGLTesterTestLib) corrade_add_test(GLPrimitiveQueryGLTest PrimitiveQueryGLTest.cpp LIBRARIES MagnumOpenGLTester) corrade_add_test(GLTextureArrayGLTest TextureArrayGLTest.cpp LIBRARIES MagnumOpenGLTester) corrade_add_test(GLTransformFeedbackGLTest TransformFeedbackGLTest.cpp LIBRARIES MagnumOpenGLTester MagnumDebugTools)