From 0e9320b5b5a611ea0eac3ea5245d866b11cc22cf Mon Sep 17 00:00:00 2001 From: Rafael Cintron Date: Thu, 23 Apr 2020 19:47:12 +0000 Subject: [PATCH] Use Ref instead of TextureBase* in more places To avoid accidental memory leaks on account of using raw pointers, use Ref as method return type except at Dawn interface boundaries. Change-Id: I6459062ee28984de2cb1d5a2059bc70cf82b2faf Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/19580 Reviewed-by: Austin Eng Commit-Queue: Rafael Cintron --- src/common/Result.h | 24 ++++++++++++----- src/dawn_native/Device.cpp | 13 +++++----- src/dawn_native/Device.h | 4 +-- src/dawn_native/d3d12/D3D12Backend.cpp | 8 +++--- src/dawn_native/d3d12/DeviceD3D12.cpp | 12 ++++----- src/dawn_native/d3d12/DeviceD3D12.h | 11 ++++---- src/dawn_native/d3d12/TextureD3D12.cpp | 18 ++++++------- src/dawn_native/d3d12/TextureD3D12.h | 14 +++++----- src/dawn_native/metal/DeviceMTL.h | 3 ++- src/dawn_native/metal/DeviceMTL.mm | 4 +-- src/dawn_native/null/DeviceNull.cpp | 4 +-- src/dawn_native/null/DeviceNull.h | 3 ++- src/dawn_native/opengl/DeviceGL.cpp | 4 +-- src/dawn_native/opengl/DeviceGL.h | 3 ++- src/dawn_native/vulkan/DeviceVk.cpp | 2 +- src/dawn_native/vulkan/DeviceVk.h | 3 ++- src/dawn_native/vulkan/TextureVk.cpp | 5 ++-- src/dawn_native/vulkan/TextureVk.h | 3 ++- src/tests/unittests/ResultTests.cpp | 36 ++++++++++++++++++++++++++ 19 files changed, 114 insertions(+), 60 deletions(-) diff --git a/src/common/Result.h b/src/common/Result.h index 85095684f3..4cea7f2f92 100644 --- a/src/common/Result.h +++ b/src/common/Result.h @@ -177,11 +177,14 @@ class DAWN_NO_DISCARD Result, E> { static_assert(alignof_if_defined_else_default >= 4, "Result, E> reserves two bits for tagging pointers"); - Result(Ref&& success); + template + Result(Ref&& success); Result(std::unique_ptr error); - Result(Result, E>&& other); - Result, E>& operator=(Result, E>&& other); + template + Result(Result, E>&& other); + template + Result, E>& operator=(Result, E>&& other); ~Result(); @@ -192,6 +195,9 @@ class DAWN_NO_DISCARD Result, E> { std::unique_ptr AcquireError(); private: + template + friend class Result; + intptr_t mPayload = detail::kEmptyPayload; }; @@ -399,8 +405,10 @@ std::unique_ptr Result::AcquireError() { // Implementation of Result, E> template -Result, E>::Result(Ref&& success) +template +Result, E>::Result(Ref&& success) : mPayload(detail::MakePayload(success.Detach(), detail::Success)) { + static_assert(std::is_convertible::value, ""); } template @@ -409,12 +417,16 @@ Result, E>::Result(std::unique_ptr error) } template -Result, E>::Result(Result, E>&& other) : mPayload(other.mPayload) { +template +Result, E>::Result(Result, E>&& other) : mPayload(other.mPayload) { + static_assert(std::is_convertible::value, ""); other.mPayload = detail::kEmptyPayload; } template -Result, E>& Result, E>::operator=(Result, E>&& other) { +template +Result, E>& Result, E>::operator=(Result, E>&& other) { + static_assert(std::is_convertible::value, ""); ASSERT(mPayload == detail::kEmptyPayload); mPayload = other.mPayload; other.mPayload = detail::kEmptyPayload; diff --git a/src/dawn_native/Device.cpp b/src/dawn_native/Device.cpp index 41c76648ce..2fbf2c88fb 100644 --- a/src/dawn_native/Device.cpp +++ b/src/dawn_native/Device.cpp @@ -628,13 +628,13 @@ namespace dawn_native { return result; } TextureBase* DeviceBase::CreateTexture(const TextureDescriptor* descriptor) { - TextureBase* result = nullptr; + Ref result; - if (ConsumedError(CreateTextureInternal(&result, descriptor))) { + if (ConsumedError(CreateTextureInternal(descriptor), &result)) { return TextureBase::MakeError(this); } - return result; + return result.Detach(); } TextureViewBase* DeviceBase::CreateTextureView(TextureBase* texture, const TextureViewDescriptor* descriptor) { @@ -918,14 +918,13 @@ namespace dawn_native { return {}; } - MaybeError DeviceBase::CreateTextureInternal(TextureBase** result, - const TextureDescriptor* descriptor) { + ResultOrError> DeviceBase::CreateTextureInternal( + const TextureDescriptor* descriptor) { DAWN_TRY(ValidateIsAlive()); if (IsValidationEnabled()) { DAWN_TRY(ValidateTextureDescriptor(this, descriptor)); } - DAWN_TRY_ASSIGN(*result, CreateTextureImpl(descriptor)); - return {}; + return CreateTextureImpl(descriptor); } MaybeError DeviceBase::CreateTextureViewInternal(TextureViewBase** result, diff --git a/src/dawn_native/Device.h b/src/dawn_native/Device.h index 8bce4aba22..5187427246 100644 --- a/src/dawn_native/Device.h +++ b/src/dawn_native/Device.h @@ -243,7 +243,7 @@ namespace dawn_native { Surface* surface, NewSwapChainBase* previousSwapChain, const SwapChainDescriptor* descriptor) = 0; - virtual ResultOrError CreateTextureImpl( + virtual ResultOrError> CreateTextureImpl( const TextureDescriptor* descriptor) = 0; virtual ResultOrError CreateTextureViewImpl( TextureBase* texture, @@ -269,7 +269,7 @@ namespace dawn_native { MaybeError CreateSwapChainInternal(SwapChainBase** result, Surface* surface, const SwapChainDescriptor* descriptor); - MaybeError CreateTextureInternal(TextureBase** result, const TextureDescriptor* descriptor); + ResultOrError> CreateTextureInternal(const TextureDescriptor* descriptor); MaybeError CreateTextureViewInternal(TextureViewBase** result, TextureBase* texture, const TextureViewDescriptor* descriptor); diff --git a/src/dawn_native/d3d12/D3D12Backend.cpp b/src/dawn_native/d3d12/D3D12Backend.cpp index 3543c638a3..f78975ef7a 100644 --- a/src/dawn_native/d3d12/D3D12Backend.cpp +++ b/src/dawn_native/d3d12/D3D12Backend.cpp @@ -63,10 +63,10 @@ namespace dawn_native { namespace d3d12 { WGPUTexture WrapSharedHandle(WGPUDevice device, const ExternalImageDescriptorDXGISharedHandle* descriptor) { Device* backendDevice = reinterpret_cast(device); - TextureBase* texture = backendDevice->WrapSharedHandle(descriptor, descriptor->sharedHandle, - descriptor->acquireMutexKey, - descriptor->isSwapChainTexture); - return reinterpret_cast(texture); + Ref texture = backendDevice->WrapSharedHandle( + descriptor, descriptor->sharedHandle, descriptor->acquireMutexKey, + descriptor->isSwapChainTexture); + return reinterpret_cast(texture.Detach()); } }} // namespace dawn_native::d3d12 diff --git a/src/dawn_native/d3d12/DeviceD3D12.cpp b/src/dawn_native/d3d12/DeviceD3D12.cpp index 41895b3f8d..8f09bf94a1 100644 --- a/src/dawn_native/d3d12/DeviceD3D12.cpp +++ b/src/dawn_native/d3d12/DeviceD3D12.cpp @@ -292,7 +292,7 @@ namespace dawn_native { namespace d3d12 { const SwapChainDescriptor* descriptor) { return DAWN_VALIDATION_ERROR("New swapchains not implemented."); } - ResultOrError Device::CreateTextureImpl(const TextureDescriptor* descriptor) { + ResultOrError> Device::CreateTextureImpl(const TextureDescriptor* descriptor) { return Texture::Create(this, descriptor); } ResultOrError Device::CreateTextureViewImpl( @@ -339,11 +339,11 @@ namespace dawn_native { namespace d3d12 { initialUsage); } - TextureBase* Device::WrapSharedHandle(const ExternalImageDescriptor* descriptor, - HANDLE sharedHandle, - uint64_t acquireMutexKey, - bool isSwapChainTexture) { - TextureBase* dawnTexture; + Ref Device::WrapSharedHandle(const ExternalImageDescriptor* descriptor, + HANDLE sharedHandle, + uint64_t acquireMutexKey, + bool isSwapChainTexture) { + Ref dawnTexture; if (ConsumedError(Texture::Create(this, descriptor, sharedHandle, acquireMutexKey, isSwapChainTexture), &dawnTexture)) diff --git a/src/dawn_native/d3d12/DeviceD3D12.h b/src/dawn_native/d3d12/DeviceD3D12.h index f7298b5a31..615f1fb54e 100644 --- a/src/dawn_native/d3d12/DeviceD3D12.h +++ b/src/dawn_native/d3d12/DeviceD3D12.h @@ -113,10 +113,10 @@ namespace dawn_native { namespace d3d12 { StagingDescriptorAllocator* GetDepthStencilViewAllocator() const; - TextureBase* WrapSharedHandle(const ExternalImageDescriptor* descriptor, - HANDLE sharedHandle, - uint64_t acquireMutexKey, - bool isSwapChainTexture); + Ref WrapSharedHandle(const ExternalImageDescriptor* descriptor, + HANDLE sharedHandle, + uint64_t acquireMutexKey, + bool isSwapChainTexture); ResultOrError> CreateKeyedMutexForTexture( ID3D12Resource* d3d12Resource); void ReleaseKeyedMutexForTexture(ComPtr dxgiKeyedMutex); @@ -146,7 +146,8 @@ namespace dawn_native { namespace d3d12 { Surface* surface, NewSwapChainBase* previousSwapChain, const SwapChainDescriptor* descriptor) override; - ResultOrError CreateTextureImpl(const TextureDescriptor* descriptor) override; + ResultOrError> CreateTextureImpl( + const TextureDescriptor* descriptor) override; ResultOrError CreateTextureViewImpl( TextureBase* texture, const TextureViewDescriptor* descriptor) override; diff --git a/src/dawn_native/d3d12/TextureD3D12.cpp b/src/dawn_native/d3d12/TextureD3D12.cpp index bbcd4e191c..a6043e71d6 100644 --- a/src/dawn_native/d3d12/TextureD3D12.cpp +++ b/src/dawn_native/d3d12/TextureD3D12.cpp @@ -273,19 +273,19 @@ namespace dawn_native { namespace d3d12 { return {}; } - ResultOrError Texture::Create(Device* device, - const TextureDescriptor* descriptor) { + ResultOrError> Texture::Create(Device* device, + const TextureDescriptor* descriptor) { Ref dawnTexture = AcquireRef(new Texture(device, descriptor, TextureState::OwnedInternal)); DAWN_TRY(dawnTexture->InitializeAsInternalTexture()); - return dawnTexture.Detach(); + return std::move(dawnTexture); } - ResultOrError Texture::Create(Device* device, - const ExternalImageDescriptor* descriptor, - HANDLE sharedHandle, - uint64_t acquireMutexKey, - bool isSwapChainTexture) { + ResultOrError> Texture::Create(Device* device, + const ExternalImageDescriptor* descriptor, + HANDLE sharedHandle, + uint64_t acquireMutexKey, + bool isSwapChainTexture) { const TextureDescriptor* textureDescriptor = reinterpret_cast(descriptor->cTextureDescriptor); @@ -297,7 +297,7 @@ namespace dawn_native { namespace d3d12 { dawnTexture->SetIsSubresourceContentInitialized(descriptor->isCleared, 0, textureDescriptor->mipLevelCount, 0, textureDescriptor->arrayLayerCount); - return dawnTexture.Detach(); + return std::move(dawnTexture); } MaybeError Texture::InitializeAsExternalTexture(const TextureDescriptor* descriptor, diff --git a/src/dawn_native/d3d12/TextureD3D12.h b/src/dawn_native/d3d12/TextureD3D12.h index 2f4380dda7..88a24f4a58 100644 --- a/src/dawn_native/d3d12/TextureD3D12.h +++ b/src/dawn_native/d3d12/TextureD3D12.h @@ -34,13 +34,13 @@ namespace dawn_native { namespace d3d12 { class Texture final : public TextureBase { public: - static ResultOrError Create(Device* device, - const TextureDescriptor* descriptor); - static ResultOrError Create(Device* device, - const ExternalImageDescriptor* descriptor, - HANDLE sharedHandle, - uint64_t acquireMutexKey, - bool isSwapChainTexture); + static ResultOrError> Create(Device* device, + const TextureDescriptor* descriptor); + static ResultOrError> Create(Device* device, + const ExternalImageDescriptor* descriptor, + HANDLE sharedHandle, + uint64_t acquireMutexKey, + bool isSwapChainTexture); Texture(Device* device, const TextureDescriptor* descriptor, ComPtr d3d12Texture); diff --git a/src/dawn_native/metal/DeviceMTL.h b/src/dawn_native/metal/DeviceMTL.h index 4a5195e2a9..bc755916c3 100644 --- a/src/dawn_native/metal/DeviceMTL.h +++ b/src/dawn_native/metal/DeviceMTL.h @@ -94,7 +94,8 @@ namespace dawn_native { namespace metal { Surface* surface, NewSwapChainBase* previousSwapChain, const SwapChainDescriptor* descriptor) override; - ResultOrError CreateTextureImpl(const TextureDescriptor* descriptor) override; + ResultOrError> CreateTextureImpl( + const TextureDescriptor* descriptor) override; ResultOrError CreateTextureViewImpl( TextureBase* texture, const TextureViewDescriptor* descriptor) override; diff --git a/src/dawn_native/metal/DeviceMTL.mm b/src/dawn_native/metal/DeviceMTL.mm index 341c131d1b..30521b85c4 100644 --- a/src/dawn_native/metal/DeviceMTL.mm +++ b/src/dawn_native/metal/DeviceMTL.mm @@ -145,8 +145,8 @@ namespace dawn_native { namespace metal { const SwapChainDescriptor* descriptor) { return new SwapChain(this, surface, previousSwapChain, descriptor); } - ResultOrError Device::CreateTextureImpl(const TextureDescriptor* descriptor) { - return new Texture(this, descriptor); + ResultOrError> Device::CreateTextureImpl(const TextureDescriptor* descriptor) { + return AcquireRef(new Texture(this, descriptor)); } ResultOrError Device::CreateTextureViewImpl( TextureBase* texture, diff --git a/src/dawn_native/null/DeviceNull.cpp b/src/dawn_native/null/DeviceNull.cpp index 701e720f7e..1f75cf9523 100644 --- a/src/dawn_native/null/DeviceNull.cpp +++ b/src/dawn_native/null/DeviceNull.cpp @@ -159,8 +159,8 @@ namespace dawn_native { namespace null { const SwapChainDescriptor* descriptor) { return new SwapChain(this, surface, previousSwapChain, descriptor); } - ResultOrError Device::CreateTextureImpl(const TextureDescriptor* descriptor) { - return new Texture(this, descriptor, TextureBase::TextureState::OwnedInternal); + ResultOrError> Device::CreateTextureImpl(const TextureDescriptor* descriptor) { + return AcquireRef(new Texture(this, descriptor, TextureBase::TextureState::OwnedInternal)); } ResultOrError Device::CreateTextureViewImpl( TextureBase* texture, diff --git a/src/dawn_native/null/DeviceNull.h b/src/dawn_native/null/DeviceNull.h index 83f09ae360..fc67316708 100644 --- a/src/dawn_native/null/DeviceNull.h +++ b/src/dawn_native/null/DeviceNull.h @@ -132,7 +132,8 @@ namespace dawn_native { namespace null { Surface* surface, NewSwapChainBase* previousSwapChain, const SwapChainDescriptor* descriptor) override; - ResultOrError CreateTextureImpl(const TextureDescriptor* descriptor) override; + ResultOrError> CreateTextureImpl( + const TextureDescriptor* descriptor) override; ResultOrError CreateTextureViewImpl( TextureBase* texture, const TextureViewDescriptor* descriptor) override; diff --git a/src/dawn_native/opengl/DeviceGL.cpp b/src/dawn_native/opengl/DeviceGL.cpp index 38b7123ef7..47d2eec44c 100644 --- a/src/dawn_native/opengl/DeviceGL.cpp +++ b/src/dawn_native/opengl/DeviceGL.cpp @@ -136,8 +136,8 @@ namespace dawn_native { namespace opengl { const SwapChainDescriptor* descriptor) { return DAWN_VALIDATION_ERROR("New swapchains not implemented."); } - ResultOrError Device::CreateTextureImpl(const TextureDescriptor* descriptor) { - return new Texture(this, descriptor); + ResultOrError> Device::CreateTextureImpl(const TextureDescriptor* descriptor) { + return AcquireRef(new Texture(this, descriptor)); } ResultOrError Device::CreateTextureViewImpl( TextureBase* texture, diff --git a/src/dawn_native/opengl/DeviceGL.h b/src/dawn_native/opengl/DeviceGL.h index f892a84cfc..a768c89b56 100644 --- a/src/dawn_native/opengl/DeviceGL.h +++ b/src/dawn_native/opengl/DeviceGL.h @@ -89,7 +89,8 @@ namespace dawn_native { namespace opengl { Surface* surface, NewSwapChainBase* previousSwapChain, const SwapChainDescriptor* descriptor) override; - ResultOrError CreateTextureImpl(const TextureDescriptor* descriptor) override; + ResultOrError> CreateTextureImpl( + const TextureDescriptor* descriptor) override; ResultOrError CreateTextureViewImpl( TextureBase* texture, const TextureViewDescriptor* descriptor) override; diff --git a/src/dawn_native/vulkan/DeviceVk.cpp b/src/dawn_native/vulkan/DeviceVk.cpp index 7ce2e6c7ba..f3b176b1ef 100644 --- a/src/dawn_native/vulkan/DeviceVk.cpp +++ b/src/dawn_native/vulkan/DeviceVk.cpp @@ -148,7 +148,7 @@ namespace dawn_native { namespace vulkan { const SwapChainDescriptor* descriptor) { return DAWN_VALIDATION_ERROR("New swapchains not implemented."); } - ResultOrError Device::CreateTextureImpl(const TextureDescriptor* descriptor) { + ResultOrError> Device::CreateTextureImpl(const TextureDescriptor* descriptor) { return Texture::Create(this, descriptor); } ResultOrError Device::CreateTextureViewImpl( diff --git a/src/dawn_native/vulkan/DeviceVk.h b/src/dawn_native/vulkan/DeviceVk.h index 255971f1c3..63758c1f76 100644 --- a/src/dawn_native/vulkan/DeviceVk.h +++ b/src/dawn_native/vulkan/DeviceVk.h @@ -123,7 +123,8 @@ namespace dawn_native { namespace vulkan { Surface* surface, NewSwapChainBase* previousSwapChain, const SwapChainDescriptor* descriptor) override; - ResultOrError CreateTextureImpl(const TextureDescriptor* descriptor) override; + ResultOrError> CreateTextureImpl( + const TextureDescriptor* descriptor) override; ResultOrError CreateTextureViewImpl( TextureBase* texture, const TextureViewDescriptor* descriptor) override; diff --git a/src/dawn_native/vulkan/TextureVk.cpp b/src/dawn_native/vulkan/TextureVk.cpp index a98a2b7bb6..0c85fa94da 100644 --- a/src/dawn_native/vulkan/TextureVk.cpp +++ b/src/dawn_native/vulkan/TextureVk.cpp @@ -406,11 +406,12 @@ namespace dawn_native { namespace vulkan { } // static - ResultOrError Texture::Create(Device* device, const TextureDescriptor* descriptor) { + ResultOrError> Texture::Create(Device* device, + const TextureDescriptor* descriptor) { Ref texture = AcquireRef(new Texture(device, descriptor, TextureState::OwnedInternal)); DAWN_TRY(texture->InitializeAsInternalTexture()); - return texture.Detach(); + return std::move(texture); } // static diff --git a/src/dawn_native/vulkan/TextureVk.h b/src/dawn_native/vulkan/TextureVk.h index 600e9a676a..f9b44cd098 100644 --- a/src/dawn_native/vulkan/TextureVk.h +++ b/src/dawn_native/vulkan/TextureVk.h @@ -40,7 +40,8 @@ namespace dawn_native { namespace vulkan { class Texture final : public TextureBase { public: // Used to create a regular texture from a descriptor. - static ResultOrError Create(Device* device, const TextureDescriptor* descriptor); + static ResultOrError> Create(Device* device, + const TextureDescriptor* descriptor); // Creates a texture and initializes it with a VkImage that references an external memory // object. Before the texture can be used, the VkDeviceMemory associated with the external diff --git a/src/tests/unittests/ResultTests.cpp b/src/tests/unittests/ResultTests.cpp index 8e2f05f211..7b21d2e398 100644 --- a/src/tests/unittests/ResultTests.cpp +++ b/src/tests/unittests/ResultTests.cpp @@ -296,6 +296,42 @@ TEST(ResultRefT, ReturningSuccess) { TestSuccess(&result, &success); } +class OtherClass { + public: + int a = 0; +}; +class Base : public RefCounted {}; +class Child : public OtherClass, public Base {}; + +// Test constructing a Result, E> +TEST(ResultRefT, ConversionFromChildConstructor) { + Child child; + Ref refChild(&child); + + Result, int> result(std::move(refChild)); + TestSuccess(&result, &child); +} + +// Test copy constructing Result, E> +TEST(ResultRefT, ConversionFromChildCopyConstructor) { + Child child; + Ref refChild(&child); + + Result, int> resultChild(std::move(refChild)); + Result, int> result(std::move(resultChild)); + TestSuccess(&result, &child); +} + +// Test assignment operator for Result, E> +TEST(ResultRefT, ConversionFromChildAssignmentOperator) { + Child child; + Ref refChild(&child); + + Result, int> resultChild(std::move(refChild)); + Result, int> result = std::move(resultChild); + TestSuccess(&result, &child); +} + // Result // Test constructing an error Result