Skip to content

Commit 180a4d2

Browse files
spot and fix a huge problem in calling a virtual method from a destructor
NOTE: https://www.artima.com/articles/never-call-virtual-functions-during-construction-or-destruction
1 parent f9ae7ee commit 180a4d2

File tree

4 files changed

+17
-10
lines changed

4 files changed

+17
-10
lines changed

include/nbl/video/IGPUCommandBuffer.h

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -126,15 +126,17 @@ class NBL_API2 IGPUCommandBuffer :
126126

127127
IGPUCommandBuffer(core::smart_refctd_ptr<const ILogicalDevice>&& dev, E_LEVEL lvl, core::smart_refctd_ptr<IGPUCommandPool>&& _cmdpool, system::logger_opt_smart_ptr&& logger) : base_t(lvl), IBackendObject(std::move(dev)), m_cmdpool(_cmdpool), m_logger(std::move(logger))
128128
{
129+
// prevent false positives on first `begin()`
130+
m_resetCheckedStamp = m_cmdpool->getResetCounter();
129131
}
130132

131-
virtual ~IGPUCommandBuffer()
133+
// meant to be invoked by most derived class
134+
// TODO: idea make a macro for overriding all `delete` operators of a class to enforce a finalizer that runs in reverse order to destructors (to allow polymorphic cleanups)
135+
inline void out_of_order_dtor()
132136
{
133137
// Only release the resources if the parent pool has not been reset because if it has been then the resources will already be released.
134138
if (!checkForParentPoolReset())
135-
{
136139
releaseResourcesBackToPool();
137-
}
138140
}
139141

140142
system::logger_opt_smart_ptr m_logger;
@@ -252,9 +254,7 @@ class NBL_API2 IGPUCommandBuffer :
252254
m_commandList.head = nullptr;
253255
m_commandList.tail = nullptr;
254256

255-
// Example 05 crashes here, with assert in `ucrt` maybe vtable gets lost or something?
256-
// For now commented out because we only have a Vulkan backend and function doesn't do anything.
257-
//checkForParentPoolReset_impl();
257+
checkForParentPoolReset_impl();
258258

259259
return true;
260260
}
@@ -288,7 +288,7 @@ class NBL_API2 IGPUCommandBuffer :
288288
return true;
289289
}
290290

291-
uint64_t m_resetCheckedStamp = 0;
291+
uint64_t m_resetCheckedStamp;
292292

293293
// This bound descriptor set record doesn't include the descriptor sets whose layout has _any_ one of its bindings
294294
// created with IGPUDescriptorSetLayout::SBinding::E_CREATE_FLAGS::ECF_UPDATE_AFTER_BIND_BIT

src/nbl/video/CVulkanCommandBuffer.h

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -401,6 +401,12 @@ class CVulkanCommandBuffer : public IGPUCommandBuffer
401401

402402
inline const void* getNativeHandle() const override {return &m_cmdbuf;}
403403
VkCommandBuffer getInternalObject() const {return m_cmdbuf;}
404+
405+
protected:
406+
virtual ~CVulkanCommandBuffer() final
407+
{
408+
out_of_order_dtor();
409+
}
404410

405411
private:
406412
VkCommandBuffer m_cmdbuf;

src/nbl/video/IGPUCommandBuffer.cpp

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -24,8 +24,7 @@ bool IGPUCommandBuffer::begin(core::bitflag<E_USAGE> flags, const SInheritanceIn
2424
}
2525

2626
checkForParentPoolReset();
27-
m_resetCheckedStamp = m_cmdpool->getResetCounter();
28-
27+
// still not initial and pool wasn't reset
2928
if (m_state != ES_INITIAL)
3029
{
3130
releaseResourcesBackToPool();
@@ -39,6 +38,8 @@ bool IGPUCommandBuffer::begin(core::bitflag<E_USAGE> flags, const SInheritanceIn
3938
m_state = ES_INITIAL;
4039
}
4140

41+
// still not initial (we're trying to single-reset a commandbuffer that cannot be individually reset)
42+
// should have been caught out above
4243
assert(m_state == ES_INITIAL);
4344

4445
if (inheritanceInfo != nullptr)

0 commit comments

Comments
 (0)