Merge pull request #8943 from RandomShaper/fix-error-handling

Implement well-defined handling of unrecoverable errors

core/dvector.h

@@ -409,14 +409,9 @@ public:
 		if (p_to < 0) {
 			p_to = size() + p_to;
 		}
-		if (p_from < 0 || p_from >= size()) {
+
-			PoolVector<T> &aux = *((PoolVector<T> *)0); // nullreturn
+		CRASH_BAD_INDEX(p_from, size());
-			ERR_FAIL_COND_V(p_from < 0 || p_from >= size(), aux)
+		CRASH_BAD_INDEX(p_to, size());
-		}
-		if (p_to < 0 || p_to >= size()) {
-			PoolVector<T> &aux = *((PoolVector<T> *)0); // nullreturn
-			ERR_FAIL_COND_V(p_to < 0 || p_to >= size(), aux)
-		}
 
 		PoolVector<T> slice;
 		int span = 1 + p_to - p_from;
@@ -506,13 +501,9 @@ void PoolVector<T>::push_back(const T &p_val) {
 template <class T>
 const T PoolVector<T>::operator[](int p_index) const {
 
-	if (p_index < 0 || p_index >= size()) {
+	CRASH_BAD_INDEX(p_index, size());
-		T &aux = *((T *)0); //nullreturn
-		ERR_FAIL_COND_V(p_index < 0 || p_index >= size(), aux);
-	}
 
 	Read r = read();
-
 	return r[p_index];
 }
 

core/error_macros.h

@@ -115,6 +115,19 @@ extern bool _err_error_exists;
 #define FUNCTION_STR __FUNCTION__
 #endif
 
+// Don't use this directly; instead, use any of the CRASH_* macros
+#ifdef _MSC_VER
+#define GENERATE_TRAP                       \
+	__debugbreak();                         \
+	/* Avoid warning about control paths */ \
+	for (;;) {                              \
+	}
+#else
+#define GENERATE_TRAP __builtin_trap();
+#endif
+
+// (*): See https://stackoverflow.com/questions/257418/do-while-0-what-is-it-good-for
+
 #define ERR_FAIL_INDEX(m_index, m_size)                                                                                    \
 	do {                                                                                                                   \
 		if ((m_index) < 0 || (m_index) >= (m_size)) {                                                                      \
@@ -122,7 +135,7 @@ extern bool _err_error_exists;
 			return;                                                                                                        \
 		} else                                                                                                             \
 			_err_error_exists = false;                                                                                     \
-	} while (0);
+	} while (0); // (*)
 
 /** An index has failed if m_index<0 or m_index >=m_size, the function exists.
 * This function returns an error value, if returning Error, please select the most
@@ -136,7 +149,18 @@ extern bool _err_error_exists;
 			return m_retval;                                                                                               \
 		} else                                                                                                             \
 			_err_error_exists = false;                                                                                     \
-	} while (0);
+	} while (0); // (*)
+
+/** Use this one if there is no sensible fallback, that is, the error is unrecoverable.
+*   We'll return a null reference and try to keep running.
+*/
+#define CRASH_BAD_INDEX(m_index, m_size)                                                                                          \
+	do {                                                                                                                          \
+		if ((m_index) < 0 || (m_index) >= (m_size)) {                                                                             \
+			_err_print_error(FUNCTION_STR, __FILE__, __LINE__, "FATAL: Index " _STR(m_index) " out of size (" _STR(m_size) ")."); \
+			GENERATE_TRAP                                                                                                         \
+		}                                                                                                                         \
+	} while (0); // (*)
 
 /** An error condition happened (m_cond tested true) (WARNING this is the opposite as assert().
   * the function will exit.
@@ -173,6 +197,17 @@ extern bool _err_error_exists;
 			_err_error_exists = false;                                                                     \
 	}
 
+/** Use this one if there is no sensible fallback, that is, the error is unrecoverable.
+ */
+
+#define CRASH_COND(m_cond)                                                                                        \
+	{                                                                                                             \
+		if (m_cond) {                                                                                             \
+			_err_print_error(FUNCTION_STR, __FILE__, __LINE__, "FATAL: Condition ' " _STR(m_cond) " ' is true."); \
+			GENERATE_TRAP                                                                                         \
+		}                                                                                                         \
+	}
+
 /** An error condition happened (m_cond tested true) (WARNING this is the opposite as assert().
  * the function will exit.
  * This function returns an error value, if returning Error, please select the most
@@ -234,6 +269,15 @@ extern bool _err_error_exists;
 		return m_value;                                                                                           \
 	}
 
+/** Use this one if there is no sensible fallback, that is, the error is unrecoverable.
+ */
+
+#define CRASH_NOW()                                                                           \
+	{                                                                                         \
+		_err_print_error(FUNCTION_STR, __FILE__, __LINE__, "FATAL: Method/Function Failed."); \
+		GENERATE_TRAP                                                                         \
+	}
+
 /** Print an error string.
  */
 

core/hash_map.h

@@ -473,8 +473,7 @@ public:
 		if (!e) {
 
 			e = create_entry(p_key);
-			if (!e)
+			CRASH_COND(!e);
-				return *(TData *)NULL; /* panic! */
 			check_hash_table(); // perform mantenience routine
 		}
 

core/list.h

@@ -398,10 +398,7 @@ public:
 
 	T &operator[](int p_index) {
 
-		if (p_index < 0 || p_index >= size()) {
+		CRASH_BAD_INDEX(p_index, size());
-			T &aux = *((T *)0); //nullreturn
-			ERR_FAIL_COND_V(p_index < 0 || p_index >= size(), aux);
-		}
 
 		Element *I = front();
 		int c = 0;
@@ -415,15 +412,12 @@ public:
 			c++;
 		}
 
-		ERR_FAIL_V(*((T *)0)); // bug!!
+		CRASH_NOW(); // bug!!
 	}
 
 	const T &operator[](int p_index) const {
 
-		if (p_index < 0 || p_index >= size()) {
+		CRASH_BAD_INDEX(p_index, size());
-			T &aux = *((T *)0); //nullreturn
-			ERR_FAIL_COND_V(p_index < 0 || p_index >= size(), aux);
-		}
 
 		const Element *I = front();
 		int c = 0;
@@ -437,7 +431,7 @@ public:
 			c++;
 		}
 
-		ERR_FAIL_V(*((T *)0)); // bug!
+		CRASH_NOW(); // bug!!
 	}
 
 	void move_to_back(Element *p_I) {

core/map.h

@@ -599,9 +599,9 @@ public:
 
 	const V &operator[](const K &p_key) const {
 
-		ERR_FAIL_COND_V(!_data._root, *(V *)NULL); // crash on purpose
+		CRASH_COND(!_data._root);
 		const Element *e = find(p_key);
-		ERR_FAIL_COND_V(!e, *(V *)NULL); // crash on purpose
+		CRASH_COND(!e);
 		return e->_value;
 	}
 	V &operator[](const K &p_key) {
@@ -613,7 +613,7 @@ public:
 		if (!e)
 			e = insert(p_key, V());
 
-		ERR_FAIL_COND_V(!e, *(V *)NULL); // crash on purpose
+		CRASH_COND(!e);
 		return e->_value;
 	}
 

core/object.h

@@ -533,6 +533,12 @@ public:
 	void add_change_receptor(Object *p_receptor);
 	void remove_change_receptor(Object *p_receptor);
 
+// TODO: ensure 'this' is never NULL since it's UB, but by now, avoid warning flood
+#ifdef __clang__
+#pragma clang diagnostic push
+#pragma clang diagnostic ignored "-Wundefined-bool-conversion"
+#endif
+
 	template <class T>
 	T *cast_to() {
 
@@ -563,6 +569,10 @@ public:
 #endif
 	}
 
+#ifdef __clang__
+#pragma clang diagnostic pop
+#endif
+
 	enum {
 
 		NOTIFICATION_POSTINITIALIZE = 0,

core/vector.h

@@ -134,10 +134,7 @@ public:
 
 	inline T &operator[](int p_index) {
 
-		if (p_index < 0 || p_index >= size()) {
+		CRASH_BAD_INDEX(p_index, size());
-			T &aux = *((T *)0); //nullreturn
-			ERR_FAIL_COND_V(p_index < 0 || p_index >= size(), aux);
-		}
 
 		_copy_on_write(); // wants to write, so copy on write.
 
@@ -146,10 +143,8 @@ public:
 
 	inline const T &operator[](int p_index) const {
 
-		if (p_index < 0 || p_index >= size()) {
+		CRASH_BAD_INDEX(p_index, size());
-			const T &aux = *((T *)0); //nullreturn
+
-			ERR_FAIL_COND_V(p_index < 0 || p_index >= size(), aux);
-		}
 		// no cow needed, since it's reading
 		return _get_data()[p_index];
 	}

core/vmap.h

@@ -180,10 +180,8 @@ public:
 	inline const V &operator[](const T &p_key) const {
 
 		int pos = _find_exact(p_key);
-		if (pos < 0) {
+
-			const T &aux = *((T *)0); //nullreturn
+		CRASH_COND(pos < 0);
-			ERR_FAIL_COND_V(pos < 1, aux);
-		}
 
 		return _data[pos].value;
 	}