From 2ccc2ec5ee5449e992e2520525c1b4190cb1acb3 Mon Sep 17 00:00:00 2001 From: Angus Gratton Date: Wed, 4 Oct 2017 13:25:16 +1100 Subject: [PATCH 1/9] cxx tests: Fix race condition w/ leak checker when tearing down test tasks --- components/cxx/test/test_cxx.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/components/cxx/test/test_cxx.cpp b/components/cxx/test/test_cxx.cpp index 5d5ff1595..7e08e9017 100644 --- a/components/cxx/test/test_cxx.cpp +++ b/components/cxx/test/test_cxx.cpp @@ -92,7 +92,6 @@ public: static SlowInit slowinit(taskId); ESP_LOGD(TAG, "obj=%d after static init, task=%d\n", obj, taskId); xSemaphoreGive(s_slow_init_sem); - vTaskDelay(10); vTaskDelete(NULL); } private: @@ -133,6 +132,8 @@ TEST_CASE("static initialization guards work as expected", "[cxx]") TEST_ASSERT_TRUE(xSemaphoreTake(s_slow_init_sem, 500/portTICK_PERIOD_MS)); } vSemaphoreDelete(s_slow_init_sem); + + vTaskDelay(10); // Allow tasks to clean up, avoids race with leak detector } struct GlobalInitTest From 3234064b6ab0e8b6f7e45b74f112f2b2935543e5 Mon Sep 17 00:00:00 2001 From: Angus Gratton Date: Thu, 5 Oct 2017 12:12:19 +1100 Subject: [PATCH 2/9] freertos: Idle task shouldn't hold xTaskQueueMutex while calling TLS destructors If the callbacks use any blocking call (ie printf), this can otherwise trigger a deadlock. --- components/freertos/tasks.c | 28 ++++++++++++++-------------- 1 file changed, 14 insertions(+), 14 deletions(-) diff --git a/components/freertos/tasks.c b/components/freertos/tasks.c index 91ee39ad8..24373a91e 100644 --- a/components/freertos/tasks.c +++ b/components/freertos/tasks.c @@ -3577,17 +3577,16 @@ static void prvCheckTasksWaitingTermination( void ) /* ucTasksDeleted is used to prevent vTaskSuspendAll() being called too often in the idle task. */ - taskENTER_CRITICAL(&xTaskQueueMutex); - while( uxTasksDeleted > ( UBaseType_t ) 0U ) + while(uxTasksDeleted > ( UBaseType_t ) 0U ) { + TCB_t *pxTCB = NULL; + taskENTER_CRITICAL(&xTaskQueueMutex); { xListIsEmpty = listLIST_IS_EMPTY( &xTasksWaitingTermination ); } if( xListIsEmpty == pdFALSE ) { - TCB_t *pxTCB; - { pxTCB = ( TCB_t * ) listGET_OWNER_OF_HEAD_ENTRY( ( &xTasksWaitingTermination ) ); /* We only want to kill tasks that ran on this core because e.g. _xt_coproc_release needs to @@ -3598,30 +3597,31 @@ static void prvCheckTasksWaitingTermination( void ) --uxTasksDeleted; } else { /* Need to wait until the idle task on the other processor kills that task first. */ + taskEXIT_CRITICAL(&xTaskQueueMutex); break; } } + } + taskEXIT_CRITICAL(&xTaskQueueMutex); - #if ( configNUM_THREAD_LOCAL_STORAGE_POINTERS > 0 ) && ( configTHREAD_LOCAL_STORAGE_DELETE_CALLBACKS ) + if (pxTCB != NULL) { + #if ( configNUM_THREAD_LOCAL_STORAGE_POINTERS > 0 ) && ( configTHREAD_LOCAL_STORAGE_DELETE_CALLBACKS ) + int x; + for( x = 0; x < ( UBaseType_t ) configNUM_THREAD_LOCAL_STORAGE_POINTERS; x++ ) { - int x; - for( x = 0; x < ( UBaseType_t ) configNUM_THREAD_LOCAL_STORAGE_POINTERS; x++ ) + if (pxTCB->pvThreadLocalStoragePointersDelCallback[ x ] != NULL) { - if (pxTCB->pvThreadLocalStoragePointersDelCallback[ x ] != NULL) - { - pxTCB->pvThreadLocalStoragePointersDelCallback[ x ](x, pxTCB->pvThreadLocalStoragePointers[ x ]); - } + pxTCB->pvThreadLocalStoragePointersDelCallback[ x ](x, pxTCB->pvThreadLocalStoragePointers[ x ]); } } - #endif - prvDeleteTCB( pxTCB ); + #endif + prvDeleteTCB( pxTCB ); } else { mtCOVERAGE_TEST_MARKER(); } } - taskEXIT_CRITICAL(&xTaskQueueMutex); } #endif /* vTaskDelete */ } From 86c89ff1694bac7471d8139c88ef2be52323ca61 Mon Sep 17 00:00:00 2001 From: Angus Gratton Date: Wed, 4 Oct 2017 15:52:19 +1100 Subject: [PATCH 3/9] pthread: Add support for pthread thread local storage Refactors LWIP to use this for the LWIP thread local semaphore --- components/freertos/Kconfig | 7 +- components/lwip/Kconfig | 7 - components/lwip/port/freertos/sys_arch.c | 41 ++-- components/pthread/pthread.c | 40 +--- components/pthread/pthread_internal.h | 16 ++ components/pthread/pthread_local_storage.c | 226 ++++++++++++++++++ .../pthread/test/test_pthread_local_storage.c | 107 +++++++++ docs/api-guides/freertos-smp.rst | 5 + 8 files changed, 385 insertions(+), 64 deletions(-) create mode 100644 components/pthread/pthread_internal.h create mode 100644 components/pthread/pthread_local_storage.c create mode 100644 components/pthread/test/test_pthread_local_storage.c diff --git a/components/freertos/Kconfig b/components/freertos/Kconfig index 21300bdac..44541a54a 100644 --- a/components/freertos/Kconfig +++ b/components/freertos/Kconfig @@ -114,11 +114,8 @@ config FREERTOS_THREAD_LOCAL_STORAGE_POINTERS FreeRTOS has the ability to store per-thread pointers in the task control block. This controls the number of pointers available. - Value 0 turns off this functionality. - - If using the LWIP TCP/IP stack (with WiFi or Ethernet), this value must be at least 1. See the - LWIP_THREAD_LOCAL_STORAGE_INDEX config item in LWIP configuration to determine which thread-local-storage - pointer is reserved for LWIP. + This value must be at least 1. Index 0 is reserved for use by the pthreads API + thread-local-storage. Other indexes can be used for any desired purpose. choice FREERTOS_ASSERT prompt "FreeRTOS assertions" diff --git a/components/lwip/Kconfig b/components/lwip/Kconfig index 708a7bd14..099b4dffd 100644 --- a/components/lwip/Kconfig +++ b/components/lwip/Kconfig @@ -26,13 +26,6 @@ config LWIP_MAX_SOCKETS the maximum amount of sockets here. The valid value is from 1 to 16. -config LWIP_THREAD_LOCAL_STORAGE_INDEX - int "Index for thread-local-storage pointer for lwip" - default 0 - help - Specify the thread-local-storage-pointer index for lwip - use. - config LWIP_SO_REUSE bool "Enable SO_REUSEADDR option" default y diff --git a/components/lwip/port/freertos/sys_arch.c b/components/lwip/port/freertos/sys_arch.c index 6f14a3a29..aa58f9bb1 100755 --- a/components/lwip/port/freertos/sys_arch.c +++ b/components/lwip/port/freertos/sys_arch.c @@ -32,6 +32,7 @@ /* lwIP includes. */ +#include #include "lwip/debug.h" #include "lwip/def.h" #include "lwip/sys.h" @@ -46,6 +47,9 @@ static sys_mutex_t g_lwip_protect_mutex = NULL; +static pthread_key_t sys_thread_sem_key; +static void sys_thread_sem_free(void* data); + #if !LWIP_COMPAT_MUTEX /** Create a new mutex * @param mutex pointer to the mutex to create @@ -433,6 +437,10 @@ sys_init(void) if (ERR_OK != sys_mutex_new(&g_lwip_protect_mutex)) { ESP_LOGE(TAG, "sys_init: failed to init lwip protect mutex\n"); } + + // Create the pthreads key for the per-thread semaphore storage + pthread_key_create(&sys_thread_sem_key, sys_thread_sem_free); + esp_vfs_lwip_sockets_register(); } @@ -483,31 +491,31 @@ sys_arch_unprotect(sys_prot_t pval) sys_mutex_unlock(&g_lwip_protect_mutex); } -#define SYS_TLS_INDEX CONFIG_LWIP_THREAD_LOCAL_STORAGE_INDEX -/* +/* * get per thread semphore */ sys_sem_t* sys_thread_sem_get(void) { - sys_sem_t *sem = (sys_sem_t*)pvTaskGetThreadLocalStoragePointer(xTaskGetCurrentTaskHandle(), SYS_TLS_INDEX); - if (!sem){ - sem = sys_thread_sem_init(); + sys_sem_t *sem = pthread_getspecific(sys_thread_sem_key); + + if (!sem) { + sem = sys_thread_sem_init(); } LWIP_DEBUGF(ESP_THREAD_SAFE_DEBUG, ("sem_get s=%p\n", sem)); return sem; } -static void sys_thread_tls_free(int index, void* data) +static void sys_thread_sem_free(void* data) // destructor for TLS semaphore { sys_sem_t *sem = (sys_sem_t*)(data); if (sem && *sem){ - LWIP_DEBUGF(ESP_THREAD_SAFE_DEBUG, ("sem del, i=%d sem=%p\n", index, *sem)); + LWIP_DEBUGF(ESP_THREAD_SAFE_DEBUG, ("sem del, sem=%p\n", *sem)); vSemaphoreDelete(*sem); } - if (sem){ - LWIP_DEBUGF(ESP_THREAD_SAFE_DEBUG, ("sem pointer del, i=%d sem_p=%p\n", index, sem)); + if (sem) { + LWIP_DEBUGF(ESP_THREAD_SAFE_DEBUG, ("sem pointer del, sem_p=%p\n", sem)); free(sem); } } @@ -528,20 +536,17 @@ sys_sem_t* sys_thread_sem_init(void) return 0; } - LWIP_DEBUGF(ESP_THREAD_SAFE_DEBUG, ("sem init sem_p=%p sem=%p cb=%p\n", sem, *sem, sys_thread_tls_free)); - vTaskSetThreadLocalStoragePointerAndDelCallback(xTaskGetCurrentTaskHandle(), SYS_TLS_INDEX, sem, (TlsDeleteCallbackFunction_t)sys_thread_tls_free); - + pthread_setspecific(sys_thread_sem_key, sem); return sem; } void sys_thread_sem_deinit(void) { - sys_sem_t *sem = (sys_sem_t*)pvTaskGetThreadLocalStoragePointer(xTaskGetCurrentTaskHandle(), SYS_TLS_INDEX); - - sys_thread_tls_free(SYS_TLS_INDEX, (void*)sem); - vTaskSetThreadLocalStoragePointerAndDelCallback(xTaskGetCurrentTaskHandle(), SYS_TLS_INDEX, 0, 0); - - return; + sys_sem_t *sem = pthread_getspecific(sys_thread_sem_key); + if (sem != NULL) { + sys_thread_sem_free(sem); + pthread_setspecific(sys_thread_sem_key, NULL); + } } void sys_delay_ms(uint32_t ms) diff --git a/components/pthread/pthread.c b/components/pthread/pthread.c index 7817355fb..70cc24552 100644 --- a/components/pthread/pthread.c +++ b/components/pthread/pthread.c @@ -28,6 +28,8 @@ #include "freertos/semphr.h" #include "freertos/list.h" +#include "pthread_internal.h" + #define LOG_LOCAL_LEVEL CONFIG_LOG_DEFAULT_LEVEL #include "esp_log.h" const static char *TAG = "esp_pthread"; @@ -145,6 +147,10 @@ static void pthread_task_func(void *arg) ESP_LOGV(TAG, "%s END %p", __FUNCTION__, task_arg->func); free(task_arg); + /* preemptively clean up thread local storage, rather than + waiting for the idle task to clean up the thread */ + pthread_internal_local_storage_destructor_callback(); + if (xSemaphoreTake(s_threads_mux, portMAX_DELAY) != pdTRUE) { assert(false && "Failed to lock threads list!"); } @@ -332,40 +338,6 @@ int pthread_equal(pthread_t t1, pthread_t t2) return t1 == t2 ? 1 : 0; } -/***************** KEY ******************/ -int pthread_key_create(pthread_key_t *key, void (*destructor)(void*)) -{ - static int s_created; - - //TODO: Key destructors not suppoted! - if (s_created) { - // key API supports just one key necessary by libstdcxx threading implementation - ESP_LOGE(TAG, "%s: multiple keys not supported!", __FUNCTION__); - return ENOSYS; - } - *key = 1; - s_created = 1; - return 0; -} - -int pthread_key_delete(pthread_key_t key) -{ - ESP_LOGE(TAG, "%s: not supported!", __FUNCTION__); - return ENOSYS; -} - -void *pthread_getspecific(pthread_key_t key) -{ - ESP_LOGE(TAG, "%s: not supported!", __FUNCTION__); - return NULL; -} - -int pthread_setspecific(pthread_key_t key, const void *value) -{ - ESP_LOGE(TAG, "%s: not supported!", __FUNCTION__); - return ENOSYS; -} - /***************** ONCE ******************/ int pthread_once(pthread_once_t *once_control, void (*init_routine)(void)) { diff --git a/components/pthread/pthread_internal.h b/components/pthread/pthread_internal.h new file mode 100644 index 000000000..6ed2fe45e --- /dev/null +++ b/components/pthread/pthread_internal.h @@ -0,0 +1,16 @@ +// Copyright 2017 Espressif Systems (Shanghai) PTE LTD +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at + +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. +#pragma once + +void pthread_internal_local_storage_destructor_callback(); diff --git a/components/pthread/pthread_local_storage.c b/components/pthread/pthread_local_storage.c new file mode 100644 index 000000000..dde877418 --- /dev/null +++ b/components/pthread/pthread_local_storage.c @@ -0,0 +1,226 @@ +// Copyright 2017 Espressif Systems (Shanghai) PTE LTD +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at + +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. +#include +#include +#include +#include "esp_err.h" +#include "esp_log.h" +#include "freertos/FreeRTOS.h" +#include "freertos/task.h" +#include "sys/lock.h" +#include "rom/queue.h" + +#include "pthread_internal.h" + +#define PTHREAD_TLS_INDEX 0 + +typedef void (*pthread_destructor_t)(void*); + +/* This is a very naive implementation of key-indexed thread local storage, using two linked lists + (one is a global list of registered keys, one per thread for thread local storage values). + + It won't work well if lots of keys & thread-local values are stored (O(n) lookup for both), + but it should work for small amounts of data. +*/ +typedef struct key_entry_t_ { + pthread_key_t key; + pthread_destructor_t destructor; + SLIST_ENTRY(key_entry_t_) next; +} key_entry_t; + +// List of all keys created with pthread_key_create() +SLIST_HEAD(key_list_t, key_entry_t_) s_keys = SLIST_HEAD_INITIALIZER(s_keys); + +static _lock_t s_keys_lock; + +// List of all value entries associated with a thread via pthread_setspecific() +typedef struct value_entry_t_ { + pthread_key_t key; + void *value; + SLIST_ENTRY(value_entry_t_) next; +} value_entry_t; + +// Type for the head of the list, as saved as a FreeRTOS thread local storage pointer +SLIST_HEAD(values_list_t_, value_entry_t_); +typedef struct values_list_t_ values_list_t; + +int pthread_key_create(pthread_key_t *key, pthread_destructor_t destructor) +{ + key_entry_t *new_key = malloc(sizeof(key_entry_t)); + if (new_key == NULL) { + return ENOMEM; + } + + _lock_acquire_recursive(&s_keys_lock); + + const key_entry_t *head = SLIST_FIRST(&s_keys); + new_key->key = (head == NULL) ? 1 : (head->key + 1); + new_key->destructor = destructor; + *key = new_key->key; + + SLIST_INSERT_HEAD(&s_keys, new_key, next); + + _lock_release_recursive(&s_keys_lock); + return 0; +} + +static key_entry_t *find_key(pthread_key_t key) +{ + _lock_acquire_recursive(&s_keys_lock); + key_entry_t *result = NULL;; + SLIST_FOREACH(result, &s_keys, next) { + if(result->key == key) { + break; + } + } + _lock_release_recursive(&s_keys_lock); + return result; +} + +int pthread_key_delete(pthread_key_t key) +{ + + _lock_acquire_recursive(&s_keys_lock); + + /* Ideally, we would also walk all tasks' thread local storage value_list here + and delete any values associated with this key. We do not do this... + */ + + key_entry_t *entry = find_key(key); + if (entry != NULL) { + SLIST_REMOVE(&s_keys, entry, key_entry_t_, next); + free(entry); + } + + _lock_release_recursive(&s_keys_lock); + + return 0; +} + +/* Clean up callback for deleted tasks. + + This is called from one of two places: + + If the thread was created via pthread_create() then it's called by pthread_task_func() when that thread ends, + and the FreeRTOS thread-local-storage is removed before the FreeRTOS task is deleted. + + For other tasks, this is called when the FreeRTOS idle task performs its task cleanup after the task is deleted. + + (The reason for calling it early for pthreads is to keep the timing consistent with "normal" pthreads, so after + pthread_join() the task's destructors have all been called even if the idle task hasn't run cleanup yet.) +*/ +static void pthread_local_storage_thread_deleted_callback(int index, void *v_tls) +{ + values_list_t *tls = (values_list_t *)v_tls; + assert(tls != NULL); + + /* Walk the list, freeing all entries and calling destructors if they are registered */ + value_entry_t *entry = SLIST_FIRST(tls); + while(entry != NULL) { + // This is a little slow, walking the linked list of keys once per value, + // but assumes that the thread's value list will have less entries + // than the keys list + key_entry_t *key = find_key(entry->key); + if (key != NULL && key->destructor != NULL) { + key->destructor(entry->value); + } + value_entry_t *next_entry = SLIST_NEXT(entry, next); + free(entry); + entry = next_entry; + } + free(tls); +} + +/* this function called from pthread_task_func for "early" cleanup of TLS in a pthread */ +void pthread_internal_local_storage_destructor_callback() +{ + void *tls = pvTaskGetThreadLocalStoragePointer(NULL, PTHREAD_TLS_INDEX); + if (tls != NULL) { + pthread_local_storage_thread_deleted_callback(PTHREAD_TLS_INDEX, tls); + /* remove the thread-local-storage pointer to avoid the idle task cleanup + calling it again... + */ + vTaskSetThreadLocalStoragePointerAndDelCallback(NULL, + PTHREAD_TLS_INDEX, + NULL, + NULL); + } +} + +static value_entry_t *find_value(const values_list_t *list, pthread_key_t key) +{ + value_entry_t *result = NULL;; + SLIST_FOREACH(result, list, next) { + if(result->key == key) { + break; + } + } + return result; +} + +void *pthread_getspecific(pthread_key_t key) +{ + values_list_t *tls = (values_list_t *) pvTaskGetThreadLocalStoragePointer(NULL, PTHREAD_TLS_INDEX); + if (tls == NULL) { + return NULL; + } + + value_entry_t *entry = find_value(tls, key); + if(entry != NULL) { + return entry->value; + } + return NULL; +} + +int pthread_setspecific(pthread_key_t key, const void *value) +{ + key_entry_t *key_entry = find_key(key); + if (key_entry == NULL) { + return ENOENT; // this situation is undefined by pthreads standard + } + + values_list_t *tls = pvTaskGetThreadLocalStoragePointer(NULL, PTHREAD_TLS_INDEX); + if (tls == NULL) { + tls = calloc(1, sizeof(values_list_t)); + if (tls == NULL) { + return ENOMEM; + } + vTaskSetThreadLocalStoragePointerAndDelCallback(NULL, + PTHREAD_TLS_INDEX, + tls, + pthread_local_storage_thread_deleted_callback); + } + + value_entry_t *entry = find_value(tls, key); + if (entry != NULL) { + if (value != NULL) { + // cast on next line is necessary as pthreads API uses + // 'const void *' here but elsewhere uses 'void *' + entry->value = (void *) value; + } else { // value == NULL, remove the entry + SLIST_REMOVE(tls, entry, value_entry_t_, next); + free(entry); + } + } else if (value != NULL) { + entry = malloc(sizeof(value_entry_t)); + if (entry == NULL) { + return ENOMEM; + } + entry->key = key; + entry->value = (void *) value; // see note above about cast + SLIST_INSERT_HEAD(tls, entry, next); + } + + return 0; +} diff --git a/components/pthread/test/test_pthread_local_storage.c b/components/pthread/test/test_pthread_local_storage.c new file mode 100644 index 000000000..5f83e87cb --- /dev/null +++ b/components/pthread/test/test_pthread_local_storage.c @@ -0,0 +1,107 @@ +// Test pthread_create_key, pthread_delete_key, pthread_setspecific, pthread_getspecific +#include +#include "unity.h" +#include "freertos/FreeRTOS.h" +#include "freertos/task.h" + +TEST_CASE("pthread local storage basics", "[pthread]") +{ + pthread_key_t key; + TEST_ASSERT_EQUAL(0, pthread_key_create(&key, NULL)); + + TEST_ASSERT_NULL(pthread_getspecific(key)); + int val = 3; + + printf("Setting to %p...\n", &val); + TEST_ASSERT_EQUAL(0, pthread_setspecific(key, &val)); + + printf("Reading back...\n"); + TEST_ASSERT_EQUAL_PTR(&val, pthread_getspecific(key)); + + printf("Setting to NULL...\n"); + TEST_ASSERT_EQUAL(0, pthread_setspecific(key, NULL)); + + printf("Reading back...\n"); + TEST_ASSERT_NULL(pthread_getspecific(key)); + + TEST_ASSERT_EQUAL(0, pthread_key_delete(key)); +} + +static void test_pthread_destructor(void *); +static void *expected_destructor_ptr; +static void *actual_destructor_ptr; +static void *thread_test_pthread_destructor(void *); + +TEST_CASE("pthread local storage destructor", "[pthread]") +{ + pthread_t thread; + pthread_key_t key = -1; + + expected_destructor_ptr = NULL; + actual_destructor_ptr = NULL; + + TEST_ASSERT_EQUAL(0, pthread_key_create(&key, test_pthread_destructor)); + + TEST_ASSERT_EQUAL(0, pthread_create(&thread, NULL, thread_test_pthread_destructor, (void *)key)); + TEST_ASSERT_EQUAL(0, pthread_join(thread, NULL)); + + printf("Joined...\n"); + TEST_ASSERT_NOT_NULL(expected_destructor_ptr); + TEST_ASSERT_NOT_NULL(actual_destructor_ptr); + TEST_ASSERT_EQUAL_PTR(expected_destructor_ptr, actual_destructor_ptr); + + TEST_ASSERT_EQUAL(0, pthread_key_delete(key)); +} + +static void task_test_pthread_destructor(void *v_key); + +TEST_CASE("pthread local storage destructor in FreeRTOS task", "[pthread]") +{ + // Same as previous test case, but doesn't use pthread APIs therefore must wait + // for the idle task to call the destructor + pthread_key_t key = -1; + + expected_destructor_ptr = NULL; + actual_destructor_ptr = NULL; + + TEST_ASSERT_EQUAL(0, pthread_key_create(&key, test_pthread_destructor)); + + xTaskCreate(task_test_pthread_destructor, + "ptdest", 8192, (void *)key, UNITY_FREERTOS_PRIORITY+1, + NULL); + + // Above task has higher priority to us, so should run immediately + // but we need to wait for the idle task cleanup to run + vTaskDelay(20); + + TEST_ASSERT_NOT_NULL(expected_destructor_ptr); + TEST_ASSERT_NOT_NULL(actual_destructor_ptr); + TEST_ASSERT_EQUAL_PTR(expected_destructor_ptr, actual_destructor_ptr); + + TEST_ASSERT_EQUAL(0, pthread_key_delete(key)); +} + + + +static void *thread_test_pthread_destructor(void *v_key) +{ + printf("Local storage thread running...\n"); + pthread_key_t key = (pthread_key_t) v_key; + expected_destructor_ptr = &key; // address of stack variable in the task... + pthread_setspecific(key, expected_destructor_ptr); + printf("Local storage thread done.\n"); + return NULL; +} + +static void test_pthread_destructor(void *value) +{ + printf("Destructor called...\n"); + actual_destructor_ptr = value; +} + +static void task_test_pthread_destructor(void *v_key) +{ + /* call the pthread main routine, then delete ourselves... */ + thread_test_pthread_destructor(v_key); + vTaskDelete(NULL); +} diff --git a/docs/api-guides/freertos-smp.rst b/docs/api-guides/freertos-smp.rst index a706320d8..acdebf87f 100644 --- a/docs/api-guides/freertos-smp.rst +++ b/docs/api-guides/freertos-smp.rst @@ -340,6 +340,11 @@ defined to call ``vTaskSetThreadLocalStoragePointerAndDelCallback()`` with a ``NULL`` pointer as the deletion call back. This results in the selected Thread Local Storage Pointer to have no deletion call back. +In IDF the FreeRTOS thread local storage at index 0 is reserved and is used to implement +the pthreads API thread local storage (pthread_getspecific() & pthread_setspecific()). +Other indexes can be used for any purpose, provided +:ref:`CONFIG_FREERTOS_THREAD_LOCAL_STORAGE_POINTERS` is set to a high enough value. + For more details see :component_file:`freertos/include/freertos/task.h` .. _esp-idf-freertos-configuration: From a231ba22f320e2b582e21a2089f3c0616822b177 Mon Sep 17 00:00:00 2001 From: Angus Gratton Date: Wed, 4 Oct 2017 16:00:11 +1100 Subject: [PATCH 4/9] cxx: Add a sanity check for C++ exception support --- components/cxx/test/test_cxx.cpp | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/components/cxx/test/test_cxx.cpp b/components/cxx/test/test_cxx.cpp index 7e08e9017..af73d9ea9 100644 --- a/components/cxx/test/test_cxx.cpp +++ b/components/cxx/test/test_cxx.cpp @@ -188,6 +188,21 @@ TEST_CASE("before scheduler has started, static initializers work correctly", "[ TEST_ASSERT_EQUAL(2, StaticInitTestBeforeScheduler::order); } +TEST_CASE("c++ exceptions work", "[cxx]") +{ + int thrown_value; + try + { + throw 20; + } + catch (int e) + { + thrown_value = e; + } + TEST_ASSERT_EQUAL(20, thrown_value); + printf("OK?\n"); +} + /* These test cases pull a lot of code from libstdc++ and are disabled for now */ #if 0 From d20fbffae1a24127b2cf71dd87f5fa6e7b94ee34 Mon Sep 17 00:00:00 2001 From: Angus Gratton Date: Wed, 4 Oct 2017 16:00:28 +1100 Subject: [PATCH 5/9] idf_monitor: Demangle C++ names --- docs/get-started/idf-monitor.rst | 2 +- tools/idf_monitor.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/docs/get-started/idf-monitor.rst b/docs/get-started/idf-monitor.rst index 933ef3d0e..4dac96c59 100644 --- a/docs/get-started/idf-monitor.rst +++ b/docs/get-started/idf-monitor.rst @@ -56,7 +56,7 @@ idf_monitor will augment the dump:: Behind the scenes, the command idf_monitor runs to decode each address is:: - xtensa-esp32-elf-addr2line -pfia -e build/PROJECT.elf ADDRESS + xtensa-esp32-elf-addr2line -pfiaC -e build/PROJECT.elf ADDRESS Launch GDB for GDBStub diff --git a/tools/idf_monitor.py b/tools/idf_monitor.py index e91ec687b..1f315f29e 100755 --- a/tools/idf_monitor.py +++ b/tools/idf_monitor.py @@ -395,7 +395,7 @@ class Monitor(object): def lookup_pc_address(self, pc_addr): translation = subprocess.check_output( ["%saddr2line" % self.toolchain_prefix, - "-pfia", "-e", self.elf_file, pc_addr], + "-pfiaC", "-e", self.elf_file, pc_addr], cwd=".") if not "?? ??:0" in translation: yellow_print(translation) From 6f07e0797d812d5963a6784f0a008fb55c81c2cb Mon Sep 17 00:00:00 2001 From: Angus Gratton Date: Wed, 4 Oct 2017 16:00:38 +1100 Subject: [PATCH 6/9] Unit tests: If a test fails due to corrupting memory, don't print a misleading source file name --- tools/unit-test-app/components/unity/unity_platform.c | 10 ++++++++-- tools/unit-test-app/sdkconfig | 1 - 2 files changed, 8 insertions(+), 3 deletions(-) diff --git a/tools/unit-test-app/components/unity/unity_platform.c b/tools/unit-test-app/components/unity/unity_platform.c index 8e73fd525..d204530dc 100644 --- a/tools/unit-test-app/components/unity/unity_platform.c +++ b/tools/unit-test-app/components/unity/unity_platform.c @@ -59,7 +59,7 @@ static void check_leak(size_t before_free, size_t after_free, const char *type) leaked < CRITICAL_LEAK_THRESHOLD ? "potential" : "critical", before_free, after_free, leaked); fflush(stdout); - TEST_ASSERT(leaked < CRITICAL_LEAK_THRESHOLD); /* fail the test if it leaks too much */ + TEST_ASSERT_MESSAGE(leaked < CRITICAL_LEAK_THRESHOLD, "The test leaked too much memory"); } /* tearDown runs after every test */ @@ -68,8 +68,12 @@ void tearDown(void) /* some FreeRTOS stuff is cleaned up by idle task */ vTaskDelay(5); + /* We want the teardown to have this file in the printout if TEST_ASSERT fails */ + const char *real_testfile = Unity.TestFile; + Unity.TestFile = __FILE__; + /* check if unit test has caused heap corruption in any heap */ - TEST_ASSERT( heap_caps_check_integrity(MALLOC_CAP_INVALID, true) ); + TEST_ASSERT_MESSAGE( heap_caps_check_integrity(MALLOC_CAP_INVALID, true), "The test has corrupted the heap"); /* check for leaks */ size_t after_free_8bit = heap_caps_get_free_size(MALLOC_CAP_8BIT); @@ -77,6 +81,8 @@ void tearDown(void) check_leak(before_free_8bit, after_free_8bit, "8BIT"); check_leak(before_free_32bit, after_free_32bit, "32BIT"); + + Unity.TestFile = real_testfile; // go back to the real filename } void unity_putc(int c) diff --git a/tools/unit-test-app/sdkconfig b/tools/unit-test-app/sdkconfig index 1d261c0a2..6816f4cb0 100644 --- a/tools/unit-test-app/sdkconfig +++ b/tools/unit-test-app/sdkconfig @@ -302,7 +302,6 @@ CONFIG_LOG_COLORS=y # CONFIG_L2_TO_L3_COPY= CONFIG_LWIP_MAX_SOCKETS=4 -CONFIG_LWIP_THREAD_LOCAL_STORAGE_INDEX=0 CONFIG_LWIP_SO_REUSE= CONFIG_LWIP_SO_RCVBUF= CONFIG_LWIP_DHCP_MAX_NTP_SERVERS=1 From 9c7477ef34ac8260c908b637fdc3c291a018425c Mon Sep 17 00:00:00 2001 From: Angus Gratton Date: Wed, 4 Oct 2017 17:29:21 +1100 Subject: [PATCH 7/9] cxx: Add KConfig option for C++ exceptions, disable by default Fixes https://github.com/espressif/esp-idf/issues/1072 (Additional 20KB is still used if C++ exception support is enabled in menuconfig.) --- Kconfig | 14 ++++- components/cxx/component.mk | 8 +++ components/cxx/cxx_exception_stubs.cpp | 87 ++++++++++++++++++++++++++ components/cxx/test/test_cxx.cpp | 6 ++ components/esp32/cpu_start.c | 2 + make/project.mk | 6 ++ 6 files changed, 122 insertions(+), 1 deletion(-) create mode 100644 components/cxx/cxx_exception_stubs.cpp diff --git a/Kconfig b/Kconfig index 2cf242481..c641882f8 100644 --- a/Kconfig +++ b/Kconfig @@ -93,7 +93,19 @@ config OPTIMIZATION_ASSERTIONS_DISABLED endchoice # assertions -endmenu # Optimization level +config CXX_EXCEPTIONS + bool "Enable C++ exceptions" + default n + help + Enabling this option compiles all IDF C++ files with exception support enabled. + + Disabling this option disables C++ exception support in all compiled files, and any libstdc++ code which throws + an exception will abort instead. + + Enabling this option currently adds an additional 20KB of heap overhead, and 4KB of additional heap is allocated + the first time an exception is thrown in user code. + +endmenu # Compiler Options menu "Component config" source "$COMPONENT_KCONFIGS" diff --git a/components/cxx/component.mk b/components/cxx/component.mk index 5d5638477..00e05e0db 100644 --- a/components/cxx/component.mk +++ b/components/cxx/component.mk @@ -1,3 +1,11 @@ # Mark __cxa_guard_dummy as undefined so that implementation of static guards # is taken from cxx_guards.o instead of libstdc++.a COMPONENT_ADD_LDFLAGS += -u __cxa_guard_dummy + +ifndef CONFIG_CXX_EXCEPTIONS +# If exceptions are disabled, ensure our fatal exception +# hooks are preferentially linked over libstdc++ which +# has full exception support +COMPONENT_ADD_LDFLAGS += -u __cxx_fatal_exception +endif + diff --git a/components/cxx/cxx_exception_stubs.cpp b/components/cxx/cxx_exception_stubs.cpp new file mode 100644 index 000000000..4c914b2b7 --- /dev/null +++ b/components/cxx/cxx_exception_stubs.cpp @@ -0,0 +1,87 @@ +#include +#include +#include +#include +#include + +#ifndef CONFIG_CXX_EXCEPTIONS + +const char *FATAL_EXCEPTION = "Fatal C++ exception: "; + +extern "C" void __cxx_fatal_exception(void) +{ + abort(); +} + +extern "C" void __cxx_fatal_exception_message(const char *msg) +{ + printf("%s%s\n", FATAL_EXCEPTION, msg); + abort(); +} + +extern "C" void __cxx_fatal_exception_int(int i) +{ + printf("%s (%d)\n", FATAL_EXCEPTION, i); + abort(); +} + +void std::__throw_bad_exception(void) __attribute__((alias("__cxx_fatal_exception"))); + +void std::__throw_bad_alloc(void) __attribute__((alias("__cxx_fatal_exception"))); + +void std::__throw_bad_cast(void) __attribute__((alias("__cxx_fatal_exception"))); + +void std::__throw_bad_typeid(void) __attribute__((alias("__cxx_fatal_exception"))); + +void std::__throw_logic_error(const char*) __attribute__((alias("__cxx_fatal_exception_message"))); + +void std::__throw_domain_error(const char*) __attribute__((alias("__cxx_fatal_exception_message"))); + +void std::__throw_invalid_argument(const char*) __attribute__((alias("__cxx_fatal_exception_message"))); + +void std::__throw_length_error(const char*) __attribute__((alias("__cxx_fatal_exception_message"))); + +void std::__throw_out_of_range(const char*) __attribute__((alias("__cxx_fatal_exception_message"))); + +void std::__throw_out_of_range_fmt(const char*, ...) __attribute__((alias("__cxx_fatal_exception_message"))); + +void std::__throw_runtime_error(const char*) __attribute__((alias("__cxx_fatal_exception_message"))); + +void std::__throw_range_error(const char*) __attribute__((alias("__cxx_fatal_exception_message"))); + +void std::__throw_overflow_error(const char*) __attribute__((alias("__cxx_fatal_exception_message"))); + +void std::__throw_underflow_error(const char*) __attribute__((alias("__cxx_fatal_exception_message"))); + +void std::__throw_ios_failure(const char*) __attribute__((alias("__cxx_fatal_exception_message"))); + +void std::__throw_system_error(int) __attribute__((alias("__cxx_fatal_exception_int"))); + +void std::__throw_bad_function_call(void) __attribute__((alias("__cxx_fatal_exception"))); + +void std::__throw_future_error(int) __attribute__((alias("__cxx_fatal_exception_int"))); + + +/* The following definitions are needed because libstdc++ is also compiled with + __throw_exception_again defined to throw, and some other exception code in a few places. + + This cause exception handler code to be emitted in the library even though it's mostly + unreachable (as any libstdc++ "throw" will first call one of the above stubs, which will abort). + + If these are left out, a bunch of unwanted exception handler code is linked. + + Note: these function prototypes are not correct. +*/ + +extern "C" void __cxa_allocate_exception(void) __attribute__((alias("__cxx_fatal_exception"))); +extern "C" void __cxa_begin_catch(void) __attribute__((alias("__cxx_fatal_exception"))); +extern "C" void __cxa_end_catch(void) __attribute__((alias("__cxx_fatal_exception"))); +extern "C" void __cxa_get_exception_ptr(void) __attribute__((alias("__cxx_fatal_exception"))); +extern "C" void __cxa_free_exception(void) __attribute__((alias("__cxx_fatal_exception"))); +extern "C" void __cxa_rethrow(void) __attribute__((alias("__cxx_fatal_exception"))); +extern "C" void __cxa_throw(void) __attribute__((alias("__cxx_fatal_exception"))); +extern "C" void __cxa_call_terminate(void) __attribute__((alias("__cxx_fatal_exception"))); + +bool std::uncaught_exception() __attribute__((alias("__cxx_fatal_exception"))); + +#endif // CONFIG_CXX_EXCEPTIONS diff --git a/components/cxx/test/test_cxx.cpp b/components/cxx/test/test_cxx.cpp index af73d9ea9..743a6869d 100644 --- a/components/cxx/test/test_cxx.cpp +++ b/components/cxx/test/test_cxx.cpp @@ -188,8 +188,12 @@ TEST_CASE("before scheduler has started, static initializers work correctly", "[ TEST_ASSERT_EQUAL(2, StaticInitTestBeforeScheduler::order); } +#ifdef CONFIG_CXX_EXCEPTIONS + TEST_CASE("c++ exceptions work", "[cxx]") { + /* Note: This test currently trips the memory leak threshold + as libunwind allocates ~4KB of data on first exception. */ int thrown_value; try { @@ -203,6 +207,8 @@ TEST_CASE("c++ exceptions work", "[cxx]") printf("OK?\n"); } +#endif + /* These test cases pull a lot of code from libstdc++ and are disabled for now */ #if 0 diff --git a/components/esp32/cpu_start.c b/components/esp32/cpu_start.c index 3aefdce0a..ee0edc2e2 100644 --- a/components/esp32/cpu_start.c +++ b/components/esp32/cpu_start.c @@ -379,8 +379,10 @@ void start_cpu1_default(void) static void do_global_ctors(void) { +#ifdef CONFIG_CXX_EXCEPTIONS static struct object ob; __register_frame_info( __eh_frame, &ob ); +#endif void (**p)(void); for (p = &__init_array_end - 1; p >= &__init_array_start; --p) { diff --git a/make/project.mk b/make/project.mk index 60ecce3ec..deaba74a3 100644 --- a/make/project.mk +++ b/make/project.mk @@ -303,6 +303,12 @@ CXXFLAGS := $(strip \ $(CXXFLAGS) \ $(EXTRA_CXXFLAGS)) +ifdef CONFIG_CXX_EXCEPTIONS +CXXFLAGS += -fexceptions +else +CXXFLAGS += -fno-exceptions +endif + export CFLAGS CPPFLAGS CXXFLAGS # Set host compiler and binutils From 69e92ee320844f14efe5cbf1032f804047671838 Mon Sep 17 00:00:00 2001 From: Angus Gratton Date: Wed, 4 Oct 2017 17:30:10 +1100 Subject: [PATCH 8/9] unit tests: If heap tracing is enabled in sdkconfig, leak trace each test --- components/pthread/test/test_pthread_cxx.cpp | 2 ++ .../components/unity/unity_platform.c | 20 +++++++++++++++++++ tools/unit-test-app/sdkconfig | 4 ++-- 3 files changed, 24 insertions(+), 2 deletions(-) diff --git a/components/pthread/test/test_pthread_cxx.cpp b/components/pthread/test/test_pthread_cxx.cpp index ced961d09..d207c1161 100644 --- a/components/pthread/test/test_pthread_cxx.cpp +++ b/components/pthread/test/test_pthread_cxx.cpp @@ -85,6 +85,8 @@ TEST_CASE("pthread C++", "[pthread]") std::cout << "Join thread " << std::hex << t4.get_id() << std::endl; t4.join(); } + + global_sp.reset(); // avoid reported leak } static void task_test_sandbox(void *arg) diff --git a/tools/unit-test-app/components/unity/unity_platform.c b/tools/unit-test-app/components/unity/unity_platform.c index d204530dc..7b96ae4f3 100644 --- a/tools/unit-test-app/components/unity/unity_platform.c +++ b/tools/unit-test-app/components/unity/unity_platform.c @@ -10,6 +10,7 @@ #include "esp_log.h" #include "soc/cpu.h" #include "esp_heap_caps.h" +#include "esp_heap_trace.h" #include "test_utils.h" #define unity_printf ets_printf @@ -37,11 +38,26 @@ const size_t CRITICAL_LEAK_THRESHOLD = 4096; /* setUp runs before every test */ void setUp(void) { +// If heap tracing is enabled in kconfig, leak trace the test +#ifdef CONFIG_HEAP_TRACING + const size_t num_heap_records = 80; + static heap_trace_record_t *record_buffer; + if (!record_buffer) { + record_buffer = malloc(sizeof(heap_trace_record_t) * num_heap_records); + assert(record_buffer); + heap_trace_init_standalone(record_buffer, num_heap_records); + } +#endif + printf("%s", ""); /* sneakily lazy-allocate the reent structure for this test task */ get_test_data_partition(); /* allocate persistent partition table structures */ before_free_8bit = heap_caps_get_free_size(MALLOC_CAP_8BIT); before_free_32bit = heap_caps_get_free_size(MALLOC_CAP_32BIT); + +#ifdef CONFIG_HEAP_TRACING + heap_trace_start(HEAP_TRACE_LEAKS); +#endif } static void check_leak(size_t before_free, size_t after_free, const char *type) @@ -76,6 +92,10 @@ void tearDown(void) TEST_ASSERT_MESSAGE( heap_caps_check_integrity(MALLOC_CAP_INVALID, true), "The test has corrupted the heap"); /* check for leaks */ +#ifdef CONFIG_HEAP_TRACING + heap_trace_stop(); + heap_trace_dump(); +#endif size_t after_free_8bit = heap_caps_get_free_size(MALLOC_CAP_8BIT); size_t after_free_32bit = heap_caps_get_free_size(MALLOC_CAP_32BIT); diff --git a/tools/unit-test-app/sdkconfig b/tools/unit-test-app/sdkconfig index 6816f4cb0..d9315630b 100644 --- a/tools/unit-test-app/sdkconfig +++ b/tools/unit-test-app/sdkconfig @@ -91,6 +91,7 @@ CONFIG_OPTIMIZATION_LEVEL_RELEASE= CONFIG_OPTIMIZATION_ASSERTIONS_ENABLED=y CONFIG_OPTIMIZATION_ASSERTIONS_SILENT= CONFIG_OPTIMIZATION_ASSERTIONS_DISABLED= +CONFIG_CXX_EXCEPTIONS= # # Component config @@ -277,8 +278,7 @@ CONFIG_FREERTOS_DEBUG_INTERNALS= CONFIG_HEAP_POISONING_DISABLED= CONFIG_HEAP_POISONING_LIGHT= CONFIG_HEAP_POISONING_COMPREHENSIVE=y -CONFIG_HEAP_TRACING=y -CONFIG_HEAP_TRACING_STACK_DEPTH=2 +CONFIG_HEAP_TRACING= # # libsodium From 3f4c8f7174cedfa9d4a1baf46885ac1c0f98a4f0 Mon Sep 17 00:00:00 2001 From: Angus Gratton Date: Tue, 10 Oct 2017 11:50:02 +1100 Subject: [PATCH 9/9] pthreads local storage: add test for unique keys --- .../pthread/test/test_pthread_local_storage.c | 23 +++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/components/pthread/test/test_pthread_local_storage.c b/components/pthread/test/test_pthread_local_storage.c index 5f83e87cb..8be22ce57 100644 --- a/components/pthread/test/test_pthread_local_storage.c +++ b/components/pthread/test/test_pthread_local_storage.c @@ -27,6 +27,29 @@ TEST_CASE("pthread local storage basics", "[pthread]") TEST_ASSERT_EQUAL(0, pthread_key_delete(key)); } +TEST_CASE("pthread local storage unique keys", "[pthread]") +{ + const int NUM_KEYS = 10; + pthread_key_t keys[NUM_KEYS]; + + for (int i = 0; i < NUM_KEYS; i++) { + TEST_ASSERT_EQUAL(0, pthread_key_create(&keys[i], NULL)); + printf("New key %d = %d\n", i, keys[i]); + } + + for (int i = 0; i < NUM_KEYS; i++) { + for (int j = 0; j < NUM_KEYS; j++) { + if (i != j) { + TEST_ASSERT_NOT_EQUAL(keys[i], keys[j]); + } + } + } + + for (int i = 0; i < NUM_KEYS; i++) { + TEST_ASSERT_EQUAL(0, pthread_key_delete(keys[i])); + } +} + static void test_pthread_destructor(void *); static void *expected_destructor_ptr; static void *actual_destructor_ptr;