Restore configurability of abnormal termination (#13784)

* Restore configurability of abnormal termination #### Problem `CodeUtils.h` builds on `nlassert.h` and contains definitions documented as configuring it, but due to an early bulk rename they don't actually apply. #### Change overview - Rename `CHIP_ASSERT_ABORT` and `CHIP_ASSERT_LOG` back to the original `NL_` forms so that they affect `nlassert.h`. - Rename `ChipDie()` to `CHIP_CONFIG_ABORT()` to avoid confusion with `chipDie()`. This was defined in most `CHIPPlatformConfig.h` files but never actually referenced; now, use it as the lowest-level abnormal termination operation. - Replace direct calls to `abort()` with suitable CodeUtils calls. #### Testing CI, plus local verification (chip-tool+m5stack) with an alternate `CHIP_CONFIG_ABORT()`. * restyle * split chipDie to reduce size while preserving current logging
project-chip · Jan 26, 2024 · 82e6ca1 · 82e6ca1
1 parent bd690ee
commit 82e6ca1
Show file tree

Hide file tree

Showing 23 changed files with 73 additions and 105 deletions.
diff --git a/examples/minimal-mdns/client.cpp b/examples/minimal-mdns/client.cpp
@@ -257,12 +257,7 @@ class QuerySplitter
 void BroadcastPacket(mdns::Minimal::ServerBase * server)
 {
     System::PacketBufferHandle buffer = System::PacketBufferHandle::New(kMdnsMaxPacketSize);
-    if (buffer.IsNull())
-    {
-        printf("Buffer allocation failure.");
-        abort();
-        return;
-    }
+    VerifyOrDie(!buffer.IsNull());
 
     QuerySplitter query;
     query.Split(gOptions.query);

diff --git a/src/app/tests/suites/pics/PICSBooleanExpressionParser.cpp b/src/app/tests/suites/pics/PICSBooleanExpressionParser.cpp
@@ -124,7 +124,7 @@ bool PICSBooleanExpressionParser::EvaluateExpression(std::vector<std::string> &
     else
     {
         ChipLogError(chipTool, "Unknown token: '%s'", token.c_str());
-        abort();
+        chipDie();
     }
 }
 
@@ -139,7 +139,7 @@ bool PICSBooleanExpressionParser::EvaluateSubExpression(std::vector<std::string>
         if (tokens[index] != ")")
         {
             ChipLogError(chipTool, "Missing ')'");
-            abort();
+            chipDie();
         }
 
         index++;

diff --git a/src/app/tests/suites/pics/PICSBooleanReader.cpp b/src/app/tests/suites/pics/PICSBooleanReader.cpp
@@ -25,11 +25,7 @@
 std::map<std::string, bool> PICSBooleanReader::Read(std::string filepath)
 {
     std::ifstream f(filepath);
-    if (!f.is_open())
-    {
-        ChipLogError(chipTool, "Error reading: %s", filepath.c_str());
-        abort();
-    }
+    VerifyOrDieWithMsg(f.is_open(), chipTool, "Error reading: %s", filepath.c_str());
 
     std::map<std::string, bool> PICS;
     std::string line;
@@ -46,11 +42,7 @@ std::map<std::string, bool> PICSBooleanReader::Read(std::string filepath)
         std::stringstream ss(line);
 
         std::getline(ss, key, '=');
-        if (key.empty())
-        {
-            ChipLogError(chipTool, "Missing PICS key at line %u", lineNumber + 1);
-            abort();
-        }
+        VerifyOrDieWithMsg(!key.empty(), chipTool, "Missing PICS key at line %u", lineNumber + 1);
 
         std::getline(ss, value);
         if (value == "0")
@@ -64,7 +56,7 @@ std::map<std::string, bool> PICSBooleanReader::Read(std::string filepath)
         else
         {
             ChipLogError(chipTool, "%s: PICS value should be either '0' or '1', got '%s'", key.c_str(), value.c_str());
-            abort();
+            chipDie();
         }
 
         lineNumber++;

diff --git a/src/controller/CHIPDeviceControllerSystemState.h b/src/controller/CHIPDeviceControllerSystemState.h
@@ -96,20 +96,14 @@ class DeviceControllerSystemState
 
     DeviceControllerSystemState * Retain()
     {
-        if (mRefCount == std::numeric_limits<uint32_t>::max())
-        {
-            abort();
-        }
+        VerifyOrDie(mRefCount < std::numeric_limits<uint32_t>::max());
         ++mRefCount;
         return this;
     };
 
     void Release()
     {
-        if (mRefCount == 0)
-        {
-            abort();
-        }
+        VerifyOrDie(mRefCount > 0);
 
         mRefCount--;
         if (mRefCount == 1)

diff --git a/src/lib/core/ReferenceCounted.h b/src/lib/core/ReferenceCounted.h
@@ -28,6 +28,7 @@
 #include <stdlib.h>
 
 #include <lib/support/CHIPMem.h>
+#include <lib/support/CodeUtils.h>
 
 namespace chip {
 
@@ -51,14 +52,8 @@ class ReferenceCounted
     /** Adds one to the usage count of this class */
     Subclass * Retain()
     {
-        if (kInitRefCount && mRefCount == 0)
-        {
-            abort();
-        }
-        if (mRefCount == std::numeric_limits<count_type>::max())
-        {
-            abort();
-        }
+        VerifyOrDie(!kInitRefCount || mRefCount > 0);
+        VerifyOrDie(mRefCount < std::numeric_limits<count_type>::max());
         ++mRefCount;
 
         return static_cast<Subclass *>(this);
@@ -67,10 +62,7 @@ class ReferenceCounted
     /** Release usage of this class */
     void Release()
     {
-        if (mRefCount == 0)
-        {
-            abort();
-        }
+        VerifyOrDie(mRefCount != 0);
 
         if (--mRefCount == 0)
         {

diff --git a/src/lib/support/CHIPMem-Malloc.cpp b/src/lib/support/CHIPMem-Malloc.cpp
@@ -25,6 +25,7 @@
 
 #include <lib/core/CHIPConfig.h>
 #include <lib/support/CHIPMem.h>
+#include <lib/support/CodeUtils.h>
 
 #include <stdlib.h>
 
@@ -56,44 +57,28 @@ static std::atomic_int memoryInitialized{ 0 };
 
 static void VerifyInitialized(const char * func)
 {
-    if (!memoryInitialized)
-    {
-        fprintf(stderr, "ABORT: chip::Platform::%s() called before chip::Platform::MemoryInit()\n", func);
-        abort();
-    }
+    VerifyOrDieWithMsg(memoryInitialized, Support, "ABORT: chip::Platform::%s() called before chip::Platform::MemoryInit()\n",
+                       func);
 }
 
 #define VERIFY_POINTER(p)                                                                                                          \
-    do                                                                                                                             \
-        if (((p) != nullptr) && (MemoryDebugCheckPointer((p)) == false))                                                           \
-        {                                                                                                                          \
-            fprintf(stderr, "ABORT: chip::Platform::%s() found corruption on %p\n", __func__, (p));                                \
-            abort();                                                                                                               \
-        }                                                                                                                          \
-    while (0)
+    VerifyOrDieWithMsg((p) == nullptr || MemoryDebugCheckPointer((p)), Support,                                                    \
+                       "ABORT: chip::Platform::%s() found corruption on %p\n", __func__, (p))
 
 #endif
 
 CHIP_ERROR MemoryAllocatorInit(void * buf, size_t bufSize)
 {
 #ifndef NDEBUG
-    if (memoryInitialized++ > 0)
-    {
-        fprintf(stderr, "ABORT: chip::Platform::MemoryInit() called twice.\n");
-        abort();
-    }
+    VerifyOrDieWithMsg(memoryInitialized++ == 0, Support, "ABORT: chip::Platform::MemoryInit() called twice.\n");
 #endif
     return CHIP_NO_ERROR;
 }
 
 void MemoryAllocatorShutdown()
 {
 #ifndef NDEBUG
-    if (--memoryInitialized < 0)
-    {
-        fprintf(stderr, "ABORT: chip::Platform::MemoryShutdown() called twice.\n");
-        abort();
-    }
+    VerifyOrDieWithMsg(--memoryInitialized == 0, Support, "ABORT: chip::Platform::MemoryShutdown() called twice.\n");
 #endif
 #if CHIP_CONFIG_MEMORY_DEBUG_DMALLOC
     dmalloc_shutdown();

diff --git a/src/lib/support/CodeUtils.h b/src/lib/support/CodeUtils.h
@@ -26,13 +26,24 @@
 
 #pragma once
 
-#ifdef __cplusplus
-
 #include <lib/core/CHIPConfig.h>
 #include <lib/core/CHIPError.h>
 #include <lib/support/ErrorStr.h>
 #include <lib/support/logging/CHIPLogging.h>
 
+/**
+ * Base-level abnormal termination.
+ *
+ * Terminate the program immediately, without invoking destructors, atexit callbacks, etc.
+ * Used to implement the default `chipDie()`.
+ *
+ * @note
+ *  This should never be invoked directly by code outside this file.
+ */
+#if !defined(CHIP_CONFIG_ABORT)
+#define CHIP_CONFIG_ABORT() abort()
+#endif
+
 /**
  *  @name chip-specific nlassert.h Overrides
  *
@@ -41,34 +52,34 @@
  */
 
 /**
- *  @def CHIP_ASSERT_ABORT()
+ *  @def NL_ASSERT_ABORT()
  *
  *  @brief
- *    This implements a chip-specific override for #CHIP_ASSERT_ABORT *
+ *    This implements a chip-specific override for #NL_ASSERT_ABORT *
  *    from nlassert.h.
  *
  */
-#if !defined(CHIP_ASSERT_ABORT)
-#define CHIP_ASSERT_ABORT() chipDie()
+#if !defined(NL_ASSERT_ABORT)
+#define NL_ASSERT_ABORT() chipAbort()
 #endif
 
 /**
- *  @def CHIP_ASSERT_LOG(aPrefix, aName, aCondition, aLabel, aFile, aLine, aMessage)
+ *  @def NL_ASSERT_LOG(aPrefix, aName, aCondition, aLabel, aFile, aLine, aMessage)
  *
  *  @brief
- *    This implements a chip-specific override for \c CHIP_ASSERT_LOG
+ *    This implements a chip-specific override for \c NL_ASSERT_LOG
  *    from nlassert.h.
  *
  *  @param[in]  aPrefix     A pointer to a NULL-terminated C string printed
  *                          at the beginning of the logged assertion
  *                          message. Typically this is and should be
- *                          \c CHIP_ASSERT_PREFIX_STRING.
+ *                          \c NL_ASSERT_PREFIX_STRING.
  *  @param[in]  aName       A pointer to a NULL-terminated C string printed
  *                          following @a aPrefix that indicates what
  *                          module, program, application or subsystem
  *                          the assertion occurred in Typically this
  *                          is and should be
- *                          \c CHIP_ASSERT_COMPONENT_STRING.
+ *                          \c NL_ASSERT_COMPONENT_STRING.
  *  @param[in]  aCondition  A pointer to a NULL-terminated C string indicating
  *                          the expression that evaluated to false in
  *                          the assertion. Typically this is a
@@ -93,12 +104,12 @@
  *
  */
 // clang-format off
-#if !defined(CHIP_ASSERT_LOG)
-#define CHIP_ASSERT_LOG(aPrefix, aName, aCondition, aLabel, aFile, aLine, aMessage)         \
+#if !defined(NL_ASSERT_LOG)
+#define NL_ASSERT_LOG(aPrefix, aName, aCondition, aLabel, aFile, aLine, aMessage)         \
     do                                                                                    \
     {                                                                                     \
         ChipLogError(NotSpecified,                                                       \
-                      CHIP_ASSERT_LOG_FORMAT_DEFAULT,                                       \
+                      NL_ASSERT_LOG_FORMAT_DEFAULT,                                       \
                       aPrefix,                                                            \
                       (((aName) == 0) || (*(aName) == '\0')) ? "" : aName,                \
                       (((aName) == 0) || (*(aName) == '\0')) ? "" : ": ",                 \
@@ -455,18 +466,25 @@ constexpr inline const _T & max(const _T & a, const _T & b)
  *  @endcode
  *
  */
+#ifndef chipAbort
+extern "C" void chipAbort(void) __attribute((noreturn));
+
+inline void chipAbort(void)
+{
+    while (true)
+    {
+        // NL_ASSERT_ABORT is redefined to be chipAbort, so not useful here.
+        CHIP_CONFIG_ABORT();
+    }
+}
+#endif // chipAbort
 #ifndef chipDie
 extern "C" void chipDie(void) __attribute((noreturn));
 
 inline void chipDie(void)
 {
     ChipLogError(NotSpecified, "chipDie chipDie chipDie");
-
-    while (true)
-    {
-        // CHIP_ASSERT_ABORT is redefined to be chipDie, so not useful here.
-        abort();
-    }
+    chipAbort();
 }
 #endif // chipDie
 
@@ -631,8 +649,6 @@ inline void chipDie(void)
 #define FALLTHROUGH (void) 0
 #endif
 
-#endif // __cplusplus
-
 /**
  * @def ArraySize(aArray)
  *

diff --git a/src/platform/Ameba/CHIPPlatformConfig.h b/src/platform/Ameba/CHIPPlatformConfig.h
@@ -40,7 +40,7 @@
 #define CHIP_CONFIG_TIME_ENABLE_CLIENT 1
 #define CHIP_CONFIG_TIME_ENABLE_SERVER 0
 
-#define ChipDie() abort()
+#define CHIP_CONFIG_ABORT() abort()
 
 // ==================== Security Adaptations ====================
 

diff --git a/src/platform/Darwin/CHIPPlatformConfig.h b/src/platform/Darwin/CHIPPlatformConfig.h
@@ -25,7 +25,7 @@
 
 // ==================== General Platform Adaptations ====================
 
-#define ChipDie() abort()
+#define CHIP_CONFIG_ABORT() abort()
 
 // TODO:(#756) Add FabricState support
 #define CHIP_CONFIG_ENABLE_FABRIC_STATE 0

diff --git a/src/platform/EFR32/CHIPPlatformConfig.h b/src/platform/EFR32/CHIPPlatformConfig.h
@@ -28,7 +28,7 @@
 
 // ==================== General Platform Adaptations ====================
 
-#define ChipDie() abort()
+#define CHIP_CONFIG_ABORT() abort()
 
 #define CHIP_CONFIG_PERSISTED_STORAGE_KEY_TYPE uint8_t
 #define CHIP_CONFIG_PERSISTED_STORAGE_ENC_MSG_CNTR_ID 1

diff --git a/src/platform/ESP32/CHIPPlatformConfig.h b/src/platform/ESP32/CHIPPlatformConfig.h
@@ -45,7 +45,7 @@
 #define CHIP_CONFIG_TIME_ENABLE_CLIENT 1
 #define CHIP_CONFIG_TIME_ENABLE_SERVER 0
 
-#define ChipDie() abort()
+#define CHIP_CONFIG_ABORT() abort()
 
 // ==================== Security Adaptations ====================
 

diff --git a/src/platform/Linux/CHIPPlatformConfig.h b/src/platform/Linux/CHIPPlatformConfig.h
@@ -25,7 +25,7 @@
 
 // ==================== General Platform Adaptations ====================
 
-#define ChipDie() abort()
+#define CHIP_CONFIG_ABORT() abort()
 
 // TODO:(#756) Add FabricState support
 #define CHIP_CONFIG_ENABLE_FABRIC_STATE 0

diff --git a/src/platform/P6/CHIPPlatformConfig.h b/src/platform/P6/CHIPPlatformConfig.h
@@ -48,7 +48,7 @@
 #define CHIP_CONFIG_TIME_ENABLE_CLIENT 1
 #define CHIP_CONFIG_TIME_ENABLE_SERVER 0
 
-#define ChipDie() abort()
+#define CHIP_CONFIG_ABORT() abort()
 
 // ==================== Security Adaptations ====================
 

diff --git a/src/platform/Tizen/CHIPPlatformConfig.h b/src/platform/Tizen/CHIPPlatformConfig.h
@@ -26,7 +26,7 @@
 
 // ==================== General Platform Adaptations ====================
 
-#define ChipDie() abort()
+#define CHIP_CONFIG_ABORT() abort()
 
 #define CHIP_CONFIG_ENABLE_FABRIC_STATE 0
 

diff --git a/src/platform/android/CHIPPlatformConfig.h b/src/platform/android/CHIPPlatformConfig.h
@@ -25,7 +25,7 @@
 
 // ==================== General Platform Adaptations ====================
 
-#define ChipDie() abort()
+#define CHIP_CONFIG_ABORT() abort()
 
 // TODO:(#756) Add FabricState support
 #define CHIP_CONFIG_ENABLE_FABRIC_STATE 0

diff --git a/src/platform/cc13x2_26x2/CHIPPlatformConfig.h b/src/platform/cc13x2_26x2/CHIPPlatformConfig.h
@@ -30,7 +30,7 @@
 
 // ==================== General Platform Adaptations ====================
 
-#define ChipDie() assert()
+#define CHIP_CONFIG_ABORT() assert()
 
 #define CHIP_CONFIG_PERSISTED_STORAGE_KEY_TYPE uint16_t
 #define CHIP_CONFIG_PERSISTED_STORAGE_ENC_MSG_CNTR_ID 1