Avoid uint64_t operation when handling context (#16) (#18)

* Update Signed-off-by: Pablo Garrido <pablogs9@gmail.com> Update Signed-off-by: Pablo Garrido <pablogs9@gmail.com> Update Signed-off-by: Pablo Garrido <pablogs9@gmail.com> Update Signed-off-by: Pablo Garrido <pablogs9@gmail.com> * Update Signed-off-by: Pablo Garrido <pablogs9@gmail.com> * Clean PR * Clean id rollover check * Fix rollover Co-authored-by: acuadros95 <acuadros1995@gmail.com> (cherry picked from commit 32aa288) Co-authored-by: Pablo Garrido <pablogs9@gmail.com>
micro-ROS · Jul 29, 2024 · a7e3df7 · a7e3df7
1 parent 908538e
commit a7e3df7
Show file tree

Hide file tree

Showing 3 changed files with 10 additions and 25 deletions.
diff --git 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
@@ -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
@@ -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;
 }