From 10470046b7ce346f0e941e3a46f0b9ab99688bec Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Robert=20M=C3=BCller?= Date: Fri, 10 Nov 2023 21:49:42 +0100 Subject: [PATCH] Use `CLock` instead of `std::mutex`, add thread-safety annotations Use `CLock` and `CLockScope` instead of `std::mutex` and add clang thread-safety analysis annotations everywhere except for usages in engine graphics and video, as those usages also involve `std::condition_variable`. Fix lock not being unlocked on all code paths in `CFutureLogger::Log`, which is caught by the static analysis. --- src/base/log.cpp | 29 +++++++---------- src/base/logger.h | 9 +++--- src/engine/client/sound.cpp | 25 +++++++-------- src/engine/client/sound.h | 43 +++++++++++++------------- src/engine/server/server_logger.h | 4 +-- src/engine/shared/assertion_logger.cpp | 18 +++++------ src/game/client/components/console.cpp | 23 ++++++-------- src/game/client/components/console.h | 17 +++++----- 8 files changed, 81 insertions(+), 87 deletions(-) diff --git a/src/base/log.cpp b/src/base/log.cpp index 6788969c5..8f294a3d7 100644 --- a/src/base/log.cpp +++ b/src/base/log.cpp @@ -317,7 +317,7 @@ class CWindowsConsoleLogger : public ILogger HANDLE m_pConsole; int m_BackgroundColor; int m_ForegroundColor; - std::mutex m_OutputLock; + CLock m_OutputLock; bool m_Finished = false; public: @@ -336,7 +336,7 @@ public: m_ForegroundColor = FOREGROUND_BLUE | FOREGROUND_GREEN | FOREGROUND_RED | FOREGROUND_INTENSITY; } } - void Log(const CLogMessage *pMessage) override + void Log(const CLogMessage *pMessage) override REQUIRES(!m_OutputLock) { if(m_Filter.Filters(pMessage)) { @@ -356,44 +356,41 @@ public: else Color |= FOREGROUND_BLUE | FOREGROUND_GREEN | FOREGROUND_RED | FOREGROUND_INTENSITY; - m_OutputLock.lock(); + const CLockScope LockScope(m_OutputLock); if(!m_Finished) { SetConsoleTextAttribute(m_pConsole, Color); WriteConsoleW(m_pConsole, WideMessage.c_str(), WideMessage.length(), NULL, NULL); } - m_OutputLock.unlock(); } - void GlobalFinish() override + void GlobalFinish() override REQUIRES(!m_OutputLock) { // Restore original color - m_OutputLock.lock(); + const CLockScope LockScope(m_OutputLock); SetConsoleTextAttribute(m_pConsole, m_BackgroundColor | m_ForegroundColor); m_Finished = true; - m_OutputLock.unlock(); } }; class CWindowsFileLogger : public ILogger { HANDLE m_pFile; - std::mutex m_OutputLock; + CLock m_OutputLock; public: CWindowsFileLogger(HANDLE pFile) : m_pFile(pFile) { } - void Log(const CLogMessage *pMessage) override + void Log(const CLogMessage *pMessage) override REQUIRES(!m_OutputLock) { if(m_Filter.Filters(pMessage)) { return; } - m_OutputLock.lock(); + const CLockScope LockScope(m_OutputLock); DWORD Written; // we don't care about the value, but Windows 7 crashes if we pass NULL WriteFile(m_pFile, pMessage->m_aLine, pMessage->m_LineLength, &Written, NULL); WriteFile(m_pFile, "\r\n", 2, &Written, NULL); - m_OutputLock.unlock(); } }; #endif @@ -447,9 +444,9 @@ std::unique_ptr log_logger_windows_debugger() void CFutureLogger::Set(std::shared_ptr pLogger) { - std::shared_ptr null; - m_PendingLock.lock(); - if(!std::atomic_compare_exchange_strong_explicit(&m_pLogger, &null, pLogger, std::memory_order_acq_rel, std::memory_order_acq_rel)) + const CLockScope LockScope(m_PendingLock); + std::shared_ptr pNullLogger; + if(!std::atomic_compare_exchange_strong_explicit(&m_pLogger, &pNullLogger, pLogger, std::memory_order_acq_rel, std::memory_order_acq_rel)) { dbg_assert(false, "future logger has already been set and can only be set once"); } @@ -461,7 +458,6 @@ void CFutureLogger::Set(std::shared_ptr pLogger) } m_vPending.clear(); m_vPending.shrink_to_fit(); - m_PendingLock.unlock(); } void CFutureLogger::Log(const CLogMessage *pMessage) @@ -472,7 +468,7 @@ void CFutureLogger::Log(const CLogMessage *pMessage) pLogger->Log(pMessage); return; } - m_PendingLock.lock(); + const CLockScope LockScope(m_PendingLock); pLogger = std::atomic_load_explicit(&m_pLogger, std::memory_order_relaxed); if(pLogger) { @@ -480,7 +476,6 @@ void CFutureLogger::Log(const CLogMessage *pMessage) return; } m_vPending.push_back(*pMessage); - m_PendingLock.unlock(); } void CFutureLogger::GlobalFinish() diff --git a/src/base/logger.h b/src/base/logger.h index 1324d84d1..cc6575b49 100644 --- a/src/base/logger.h +++ b/src/base/logger.h @@ -1,10 +1,11 @@ #ifndef BASE_LOGGER_H #define BASE_LOGGER_H +#include "lock.h" #include "log.h" + #include #include -#include #include typedef void *IOHANDLE; @@ -226,15 +227,15 @@ class CFutureLogger : public ILogger private: std::shared_ptr m_pLogger; std::vector m_vPending; - std::mutex m_PendingLock; + CLock m_PendingLock; public: /** * Replace the `CFutureLogger` instance with the given logger. It'll * receive all log messages sent to the `CFutureLogger` so far. */ - void Set(std::shared_ptr pLogger); - void Log(const CLogMessage *pMessage) override; + void Set(std::shared_ptr pLogger) REQUIRES(!m_PendingLock); + void Log(const CLogMessage *pMessage) override REQUIRES(!m_PendingLock); void GlobalFinish() override; void OnFilterChange() override; }; diff --git a/src/engine/client/sound.cpp b/src/engine/client/sound.cpp index 0680d4667..7784e162b 100644 --- a/src/engine/client/sound.cpp +++ b/src/engine/client/sound.cpp @@ -687,7 +687,7 @@ void CSound::SetVoiceVolume(CVoiceHandle Voice, float Volume) int VoiceID = Voice.Id(); - std::unique_lock Lock(m_SoundLock); + const CLockScope LockScope(m_SoundLock); if(m_aVoices[VoiceID].m_Age != Voice.Age()) return; @@ -702,7 +702,7 @@ void CSound::SetVoiceFalloff(CVoiceHandle Voice, float Falloff) int VoiceID = Voice.Id(); - std::unique_lock Lock(m_SoundLock); + const CLockScope LockScope(m_SoundLock); if(m_aVoices[VoiceID].m_Age != Voice.Age()) return; @@ -717,7 +717,7 @@ void CSound::SetVoiceLocation(CVoiceHandle Voice, float x, float y) int VoiceID = Voice.Id(); - std::unique_lock Lock(m_SoundLock); + const CLockScope LockScope(m_SoundLock); if(m_aVoices[VoiceID].m_Age != Voice.Age()) return; @@ -732,7 +732,7 @@ void CSound::SetVoiceTimeOffset(CVoiceHandle Voice, float TimeOffset) int VoiceID = Voice.Id(); - std::unique_lock Lock(m_SoundLock); + const CLockScope LockScope(m_SoundLock); if(m_aVoices[VoiceID].m_Age != Voice.Age()) return; @@ -766,7 +766,7 @@ void CSound::SetVoiceCircle(CVoiceHandle Voice, float Radius) int VoiceID = Voice.Id(); - std::unique_lock Lock(m_SoundLock); + const CLockScope LockScope(m_SoundLock); if(m_aVoices[VoiceID].m_Age != Voice.Age()) return; @@ -781,7 +781,7 @@ void CSound::SetVoiceRectangle(CVoiceHandle Voice, float Width, float Height) int VoiceID = Voice.Id(); - std::unique_lock Lock(m_SoundLock); + const CLockScope LockScope(m_SoundLock); if(m_aVoices[VoiceID].m_Age != Voice.Age()) return; @@ -792,7 +792,7 @@ void CSound::SetVoiceRectangle(CVoiceHandle Voice, float Width, float Height) ISound::CVoiceHandle CSound::Play(int ChannelID, int SampleID, int Flags, float x, float y) { - m_SoundLock.lock(); + const CLockScope LockScope(m_SoundLock); // search for voice int VoiceID = -1; @@ -836,7 +836,6 @@ ISound::CVoiceHandle CSound::Play(int ChannelID, int SampleID, int Flags, float Age = m_aVoices[VoiceID].m_Age; } - m_SoundLock.unlock(); return CreateVoiceHandle(VoiceID, Age); } @@ -853,7 +852,7 @@ ISound::CVoiceHandle CSound::Play(int ChannelID, int SampleID, int Flags) void CSound::Pause(int SampleID) { // TODO: a nice fade out - std::unique_lock Lock(m_SoundLock); + const CLockScope LockScope(m_SoundLock); CSample *pSample = &m_aSamples[SampleID]; for(auto &Voice : m_aVoices) { @@ -868,7 +867,7 @@ void CSound::Pause(int SampleID) void CSound::Stop(int SampleID) { // TODO: a nice fade out - std::unique_lock Lock(m_SoundLock); + const CLockScope LockScope(m_SoundLock); CSample *pSample = &m_aSamples[SampleID]; for(auto &Voice : m_aVoices) { @@ -886,7 +885,7 @@ void CSound::Stop(int SampleID) void CSound::StopAll() { // TODO: a nice fade out - std::unique_lock Lock(m_SoundLock); + const CLockScope LockScope(m_SoundLock); for(auto &Voice : m_aVoices) { if(Voice.m_pSample) @@ -907,7 +906,7 @@ void CSound::StopVoice(CVoiceHandle Voice) int VoiceID = Voice.Id(); - std::unique_lock Lock(m_SoundLock); + const CLockScope LockScope(m_SoundLock); if(m_aVoices[VoiceID].m_Age != Voice.Age()) return; @@ -917,7 +916,7 @@ void CSound::StopVoice(CVoiceHandle Voice) bool CSound::IsPlaying(int SampleID) { - std::unique_lock Lock(m_SoundLock); + const CLockScope LockScope(m_SoundLock); const CSample *pSample = &m_aSamples[SampleID]; return std::any_of(std::begin(m_aVoices), std::end(m_aVoices), [pSample](const auto &Voice) { return Voice.m_pSample == pSample; }); } diff --git a/src/engine/client/sound.h b/src/engine/client/sound.h index 7ea40cca3..3780e1922 100644 --- a/src/engine/client/sound.h +++ b/src/engine/client/sound.h @@ -3,12 +3,13 @@ #ifndef ENGINE_CLIENT_SOUND_H #define ENGINE_CLIENT_SOUND_H +#include + #include #include #include -#include struct CSample { @@ -57,7 +58,7 @@ class CSound : public IEngineSound bool m_SoundEnabled = false; SDL_AudioDeviceID m_Device = 0; - std::mutex m_SoundLock; + CLock m_SoundLock; CSample m_aSamples[NUM_SAMPLES] = {{0}}; CVoice m_aVoices[NUM_VOICES] = {{0}}; @@ -86,7 +87,7 @@ class CSound : public IEngineSound public: int Init() override; int Update() override; - void Shutdown() override; + void Shutdown() override REQUIRES(!m_SoundLock); bool IsSoundEnabled() override { return m_SoundEnabled; } @@ -94,33 +95,33 @@ public: int LoadWV(const char *pFilename, int StorageType = IStorage::TYPE_ALL) override; int LoadOpusFromMem(const void *pData, unsigned DataSize, bool FromEditor) override; int LoadWVFromMem(const void *pData, unsigned DataSize, bool FromEditor) override; - void UnloadSample(int SampleID) override; + void UnloadSample(int SampleID) override REQUIRES(!m_SoundLock); float GetSampleTotalTime(int SampleID) override; // in s - float GetSampleCurrentTime(int SampleID) override; // in s - void SetSampleCurrentTime(int SampleID, float Time) override; + float GetSampleCurrentTime(int SampleID) override REQUIRES(!m_SoundLock); // in s + void SetSampleCurrentTime(int SampleID, float Time) override REQUIRES(!m_SoundLock); void SetChannel(int ChannelID, float Vol, float Pan) override; void SetListenerPos(float x, float y) override; - void SetVoiceVolume(CVoiceHandle Voice, float Volume) override; - void SetVoiceFalloff(CVoiceHandle Voice, float Falloff) override; - void SetVoiceLocation(CVoiceHandle Voice, float x, float y) override; - void SetVoiceTimeOffset(CVoiceHandle Voice, float TimeOffset) override; // in s + void SetVoiceVolume(CVoiceHandle Voice, float Volume) override REQUIRES(!m_SoundLock); + void SetVoiceFalloff(CVoiceHandle Voice, float Falloff) override REQUIRES(!m_SoundLock); + void SetVoiceLocation(CVoiceHandle Voice, float x, float y) override REQUIRES(!m_SoundLock); + void SetVoiceTimeOffset(CVoiceHandle Voice, float TimeOffset) override REQUIRES(!m_SoundLock); // in s - void SetVoiceCircle(CVoiceHandle Voice, float Radius) override; - void SetVoiceRectangle(CVoiceHandle Voice, float Width, float Height) override; + void SetVoiceCircle(CVoiceHandle Voice, float Radius) override REQUIRES(!m_SoundLock); + void SetVoiceRectangle(CVoiceHandle Voice, float Width, float Height) override REQUIRES(!m_SoundLock); - CVoiceHandle Play(int ChannelID, int SampleID, int Flags, float x, float y); - CVoiceHandle PlayAt(int ChannelID, int SampleID, int Flags, float x, float y) override; - CVoiceHandle Play(int ChannelID, int SampleID, int Flags) override; - void Pause(int SampleID) override; - void Stop(int SampleID) override; - void StopAll() override; - void StopVoice(CVoiceHandle Voice) override; - bool IsPlaying(int SampleID) override; + CVoiceHandle Play(int ChannelID, int SampleID, int Flags, float x, float y) REQUIRES(!m_SoundLock); + CVoiceHandle PlayAt(int ChannelID, int SampleID, int Flags, float x, float y) override REQUIRES(!m_SoundLock); + CVoiceHandle Play(int ChannelID, int SampleID, int Flags) override REQUIRES(!m_SoundLock); + void Pause(int SampleID) override REQUIRES(!m_SoundLock); + void Stop(int SampleID) override REQUIRES(!m_SoundLock); + void StopAll() override REQUIRES(!m_SoundLock); + void StopVoice(CVoiceHandle Voice) override REQUIRES(!m_SoundLock); + bool IsPlaying(int SampleID) override REQUIRES(!m_SoundLock); - void Mix(short *pFinalOut, unsigned Frames) override; + void Mix(short *pFinalOut, unsigned Frames) override REQUIRES(!m_SoundLock); void PauseAudioDevice() override; void UnpauseAudioDevice() override; }; diff --git a/src/engine/server/server_logger.h b/src/engine/server/server_logger.h index 86406d3c0..18c5d9f5a 100644 --- a/src/engine/server/server_logger.h +++ b/src/engine/server/server_logger.h @@ -9,13 +9,13 @@ class CServer; class CServerLogger : public ILogger { CServer *m_pServer = nullptr; - std::mutex m_PendingLock; + CLock m_PendingLock; std::vector m_vPending; std::thread::id m_MainThread; public: CServerLogger(CServer *pServer); - void Log(const CLogMessage *pMessage) override; + void Log(const CLogMessage *pMessage) override REQUIRES(!m_PendingLock); // Must be called from the main thread! void OnServerDeletion(); }; diff --git a/src/engine/shared/assertion_logger.cpp b/src/engine/shared/assertion_logger.cpp index c37d6e054..b6b2510bf 100644 --- a/src/engine/shared/assertion_logger.cpp +++ b/src/engine/shared/assertion_logger.cpp @@ -1,31 +1,31 @@ #include "assertion_logger.h" +#include #include #include + #include #include -#include - class CAssertionLogger : public ILogger { - void Dump(); - struct SDebugMessageItem { char m_aMessage[1024]; }; - std::mutex m_DbgMessageMutex; + CLock m_DbgMessageMutex; CStaticRingBuffer m_DbgMessages; char m_aAssertLogPath[IO_MAX_PATH_LENGTH]; char m_aGameName[256]; + void Dump() REQUIRES(!m_DbgMessageMutex); + public: CAssertionLogger(const char *pAssertLogPath, const char *pGameName); - void Log(const CLogMessage *pMessage) override; - void GlobalFinish() override; + void Log(const CLogMessage *pMessage) override REQUIRES(!m_DbgMessageMutex); + void GlobalFinish() override REQUIRES(!m_DbgMessageMutex); }; void CAssertionLogger::Log(const CLogMessage *pMessage) @@ -34,7 +34,7 @@ void CAssertionLogger::Log(const CLogMessage *pMessage) { return; } - std::unique_lock Lock(m_DbgMessageMutex); + const CLockScope LockScope(m_DbgMessageMutex); SDebugMessageItem *pMsgItem = (SDebugMessageItem *)m_DbgMessages.Allocate(sizeof(SDebugMessageItem)); str_copy(pMsgItem->m_aMessage, pMessage->m_aLine); } @@ -53,7 +53,7 @@ void CAssertionLogger::Dump() char aDate[64]; str_timestamp(aDate, sizeof(aDate)); str_format(aAssertLogFile, std::size(aAssertLogFile), "%s%s_assert_log_%s_%d.txt", m_aAssertLogPath, m_aGameName, aDate, pid()); - std::unique_lock Lock(m_DbgMessageMutex); + const CLockScope LockScope(m_DbgMessageMutex); IOHANDLE FileHandle = io_open(aAssertLogFile, IOFLAG_WRITE); if(FileHandle) { diff --git a/src/game/client/components/console.cpp b/src/game/client/components/console.cpp index 99754dbfc..28de584bc 100644 --- a/src/game/client/components/console.cpp +++ b/src/game/client/components/console.cpp @@ -1,6 +1,7 @@ /* (c) Magnus Auvinen. See licence.txt in the root of the distribution for more information. */ /* If you are missing that file, acquire a complete release at teeworlds.com. */ +#include #include #include #include @@ -28,7 +29,7 @@ class CConsoleLogger : public ILogger { CGameConsole *m_pConsole; - std::mutex m_ConsoleMutex; + CLock m_ConsoleMutex; public: CConsoleLogger(CGameConsole *pConsole) : @@ -37,8 +38,8 @@ public: dbg_assert(pConsole != nullptr, "console pointer must not be null"); } - void Log(const CLogMessage *pMessage) override; - void OnConsoleDeletion(); + void Log(const CLogMessage *pMessage) override REQUIRES(!m_ConsoleMutex); + void OnConsoleDeletion() REQUIRES(!m_ConsoleMutex); }; void CConsoleLogger::Log(const CLogMessage *pMessage) @@ -54,7 +55,7 @@ void CConsoleLogger::Log(const CLogMessage *pMessage) Color.g = pMessage->m_Color.g / 255.0; Color.b = pMessage->m_Color.b / 255.0; } - std::unique_lock Guard(m_ConsoleMutex); + const CLockScope LockScope(m_ConsoleMutex); if(m_pConsole) { m_pConsole->m_LocalConsole.PrintLine(pMessage->m_aLine, pMessage->m_LineLength, Color); @@ -63,7 +64,7 @@ void CConsoleLogger::Log(const CLogMessage *pMessage) void CConsoleLogger::OnConsoleDeletion() { - std::unique_lock Guard(m_ConsoleMutex); + const CLockScope LockScope(m_ConsoleMutex); m_pConsole = nullptr; } @@ -119,22 +120,20 @@ void CGameConsole::CInstance::Init(CGameConsole *pGameConsole) void CGameConsole::CInstance::ClearBacklog() { - m_BacklogLock.lock(); + const CLockScope LockScope(m_BacklogLock); m_Backlog.Init(); m_BacklogCurPage = 0; - m_BacklogLock.unlock(); } void CGameConsole::CInstance::ClearBacklogYOffsets() { - m_BacklogLock.lock(); + const CLockScope LockScope(m_BacklogLock); auto *pEntry = m_Backlog.First(); while(pEntry) { pEntry->m_YOffset = -1.0f; pEntry = m_Backlog.Next(pEntry); } - m_BacklogLock.unlock(); } void CGameConsole::CInstance::ClearHistory() @@ -385,13 +384,12 @@ bool CGameConsole::CInstance::OnInput(const IInput::CEvent &Event) void CGameConsole::CInstance::PrintLine(const char *pLine, int Len, ColorRGBA PrintColor) { - m_BacklogLock.lock(); + const CLockScope LockScope(m_BacklogLock); CBacklogEntry *pEntry = m_Backlog.Allocate(sizeof(CBacklogEntry) + Len); pEntry->m_YOffset = -1.0f; pEntry->m_PrintColor = PrintColor; str_copy(pEntry->m_aText, pLine, Len + 1); m_NewLineCounter++; - m_BacklogLock.unlock(); } CGameConsole::CGameConsole() : @@ -901,13 +899,12 @@ void CGameConsole::Dump(int Type) IOHANDLE File = Storage()->OpenFile(aFilename, IOFLAG_WRITE, IStorage::TYPE_SAVE); if(File) { - pConsole->m_BacklogLock.lock(); + const CLockScope LockScope(pConsole->m_BacklogLock); for(CInstance::CBacklogEntry *pEntry = pConsole->m_Backlog.First(); pEntry; pEntry = pConsole->m_Backlog.Next(pEntry)) { io_write(File, pEntry->m_aText, str_length(pEntry->m_aText)); io_write_newline(File); } - pConsole->m_BacklogLock.unlock(); io_close(File); str_format(aBuf, sizeof(aBuf), "%s contents were written to '%s'", pConsole->m_pName, aFilename); } diff --git a/src/game/client/components/console.h b/src/game/client/components/console.h index ba734a849..087f1097c 100644 --- a/src/game/client/components/console.h +++ b/src/game/client/components/console.h @@ -2,13 +2,14 @@ /* If you are missing that file, acquire a complete release at teeworlds.com. */ #ifndef GAME_CLIENT_COMPONENTS_CONSOLE_H #define GAME_CLIENT_COMPONENTS_CONSOLE_H -#include -#include -#include + +#include #include +#include -#include +#include +#include enum { @@ -32,7 +33,7 @@ class CGameConsole : public CComponent ColorRGBA m_PrintColor; char m_aText[1]; }; - std::mutex m_BacklogLock; + CLock m_BacklogLock; CStaticRingBuffer m_Backlog; CStaticRingBuffer m_History; char *m_pHistoryEntry; @@ -76,15 +77,15 @@ class CGameConsole : public CComponent CInstance(int t); void Init(CGameConsole *pGameConsole); - void ClearBacklog(); - void ClearBacklogYOffsets(); + void ClearBacklog() REQUIRES(!m_BacklogLock); + void ClearBacklogYOffsets() REQUIRES(!m_BacklogLock); void ClearHistory(); void Reset(); void ExecuteLine(const char *pLine); bool OnInput(const IInput::CEvent &Event); - void PrintLine(const char *pLine, int Len, ColorRGBA PrintColor); + void PrintLine(const char *pLine, int Len, ColorRGBA PrintColor) REQUIRES(!m_BacklogLock); const char *GetString() const { return m_Input.GetString(); } static void PossibleCommandsCompleteCallback(int Index, const char *pStr, void *pUser);