From e592afc98c16f35a169974c3a4077561e41216a1 Mon Sep 17 00:00:00 2001 From: def Date: Wed, 2 Dec 2020 19:11:19 +0100 Subject: [PATCH 1/2] Add Thread Safety Analysis and annotate some code: https://clang.llvm.org/docs/ThreadSafetyAnalysis.html --- CMakeLists.txt | 1 + src/base/system.c | 14 +++++-- src/base/system.h | 76 +++++++++++++++++++++++++++++++++++-- src/base/tl/threading.h | 10 ++--- src/engine/client/http.cpp | 4 +- src/engine/client/sound.cpp | 2 +- src/engine/client/updater.h | 4 +- src/engine/client/video.cpp | 4 +- src/engine/client/video.h | 4 +- src/engine/shared/jobs.h | 4 +- 10 files changed, 100 insertions(+), 23 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index 1bd53655c..8ba29845f 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -202,6 +202,7 @@ if(NOT MSVC) add_c_compiler_flag_if_supported(OUR_FLAGS_OWN -Wduplicated-branches) add_c_compiler_flag_if_supported(OUR_FLAGS_OWN -Wlogical-op) add_c_compiler_flag_if_supported(OUR_FLAGS_OWN -Wrestrict) + add_c_compiler_flag_if_supported(OUR_FLAGS_OWN -Wthread-safety) # TODO: Enable for C++ code except gtest #add_cxx_compiler_flag_if_supported(OUR_FLAGS_OWN "-Wuseless-cast") endif() diff --git a/src/base/system.c b/src/base/system.c index 6cc508c08..729a0bfe1 100644 --- a/src/base/system.c +++ b/src/base/system.c @@ -361,6 +361,7 @@ int io_flush(IOHANDLE io) #define ASYNC_BUFSIZE 8 * 1024 #define ASYNC_LOCAL_BUFSIZE 64 * 1024 +// TODO: Use Thread Safety Analysis when this file is converted to C++ struct ASYNCIO { LOCK lock; @@ -410,7 +411,7 @@ static void buffer_ptrs(ASYNCIO *aio, struct BUFFERS *buffers) } } -static void aio_handle_free_and_unlock(ASYNCIO *aio) +static void aio_handle_free_and_unlock(ASYNCIO *aio) RELEASE(aio->lock) { int do_free; aio->refcount--; @@ -546,12 +547,12 @@ static unsigned int next_buffer_size(unsigned int cur_size, unsigned int need_si return cur_size; } -void aio_lock(ASYNCIO *aio) +void aio_lock(ASYNCIO *aio) ACQUIRE(aio->lock) { lock_wait(aio->lock); } -void aio_unlock(ASYNCIO *aio) +void aio_unlock(ASYNCIO *aio) RELEASE(aio->lock) { lock_unlock(aio->lock); sphore_signal(&aio->sphore); @@ -839,6 +840,10 @@ int lock_trylock(LOCK lock) #endif } +#ifdef __clang__ +#pragma clang diagnostic push +#pragma clang diagnostic ignored "-Wthread-safety-analysis" +#endif void lock_wait(LOCK lock) { #if defined(CONF_FAMILY_UNIX) @@ -864,6 +869,9 @@ void lock_unlock(LOCK lock) #error not implemented on this platform #endif } +#ifdef __clang__ +#pragma clang diagnostic pop +#endif #if defined(CONF_FAMILY_WINDOWS) void sphore_init(SEMAPHORE *sem) diff --git a/src/base/system.h b/src/base/system.h index 7cbe0c94b..c6e8428cb 100644 --- a/src/base/system.h +++ b/src/base/system.h @@ -530,15 +530,83 @@ void thread_detach(void *thread); */ void *thread_init_and_detach(void (*threadfunc)(void *), void *user, const char *name); +// Enable thread safety attributes only with clang. +// The attributes can be safely erased when compiling with other compilers. +#if defined(__clang__) && (!defined(SWIG)) +#define THREAD_ANNOTATION_ATTRIBUTE__(x) __attribute__((x)) +#else +#define THREAD_ANNOTATION_ATTRIBUTE__(x) // no-op +#endif + +#define CAPABILITY(x) \ + THREAD_ANNOTATION_ATTRIBUTE__(capability(x)) + +#define SCOPED_CAPABILITY \ + THREAD_ANNOTATION_ATTRIBUTE__(scoped_lockable) + +#define GUARDED_BY(x) \ + THREAD_ANNOTATION_ATTRIBUTE__(guarded_by(x)) + +#define PT_GUARDED_BY(x) \ + THREAD_ANNOTATION_ATTRIBUTE__(pt_guarded_by(x)) + +#define ACQUIRED_BEFORE(...) \ + THREAD_ANNOTATION_ATTRIBUTE__(acquired_before(__VA_ARGS__)) + +#define ACQUIRED_AFTER(...) \ + THREAD_ANNOTATION_ATTRIBUTE__(acquired_after(__VA_ARGS__)) + +#define REQUIRES(...) \ + THREAD_ANNOTATION_ATTRIBUTE__(requires_capability(__VA_ARGS__)) + +#define REQUIRES_SHARED(...) \ + THREAD_ANNOTATION_ATTRIBUTE__(requires_shared_capability(__VA_ARGS__)) + +#define ACQUIRE(...) \ + THREAD_ANNOTATION_ATTRIBUTE__(acquire_capability(__VA_ARGS__)) + +#define ACQUIRE_SHARED(...) \ + THREAD_ANNOTATION_ATTRIBUTE__(acquire_shared_capability(__VA_ARGS__)) + +#define RELEASE(...) \ + THREAD_ANNOTATION_ATTRIBUTE__(release_capability(__VA_ARGS__)) + +#define RELEASE_SHARED(...) \ + THREAD_ANNOTATION_ATTRIBUTE__(release_shared_capability(__VA_ARGS__)) + +#define RELEASE_GENERIC(...) \ + THREAD_ANNOTATION_ATTRIBUTE__(release_generic_capability(__VA_ARGS__)) + +#define TRY_ACQUIRE(...) \ + THREAD_ANNOTATION_ATTRIBUTE__(try_acquire_capability(__VA_ARGS__)) + +#define TRY_ACQUIRE_SHARED(...) \ + THREAD_ANNOTATION_ATTRIBUTE__(try_acquire_shared_capability(__VA_ARGS__)) + +#define EXCLUDES(...) \ + THREAD_ANNOTATION_ATTRIBUTE__(locks_excluded(__VA_ARGS__)) + +#define ASSERT_CAPABILITY(x) \ + THREAD_ANNOTATION_ATTRIBUTE__(assert_capability(x)) + +#define ASSERT_SHARED_CAPABILITY(x) \ + THREAD_ANNOTATION_ATTRIBUTE__(assert_shared_capability(x)) + +#define RETURN_CAPABILITY(x) \ + THREAD_ANNOTATION_ATTRIBUTE__(lock_returned(x)) + +#define NO_THREAD_SAFETY_ANALYSIS \ + THREAD_ANNOTATION_ATTRIBUTE__(no_thread_safety_analysis) + /* Group: Locks */ -typedef void *LOCK; +typedef CAPABILITY("mutex") void *LOCK; LOCK lock_create(void); void lock_destroy(LOCK lock); -int lock_trylock(LOCK lock); -void lock_wait(LOCK lock); -void lock_unlock(LOCK lock); +int lock_trylock(LOCK lock) TRY_ACQUIRE(1, lock); +void lock_wait(LOCK lock) ACQUIRE(lock); +void lock_unlock(LOCK lock) RELEASE(lock); /* Group: Semaphores */ #if defined(CONF_FAMILY_WINDOWS) diff --git a/src/base/tl/threading.h b/src/base/tl/threading.h index 6944b50a2..33eb849a4 100644 --- a/src/base/tl/threading.h +++ b/src/base/tl/threading.h @@ -15,25 +15,25 @@ public: void Signal() { sphore_signal(&m_Sem); } }; -class CLock +class SCOPED_CAPABILITY CLock { LOCK m_Lock; public: - CLock() + CLock() ACQUIRE(m_Lock) { m_Lock = lock_create(); } - ~CLock() + ~CLock() RELEASE() { lock_destroy(m_Lock); } CLock(const CLock &) = delete; - void Take() { lock_wait(m_Lock); } - void Release() { lock_unlock(m_Lock); } + void Take() ACQUIRE(m_Lock) { lock_wait(m_Lock); } + void Release() RELEASE() { lock_unlock(m_Lock); } }; class CScopeLock diff --git a/src/engine/client/http.cpp b/src/engine/client/http.cpp index 388a9430c..c822397d3 100644 --- a/src/engine/client/http.cpp +++ b/src/engine/client/http.cpp @@ -24,7 +24,7 @@ static int GetLockIndex(int Data) return Data; } -static void CurlLock(CURL *pHandle, curl_lock_data Data, curl_lock_access Access, void *pUser) +static void CurlLock(CURL *pHandle, curl_lock_data Data, curl_lock_access Access, void *pUser) ACQUIRE(gs_aLocks[GetLockIndex(Data)]) { (void)pHandle; (void)Access; @@ -32,7 +32,7 @@ static void CurlLock(CURL *pHandle, curl_lock_data Data, curl_lock_access Access lock_wait(gs_aLocks[GetLockIndex(Data)]); } -static void CurlUnlock(CURL *pHandle, curl_lock_data Data, void *pUser) +static void CurlUnlock(CURL *pHandle, curl_lock_data Data, void *pUser) RELEASE(gs_aLocks[GetLockIndex(Data)]) { (void)pHandle; (void)pUser; diff --git a/src/engine/client/sound.cpp b/src/engine/client/sound.cpp index ab4dd89d2..efdc0eb2d 100644 --- a/src/engine/client/sound.cpp +++ b/src/engine/client/sound.cpp @@ -76,7 +76,7 @@ static int m_CenterY = 0; static int m_MixingRate = 48000; static volatile int m_SoundVolume = 100; -static int m_NextVoice = 0; +static int m_NextVoice GUARDED_BY(m_SoundLock) = 0; static int *m_pMixBuffer = 0; // buffer only used by the thread callback function static unsigned m_MaxFrames = 0; diff --git a/src/engine/client/updater.h b/src/engine/client/updater.h index 5ba3fadbd..c6c1e12a6 100644 --- a/src/engine/client/updater.h +++ b/src/engine/client/updater.h @@ -45,8 +45,8 @@ class CUpdater : public IUpdater LOCK m_Lock; int m_State; - char m_aStatus[256]; - int m_Percent; + char m_aStatus[256] GUARDED_BY(m_Lock); + int m_Percent GUARDED_BY(m_Lock); char m_aLastFile[256]; char m_aClientExecTmp[64]; char m_aServerExecTmp[64]; diff --git a/src/engine/client/video.cpp b/src/engine/client/video.cpp index 0bc18321c..9ac9c3639 100644 --- a/src/engine/client/video.cpp +++ b/src/engine/client/video.cpp @@ -11,7 +11,7 @@ #define STREAM_PIX_FMT AV_PIX_FMT_YUV420P /* default pix_fmt */ const size_t FORMAT_NCHANNELS = 3; -static LOCK g_WriteLock = 0; +LOCK g_WriteLock = 0; CVideo::CVideo(CGraphics_Threaded *pGraphics, IStorage *pStorage, IConsole *pConsole, int Width, int Height, const char *pName) : m_pGraphics(pGraphics), @@ -667,7 +667,6 @@ bool CVideo::AddStream(OutputStream *pStream, AVFormatContext *pOC, AVCodec **pp void CVideo::WriteFrame(OutputStream *pStream) { - //lock_wait(g_WriteLock); int RetRecv = 0; AVPacket Packet = {0}; @@ -701,7 +700,6 @@ void CVideo::WriteFrame(OutputStream *pStream) { dbg_msg("video_recorder", "Error encoding frame, error: %d", RetRecv); } - //lock_unlock(g_WriteLock); } void CVideo::FinishFrames(OutputStream *pStream) diff --git a/src/engine/client/video.h b/src/engine/client/video.h index 47b1fc766..1422e5c60 100644 --- a/src/engine/client/video.h +++ b/src/engine/client/video.h @@ -32,6 +32,8 @@ extern "C" { #include #define ALEN 2048 +extern LOCK g_WriteLock; + // a wrapper around a single output AVStream typedef struct OutputStream { @@ -83,7 +85,7 @@ private: AVFrame *AllocPicture(enum AVPixelFormat PixFmt, int Width, int Height); AVFrame *AllocAudioFrame(enum AVSampleFormat SampleFmt, uint64 ChannelLayout, int SampleRate, int NbSamples); - void WriteFrame(OutputStream *pStream); + void WriteFrame(OutputStream *pStream) REQUIRES(g_WriteLock); void FinishFrames(OutputStream *pStream); void CloseStream(OutputStream *pStream); diff --git a/src/engine/shared/jobs.h b/src/engine/shared/jobs.h index 44dc07040..97e98058e 100644 --- a/src/engine/shared/jobs.h +++ b/src/engine/shared/jobs.h @@ -48,8 +48,8 @@ class CJobPool LOCK m_Lock; SEMAPHORE m_Semaphore; - std::shared_ptr m_pFirstJob; - std::shared_ptr m_pLastJob; + std::shared_ptr m_pFirstJob GUARDED_BY(m_Lock); + std::shared_ptr m_pLastJob GUARDED_BY(m_Lock); static void WorkerThread(void *pUser); From 7fa331a5e9ff3981b9044231d829921977ee7b26 Mon Sep 17 00:00:00 2001 From: def Date: Fri, 8 Jan 2021 11:21:37 +0100 Subject: [PATCH 2/2] Fix centisecs rounding in str_time_float --- src/base/system.c | 3 ++- src/test/str.cpp | 3 +++ 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/src/base/system.c b/src/base/system.c index ddc0710b6..2300cb9a4 100644 --- a/src/base/system.c +++ b/src/base/system.c @@ -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 #include @@ -2885,7 +2886,7 @@ int str_time(int64 centisecs, int format, char *buffer, int buffer_size) int str_time_float(float secs, int format, char *buffer, int buffer_size) { - return str_time((int64)(secs * 100.0), format, buffer, buffer_size); + return str_time(llroundf(secs * 100.0), format, buffer, buffer_size); } void str_escape(char **dst, const char *src, const char *end) diff --git a/src/test/str.cpp b/src/test/str.cpp index 6eecf7949..3c6d8ab3f 100644 --- a/src/test/str.cpp +++ b/src/test/str.cpp @@ -309,4 +309,7 @@ TEST(Str, StrTimeFloat) char aBuf[64]; EXPECT_EQ(str_time_float(123456.78, TIME_DAYS, aBuf, sizeof(aBuf)), 11); EXPECT_STREQ(aBuf, "1d 10:17:36"); + + EXPECT_EQ(str_time_float(12.16, TIME_HOURS_CENTISECS, aBuf, sizeof(aBuf)), 8); + EXPECT_STREQ(aBuf, "00:12.16"); }