Merge #5260

5260: Pr thread safety negative r=heinrich5991 a=def-

WorkerThread is hard because `REQUIRES(!((CJobPool *)pUser)->m_Lock)` would require alias analysis or the function using that everywhere.

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

## 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: Dennis Felsing <dennis@felsin9.de>

CMakeLists.txt

@@ -344,6 +344,7 @@ if(NOT MSVC AND NOT HAIKU)
   add_cxx_compiler_flag_if_supported(OUR_FLAGS_OWN -Wshadow-all) # clang
   add_cxx_compiler_flag_if_supported(OUR_FLAGS_OWN -Wshadow=global) # gcc
   add_cxx_compiler_flag_if_supported(OUR_FLAGS_OWN -Wthread-safety)
+  add_cxx_compiler_flag_if_supported(OUR_FLAGS_OWN -Wthread-safety-negative)
   add_cxx_compiler_flag_if_supported(OUR_FLAGS_OWN -Wsuggest-override)
   add_linker_flag_if_supported(OUR_FLAGS_LINK -Wno-alloc-size-larger-than) # save.cpp with LTO
   # add_cxx_compiler_flag_if_supported(OUR_FLAGS_OWN -Wdouble-promotion) # Many occurences

src/base/tl/threading.h

@@ -45,8 +45,8 @@ public:
 
 	CLock(const CLock &) = delete;
 
-	void Take() ACQUIRE(m_Lock) { lock_wait(m_Lock); }
-	void Release() RELEASE() { lock_unlock(m_Lock); }
+	void Take() ACQUIRE(m_Lock) REQUIRES(!m_Lock) { lock_wait(m_Lock); }
+	void Release() RELEASE() REQUIRES(m_Lock) { lock_unlock(m_Lock); }
 };
 
 class CScopeLock

src/engine/client/serverbrowser_http.cpp

@@ -54,13 +54,13 @@ private:
 		std::shared_ptr<CData> m_pData;
 		std::unique_ptr<CHttpRequest> m_pHead PT_GUARDED_BY(m_Lock);
 		std::unique_ptr<CHttpRequest> m_pGet PT_GUARDED_BY(m_Lock);
-		void Run() override;
+		void Run() override REQUIRES(!m_Lock);
 
 	public:
 		CJob(std::shared_ptr<CData> pData) :
 			m_pData(std::move(pData)) { m_Lock = lock_create(); }
 		virtual ~CJob() { lock_destroy(m_Lock); }
-		void Abort();
+		void Abort() REQUIRES(!m_Lock);
 	};
 
 	IEngine *m_pEngine;

src/engine/client/updater.h

@@ -64,15 +64,15 @@ class CUpdater : public IUpdater
 	bool ReplaceClient();
 	bool ReplaceServer();
 
-	void SetCurrentState(int NewState);
+	void SetCurrentState(int NewState) REQUIRES(!m_Lock);
 
 public:
 	CUpdater();
 	~CUpdater();
 
-	int GetCurrentState() override;
-	void GetCurrentFile(char *pBuf, int BufSize) override;
-	int GetCurrentPercent() override;
+	int GetCurrentState() override REQUIRES(!m_Lock);
+	void GetCurrentFile(char *pBuf, int BufSize) override REQUIRES(!m_Lock);
+	int GetCurrentPercent() override REQUIRES(!m_Lock);
 
 	void InitiateUpdate() override;
 	void Init();

src/engine/client/video.cpp

@@ -157,7 +157,7 @@ void CVideo::Start()
 	for(size_t i = 0; i < m_VideoThreads; ++i)
 	{
 		std::unique_lock<std::mutex> Lock(m_vVideoThreads[i]->m_Mutex);
-		m_vVideoThreads[i]->m_Thread = std::thread([this, i]() { RunVideoThread(i == 0 ? (m_VideoThreads - 1) : (i - 1), i); });
+		m_vVideoThreads[i]->m_Thread = std::thread([this, i]() REQUIRES(!g_WriteLock) { RunVideoThread(i == 0 ? (m_VideoThreads - 1) : (i - 1), i); });
 		m_vVideoThreads[i]->m_Cond.wait(Lock, [this, i]() -> bool { return m_vVideoThreads[i]->m_Started; });
 	}
 
@@ -169,7 +169,7 @@ void CVideo::Start()
 	for(size_t i = 0; i < m_AudioThreads; ++i)
 	{
 		std::unique_lock<std::mutex> Lock(m_vAudioThreads[i]->m_Mutex);
-		m_vAudioThreads[i]->m_Thread = std::thread([this, i]() { RunAudioThread(i == 0 ? (m_AudioThreads - 1) : (i - 1), i); });
+		m_vAudioThreads[i]->m_Thread = std::thread([this, i]() REQUIRES(!g_WriteLock) { RunAudioThread(i == 0 ? (m_AudioThreads - 1) : (i - 1), i); });
 		m_vAudioThreads[i]->m_Cond.wait(Lock, [this, i]() -> bool { return m_vAudioThreads[i]->m_Started; });
 	}
 

src/engine/client/video.h

@@ -45,7 +45,7 @@ public:
 	CVideo(class CGraphics_Threaded *pGraphics, class ISound *pSound, class IStorage *pStorage, class IConsole *pConsole, int width, int height, const char *name);
 	~CVideo();
 
-	void Start() override;
+	void Start() override REQUIRES(!g_WriteLock);
 	void Stop() override;
 	void Pause(bool Pause) override;
 	bool IsRecording() override { return m_Recording; }
@@ -61,11 +61,11 @@ public:
 	static void Init() { av_log_set_level(AV_LOG_DEBUG); }
 
 private:
-	void RunVideoThread(size_t ParentThreadIndex, size_t ThreadIndex);
-	void FillVideoFrame(size_t ThreadIndex);
+	void RunVideoThread(size_t ParentThreadIndex, size_t ThreadIndex) REQUIRES(!g_WriteLock);
+	void FillVideoFrame(size_t ThreadIndex) REQUIRES(!g_WriteLock);
 	void ReadRGBFromGL(size_t ThreadIndex);
 
-	void RunAudioThread(size_t ParentThreadIndex, size_t ThreadIndex);
+	void RunAudioThread(size_t ParentThreadIndex, size_t ThreadIndex) REQUIRES(!g_WriteLock);
 	void FillAudioFrame(size_t ThreadIndex);
 
 	bool OpenVideo();

src/engine/shared/jobs.h

@@ -50,7 +50,7 @@ class CJobPool
 	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);
+	static void WorkerThread(void *pUser) NO_THREAD_SAFETY_ANALYSIS;
 
 public:
 	CJobPool();
@@ -58,7 +58,7 @@ public:
 
 	void Init(int NumThreads);
 	void Destroy();
-	void Add(std::shared_ptr<IJob> pJob);
+	void Add(std::shared_ptr<IJob> pJob) REQUIRES(!m_Lock);
 	static void RunBlocking(IJob *pJob);
 };
 #endif