From 211c4518903d82068c061943064824ac5595fd38 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pedro=20J=2E=20Est=C3=A9banez?= Date: Fri, 26 May 2017 21:11:16 +0200 Subject: [PATCH] Implement well-defined handling of unrecoverable errors Plus the addition of some convenience CRASH_* error macros. Plus transient avoidance of the flood of warnings emitted by Clang when checking 'this' for NULL. Plus explanation about the do-while(0) loop in some error macros. --- core/dvector.h | 17 ++++---------- core/error_macros.h | 54 ++++++++++++++++++++++++++++++++++++++++----- core/hash_map.h | 3 +-- core/list.h | 14 ++++-------- core/map.h | 6 ++--- core/object.h | 10 +++++++++ core/vector.h | 11 +++------ core/vmap.h | 6 ++--- 8 files changed, 76 insertions(+), 45 deletions(-) diff --git a/core/dvector.h b/core/dvector.h index 2e951b96612..96cbd2cdece 100644 --- a/core/dvector.h +++ b/core/dvector.h @@ -403,14 +403,9 @@ public: if (p_to < 0) { p_to = size() + p_to; } - if (p_from < 0 || p_from >= size()) { - PoolVector &aux = *((PoolVector *)0); // nullreturn - ERR_FAIL_COND_V(p_from < 0 || p_from >= size(), aux) - } - if (p_to < 0 || p_to >= size()) { - PoolVector &aux = *((PoolVector *)0); // nullreturn - ERR_FAIL_COND_V(p_to < 0 || p_to >= size(), aux) - } + + CRASH_BAD_INDEX(p_from, size()); + CRASH_BAD_INDEX(p_to, size()); PoolVector slice; int span = 1 + p_to - p_from; @@ -500,13 +495,9 @@ void PoolVector::push_back(const T &p_val) { template const T PoolVector::operator[](int p_index) const { - if (p_index < 0 || p_index >= size()) { - T &aux = *((T *)0); //nullreturn - ERR_FAIL_COND_V(p_index < 0 || p_index >= size(), aux); - } + CRASH_BAD_INDEX(p_index, size()); Read r = read(); - return r[p_index]; } diff --git a/core/error_macros.h b/core/error_macros.h index 00fced35863..6c803951a1a 100644 --- a/core/error_macros.h +++ b/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,12 +135,12 @@ 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 - * appropriate error condition from error_macros.h - */ +* This function returns an error value, if returning Error, please select the most +* appropriate error condition from error_macros.h +*/ #define ERR_FAIL_INDEX_V(m_index, m_size, m_retval) \ do { \ @@ -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. */ diff --git a/core/hash_map.h b/core/hash_map.h index 49701188ab3..45e7b82d243 100644 --- a/core/hash_map.h +++ b/core/hash_map.h @@ -473,8 +473,7 @@ public: if (!e) { e = create_entry(p_key); - if (!e) - return *(TData *)NULL; /* panic! */ + CRASH_COND(!e); check_hash_table(); // perform mantenience routine } diff --git a/core/list.h b/core/list.h index 4390cb65fcc..df69b1dc405 100644 --- a/core/list.h +++ b/core/list.h @@ -398,10 +398,7 @@ public: T &operator[](int p_index) { - if (p_index < 0 || p_index >= size()) { - T &aux = *((T *)0); //nullreturn - ERR_FAIL_COND_V(p_index < 0 || p_index >= size(), aux); - } + CRASH_BAD_INDEX(p_index, size()); 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()) { - T &aux = *((T *)0); //nullreturn - ERR_FAIL_COND_V(p_index < 0 || p_index >= size(), aux); - } + CRASH_BAD_INDEX(p_index, size()); 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) { diff --git a/core/map.h b/core/map.h index acf1d608d84..ef0f75fc9be 100644 --- a/core/map.h +++ b/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; } diff --git a/core/object.h b/core/object.h index 3b39224af0d..176f69fdb8f 100644 --- a/core/object.h +++ b/core/object.h @@ -531,6 +531,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 T *cast_to() { @@ -561,6 +567,10 @@ public: #endif } +#ifdef __clang__ +#pragma clang diagnostic pop +#endif + enum { NOTIFICATION_POSTINITIALIZE = 0, diff --git a/core/vector.h b/core/vector.h index fe1c1b05ddc..5eed8dce96e 100644 --- a/core/vector.h +++ b/core/vector.h @@ -134,10 +134,7 @@ public: inline T &operator[](int p_index) { - if (p_index < 0 || p_index >= size()) { - T &aux = *((T *)0); //nullreturn - ERR_FAIL_COND_V(p_index < 0 || p_index >= size(), aux); - } + CRASH_BAD_INDEX(p_index, size()); _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()) { - const T &aux = *((T *)0); //nullreturn - ERR_FAIL_COND_V(p_index < 0 || p_index >= size(), aux); - } + CRASH_BAD_INDEX(p_index, size()); + // no cow needed, since it's reading return _get_data()[p_index]; } diff --git a/core/vmap.h b/core/vmap.h index ad079733080..66f935f58d1 100644 --- a/core/vmap.h +++ b/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 - ERR_FAIL_COND_V(pos < 1, aux); - } + + CRASH_COND(pos < 0); return _data[pos].value; }