From 8144db8e13320294659cba060a148f2c4e80bfe5 Mon Sep 17 00:00:00 2001 From: Wojtek Figat Date: Sat, 13 Apr 2024 13:09:11 +0200 Subject: [PATCH] Fix various issues found with adrress sanitizer on macOS --- Source/Engine/Core/Utilities.h | 11 +++++++++++ Source/Engine/Debug/DebugDraw.cpp | 2 +- Source/Engine/Engine/CommandLine.cpp | 7 ++++--- .../Vulkan/DescriptorSetVulkan.cpp | 17 +++++------------ .../GraphicsDevice/Vulkan/DescriptorSetVulkan.h | 14 ++------------ .../Vulkan/GPUPipelineStateVulkan.cpp | 12 ++++++++++-- .../Vulkan/GPUPipelineStateVulkan.h | 9 ++++++--- Source/Engine/Scripting/BinaryModule.cpp | 6 ++++-- 8 files changed, 43 insertions(+), 35 deletions(-) diff --git a/Source/Engine/Core/Utilities.h b/Source/Engine/Core/Utilities.h index afed898bf..1a637cfe7 100644 --- a/Source/Engine/Core/Utilities.h +++ b/Source/Engine/Core/Utilities.h @@ -94,4 +94,15 @@ namespace Utilities return (x * 0x01010101) >> 24; #endif } + + // Copy memory region but ignoring address sanatizer checks for memory regions. + NO_SANITIZE_ADDRESS static void UnsafeMemoryCopy(void* dst, const void* src, uint64 size) + { +#if BUILD_RELEASE + memcpy(dst, src, static_cast(size)); +#else + for (uint64 i = 0; i < size; i++) + ((byte*)dst)[i] = ((byte*)src)[i]; +#endif + } } diff --git a/Source/Engine/Debug/DebugDraw.cpp b/Source/Engine/Debug/DebugDraw.cpp index ea8cdb196..942069130 100644 --- a/Source/Engine/Debug/DebugDraw.cpp +++ b/Source/Engine/Debug/DebugDraw.cpp @@ -129,7 +129,7 @@ PACK_STRUCT(struct Data { Matrix ViewProjection; Float2 Padding; float ClipPosZBias; - bool EnableDepthTest; + uint32 EnableDepthTest; }); struct PsData diff --git a/Source/Engine/Engine/CommandLine.cpp b/Source/Engine/Engine/CommandLine.cpp index fd7547f48..c1d1a8540 100644 --- a/Source/Engine/Engine/CommandLine.cpp +++ b/Source/Engine/Engine/CommandLine.cpp @@ -2,6 +2,7 @@ #include "CommandLine.h" #include "Engine/Core/Collections/Array.h" +#include "Engine/Core/Utilities.h" #include CommandLine::OptionsData CommandLine::Options; @@ -81,7 +82,7 @@ bool CommandLine::Parse(const Char* cmdLine) if (pos) \ { \ len = ARRAY_COUNT(text) - 1; \ - Platform::MemoryCopy(pos, pos + len, (end - pos - len) * 2); \ + Utilities::UnsafeMemoryCopy(pos, pos + len, (end - pos - len) * 2); \ *(end - len) = 0; \ end -= len; \ Options.field = true; \ @@ -98,7 +99,7 @@ bool CommandLine::Parse(const Char* cmdLine) } \ Options.field = String(argStart, static_cast(argEnd - argStart)); \ len = static_cast((argEnd - pos) + 1); \ - Platform::MemoryCopy(pos, pos + len, (end - pos - len) * 2); \ + Utilities::UnsafeMemoryCopy(pos, pos + len, (end - pos - len) * 2); \ *(end - len) = 0; \ end -= len; \ } @@ -114,7 +115,7 @@ bool CommandLine::Parse(const Char* cmdLine) { \ Options.field = String(argStart, static_cast(argEnd - argStart)); \ len = static_cast((argEnd - pos) + 1); \ - Platform::MemoryCopy(pos, pos + len, (end - pos - len) * 2); \ + Utilities::UnsafeMemoryCopy(pos, pos + len, (end - pos - len) * 2); \ *(end - len) = 0; \ end -= len; \ } \ diff --git a/Source/Engine/GraphicsDevice/Vulkan/DescriptorSetVulkan.cpp b/Source/Engine/GraphicsDevice/Vulkan/DescriptorSetVulkan.cpp index 0648d917b..eb17b5de0 100644 --- a/Source/Engine/GraphicsDevice/Vulkan/DescriptorSetVulkan.cpp +++ b/Source/Engine/GraphicsDevice/Vulkan/DescriptorSetVulkan.cpp @@ -247,8 +247,7 @@ void TypedDescriptorPoolSetVulkan::Reset() DescriptorPoolSetContainerVulkan::DescriptorPoolSetContainerVulkan(GPUDeviceVulkan* device) : _device(device) - , _lastFrameUsed(Engine::FrameCount) - , _used(true) + , LastFrameUsed(Engine::FrameCount) { } @@ -278,12 +277,6 @@ void DescriptorPoolSetContainerVulkan::Reset() } } -void DescriptorPoolSetContainerVulkan::SetUsed(bool used) -{ - _used = used; - _lastFrameUsed = used ? Engine::FrameCount : _lastFrameUsed; -} - DescriptorPoolsManagerVulkan::DescriptorPoolsManagerVulkan(GPUDeviceVulkan* device) : _device(device) { @@ -299,9 +292,9 @@ DescriptorPoolSetContainerVulkan& DescriptorPoolsManagerVulkan::AcquirePoolSetCo ScopeLock lock(_locker); for (auto* poolSet : _poolSets) { - if (poolSet->IsUnused()) + if (poolSet->Refs == 0) { - poolSet->SetUsed(true); + poolSet->LastFrameUsed = Engine::FrameCount; poolSet->Reset(); return *poolSet; } @@ -313,7 +306,7 @@ DescriptorPoolSetContainerVulkan& DescriptorPoolsManagerVulkan::AcquirePoolSetCo void DescriptorPoolsManagerVulkan::ReleasePoolSet(DescriptorPoolSetContainerVulkan& poolSet) { - poolSet.SetUsed(false); + poolSet.LastFrameUsed = Engine::FrameCount; } void DescriptorPoolsManagerVulkan::GC() @@ -322,7 +315,7 @@ void DescriptorPoolsManagerVulkan::GC() for (int32 i = _poolSets.Count() - 1; i >= 0; i--) { const auto poolSet = _poolSets[i]; - if (poolSet->IsUnused() && Engine::FrameCount - poolSet->GetLastFrameUsed() > VULKAN_RESOURCE_DELETE_SAFE_FRAMES_COUNT) + if (poolSet->Refs == 0 && Engine::FrameCount - poolSet->LastFrameUsed > VULKAN_RESOURCE_DELETE_SAFE_FRAMES_COUNT) { _poolSets.RemoveAt(i); Delete(poolSet); diff --git a/Source/Engine/GraphicsDevice/Vulkan/DescriptorSetVulkan.h b/Source/Engine/GraphicsDevice/Vulkan/DescriptorSetVulkan.h index 1b3de5eaa..ede55fd56 100644 --- a/Source/Engine/GraphicsDevice/Vulkan/DescriptorSetVulkan.h +++ b/Source/Engine/GraphicsDevice/Vulkan/DescriptorSetVulkan.h @@ -212,8 +212,6 @@ class DescriptorPoolSetContainerVulkan private: GPUDeviceVulkan* _device; Dictionary _typedDescriptorPools; - uint64 _lastFrameUsed; - bool _used; public: DescriptorPoolSetContainerVulkan(GPUDeviceVulkan* device); @@ -222,17 +220,9 @@ public: public: TypedDescriptorPoolSetVulkan* AcquireTypedPoolSet(const DescriptorSetLayoutVulkan& layout); void Reset(); - void SetUsed(bool used); - bool IsUnused() const - { - return !_used; - } - - uint64 GetLastFrameUsed() const - { - return _lastFrameUsed; - } + mutable uint64 Refs = 0; + mutable uint32 LastFrameUsed; }; class DescriptorPoolsManagerVulkan diff --git a/Source/Engine/GraphicsDevice/Vulkan/GPUPipelineStateVulkan.cpp b/Source/Engine/GraphicsDevice/Vulkan/GPUPipelineStateVulkan.cpp index 7686d9c5b..3f3b3074c 100644 --- a/Source/Engine/GraphicsDevice/Vulkan/GPUPipelineStateVulkan.cpp +++ b/Source/Engine/GraphicsDevice/Vulkan/GPUPipelineStateVulkan.cpp @@ -112,7 +112,11 @@ ComputePipelineStateVulkan::ComputePipelineStateVulkan(GPUDeviceVulkan* device, ComputePipelineStateVulkan::~ComputePipelineStateVulkan() { DSWriteContainer.Release(); - CurrentTypedDescriptorPoolSet = nullptr; + if (CurrentTypedDescriptorPoolSet) + { + CurrentTypedDescriptorPoolSet->GetOwner()->Refs--; + CurrentTypedDescriptorPoolSet = nullptr; + } DescriptorSetsLayout = nullptr; DescriptorSetHandles.Resize(0); DynamicOffsets.Resize(0); @@ -206,7 +210,11 @@ VkPipeline GPUPipelineStateVulkan::GetState(RenderPassVulkan* renderPass) void GPUPipelineStateVulkan::OnReleaseGPU() { DSWriteContainer.Release(); - CurrentTypedDescriptorPoolSet = nullptr; + if (CurrentTypedDescriptorPoolSet) + { + CurrentTypedDescriptorPoolSet->GetOwner()->Refs--; + CurrentTypedDescriptorPoolSet = nullptr; + } DescriptorSetsLayout = nullptr; DescriptorSetHandles.Resize(0); DynamicOffsets.Resize(0); diff --git a/Source/Engine/GraphicsDevice/Vulkan/GPUPipelineStateVulkan.h b/Source/Engine/GraphicsDevice/Vulkan/GPUPipelineStateVulkan.h index b8b326e83..73e68a897 100644 --- a/Source/Engine/GraphicsDevice/Vulkan/GPUPipelineStateVulkan.h +++ b/Source/Engine/GraphicsDevice/Vulkan/GPUPipelineStateVulkan.h @@ -41,17 +41,17 @@ public: DescriptorPoolSetContainerVulkan* cmdBufferPoolSet = cmdBuffer->GetDescriptorPoolSet(); if (CurrentTypedDescriptorPoolSet == nullptr || CurrentTypedDescriptorPoolSet->GetOwner() != cmdBufferPoolSet) { - ASSERT(cmdBufferPoolSet); + if (CurrentTypedDescriptorPoolSet) + CurrentTypedDescriptorPoolSet->GetOwner()->Refs--; CurrentTypedDescriptorPoolSet = cmdBufferPoolSet->AcquireTypedPoolSet(*DescriptorSetsLayout); + CurrentTypedDescriptorPoolSet->GetOwner()->Refs++; return true; } - return false; } inline bool AllocateDescriptorSets() { - ASSERT(CurrentTypedDescriptorPoolSet); return CurrentTypedDescriptorPoolSet->AllocateDescriptorSets(*DescriptorSetsLayout, DescriptorSetHandles.Get()); } @@ -165,7 +165,10 @@ public: DescriptorPoolSetContainerVulkan* cmdBufferPoolSet = cmdBuffer->GetDescriptorPoolSet(); if (CurrentTypedDescriptorPoolSet == nullptr || CurrentTypedDescriptorPoolSet->GetOwner() != cmdBufferPoolSet) { + if (CurrentTypedDescriptorPoolSet) + CurrentTypedDescriptorPoolSet->GetOwner()->Refs--; CurrentTypedDescriptorPoolSet = cmdBufferPoolSet->AcquireTypedPoolSet(*DescriptorSetsLayout); + CurrentTypedDescriptorPoolSet->GetOwner()->Refs++; return true; } return false; diff --git a/Source/Engine/Scripting/BinaryModule.cpp b/Source/Engine/Scripting/BinaryModule.cpp index bd3df861e..ac0d6da2d 100644 --- a/Source/Engine/Scripting/BinaryModule.cpp +++ b/Source/Engine/Scripting/BinaryModule.cpp @@ -3,6 +3,7 @@ #include "BinaryModule.h" #include "ScriptingObject.h" #include "Engine/Core/Log.h" +#include "Engine/Core/Utilities.h" #include "Engine/Threading/Threading.h" #include "Engine/Profiler/ProfilerCPU.h" #include "ManagedCLR/MAssembly.h" @@ -436,6 +437,7 @@ void ScriptingType::SetupScriptVTable(ScriptingTypeHandle baseTypeHandle) } } +NO_SANITIZE_ADDRESS void ScriptingType::SetupScriptObjectVTable(void* object, ScriptingTypeHandle baseTypeHandle, int32 wrapperIndex) { // Analyze vtable size @@ -475,7 +477,7 @@ void ScriptingType::SetupScriptObjectVTable(void* object, ScriptingTypeHandle ba // Duplicate vtable Script.VTable = (void**)((byte*)Platform::Allocate(totalSize, 16) + prefixSize); - Platform::MemoryCopy((byte*)Script.VTable - prefixSize, (byte*)vtable - prefixSize, prefixSize + size); + Utilities::UnsafeMemoryCopy((byte*)Script.VTable - prefixSize, (byte*)vtable - prefixSize, prefixSize + size); // Override vtable entries if (interfacesCount) @@ -508,7 +510,7 @@ void ScriptingType::SetupScriptObjectVTable(void* object, ScriptingTypeHandle ba const int32 interfaceSize = interfaceCount * sizeof(void*); // Duplicate interface vtable - Platform::MemoryCopy((byte*)Script.VTable + interfaceOffset, (byte*)vtableInterface - prefixSize, prefixSize + interfaceSize); + Utilities::UnsafeMemoryCopy((byte*)Script.VTable + interfaceOffset, (byte*)vtableInterface - prefixSize, prefixSize + interfaceSize); // Override interface vtable entries const auto scriptOffset = interfaces->ScriptVTableOffset;