Merge #3377 #3487

3377: Add Thread Safety Analysis r=heinrich5991 a=def-

https://clang.llvm.org/docs/ThreadSafetyAnalysis.html

## Checklist

- [ ] Tested the change ingame
- [ ] Provided screenshots if it is a visual change
- [ ] Tested in combination with possibly related configuration options
- [ ] Written a unit test if it works standalone, system.c especially
- [ ] Considered possible null pointers and out of bounds array indexing
- [ ] Changed no physics that affect existing maps
- [ ] Tested the change with [ASan+UBSan or valgrind's memcheck](https://github.com/ddnet/ddnet/#using-addresssanitizer--undefinedbehavioursanitizer-or-valgrinds-memcheck) (optional)


3487: Fix centisecs rounding in str_time_float r=heinrich5991 a=def-

<!-- What is the motivation for the changes of this pull request -->

## Checklist

- [ ] Tested the change ingame
- [ ] Provided screenshots if it is a visual change
- [ ] Tested in combination with possibly related configuration options
- [ ] Written a unit test if it works standalone, system.c especially
- [ ] Considered possible null pointers and out of bounds array indexing
- [ ] Changed no physics that affect existing maps
- [ ] Tested the change with [ASan+UBSan or valgrind's memcheck](https://github.com/ddnet/ddnet/#using-addresssanitizer--undefinedbehavioursanitizer-or-valgrinds-memcheck) (optional)


Co-authored-by: def <dennis@felsin9.de>

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()

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 <ctype.h>
+#include <math.h>
 #include <stdarg.h>
 #include <stdio.h>
 #include <stdlib.h>
@@ -361,6 +362,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 +412,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 +548,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 +841,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 +870,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)
@@ -2884,7 +2893,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)

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)

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

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;

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;
 

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];

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)

src/engine/client/video.h

@@ -32,6 +32,8 @@ extern "C" {
 #include <engine/shared/video.h>
 #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);
 

src/engine/shared/jobs.h

@@ -48,8 +48,8 @@ class CJobPool
 
 	LOCK m_Lock;
 	SEMAPHORE m_Semaphore;
-	std::shared_ptr<IJob> m_pFirstJob;
-	std::shared_ptr<IJob> m_pLastJob;
+	std::shared_ptr<IJob> m_pFirstJob GUARDED_BY(m_Lock);
+	std::shared_ptr<IJob> m_pLastJob GUARDED_BY(m_Lock);
 
 	static void WorkerThread(void *pUser);
 

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");
 }