diff --git a/components/pthread/pthread_local_storage.c b/components/pthread/pthread_local_storage.c index 5572768470..e784c4cb6b 100644 --- a/components/pthread/pthread_local_storage.c +++ b/components/pthread/pthread_local_storage.c @@ -113,12 +113,17 @@ int pthread_key_delete(pthread_key_t key) 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. + or calls pthread_exit(), 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.) + There are two reasons for calling it early for pthreads: + + - 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. + + - The destructor is always called in the context of the thread itself - which is important if the task then calls + pthread_getspecific() or pthread_setspecific() to update the state further, as allowed for in the spec. */ static void pthread_local_storage_thread_deleted_callback(int index, void *v_tls) { @@ -126,8 +131,13 @@ static void pthread_local_storage_thread_deleted_callback(int index, void *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) { + while (1) { + value_entry_t *entry = SLIST_FIRST(tls); + if (entry == NULL) { + break; + } + SLIST_REMOVE_HEAD(tls, next); + // 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 @@ -135,9 +145,7 @@ static void pthread_local_storage_thread_deleted_callback(int index, void *v_tls 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); } @@ -250,7 +258,22 @@ int pthread_setspecific(pthread_key_t key, const void *value) } entry->key = key; entry->value = (void *) value; // see note above about cast - SLIST_INSERT_HEAD(tls, entry, next); + + // insert the new entry at the end of the list. this is important because + // a destructor may call pthread_setspecific() to add a new non-NULL value + // to the list, and this should be processed after all other entries. + // + // See pthread_local_storage_thread_deleted_callback() + value_entry_t *last_entry = NULL; + value_entry_t *it; + SLIST_FOREACH(it, tls, next) { + last_entry = it; + } + if (last_entry == NULL) { + SLIST_INSERT_HEAD(tls, entry, next); + } else { + SLIST_INSERT_AFTER(last_entry, entry, next); + } } return 0; diff --git a/components/pthread/test/test_pthread_local_storage.c b/components/pthread/test/test_pthread_local_storage.c index ebcaafcbe8..98e6fd7d99 100644 --- a/components/pthread/test/test_pthread_local_storage.c +++ b/components/pthread/test/test_pthread_local_storage.c @@ -162,3 +162,90 @@ TEST_CASE("pthread local storage stress test", "[pthread]") TEST_ASSERT_EQUAL(0, pthread_join(threads[i], NULL)); } } + + +#define NUM_KEYS 4 // number of keys used in repeat destructor test +#define NUM_REPEATS 17 // number of times we re-set a key to a non-NULL value to re-trigger destructor + +typedef struct { + pthread_key_t keys[NUM_KEYS]; // pthread local storage keys used in test + unsigned count; // number of times the destructor has been called + int last_idx; // index of last key where destructor was called +} destr_test_state_t; + +static void s_test_repeat_destructor(void *vp_state); +static void *s_test_repeat_destructor_thread(void *vp_state); + +// Test the correct behaviour of a pthread destructor function that uses +// pthread_setspecific() to set another value when it runs, and also +// +// As described in https://pubs.opengroup.org/onlinepubs/009695399/functions/pthread_key_create.html +TEST_CASE("pthread local storage 'repeat' destructor test", "[pthread]") +{ + int r; + destr_test_state_t state = { .last_idx = -1 }; + pthread_t thread; + + for (int i = 0; i < NUM_KEYS; i++) { + r = pthread_key_create(&state.keys[i], s_test_repeat_destructor); + TEST_ASSERT_EQUAL(0, r); + } + + r = pthread_create(&thread, NULL, s_test_repeat_destructor_thread, &state); + TEST_ASSERT_EQUAL(0, r); + + r = pthread_join(thread, NULL); + TEST_ASSERT_EQUAL(0 ,r); + + // Cheating here to make sure compiler reads the value of 'count' from memory not from a register + // + // We expect the destructor was called NUM_REPEATS times when it repeated, then NUM_KEYS times when it didn't + TEST_ASSERT_EQUAL(NUM_REPEATS + NUM_KEYS, ((volatile destr_test_state_t)state).count); + + // cleanup + for (int i = 0; i < NUM_KEYS; i++) { + r = pthread_key_delete(state.keys[i]); + TEST_ASSERT_EQUAL(0, r); + } +} + +static void s_test_repeat_destructor(void *vp_state) +{ + destr_test_state_t *state = vp_state; + + state->count++; + printf("Destructor! Arg %p Count %d\n", state, state->count); + if (state->count > NUM_REPEATS) { + return; // Stop replacing values after NUM_REPEATS destructors have been called, they will be NULLed out now + } + + // Find the key which has a NULL value, this is the key for this destructor. We will set it back to 'state' to repeat later. + // At this point only one key should have a NULL value + int null_idx = -1; + for (int i = 0; i < NUM_KEYS; i++) { + if (pthread_getspecific(state->keys[i]) == NULL) { + TEST_ASSERT_EQUAL(-1, null_idx); // If more than one key has a NULL value, something has gone wrong + null_idx = i; + // don't break, verify the other keys have non-NULL values + } + } + + TEST_ASSERT_NOT_EQUAL(-1, null_idx); // One key should have a NULL value + + // The same key shouldn't be destroyed twice in a row, as new non-NULL values should be destroyed + // after existing non-NULL values (to match spec behaviour) + TEST_ASSERT_NOT_EQUAL(null_idx, state->last_idx); + + printf("Re-setting index %d\n", null_idx); + pthread_setspecific(state->keys[null_idx], state); + state->last_idx = null_idx; +} + +static void *s_test_repeat_destructor_thread(void *vp_state) +{ + destr_test_state_t *state = vp_state; + for (int i = 0; i < NUM_KEYS; i++) { + pthread_setspecific(state->keys[i], state); + } + pthread_exit(NULL); +}