From 745830e1a967b51bfa612853265bd626b19d725b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Vladim=C3=ADr=20Vondru=C5=A1?= Date: Wed, 1 Jun 2022 18:45:19 +0200 Subject: [PATCH] Vk: properly redirect internal pointers on ShaderSet move. Discovered by an upgraded ASan on the new Ubuntu 18.04 CI image. Yay! --- src/Magnum/Vk/ShaderSet.cpp | 22 ++++++++ src/Magnum/Vk/Test/ShaderSetTest.cpp | 77 +++++++++++++++++++++++++--- 2 files changed, 92 insertions(+), 7 deletions(-) diff --git a/src/Magnum/Vk/ShaderSet.cpp b/src/Magnum/Vk/ShaderSet.cpp index 3d392ca1a..020d56d1f 100644 --- a/src/Magnum/Vk/ShaderSet.cpp +++ b/src/Magnum/Vk/ShaderSet.cpp @@ -48,6 +48,14 @@ ShaderSet::ShaderSet(ShaderSet&& other) noexcept: _stageCount{other._stageCount} you?! */ Utility::copy(other._stages, _stages); Utility::copy(other._specializations, _specializations); + + /* Redirect the specialization pointers to our state if they point to the + original object */ + for(std::size_t i = 0; i != _stageCount; ++i) { + if(_stages[i].pSpecializationInfo == &other._specializations[i]) + _stages[i].pSpecializationInfo = &_specializations[i]; + } + /* The easiest is to just make the original stage list empty and leave whatever dangling internal pointers are there. Otherwise we'd need to clear even the entrypoint field in case the name is owned, which would @@ -63,6 +71,20 @@ ShaderSet& ShaderSet::operator=(ShaderSet&& other) noexcept { swap(other._specializations, _specializations); swap(other._stageCount, _stageCount); swap(other._state, _state); + + /* Redirect the specialization pointers to our state if they point to the + original object */ + for(std::size_t i = 0; i != _stageCount; ++i) { + if(_stages[i].pSpecializationInfo == &other._specializations[i]) + _stages[i].pSpecializationInfo = &_specializations[i]; + } + + /* Vice versa for the original object */ + for(std::size_t i = 0; i != other._stageCount; ++i) { + if(other._stages[i].pSpecializationInfo == &_specializations[i]) + other._stages[i].pSpecializationInfo = &other._specializations[i]; + } + return *this; } diff --git a/src/Magnum/Vk/Test/ShaderSetTest.cpp b/src/Magnum/Vk/Test/ShaderSetTest.cpp index 1f6bec8ff..630cbed12 100644 --- a/src/Magnum/Vk/Test/ShaderSetTest.cpp +++ b/src/Magnum/Vk/Test/ShaderSetTest.cpp @@ -27,6 +27,7 @@ #include #include #include +#include #include #include "Magnum/Vk/Device.h" @@ -107,50 +108,112 @@ void ShaderSetTest::constructCopy() { } void ShaderSetTest::constructMove() { + VkSpecializationInfo externalSpecializationInfo1, + externalSpecializationInfo2; + ShaderSet c; + c.addShader(ShaderStage::RayAnyHit, VkShaderModule{}, "mine", { + {57, 0.3f} + }); + /* This one has an externally-supplied specialization pointer, to + verify it's preserved on move (the first has to be redirected) */ + c.addShader(ShaderStage::Vertex, VkShaderModule{}, "theirs"); + c.stages()[1].pSpecializationInfo = &externalSpecializationInfo1; { ShaderSet a; + /* The double reinterpret_cast is needed because the handle is an uint64_t instead of a pointer on 32-bit builds and only this works on both */ a.addShader(ShaderStage::Geometry, reinterpret_cast(reinterpret_cast(0xdeadbeef)), "main!"_s.exceptSuffix(1), { {42, 1.15f} }); - CORRADE_COMPARE(a.stages().size(), 1); + + /* This one has an externally-supplied specialization pointer, to + verify it's preserved on move (others will have it redirected) */ + a.addShader(ShaderStage::Fragment, reinterpret_cast(reinterpret_cast(0xbadf00d)), "yours"); + a.stages()[1].pSpecializationInfo = &externalSpecializationInfo2; + + CORRADE_COMPARE(a.stages().size(), 2); CORRADE_COMPARE(a.stages()[0].pName, "main"_s); - CORRADE_VERIFY(a.stages()[0].pSpecializationInfo); + /* This specialization info is stored inside the instance */ + CORRADE_COMPARE_AS(reinterpret_cast(a.stages()[0].pSpecializationInfo), + reinterpret_cast(&a), + TestSuite::Compare::GreaterOrEqual); + CORRADE_COMPARE_AS(reinterpret_cast(a.stages()[0].pSpecializationInfo), + reinterpret_cast(&a + 1), + TestSuite::Compare::Less); CORRADE_COMPARE(a.stages()[0].pSpecializationInfo->mapEntryCount, 1); CORRADE_VERIFY(a.stages()[0].pSpecializationInfo->pMapEntries); CORRADE_COMPARE(a.stages()[0].pSpecializationInfo->pMapEntries[0].constantID, 42); CORRADE_VERIFY(a.stages()[0].pSpecializationInfo->pData); CORRADE_COMPARE(*reinterpret_cast(a.stages()[0].pSpecializationInfo->pData), 1.15f); + CORRADE_COMPARE(a.stages()[1].pName, "yours"_s); + /* This specialization info points elsewhere */ + CORRADE_COMPARE(a.stages()[1].pSpecializationInfo, &externalSpecializationInfo2); + ShaderSet b = std::move(a); CORRADE_VERIFY(a.stages().isEmpty()); - CORRADE_COMPARE(b.stages().size(), 1); + CORRADE_COMPARE(b.stages().size(), 2); CORRADE_COMPARE(b.stages()[0].pName, "main"_s); - CORRADE_VERIFY(b.stages()[0].pSpecializationInfo); + /* This specialization info should be redirected inside the new + instance */ + CORRADE_COMPARE_AS(reinterpret_cast(b.stages()[0].pSpecializationInfo), + reinterpret_cast(&b), + TestSuite::Compare::GreaterOrEqual); + CORRADE_COMPARE_AS(reinterpret_cast(b.stages()[0].pSpecializationInfo), + reinterpret_cast(&b + 1), + TestSuite::Compare::Less); CORRADE_COMPARE(b.stages()[0].pSpecializationInfo->mapEntryCount, 1); CORRADE_VERIFY(b.stages()[0].pSpecializationInfo->pMapEntries); CORRADE_COMPARE(b.stages()[0].pSpecializationInfo->pMapEntries[0].constantID, 42); CORRADE_VERIFY(b.stages()[0].pSpecializationInfo->pData); CORRADE_COMPARE(*reinterpret_cast(b.stages()[0].pSpecializationInfo->pData), 1.15f); + CORRADE_COMPARE(b.stages()[1].pName, "yours"_s); + /* This specialization info should not be redirected anywhere */ + CORRADE_COMPARE(b.stages()[1].pSpecializationInfo, &externalSpecializationInfo2); + c = std::move(b); - CORRADE_VERIFY(b.stages().isEmpty()); + CORRADE_COMPARE(b.stages().size(), 2); + CORRADE_COMPARE(b.stages()[0].pName, "mine"_s); + /* The two classes got swapped, so the original C content should now + point to inside the B instance */ + CORRADE_COMPARE_AS(reinterpret_cast(b.stages()[0].pSpecializationInfo), + reinterpret_cast(&b), + TestSuite::Compare::GreaterOrEqual); + CORRADE_COMPARE_AS(reinterpret_cast(b.stages()[0].pSpecializationInfo), + reinterpret_cast(&b + 1), + TestSuite::Compare::Less); + CORRADE_COMPARE(b.stages()[1].pName, "theirs"_s); + /* This specialization info should not be redirected anywhere */ + CORRADE_COMPARE(b.stages()[1].pSpecializationInfo, &externalSpecializationInfo1); } /* Doing this in outer scope to verify that the internal state pointer got properly transferred as well and we're not referencing destroyed data */ - CORRADE_COMPARE(c.stages().size(), 1); + CORRADE_COMPARE(c.stages().size(), 2); + CORRADE_COMPARE(c.stages()[0].pName, "main"_s); - CORRADE_VERIFY(c.stages()[0].pSpecializationInfo); + /* The specialization info should be redirected inside the new instance + again */ + CORRADE_COMPARE_AS(reinterpret_cast(c.stages()[0].pSpecializationInfo), + reinterpret_cast(&c), + TestSuite::Compare::GreaterOrEqual); + CORRADE_COMPARE_AS(reinterpret_cast(c.stages()[0].pSpecializationInfo), + reinterpret_cast(&c + 1), + TestSuite::Compare::Less); CORRADE_COMPARE(c.stages()[0].pSpecializationInfo->mapEntryCount, 1); CORRADE_VERIFY(c.stages()[0].pSpecializationInfo->pMapEntries); CORRADE_COMPARE(c.stages()[0].pSpecializationInfo->pMapEntries[0].constantID, 42); CORRADE_VERIFY(c.stages()[0].pSpecializationInfo->pData); CORRADE_COMPARE(*reinterpret_cast(c.stages()[0].pSpecializationInfo->pData), 1.15f); + + CORRADE_COMPARE(c.stages()[1].pName, "yours"_s); + /* This specialization info should not be redirected anywhere */ + CORRADE_COMPARE(c.stages()[1].pSpecializationInfo, &externalSpecializationInfo2); } void ShaderSetTest::addShader() {