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.
This commit is contained in:
Robert Müller 2023-11-10 21:49:42 +01:00
parent 3d858c28ee
commit 10470046b7
8 changed files with 81 additions and 87 deletions

View file

@ -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<ILogger> log_logger_windows_debugger()
void CFutureLogger::Set(std::shared_ptr<ILogger> pLogger)
{
std::shared_ptr<ILogger> 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<ILogger> 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<ILogger> 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()

View file

@ -1,10 +1,11 @@
#ifndef BASE_LOGGER_H
#define BASE_LOGGER_H
#include "lock.h"
#include "log.h"
#include <atomic>
#include <memory>
#include <mutex>
#include <vector>
typedef void *IOHANDLE;
@ -226,15 +227,15 @@ class CFutureLogger : public ILogger
private:
std::shared_ptr<ILogger> m_pLogger;
std::vector<CLogMessage> 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<ILogger> pLogger);
void Log(const CLogMessage *pMessage) override;
void Set(std::shared_ptr<ILogger> pLogger) REQUIRES(!m_PendingLock);
void Log(const CLogMessage *pMessage) override REQUIRES(!m_PendingLock);
void GlobalFinish() override;
void OnFilterChange() override;
};

View file

@ -687,7 +687,7 @@ void CSound::SetVoiceVolume(CVoiceHandle Voice, float Volume)
int VoiceID = Voice.Id();
std::unique_lock<std::mutex> 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<std::mutex> 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<std::mutex> 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<std::mutex> 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<std::mutex> 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<std::mutex> 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<std::mutex> 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<std::mutex> 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<std::mutex> 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<std::mutex> 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<std::mutex> 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; });
}

View file

@ -3,12 +3,13 @@
#ifndef ENGINE_CLIENT_SOUND_H
#define ENGINE_CLIENT_SOUND_H
#include <base/lock.h>
#include <engine/sound.h>
#include <SDL_audio.h>
#include <atomic>
#include <mutex>
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;
};

View file

@ -9,13 +9,13 @@ class CServer;
class CServerLogger : public ILogger
{
CServer *m_pServer = nullptr;
std::mutex m_PendingLock;
CLock m_PendingLock;
std::vector<CLogMessage> 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();
};

View file

@ -1,31 +1,31 @@
#include "assertion_logger.h"
#include <base/lock.h>
#include <base/logger.h>
#include <base/system.h>
#include <engine/shared/ringbuffer.h>
#include <engine/storage.h>
#include <mutex>
class CAssertionLogger : public ILogger
{
void Dump();
struct SDebugMessageItem
{
char m_aMessage[1024];
};
std::mutex m_DbgMessageMutex;
CLock m_DbgMessageMutex;
CStaticRingBuffer<SDebugMessageItem, sizeof(SDebugMessageItem) * 64, CRingBufferBase::FLAG_RECYCLE> 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<std::mutex> 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<std::mutex> Lock(m_DbgMessageMutex);
const CLockScope LockScope(m_DbgMessageMutex);
IOHANDLE FileHandle = io_open(aAssertLogFile, IOFLAG_WRITE);
if(FileHandle)
{

View file

@ -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 <base/lock.h>
#include <base/logger.h>
#include <base/math.h>
#include <base/system.h>
@ -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<std::mutex> 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<std::mutex> 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);
}

View file

@ -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 <engine/shared/ringbuffer.h>
#include <game/client/component.h>
#include <game/client/lineinput.h>
#include <base/lock.h>
#include <engine/console.h>
#include <engine/shared/ringbuffer.h>
#include <mutex>
#include <game/client/component.h>
#include <game/client/lineinput.h>
enum
{
@ -32,7 +33,7 @@ class CGameConsole : public CComponent
ColorRGBA m_PrintColor;
char m_aText[1];
};
std::mutex m_BacklogLock;
CLock m_BacklogLock;
CStaticRingBuffer<CBacklogEntry, 1024 * 1024, CRingBufferBase::FLAG_RECYCLE> m_Backlog;
CStaticRingBuffer<char, 64 * 1024, CRingBufferBase::FLAG_RECYCLE> 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);