From 1269959268ceb3d8a71cd279344966c9784bc87e Mon Sep 17 00:00:00 2001 From: Pablo Garrido Date: Thu, 28 Apr 2022 15:43:21 +0200 Subject: [PATCH 1/5] Update Signed-off-by: Pablo Garrido Update Signed-off-by: Pablo Garrido Update Signed-off-by: Pablo Garrido Update Signed-off-by: Pablo Garrido --- rcl/include/rcl/context.h | 9 +++++---- rcl/src/rcl/context.c | 18 ++++++++++-------- rcl/src/rcl/init.c | 23 ++++++++++++++--------- 3 files changed, 29 insertions(+), 21 deletions(-) diff --git a/rcl/include/rcl/context.h b/rcl/include/rcl/context.h index 01bc87fbd..05b067fd8 100644 --- a/rcl/include/rcl/context.h +++ b/rcl/include/rcl/context.h @@ -127,9 +127,9 @@ typedef struct rcl_context_t // ensured with a static_assert in the context.c file. // In most cases it should just be a plain uint64_t. /// @cond Doxygen_Suppress -#if !defined(RCL_CONTEXT_ATOMIC_INSTANCE_ID_STORAGE_SIZE) -#define RCL_CONTEXT_ATOMIC_INSTANCE_ID_STORAGE_SIZE sizeof(uint_least64_t) -#endif +// #if !defined(RCL_CONTEXT_ATOMIC_INSTANCE_ID_STORAGE_SIZE) +// #define RCL_CONTEXT_ATOMIC_INSTANCE_ID_STORAGE_SIZE sizeof(uint_least64_t) +// #endif /// @endcond /// Private storage for instance ID atomic. /** @@ -150,7 +150,8 @@ typedef struct rcl_context_t * See this paper for an effort to make this possible in the future: * http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2018/p0943r1.html */ - RCL_ALIGNAS(8) uint8_t instance_id_storage[RCL_CONTEXT_ATOMIC_INSTANCE_ID_STORAGE_SIZE]; + // RCL_ALIGNAS(8) uint8_t instance_id_storage[RCL_CONTEXT_ATOMIC_INSTANCE_ID_STORAGE_SIZE]; + uint32_t instance_id_storage; } rcl_context_t; /// Return a zero initialization context object. diff --git a/rcl/src/rcl/context.c b/rcl/src/rcl/context.c index 67cd89d9f..74e63007a 100644 --- a/rcl/src/rcl/context.c +++ b/rcl/src/rcl/context.c @@ -30,18 +30,18 @@ rcl_get_zero_initialized_context(void) { static rcl_context_t context = { .impl = NULL, - .instance_id_storage = {0}, + .instance_id_storage = 0, }; // this is not constexpr so it cannot be in the struct initialization #ifdef RCL_COMMAND_LINE_ENABLED context.global_arguments = rcl_get_zero_initialized_arguments(); #endif // RCL_COMMAND_LINE_ENABLED // ensure assumption about static storage - static_assert( - sizeof(context.instance_id_storage) >= sizeof(atomic_uint_least64_t), - "expected rcl_context_t's instance id storage to be >= size of atomic_uint_least64_t"); - // initialize atomic - atomic_init((atomic_uint_least64_t *)(&context.instance_id_storage), 0); + // static_assert( + // sizeof(context.instance_id_storage) >= sizeof(atomic_uint_least64_t), + // "expected rcl_context_t's instance id storage to be >= size of atomic_uint_least64_t"); + // // initialize atomic + // atomic_init((atomic_uint_least64_t *)(&context.instance_id_storage), 0); return context; } @@ -78,7 +78,8 @@ rcl_context_instance_id_t rcl_context_get_instance_id(const rcl_context_t * context) { RCL_CHECK_ARGUMENT_FOR_NULL(context, 0); - return rcutils_atomic_load_uint64_t((atomic_uint_least64_t *)(&context->instance_id_storage)); + // return rcutils_atomic_load_uint64_t((atomic_uint_least64_t *)(&context->instance_id_storage)); + return context->instance_id_storage; } rcl_ret_t @@ -112,7 +113,8 @@ __cleanup_context(rcl_context_t * context) { rcl_ret_t ret = RCL_RET_OK; // reset the instance id to 0 to indicate "invalid" (should already be 0, but this is defensive) - rcutils_atomic_store((atomic_uint_least64_t *)(&context->instance_id_storage), 0); + // rcutils_atomic_store((atomic_uint_least64_t *)(&context->instance_id_storage), 0); + context->instance_id_storage = 0; #ifdef RCL_COMMAND_LINE_ENABLED // clean up global_arguments if initialized diff --git a/rcl/src/rcl/init.c b/rcl/src/rcl/init.c index b642424fe..ba0edadb8 100644 --- a/rcl/src/rcl/init.c +++ b/rcl/src/rcl/init.c @@ -46,7 +46,7 @@ extern "C" #include "./context_impl.h" #include "./init_options_impl.h" -static atomic_uint_least64_t __rcl_next_unique_id = ATOMIC_VAR_INIT(1); +// static atomic_uint_least64_t __rcl_next_unique_id = ATOMIC_VAR_INIT(1); rcl_ret_t rcl_init( @@ -142,15 +142,19 @@ rcl_init( #endif // RCL_COMMAND_LINE_ENABLED // Set the instance id. - uint64_t next_instance_id = rcutils_atomic_fetch_add_uint64_t(&__rcl_next_unique_id, 1); + // uint64_t next_instance_id = rcutils_atomic_fetch_add_uint64_t(&__rcl_next_unique_id, 1); + static uint32_t next_instance_id = 0; + next_instance_id++; if (0 == next_instance_id) { - // Roll over occurred, this is an extremely unlikely occurrence. - RCL_SET_ERROR_MSG("unique rcl instance ids exhausted"); - // Roll back to try to avoid the next call succeeding, but there's a data race here. - rcutils_atomic_store(&__rcl_next_unique_id, -1); - goto fail; + next_instance_id++; + // // Roll over occurred, this is an extremely unlikely occurrence. + // RCL_SET_ERROR_MSG("unique rcl instance ids exhausted"); + // // Roll back to try to avoid the next call succeeding, but there's a data race here. + // rcutils_atomic_store(&__rcl_next_unique_id, -1); + // goto fail; } - rcutils_atomic_store((atomic_uint_least64_t *)(&context->instance_id_storage), next_instance_id); + // rcutils_atomic_store((atomic_uint_least64_t *)(&context->instance_id_storage), next_instance_id); + context->impl->instance_id_storage = next_instance_id; context->impl->init_options.impl->rmw_init_options.instance_id = next_instance_id; size_t * domain_id = &context->impl->init_options.impl->rmw_init_options.domain_id; @@ -261,7 +265,8 @@ rcl_shutdown(rcl_context_t * context) } // reset the instance id to 0 to indicate "invalid" - rcutils_atomic_store((atomic_uint_least64_t *)(&context->instance_id_storage), 0); + // rcutils_atomic_store((atomic_uint_least64_t *)(&context->instance_id_storage), 0); + context->instance_id_storage = 0; return RCL_RET_OK; } From 40952469c5655adc227909ceef78476d50ed1981 Mon Sep 17 00:00:00 2001 From: Pablo Garrido Date: Thu, 28 Apr 2022 15:49:03 +0200 Subject: [PATCH 2/5] Update Signed-off-by: Pablo Garrido --- rcl/src/rcl/init.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/rcl/src/rcl/init.c b/rcl/src/rcl/init.c index ba0edadb8..2604d5b35 100644 --- a/rcl/src/rcl/init.c +++ b/rcl/src/rcl/init.c @@ -154,7 +154,7 @@ rcl_init( // goto fail; } // rcutils_atomic_store((atomic_uint_least64_t *)(&context->instance_id_storage), next_instance_id); - context->impl->instance_id_storage = next_instance_id; + context->instance_id_storage = next_instance_id; context->impl->init_options.impl->rmw_init_options.instance_id = next_instance_id; size_t * domain_id = &context->impl->init_options.impl->rmw_init_options.domain_id; From 1f366c3298cd712b0dfdc64d11440ea1f9a4fce2 Mon Sep 17 00:00:00 2001 From: acuadros95 Date: Tue, 3 May 2022 09:52:13 +0200 Subject: [PATCH 3/5] Clean PR --- rcl/include/rcl/context.h | 6 ------ rcl/src/rcl/context.c | 8 -------- rcl/src/rcl/init.c | 5 ----- 3 files changed, 19 deletions(-) diff --git a/rcl/include/rcl/context.h b/rcl/include/rcl/context.h index 05b067fd8..6df587a35 100644 --- a/rcl/include/rcl/context.h +++ b/rcl/include/rcl/context.h @@ -126,11 +126,6 @@ typedef struct rcl_context_t // The assumption that this is big enough for an atomic_uint_least64_t is // ensured with a static_assert in the context.c file. // In most cases it should just be a plain uint64_t. -/// @cond Doxygen_Suppress -// #if !defined(RCL_CONTEXT_ATOMIC_INSTANCE_ID_STORAGE_SIZE) -// #define RCL_CONTEXT_ATOMIC_INSTANCE_ID_STORAGE_SIZE sizeof(uint_least64_t) -// #endif -/// @endcond /// Private storage for instance ID atomic. /** * Accessing the instance id should be done using the function @@ -150,7 +145,6 @@ typedef struct rcl_context_t * See this paper for an effort to make this possible in the future: * http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2018/p0943r1.html */ - // RCL_ALIGNAS(8) uint8_t instance_id_storage[RCL_CONTEXT_ATOMIC_INSTANCE_ID_STORAGE_SIZE]; uint32_t instance_id_storage; } rcl_context_t; diff --git a/rcl/src/rcl/context.c b/rcl/src/rcl/context.c index 74e63007a..c2f6aaa86 100644 --- a/rcl/src/rcl/context.c +++ b/rcl/src/rcl/context.c @@ -36,12 +36,6 @@ rcl_get_zero_initialized_context(void) #ifdef RCL_COMMAND_LINE_ENABLED context.global_arguments = rcl_get_zero_initialized_arguments(); #endif // RCL_COMMAND_LINE_ENABLED - // ensure assumption about static storage - // static_assert( - // sizeof(context.instance_id_storage) >= sizeof(atomic_uint_least64_t), - // "expected rcl_context_t's instance id storage to be >= size of atomic_uint_least64_t"); - // // initialize atomic - // atomic_init((atomic_uint_least64_t *)(&context.instance_id_storage), 0); return context; } @@ -78,7 +72,6 @@ rcl_context_instance_id_t rcl_context_get_instance_id(const rcl_context_t * context) { RCL_CHECK_ARGUMENT_FOR_NULL(context, 0); - // return rcutils_atomic_load_uint64_t((atomic_uint_least64_t *)(&context->instance_id_storage)); return context->instance_id_storage; } @@ -113,7 +106,6 @@ __cleanup_context(rcl_context_t * context) { rcl_ret_t ret = RCL_RET_OK; // reset the instance id to 0 to indicate "invalid" (should already be 0, but this is defensive) - // rcutils_atomic_store((atomic_uint_least64_t *)(&context->instance_id_storage), 0); context->instance_id_storage = 0; #ifdef RCL_COMMAND_LINE_ENABLED diff --git a/rcl/src/rcl/init.c b/rcl/src/rcl/init.c index 2604d5b35..af5db265c 100644 --- a/rcl/src/rcl/init.c +++ b/rcl/src/rcl/init.c @@ -46,8 +46,6 @@ extern "C" #include "./context_impl.h" #include "./init_options_impl.h" -// static atomic_uint_least64_t __rcl_next_unique_id = ATOMIC_VAR_INIT(1); - rcl_ret_t rcl_init( int argc, @@ -142,7 +140,6 @@ rcl_init( #endif // RCL_COMMAND_LINE_ENABLED // Set the instance id. - // uint64_t next_instance_id = rcutils_atomic_fetch_add_uint64_t(&__rcl_next_unique_id, 1); static uint32_t next_instance_id = 0; next_instance_id++; if (0 == next_instance_id) { @@ -153,7 +150,6 @@ rcl_init( // rcutils_atomic_store(&__rcl_next_unique_id, -1); // goto fail; } - // rcutils_atomic_store((atomic_uint_least64_t *)(&context->instance_id_storage), next_instance_id); context->instance_id_storage = next_instance_id; context->impl->init_options.impl->rmw_init_options.instance_id = next_instance_id; @@ -265,7 +261,6 @@ rcl_shutdown(rcl_context_t * context) } // reset the instance id to 0 to indicate "invalid" - // rcutils_atomic_store((atomic_uint_least64_t *)(&context->instance_id_storage), 0); context->instance_id_storage = 0; return RCL_RET_OK; From 36350a5ab4fe1ab908882fd0fa8b07de6c027d1d Mon Sep 17 00:00:00 2001 From: acuadros95 Date: Mon, 9 May 2022 08:32:25 +0200 Subject: [PATCH 4/5] Clean id rollover check --- rcl/src/rcl/init.c | 8 -------- 1 file changed, 8 deletions(-) diff --git a/rcl/src/rcl/init.c b/rcl/src/rcl/init.c index af5db265c..25f26d4ee 100644 --- a/rcl/src/rcl/init.c +++ b/rcl/src/rcl/init.c @@ -142,14 +142,6 @@ rcl_init( // Set the instance id. static uint32_t next_instance_id = 0; next_instance_id++; - if (0 == next_instance_id) { - next_instance_id++; - // // Roll over occurred, this is an extremely unlikely occurrence. - // RCL_SET_ERROR_MSG("unique rcl instance ids exhausted"); - // // Roll back to try to avoid the next call succeeding, but there's a data race here. - // rcutils_atomic_store(&__rcl_next_unique_id, -1); - // goto fail; - } context->instance_id_storage = next_instance_id; context->impl->init_options.impl->rmw_init_options.instance_id = next_instance_id; From a5d197d0029b67d59998f60d82ab3775274c5a7f Mon Sep 17 00:00:00 2001 From: acuadros95 Date: Mon, 9 May 2022 09:18:17 +0200 Subject: [PATCH 5/5] Fix rollover --- rcl/src/rcl/init.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/rcl/src/rcl/init.c b/rcl/src/rcl/init.c index 25f26d4ee..67e70ecf1 100644 --- a/rcl/src/rcl/init.c +++ b/rcl/src/rcl/init.c @@ -142,6 +142,10 @@ rcl_init( // Set the instance id. static uint32_t next_instance_id = 0; next_instance_id++; + if (0 == next_instance_id) { + // Avoid invalid value on roll over + next_instance_id++; + } context->instance_id_storage = next_instance_id; context->impl->init_options.impl->rmw_init_options.instance_id = next_instance_id;