Improve robustness of atomics

And fix increment in `CowData` not being conditional anymore after the recent changes.

Co-authored-by: Hein-Pieter van Braam-Stewart <[email protected]>

core/cowdata.h

@@ -44,8 +44,9 @@ class CharString;
 template <class T, class V>
 class VMap;
 
-// CowData is relying on this to be true
-static_assert(sizeof(SafeNumeric<uint32_t>) == sizeof(uint32_t), "");
+#if !defined(NO_THREADS)
+SAFE_NUMERIC_TYPE_PUN_GUARANTEES(uint32_t)
+#endif
 
 template <class T>
 class CowData {
@@ -111,7 +112,7 @@ private:
 	void _unref(void *p_data);
 	void _ref(const CowData *p_from);
 	void _ref(const CowData &p_from);
-	void _copy_on_write();
+	uint32_t _copy_on_write();
 
 public:
 	void operator=(const CowData<T> &p_from) { _ref(p_from); }
@@ -217,20 +218,21 @@ void CowData<T>::_unref(void *p_data) {
 }
 
 template <class T>
-void CowData<T>::_copy_on_write() {
+uint32_t CowData<T>::_copy_on_write() {
 
 	if (!_ptr)
-		return;
+		return 0;
 
 	SafeNumeric<uint32_t> *refc = _get_refcount();
 
-	if (unlikely(refc->get() > 1)) {
+	uint32_t rc = refc->get();
+	if (likely(rc > 1)) {
 		/* in use by more than me */
 		uint32_t current_size = *_get_size();
 
 		uint32_t *mem_new = (uint32_t *)Memory::alloc_static(_get_alloc_size(current_size), true);
 
-		reinterpret_cast<SafeNumeric<uint32_t> *>(mem_new - 2)->set(1); //refcount
+		new (mem_new - 2, sizeof(uint32_t), "") SafeNumeric<uint32_t>(1); //refcount
 		*(mem_new - 1) = current_size; //size
 
 		T *_data = (T *)(mem_new);
@@ -247,7 +249,10 @@ void CowData<T>::_copy_on_write() {
 
 		_unref(_ptr);
 		_ptr = _data;
+
+		rc = 1;
 	}
+	return rc;
 }
 
 template <class T>
@@ -268,7 +273,7 @@ Error CowData<T>::resize(int p_size) {
 	}
 
 	// possibly changing size, copy on write
-	_copy_on_write();
+	uint32_t rc = _copy_on_write();
 
 	size_t current_alloc_size = _get_alloc_size(current_size);
 	size_t alloc_size;
@@ -282,13 +287,15 @@ Error CowData<T>::resize(int p_size) {
 				uint32_t *ptr = (uint32_t *)Memory::alloc_static(alloc_size, true);
 				ERR_FAIL_COND_V(!ptr, ERR_OUT_OF_MEMORY);
 				*(ptr - 1) = 0; //size, currently none
-				reinterpret_cast<SafeNumeric<uint32_t> *>(ptr - 2)->set(1); //refcount
+				new (ptr - 2, sizeof(uint32_t), "") SafeNumeric<uint32_t>(1); //refcount
 
 				_ptr = (T *)ptr;
 
 			} else {
-				void *_ptrnew = (T *)Memory::realloc_static(_ptr, alloc_size, true);
+				uint32_t *_ptrnew = (uint32_t *)Memory::realloc_static(_ptr, alloc_size, true);
 				ERR_FAIL_COND_V(!_ptrnew, ERR_OUT_OF_MEMORY);
+				new (_ptrnew - 2, sizeof(uint32_t), "") SafeNumeric<uint32_t>(rc); //refcount
+
 				_ptr = (T *)(_ptrnew);
 			}
 		}
@@ -316,8 +323,9 @@ Error CowData<T>::resize(int p_size) {
 		}
 
 		if (alloc_size != current_alloc_size) {
-			void *_ptrnew = (T *)Memory::realloc_static(_ptr, alloc_size, true);
+			uint32_t *_ptrnew = (uint32_t *)Memory::realloc_static(_ptr, alloc_size, true);
 			ERR_FAIL_COND_V(!_ptrnew, ERR_OUT_OF_MEMORY);
+			new (_ptrnew - 2, sizeof(uint32_t), "") SafeNumeric<uint32_t>(rc); //refcount
 
 			_ptr = (T *)(_ptrnew);
 		}
@@ -363,7 +371,7 @@ void CowData<T>::_ref(const CowData &p_from) {
 	if (!p_from._ptr)
 		return; //nothing to do
 
-	if (p_from._get_refcount()->increment() > 0) { // could reference
+	if (p_from._get_refcount()->conditional_increment() > 0) { // could reference
 		_ptr = p_from._ptr;
 	}
 }

core/safe_refcount.cpp

@@ -0,0 +1,45 @@
+/*************************************************************************/
+/*  safe_refcount.cpp                                                    */
+/*************************************************************************/
+/*                       This file is part of:                           */
+/*                           GODOT ENGINE                                */
+/*                      https://godotengine.org                          */
+/*************************************************************************/
+/* Copyright (c) 2007-2021 Juan Linietsky, Ariel Manzur.                 */
+/* Copyright (c) 2014-2021 Godot Engine contributors (cf. AUTHORS.md).   */
+/*                                                                       */
+/* Permission is hereby granted, free of charge, to any person obtaining */
+/* a copy of this software and associated documentation files (the       */
+/* "Software"), to deal in the Software without restriction, including   */
+/* without limitation the rights to use, copy, modify, merge, publish,   */
+/* distribute, sublicense, and/or sell copies of the Software, and to    */
+/* permit persons to whom the Software is furnished to do so, subject to */
+/* the following conditions:                                             */
+/*                                                                       */
+/* The above copyright notice and this permission notice shall be        */
+/* included in all copies or substantial portions of the Software.       */
+/*                                                                       */
+/* THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,       */
+/* EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF    */
+/* MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.*/
+/* IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY  */
+/* CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT,  */
+/* TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE     */
+/* SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE.                */
+/*************************************************************************/
+
+#if defined(DEBUG_ENABLED) && !defined(NO_THREADS)
+
+#include "safe_refcount.h"
+
+#include "core/error_macros.h"
+
+// On C++14 we don't have std::atomic::is_always_lockfree, so this is the best we can do
+void check_lockless_atomics() {
+	// Doing the check for the types we actually care about
+	if (!std::atomic<uint32_t>{}.is_lock_free() || !std::atomic<uint64_t>{}.is_lock_free() || !std::atomic_bool{}.is_lock_free()) {
+		WARN_PRINT("Your compiler doesn't seem to support lockless atomics. Performance will be degraded. Please consider upgrading to a different or newer compiler.");
+	}
+}
+
+#endif

core/safe_refcount.h

@@ -47,6 +47,16 @@
 //   value and, as an important benefit, you can be sure the value is properly synchronized
 //   even with threads that are already running.
 
+// This is used in very specific areas of the engine where it's critical that these guarantees are held
+#define SAFE_NUMERIC_TYPE_PUN_GUARANTEES(m_type)                        \
+	static_assert(sizeof(SafeNumeric<m_type>) == sizeof(m_type), "");   \
+	static_assert(alignof(SafeNumeric<m_type>) == alignof(m_type), ""); \
+	static_assert(std::is_trivially_destructible<std::atomic<m_type> >::value, "");
+
+#if defined(DEBUG_ENABLED)
+void check_lockless_atomics();
+#endif
+
 template <class T>
 class SafeNumeric {
 	std::atomic<T> value;

main/main.cpp

@@ -346,6 +346,10 @@ void Main::print_help(const char *p_binary) {
  */
 
 Error Main::setup(const char *execpath, int argc, char *argv[], bool p_second_phase) {
+#if defined(DEBUG_ENABLED) && !defined(NO_THREADS)
+	check_lockless_atomics();
+#endif
+
 	RID_OwnerBase::init_rid();
 
 	OS::get_singleton()->initialize_core();