Merge pull request #4389 from c1728p9/assert_mutex_not_in_isr

sg- · web-flow · commit 027843a64b5b · 2017-06-03T10:16:20.000-05:00
Assert that mutexes and prints are not use in interrupt or critical context
diff --git a/TESTS/storage_abstraction/.mbedignore b/TESTS/storage_abstraction/.mbedignore
@@ -0,0 +1 @@
+*
diff --git a/features/FEATURE_COMMON_PAL/nanostack-hal-mbed-cmsis-rtos/arm_hal_interrupt.c b/features/FEATURE_COMMON_PAL/nanostack-hal-mbed-cmsis-rtos/arm_hal_interrupt.c
@@ -13,7 +13,7 @@ static uint8_t sys_irq_disable_counter;
 static mbed_rtos_storage_mutex_t critical_mutex;
 static const osMutexAttr_t critical_mutex_attr = {
   .name = "nanostack_critical_mutex",
-  .attr_bits = osMutexRecursive,
+  .attr_bits = osMutexRecursive | osMutexPrioInherit | osMutexRobust,
   .cb_mem = &critical_mutex,
   .cb_size = sizeof critical_mutex,
 };
diff --git a/features/FEATURE_COMMON_PAL/nanostack-hal-mbed-cmsis-rtos/ns_event_loop.c b/features/FEATURE_COMMON_PAL/nanostack-hal-mbed-cmsis-rtos/ns_event_loop.c
@@ -30,7 +30,7 @@ static osThreadId_t event_thread_id;
 static mbed_rtos_storage_mutex_t event_mutex;
 static const osMutexAttr_t event_mutex_attr = {
   .name = "nanostack_event_mutex",
-  .attr_bits = osMutexRecursive,
+  .attr_bits = osMutexRecursive | osMutexPrioInherit | osMutexRobust,
   .cb_mem = &event_mutex,
   .cb_size = sizeof event_mutex,
 };
diff --git a/platform/mbed_critical.c b/platform/mbed_critical.c
@@ -37,6 +37,22 @@ bool core_util_are_interrupts_enabled(void)
 #endif
 }
 
+bool core_util_is_isr_active(void)
+{
+#if defined(__CORTEX_A9)
+    switch(__get_CPSR() & 0x1FU) {
+        case MODE_USR:
+        case MODE_SYS:
+            return false;
+        case MODE_SVC:
+        default:
+            return true;
+    }
+#else
+    return (__get_IPSR() != 0U);
+#endif
+}
+
 MBED_WEAK void core_util_critical_section_enter(void)
 {
     bool interrupts_disabled = !core_util_are_interrupts_enabled();
diff --git a/platform/mbed_critical.h b/platform/mbed_critical.h
@@ -41,6 +41,17 @@ extern "C" {
   */
 bool core_util_are_interrupts_enabled(void);
 
+/** Determine if this code is executing from an interrupt
+  *
+  * This function can be called to determine if the code is running on interrupt context.
+  * @note
+  * NOTE:
+  * This function works for both cortex-A and cortex-M, although the underlyng implementation
+  * differs.
+  * @return true if in an isr, false otherwise
+  */
+bool core_util_is_isr_active(void);
+
 /** Mark the start of a critical section
   *
   * This function should be called to mark the start of a critical section of code.
diff --git a/platform/mbed_error.c b/platform/mbed_error.c
@@ -23,7 +23,16 @@
 #include <stdio.h>
 #endif
 
+static uint8_t error_in_progress = 0;
+
 WEAK void error(const char* format, ...) {
+
+    // Prevent recursion if error is called again
+    if (error_in_progress) {
+        return;
+    }
+    error_in_progress = 1;
+
 #ifndef NDEBUG
     va_list arg;
     va_start(arg, format);
diff --git a/platform/mbed_retarget.cpp b/platform/mbed_retarget.cpp
@@ -23,6 +23,7 @@
 #include "platform/PlatformMutex.h"
 #include "platform/mbed_error.h"
 #include "platform/mbed_stats.h"
+#include "platform/mbed_critical.h"
 #if MBED_CONF_FILESYSTEM_PRESENT
 #include "filesystem/FileSystem.h"
 #include "filesystem/File.h"
@@ -309,6 +310,12 @@ extern "C" int PREFIX(_write)(FILEHANDLE fh, const unsigned char *buffer, unsign
 #endif
     int n; // n is the number of bytes written
 
+#if defined(MBED_TRAP_ERRORS_ENABLED) && MBED_TRAP_ERRORS_ENABLED
+    if (core_util_is_isr_active() || !core_util_are_interrupts_enabled()) {
+        error("Error - writing to a file in an ISR or critical section\r\n");
+    }
+#endif
+
     if (fh < 3) {
 #if DEVICE_SERIAL
         if (!stdio_uart_inited) init_serial();
@@ -353,6 +360,12 @@ extern "C" int PREFIX(_read)(FILEHANDLE fh, unsigned char *buffer, unsigned int
 #endif
     int n; // n is the number of bytes read
 
+#if defined(MBED_TRAP_ERRORS_ENABLED) && MBED_TRAP_ERRORS_ENABLED
+    if (core_util_is_isr_active() || !core_util_are_interrupts_enabled()) {
+        error("Error - reading from a file in an ISR or critical section\r\n");
+    }
+#endif
+
     if (fh < 3) {
         // only read a character at a time from stdin
 #if DEVICE_SERIAL
@@ -1033,3 +1046,67 @@ void operator delete[](void *ptr)
         free(ptr);
     }
 }
+
+#if defined(MBED_CONF_RTOS_PRESENT) && defined(MBED_TRAP_ERRORS_ENABLED) && MBED_TRAP_ERRORS_ENABLED
+
+static const char* error_msg(int32_t status)
+{
+    switch (status) {
+    case osError:
+        return "Unspecified RTOS error";
+    case osErrorTimeout:
+        return "Operation not completed within the timeout period";
+    case osErrorResource:
+        return "Resource not available";
+    case osErrorParameter:
+        return "Parameter error";
+    case osErrorNoMemory:
+        return "System is out of memory";
+    case osErrorISR:
+        return "Not allowed in ISR context";
+    default:
+        return "Unknown";
+    }
+}
+
+extern "C" void EvrRtxKernelError (int32_t status)
+{
+    error("Kernel error %i: %s\r\n", status, error_msg(status));
+}
+
+extern "C" void EvrRtxThreadError (osThreadId_t thread_id, int32_t status)
+{
+    error("Thread %p error %i: %s\r\n", thread_id, status, error_msg(status));
+}
+
+extern "C" void EvrRtxTimerError (osTimerId_t timer_id, int32_t status)
+{
+    error("Timer %p error %i: %s\r\n", timer_id, status, error_msg(status));
+}
+
+extern "C" void EvrRtxEventFlagsError (osEventFlagsId_t ef_id, int32_t status)
+{
+    error("Event %p error %i: %s\r\n", ef_id, status, error_msg(status));
+}
+
+extern "C" void EvrRtxMutexError (osMutexId_t mutex_id, int32_t status)
+{
+    error("Mutex %p error %i: %s\r\n", mutex_id, status, error_msg(status));
+}
+
+extern "C" void EvrRtxSemaphoreError (osSemaphoreId_t semaphore_id, int32_t status)
+{
+    error("Semaphore %p error %i\r\n", semaphore_id, status);
+}
+
+extern "C" void EvrRtxMemoryPoolError (osMemoryPoolId_t mp_id, int32_t status)
+{
+    error("Memory Pool %p error %i\r\n", mp_id, status);
+}
+
+extern "C" void EvrRtxMessageQueueError (osMessageQueueId_t mq_id, int32_t status)
+{
+    error("Message Queue %p error %i\r\n", mq_id, status);
+}
+
+#endif
diff --git a/rtos/Mutex.cpp b/rtos/Mutex.cpp
@@ -44,7 +44,7 @@ void Mutex::constructor(const char *name)
     _attr.name = name ? name : "aplication_unnamed_mutex";
     _attr.cb_mem = &_obj_mem;
     _attr.cb_size = sizeof(_obj_mem);
-    _attr.attr_bits = osMutexRecursive;
+    _attr.attr_bits = osMutexRecursive | osMutexPrioInherit | osMutexRobust;
     _id = osMutexNew(&_attr);
     MBED_ASSERT(_id);
 }
diff --git a/rtos/Thread.cpp b/rtos/Thread.cpp
@@ -103,7 +103,7 @@ osStatus Thread::start(Callback<void()> task) {
 }
 
 osStatus Thread::terminate() {
-    osStatus_t ret;
+    osStatus_t ret = osOK;
     _mutex.lock();
 
     // Set the Thread's tid to NULL and
@@ -112,11 +112,11 @@ osStatus Thread::terminate() {
     osThreadId_t local_id = _tid;
     _join_sem.release();
     _tid = (osThreadId_t)NULL;
-    _finished = true;
+    if (!_finished) {
+        _finished = true;
+        ret = osThreadTerminate(local_id);
+    }
     _mutex.unlock();
-
-    ret = osThreadTerminate(local_id);
-
     return ret;
 }
 
@@ -352,8 +352,8 @@ void Thread::_thunk(void * thread_ptr)
     t->_mutex.lock();
     t->_tid = (osThreadId)NULL;
     t->_finished = true;
-    t->_mutex.unlock();
     t->_join_sem.release();
+    // rtos will release the mutex automatically
 }
 
 }
diff --git a/rtos/mbed_boot.c b/rtos/mbed_boot.c
@@ -363,7 +363,7 @@ void $Sub$$__cpp_initialize__aeabi_(void)
 void pre_main()
 {
     singleton_mutex_attr.name = "singleton_mutex";
-    singleton_mutex_attr.attr_bits = osMutexRecursive;
+    singleton_mutex_attr.attr_bits = osMutexRecursive | osMutexPrioInherit | osMutexRobust;
     singleton_mutex_attr.cb_size = sizeof(singleton_mutex_obj);
     singleton_mutex_attr.cb_mem = &singleton_mutex_obj;
     singleton_mutex_id = osMutexNew(&singleton_mutex_attr);
@@ -383,7 +383,7 @@ extern int main(int argc, char* argv[]);
 void pre_main (void)
 {
     singleton_mutex_attr.name = "singleton_mutex";
-    singleton_mutex_attr.attr_bits = osMutexRecursive;
+    singleton_mutex_attr.attr_bits = osMutexRecursive | osMutexPrioInherit | osMutexRobust;
     singleton_mutex_attr.cb_size = sizeof(singleton_mutex_obj);
     singleton_mutex_attr.cb_mem = &singleton_mutex_obj;
     singleton_mutex_id = osMutexNew(&singleton_mutex_attr);
@@ -440,19 +440,19 @@ int __wrap_main(void) {
 void pre_main(void)
 {
     singleton_mutex_attr.name = "singleton_mutex";
-    singleton_mutex_attr.attr_bits = osMutexRecursive;
+    singleton_mutex_attr.attr_bits = osMutexRecursive | osMutexPrioInherit | osMutexRobust;
     singleton_mutex_attr.cb_size = sizeof(singleton_mutex_obj);
     singleton_mutex_attr.cb_mem = &singleton_mutex_obj;
     singleton_mutex_id = osMutexNew(&singleton_mutex_attr);
 
     malloc_mutex_attr.name = "malloc_mutex";
-    malloc_mutex_attr.attr_bits = osMutexRecursive;
+    malloc_mutex_attr.attr_bits = osMutexRecursive | osMutexPrioInherit | osMutexRobust;
     malloc_mutex_attr.cb_size = sizeof(malloc_mutex_obj);
     malloc_mutex_attr.cb_mem = &malloc_mutex_obj;
     malloc_mutex_id = osMutexNew(&malloc_mutex_attr);
 
     env_mutex_attr.name = "env_mutex";
-    env_mutex_attr.attr_bits = osMutexRecursive;
+    env_mutex_attr.attr_bits = osMutexRecursive | osMutexPrioInherit | osMutexRobust;
     env_mutex_attr.cb_size = sizeof(env_mutex_obj);
     env_mutex_attr.cb_mem = &env_mutex_obj;
     env_mutex_id = osMutexNew(&env_mutex_attr);
@@ -526,7 +526,7 @@ static uint8_t low_level_init_needed;
 void pre_main(void)
 {
     singleton_mutex_attr.name = "singleton_mutex";
-    singleton_mutex_attr.attr_bits = osMutexRecursive;
+    singleton_mutex_attr.attr_bits = osMutexRecursive | osMutexPrioInherit | osMutexRobust;
     singleton_mutex_attr.cb_size = sizeof(singleton_mutex_obj);
     singleton_mutex_attr.cb_mem = &singleton_mutex_obj;
     singleton_mutex_id = osMutexNew(&singleton_mutex_attr);
@@ -583,7 +583,7 @@ void __iar_system_Mtxinit(__iar_Rmtx *mutex) /* Initialize a system lock */
             attr.name = "system_mutex";
             attr.cb_mem = &std_mutex_sys[index];
             attr.cb_size = sizeof(std_mutex_sys[index]);
-            attr.attr_bits = osMutexRecursive;
+            attr.attr_bits = osMutexRecursive | osMutexPrioInherit | osMutexRobust;
             std_mutex_id_sys[index] = osMutexNew(&attr);
             *mutex = (__iar_Rmtx*)&std_mutex_id_sys[index];
             return;
@@ -619,7 +619,7 @@ void __iar_file_Mtxinit(__iar_Rmtx *mutex) /* Initialize a file lock */
             attr.name = "file_mutex";
             attr.cb_mem = &std_mutex_file[index];
             attr.cb_size = sizeof(std_mutex_file[index]);
-            attr.attr_bits = osMutexRecursive;
+            attr.attr_bits = osMutexRecursive | osMutexPrioInherit | osMutexRobust;
             std_mutex_id_file[index] = osMutexNew(&attr);
             *mutex = (__iar_Rmtx*)&std_mutex_id_file[index];
             return;
diff --git a/rtos/mbed_rtos_storage.h b/rtos/mbed_rtos_storage.h
@@ -22,6 +22,10 @@
 #ifndef MBED_RTOS_STORAGE_H
 #define MBED_RTOS_STORAGE_H
 
+#ifdef __cplusplus
+extern "C" {
+#endif
+
 /** \addtogroup rtos */
 /** @{*/
 
@@ -47,6 +51,10 @@ typedef os_event_flags_t mbed_rtos_storage_event_flags_t;
 typedef os_message_t mbed_rtos_storage_message_t;
 typedef os_timer_t mbed_rtos_storage_timer_t;
 
+#ifdef __cplusplus
+}
+#endif
+
 #endif
 
 /** @}*/
diff --git a/tools/profiles/debug.json b/tools/profiles/debug.json
@@ -5,7 +5,8 @@
                    "-fmessage-length=0", "-fno-exceptions", "-fno-builtin",
                    "-ffunction-sections", "-fdata-sections", "-funsigned-char",
                    "-MMD", "-fno-delete-null-pointer-checks",
-                   "-fomit-frame-pointer", "-O0", "-g3", "-DMBED_DEBUG"],
+                   "-fomit-frame-pointer", "-O0", "-g3", "-DMBED_DEBUG",
+                   "-DMBED_TRAP_ERRORS_ENABLED=1"],
         "asm": ["-x", "assembler-with-cpp"],
         "c": ["-std=gnu99"],
         "cxx": ["-std=gnu++98", "-fno-rtti", "-Wvla"],
@@ -17,7 +18,8 @@
     "ARM": {
         "common": ["-c", "--gnu", "-Otime", "--split_sections",
                    "--apcs=interwork", "--brief_diagnostics", "--restrict",
-                   "--multibyte_chars", "-O0", "-g", "-DMBED_DEBUG"],
+                   "--multibyte_chars", "-O0", "-g", "-DMBED_DEBUG",
+                   "-DMBED_TRAP_ERRORS_ENABLED=1"],
         "asm": [],
         "c": ["--md", "--no_depend_system_headers", "--c99", "-D__ASSERT_MSG"],
         "cxx": ["--cpp", "--no_rtti", "--no_vla"],
@@ -27,7 +29,8 @@
         "common": ["-c", "--gnu", "-Otime", "--split_sections",
                    "--apcs=interwork", "--brief_diagnostics", "--restrict",
                    "--multibyte_chars", "-O0", "-D__MICROLIB", "-g",
-                   "--library_type=microlib", "-DMBED_RTOS_SINGLE_THREAD", "-DMBED_DEBUG"],
+                   "--library_type=microlib", "-DMBED_RTOS_SINGLE_THREAD", "-DMBED_DEBUG",
+                   "-DMBED_TRAP_ERRORS_ENABLED=1"],
         "asm": [],
         "c": ["--md", "--no_depend_system_headers", "--c99", "-D__ASSERT_MSG"],
         "cxx": ["--cpp", "--no_rtti", "--no_vla"],
@@ -36,7 +39,8 @@
     "IAR": {
         "common": [
             "--no_wrap_diagnostics",  "-e",
-            "--diag_suppress=Pa050,Pa084,Pa093,Pa082", "-On", "-r", "-DMBED_DEBUG"],
+            "--diag_suppress=Pa050,Pa084,Pa093,Pa082", "-On", "-r", "-DMBED_DEBUG",
+            "-DMBED_TRAP_ERRORS_ENABLED=1"],
         "asm": [],
         "c": ["--vla"],
         "cxx": ["--guard_calls", "--no_static_destruction"],

Original file line number	Diff line number	Diff line change
`@@ -44,7 +44,7 @@ void Mutex::constructor(const char *name)`
`44`	`44`	`_attr.name = name ? name : "aplication_unnamed_mutex";`
`45`	`45`	`_attr.cb_mem = &_obj_mem;`
`46`	`46`	`_attr.cb_size = sizeof(_obj_mem);`
`47`		`- _attr.attr_bits = osMutexRecursive;`
	`47`	`+ _attr.attr_bits = osMutexRecursive \| osMutexPrioInherit \| osMutexRobust;`
`48`	`48`	`_id = osMutexNew(&_attr);`
`49`	`49`	`MBED_ASSERT(_id);`
`50`	`50`	`}`