From cb743455658b4e6fed935817a5df960f3da1210a Mon Sep 17 00:00:00 2001 From: "mergify[bot]" <37929162+mergify[bot]@users.noreply.github.com> Date: Mon, 9 May 2022 09:27:33 +0200 Subject: [PATCH] Avoid uint64_t operation when handling context (#16) (#18) * Update Signed-off-by: Pablo Garrido Update Signed-off-by: Pablo Garrido Update Signed-off-by: Pablo Garrido Update Signed-off-by: Pablo Garrido * Update Signed-off-by: Pablo Garrido * Clean PR * Clean id rollover check * Fix rollover Co-authored-by: acuadros95 (cherry picked from commit 32aa288817ee54473213ec6d4c6c075c0135c682) Co-authored-by: Pablo Garrido --- rcl/include/rcl/context.h | 7 +------ rcl/src/rcl/context.c | 12 +++--------- rcl/src/rcl/init.c | 16 ++++++---------- 3 files changed, 10 insertions(+), 25 deletions(-) diff --git a/rcl/include/rcl/context.h b/rcl/include/rcl/context.h index 50061df3f4..f19e96cf09 100644 --- a/rcl/include/rcl/context.h +++ b/rcl/include/rcl/context.h @@ -126,11 +126,6 @@ typedef struct rcl_context_s // 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,7 @@ typedef struct rcl_context_s * 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; /// Return a zero initialization context object. diff --git a/rcl/src/rcl/context.c b/rcl/src/rcl/context.c index 67cd89d9f1..c2f6aaa863 100644 --- a/rcl/src/rcl/context.c +++ b/rcl/src/rcl/context.c @@ -30,18 +30,12 @@ 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); return context; } @@ -78,7 +72,7 @@ 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; } rcl_ret_t @@ -112,7 +106,7 @@ __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 // clean up global_arguments if initialized diff --git a/rcl/src/rcl/init.c b/rcl/src/rcl/init.c index f6d4e9607d..c953ff076c 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,15 +140,13 @@ 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) { - // 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; + // Avoid invalid value on roll over + next_instance_id++; } - 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; size_t * domain_id = &context->impl->init_options.impl->rmw_init_options.domain_id; @@ -261,7 +257,7 @@ 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; }