GoaLitiuM/FlaxEngine
1
0
Fork 0
Code Issues Pull Requests Actions 7 Packages Projects Releases Wiki Activity

Fix various issues found with thread sanitizer on macOS

Browse Source
This commit is contained in:
Wojtek Figat
2024-04-13 13:20:17 +02:00
parent 8144db8e13
commit 9c2c02c1cf
10 changed files with 70 additions and 75 deletions
Download Patch File Download Diff File Expand all files Collapse all files

51
Source/Engine/Content/Asset.cpp
View File

@@ -184,9 +184,8 @@ void SoftAssetReferenceBase::OnUnloaded(Asset* asset)
Asset::Asset(const SpawnParams& params, const AssetInfo* info) Asset::Asset(const SpawnParams& params, const AssetInfo* info)
: ManagedScriptingObject(params) : ManagedScriptingObject(params)
, _refCount(0) , _refCount(0)
, _loadingTask(nullptr) , _loadState(0)
, _isLoaded(false) , _loadingTask(0)
, _loadFailed(false)
, _deleteFileOnUnload(false) , _deleteFileOnUnload(false)
, _isVirtual(false) , _isVirtual(false)
{ {
@@ -225,10 +224,10 @@ void Asset::OnDeleteObject()
// Unload asset data (in a safe way to protect asset data) // Unload asset data (in a safe way to protect asset data)
Locker.Lock(); Locker.Lock();
if (_isLoaded) if (IsLoaded())
{ {
unload(false); unload(false);
_isLoaded = false; Platform::AtomicStore(&_loadState, (int64)LoadState::Unloaded);
} }
Locker.Unlock(); Locker.Unlock();
@@ -319,11 +318,6 @@ void Asset::ChangeID(const Guid& newId)
Content::onAssetChangeId(this, oldId, newId); Content::onAssetChangeId(this, oldId, newId);
} }
bool Asset::LastLoadFailed() const
{
return _loadFailed != 0;
}
#if USE_EDITOR #if USE_EDITOR
bool Asset::ShouldDeleteFileOnUnload() const bool Asset::ShouldDeleteFileOnUnload() const
@@ -337,7 +331,7 @@ uint64 Asset::GetMemoryUsage() const
{ {
uint64 result = sizeof(Asset); uint64 result = sizeof(Asset);
Locker.Lock(); Locker.Lock();
if (_loadingTask) if (Platform::AtomicRead(&_loadingTask))
result += sizeof(ContentLoadTask); result += sizeof(ContentLoadTask);
result += (OnLoaded.Capacity() + OnReloading.Capacity() + OnUnloaded.Capacity()) * sizeof(EventType::FunctionType); result += (OnLoaded.Capacity() + OnReloading.Capacity() + OnUnloaded.Capacity()) * sizeof(EventType::FunctionType);
Locker.Unlock(); Locker.Unlock();
@@ -368,7 +362,7 @@ void Asset::Reload()
{ {
// Unload current data // Unload current data
unload(true); unload(true);
_isLoaded = false; Platform::AtomicStore(&_loadState, (int64)LoadState::Unloaded);
} }
// Start reloading process // Start reloading process
@@ -426,7 +420,7 @@ bool Asset::WaitForLoaded(double timeoutInMilliseconds) const
// Check if has missing loading task // Check if has missing loading task
Platform::MemoryBarrier(); Platform::MemoryBarrier();
const auto loadingTask = _loadingTask; const auto loadingTask = (ContentLoadTask*)Platform::AtomicRead(&_loadingTask);
if (loadingTask == nullptr) if (loadingTask == nullptr)
{ {
LOG(Warning, "WaitForLoaded asset \'{0}\' failed. No loading task attached and asset is not loaded.", ToString()); LOG(Warning, "WaitForLoaded asset \'{0}\' failed. No loading task attached and asset is not loaded.", ToString());
@@ -516,7 +510,7 @@ bool Asset::WaitForLoaded(double timeoutInMilliseconds) const
Content::tryCallOnLoaded((Asset*)this); Content::tryCallOnLoaded((Asset*)this);
} }
return _isLoaded == 0; return !IsLoaded();
} }
void Asset::InitAsVirtual() void Asset::InitAsVirtual()
@@ -525,14 +519,14 @@ void Asset::InitAsVirtual()
_isVirtual = true; _isVirtual = true;
// Be a loaded thing // Be a loaded thing
_isLoaded = true; Platform::AtomicStore(&_loadState, (int64)LoadState::Loaded);
} }
void Asset::CancelStreaming() void Asset::CancelStreaming()
{ {
// Cancel loading task but go over asset locker to prevent case if other load threads still loads asset while it's reimported on other thread // Cancel loading task but go over asset locker to prevent case if other load threads still loads asset while it's reimported on other thread
Locker.Lock(); Locker.Lock();
ContentLoadTask* loadTask = _loadingTask; auto loadTask = (ContentLoadTask*)Platform::AtomicRead(&_loadingTask);
Locker.Unlock(); Locker.Unlock();
if (loadTask) if (loadTask)
{ {
@@ -575,10 +569,11 @@ ContentLoadTask* Asset::createLoadingTask()
void Asset::startLoading() void Asset::startLoading()
{ {
ASSERT(!IsLoaded()); ASSERT(!IsLoaded());
ASSERT(_loadingTask == nullptr); ASSERT(Platform::AtomicRead(&_loadingTask) == 0);
_loadingTask = createLoadingTask(); auto loadingTask = createLoadingTask();
ASSERT(_loadingTask != nullptr); ASSERT(loadingTask != nullptr);
_loadingTask->Start(); Platform::AtomicStore(&_loadingTask, (intptr)loadingTask);
loadingTask->Start();
} }
void Asset::releaseStorage() void Asset::releaseStorage()
@@ -593,7 +588,7 @@ bool Asset::IsInternalType() const
bool Asset::onLoad(LoadAssetTask* task) bool Asset::onLoad(LoadAssetTask* task)
{ {
// It may fail when task is cancelled and new one is created later (don't crash but just end with an error) // It may fail when task is cancelled and new one is created later (don't crash but just end with an error)
if (task->Asset.Get() != this || _loadingTask == nullptr) if (task->Asset.Get() != this || Platform::AtomicRead(&_loadingTask) == 0)
return true; return true;
Locker.Lock(); Locker.Lock();
@@ -606,15 +601,15 @@ bool Asset::onLoad(LoadAssetTask* task)
} }
const bool isLoaded = result == LoadResult::Ok; const bool isLoaded = result == LoadResult::Ok;
const bool failed = !isLoaded; const bool failed = !isLoaded;
_loadFailed = failed; LoadState state = LoadState::Loaded;
_isLoaded = !failed; Platform::AtomicStore(&_loadState, (int64)(isLoaded ? LoadState::Loaded : LoadState::LoadFailed));
if (failed) if (failed)
{ {
LOG(Error, "Loading asset \'{0}\' result: {1}.", ToString(), ToString(result)); LOG(Error, "Loading asset \'{0}\' result: {1}.", ToString(), ToString(result));
} }
// Unlink task // Unlink task
_loadingTask = nullptr; Platform::AtomicStore(&_loadingTask, 0);
ASSERT(failed || IsLoaded() == isLoaded); ASSERT(failed || IsLoaded() == isLoaded);
Locker.Unlock(); Locker.Unlock();
@@ -663,12 +658,12 @@ void Asset::onUnload_MainThread()
OnUnloaded(this); OnUnloaded(this);
// Check if is during loading // Check if is during loading
if (_loadingTask != nullptr) auto loadingTask = (ContentLoadTask*)Platform::AtomicRead(&_loadingTask);
if (loadingTask != nullptr)
{ {
// Cancel loading // Cancel loading
auto task = _loadingTask; Platform::AtomicStore(&_loadingTask, 0);
_loadingTask = nullptr;
LOG(Warning, "Cancel loading task for \'{0}\'", ToString()); LOG(Warning, "Cancel loading task for \'{0}\'", ToString());
task->Cancel(); loadingTask->Cancel();
} }
} }

21
Source/Engine/Content/Asset.h
View File

@@ -34,11 +34,17 @@ public:
DECLARE_ENUM_7(LoadResult, Ok, Failed, MissingDataChunk, CannotLoadData, CannotLoadStorage, CannotLoadInitData, InvalidData); DECLARE_ENUM_7(LoadResult, Ok, Failed, MissingDataChunk, CannotLoadData, CannotLoadStorage, CannotLoadInitData, InvalidData);
protected: protected:
volatile int64 _refCount; enum class LoadState : int64
ContentLoadTask* _loadingTask; {
Unloaded,
Loaded,
LoadFailed,
};
volatile int64 _refCount;
volatile int64 _loadState;
volatile intptr _loadingTask;
int8 _isLoaded : 1; // Indicates that asset is loaded
int8 _loadFailed : 1; // Indicates that last asset loading has failed
int8 _deleteFileOnUnload : 1; // Indicates that asset source file should be removed on asset unload int8 _deleteFileOnUnload : 1; // Indicates that asset source file should be removed on asset unload
int8 _isVirtual : 1; // Indicates that asset is pure virtual (generated or temporary, has no storage so won't be saved) int8 _isVirtual : 1; // Indicates that asset is pure virtual (generated or temporary, has no storage so won't be saved)
@@ -111,13 +117,16 @@ public:
/// </summary> /// </summary>
API_PROPERTY() FORCE_INLINE bool IsLoaded() const API_PROPERTY() FORCE_INLINE bool IsLoaded() const
{ {
return _isLoaded != 0; return Platform::AtomicRead(&_loadState) == (int64)LoadState::Loaded;
} }
/// <summary> /// <summary>
/// Returns true if last asset loading failed, otherwise false. /// Returns true if last asset loading failed, otherwise false.
/// </summary> /// </summary>
API_PROPERTY() bool LastLoadFailed() const; API_PROPERTY() bool LastLoadFailed() const
{
return Platform::AtomicRead(&_loadState) == (int64)LoadState::LoadFailed;
}
/// <summary> /// <summary>
/// Determines whether this asset is virtual (generated or temporary, has no storage so it won't be saved). /// Determines whether this asset is virtual (generated or temporary, has no storage so it won't be saved).

3
Source/Engine/Content/Assets/VisualScript.cpp
View File

@@ -1434,8 +1434,7 @@ Asset::LoadResult VisualScript::load()
if (_instances.HasItems()) if (_instances.HasItems())
{ {
// Mark as already loaded so any WaitForLoaded checks during GetDefaultInstance bellow will handle this Visual Script as ready to use // Mark as already loaded so any WaitForLoaded checks during GetDefaultInstance bellow will handle this Visual Script as ready to use
_loadFailed = false; Platform::AtomicStore(&_loadState, (int64)LoadState::Loaded);
_isLoaded = true;
// Setup scripting type // Setup scripting type
CacheScriptingType(); CacheScriptingType();

15
Source/Engine/Content/Loading/Tasks/LoadAssetTask.h
View File

@@ -31,12 +31,11 @@ public:
if (Asset) if (Asset)
{ {
Asset->Locker.Lock(); Asset->Locker.Lock();
if (Asset->_loadingTask == this) if (Platform::AtomicRead(&Asset->_loadingTask) == (intptr)this)
{ {
Asset->_loadFailed = true; Platform::AtomicStore(&Asset->_loadState, (int64)Asset::LoadState::LoadFailed);
Asset->_isLoaded = false; Platform::AtomicStore(&Asset->_loadingTask, 0);
LOG(Error, "Loading asset \'{0}\' result: {1}.", ToString(), ToString(Result::TaskFailed)); LOG(Error, "Loading asset \'{0}\' result: {1}.", ToString(), ToString(Result::TaskFailed));
Asset->_loadingTask = nullptr;
} }
Asset->Locker.Unlock(); Asset->Locker.Unlock();
} }
@@ -77,8 +76,8 @@ protected:
if (Asset) if (Asset)
{ {
Asset->Locker.Lock(); Asset->Locker.Lock();
if (Asset->_loadingTask == this) if (Platform::AtomicRead(&Asset->_loadingTask) == (intptr)this)
Asset->_loadingTask = nullptr; Platform::AtomicStore(&Asset->_loadingTask, 0);
Asset->Locker.Unlock(); Asset->Locker.Unlock();
Asset = nullptr; Asset = nullptr;
} }
@@ -91,8 +90,8 @@ protected:
if (Asset) if (Asset)
{ {
Asset->Locker.Lock(); Asset->Locker.Lock();
if (Asset->_loadingTask == this) if (Platform::AtomicRead(&Asset->_loadingTask) == (intptr)this)
Asset->_loadingTask = nullptr; Platform::AtomicStore(&Asset->_loadingTask, 0);
Asset->Locker.Unlock(); Asset->Locker.Unlock();
Asset = nullptr; Asset = nullptr;
} }

3
Source/Engine/Core/Compiler.h
View File

@@ -15,6 +15,7 @@
#define FORCE_NOINLINE __attribute__((noinline)) #define FORCE_NOINLINE __attribute__((noinline))
#define NO_RETURN __attribute__((noreturn)) #define NO_RETURN __attribute__((noreturn))
#define NO_SANITIZE_ADDRESS __attribute__((no_sanitize_address)) #define NO_SANITIZE_ADDRESS __attribute__((no_sanitize_address))
#define NO_SANITIZE_THREAD __attribute__((no_sanitize_thread))
#define PACK_BEGIN() #define PACK_BEGIN()
#define PACK_END() __attribute__((__packed__)) #define PACK_END() __attribute__((__packed__))
#define ALIGN_BEGIN(_align) #define ALIGN_BEGIN(_align)
@@ -46,6 +47,7 @@
#define FORCE_NOINLINE __attribute__((noinline)) #define FORCE_NOINLINE __attribute__((noinline))
#define NO_RETURN __attribute__((noreturn)) #define NO_RETURN __attribute__((noreturn))
#define NO_SANITIZE_ADDRESS __attribute__((no_sanitize_address)) #define NO_SANITIZE_ADDRESS __attribute__((no_sanitize_address))
#define NO_SANITIZE_THREAD __attribute__((no_sanitize_thread))
#define PACK_BEGIN() #define PACK_BEGIN()
#define PACK_END() __attribute__((__packed__)) #define PACK_END() __attribute__((__packed__))
#define ALIGN_BEGIN(_align) #define ALIGN_BEGIN(_align)
@@ -72,6 +74,7 @@
#define FORCE_NOINLINE __declspec(noinline) #define FORCE_NOINLINE __declspec(noinline)
#define NO_RETURN __declspec(noreturn) #define NO_RETURN __declspec(noreturn)
#define NO_SANITIZE_ADDRESS #define NO_SANITIZE_ADDRESS
#define NO_SANITIZE_THREAD
#define PACK_BEGIN() __pragma(pack(push, 1)) #define PACK_BEGIN() __pragma(pack(push, 1))
#define PACK_END() ; __pragma(pack(pop)) #define PACK_END() ; __pragma(pack(pop))
#define ALIGN_BEGIN(_align) __declspec(align(_align)) #define ALIGN_BEGIN(_align) __declspec(align(_align))

5
Source/Engine/Graphics/Async/GPUTask.h
View File

@@ -115,7 +115,7 @@ public:
// Rollback state and cancel // Rollback state and cancel
_context = nullptr; _context = nullptr;
_state = TaskState::Queued; SetState(TaskState::Queued);
Cancel(); Cancel();
} }
@@ -148,8 +148,7 @@ protected:
ASSERT(_context != nullptr); ASSERT(_context != nullptr);
_context->OnCancelSync(this); _context->OnCancelSync(this);
_context = nullptr; _context = nullptr;
SetState(TaskState::Canceled);
_state = TaskState::Canceled;
} }
else else
{ {

5
Source/Engine/Graphics/Async/GPUTasksManager.cpp
View File

@@ -9,9 +9,8 @@
void GPUTask::Execute(GPUTasksContext* context) void GPUTask::Execute(GPUTasksContext* context)
{ {
// Begin
ASSERT(IsQueued() && _context == nullptr); ASSERT(IsQueued() && _context == nullptr);
_state = TaskState::Running; SetState(TaskState::Running);
// Perform an operation // Perform an operation
const auto result = run(context); const auto result = run(context);
@@ -19,7 +18,7 @@ void GPUTask::Execute(GPUTasksContext* context)
// Process result // Process result
if (IsCancelRequested()) if (IsCancelRequested())
{ {
_state = TaskState::Canceled; SetState(TaskState::Canceled);
} }
else if (result != Result::Ok) else if (result != Result::Ok)
{ {

6
Source/Engine/Platform/Unix/UnixCriticalSection.h
View File

@@ -57,7 +57,7 @@ public:
/// <summary> /// <summary>
/// Locks the critical section. /// Locks the critical section.
/// </summary> /// </summary>
void Lock() const NO_SANITIZE_THREAD void Lock() const
{ {
pthread_mutex_lock(_mutexPtr); pthread_mutex_lock(_mutexPtr);
#if BUILD_DEBUG #if BUILD_DEBUG
@@ -69,7 +69,7 @@ public:
/// Attempts to enter a critical section without blocking. If the call is successful, the calling thread takes ownership of the critical section. /// Attempts to enter a critical section without blocking. If the call is successful, the calling thread takes ownership of the critical section.
/// </summary> /// </summary>
/// <returns>True if calling thread took ownership of the critical section.</returns> /// <returns>True if calling thread took ownership of the critical section.</returns>
bool TryLock() const NO_SANITIZE_THREAD bool TryLock() const
{ {
return pthread_mutex_trylock(_mutexPtr) == 0; return pthread_mutex_trylock(_mutexPtr) == 0;
} }
@@ -77,7 +77,7 @@ public:
/// <summary> /// <summary>
/// Releases the lock on the critical section. /// Releases the lock on the critical section.
/// </summary> /// </summary>
void Unlock() const NO_SANITIZE_THREAD void Unlock() const
{ {
#if BUILD_DEBUG #if BUILD_DEBUG
((UnixCriticalSection*)this)->_owningThreadId = 0; ((UnixCriticalSection*)this)->_owningThreadId = 0;

21
Source/Engine/Threading/Task.cpp
View File

@@ -11,13 +11,13 @@
void Task::Start() void Task::Start()
{ {
if (_state != TaskState::Created) if (GetState() != TaskState::Created)
return; return;
OnStart(); OnStart();
// Change state // Change state
_state = TaskState::Queued; SetState(TaskState::Queued);
// Add task to the execution queue // Add task to the execution queue
Enqueue(); Enqueue();
@@ -110,7 +110,6 @@ Task* Task::ContinueWith(const Function<bool()>& action, Object* target)
Task* Task::StartNew(Task* task) Task* Task::StartNew(Task* task)
{ {
ASSERT(task); ASSERT(task);
task->Start(); task->Start();
return task; return task;
} }
@@ -137,11 +136,10 @@ Task* Task::StartNew(Function<bool()>::Signature& action, Object* target)
void Task::Execute() void Task::Execute()
{ {
// Begin
if (IsCanceled()) if (IsCanceled())
return; return;
ASSERT(IsQueued()); ASSERT(IsQueued());
_state = TaskState::Running; SetState(TaskState::Running);
// Perform an operation // Perform an operation
bool failed = Run(); bool failed = Run();
@@ -149,7 +147,7 @@ void Task::Execute()
// Process result // Process result
if (IsCancelRequested()) if (IsCancelRequested())
{ {
_state = TaskState::Canceled; SetState(TaskState::Canceled);
} }
else if (failed) else if (failed)
{ {
@@ -167,10 +165,8 @@ void Task::OnStart()
void Task::OnFinish() void Task::OnFinish()
{ {
ASSERT(IsRunning()); ASSERT(IsRunning() && !IsCancelRequested());
ASSERT(!IsCancelRequested()); SetState(TaskState::Finished);
_state = TaskState::Finished;
// Send event further // Send event further
if (_continueWith) if (_continueWith)
@@ -181,7 +177,7 @@ void Task::OnFinish()
void Task::OnFail() void Task::OnFail()
{ {
_state = TaskState::Failed; SetState(TaskState::Failed);
// Send event further // Send event further
if (_continueWith) if (_continueWith)
@@ -209,8 +205,7 @@ void Task::OnCancel()
const auto state = GetState(); const auto state = GetState();
if (state != TaskState::Finished && state != TaskState::Failed) if (state != TaskState::Finished && state != TaskState::Failed)
{ {
_state = TaskState::Canceled; SetState(TaskState::Canceled);
OnEnd(); OnEnd();
} }
} }

15
Source/Engine/Threading/Task.h
View File

@@ -49,7 +49,6 @@ class FLAXENGINE_API Task : public Object, public NonCopyable
// //
protected: protected:
/// <summary> /// <summary>
/// The cancel flag used to indicate that there is request to cancel task operation. /// The cancel flag used to indicate that there is request to cancel task operation.
/// </summary> /// </summary>
@@ -65,14 +64,18 @@ protected:
/// </summary> /// </summary>
Task* _continueWith = nullptr; Task* _continueWith = nullptr;
public: void SetState(TaskState state)
{
Platform::AtomicStore((int64 volatile*)&_state, (uint64)state);
}
public:
/// <summary> /// <summary>
/// Gets the task state. /// Gets the task state.
/// </summary> /// </summary>
FORCE_INLINE TaskState GetState() const FORCE_INLINE TaskState GetState() const
{ {
return static_cast<TaskState>(Platform::AtomicRead((int64 volatile*)&_state)); return (TaskState)Platform::AtomicRead((int64 const volatile*)&_state);
} }
/// <summary> /// <summary>
@@ -94,7 +97,6 @@ public:
} }
public: public:
/// <summary> /// <summary>
/// Checks if operation failed. /// Checks if operation failed.
/// </summary> /// </summary>
@@ -153,7 +155,6 @@ public:
} }
public: public:
/// <summary> /// <summary>
/// Starts this task execution (and will continue with all children). /// Starts this task execution (and will continue with all children).
/// </summary> /// </summary>
@@ -199,7 +200,6 @@ public:
} }
public: public:
/// <summary> /// <summary>
/// Continues that task execution with a given task (will call Start on given task after finishing that one). /// Continues that task execution with a given task (will call Start on given task after finishing that one).
/// </summary> /// </summary>
@@ -232,7 +232,6 @@ public:
Task* ContinueWith(const Function<bool()>& action, Object* target = nullptr); Task* ContinueWith(const Function<bool()>& action, Object* target = nullptr);
public: public:
/// <summary> /// <summary>
/// Starts the new task. /// Starts the new task.
/// </summary> /// </summary>
@@ -312,7 +311,6 @@ public:
} }
protected: protected:
/// <summary> /// <summary>
/// Executes this task. /// Executes this task.
/// It should be called by the task consumer (thread pool or other executor of this task type). /// It should be called by the task consumer (thread pool or other executor of this task type).
@@ -328,7 +326,6 @@ protected:
virtual bool Run() = 0; virtual bool Run() = 0;
protected: protected:
virtual void Enqueue() = 0; virtual void Enqueue() = 0;
virtual void OnStart(); virtual void OnStart();
virtual void OnFinish(); virtual void OnFinish();