Skip to content

Commit

Permalink
Avoid uint64_t operation when handling context (#16) (#18)
Browse files Browse the repository at this point in the history
* 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>
  • Loading branch information
mergify[bot] and pablogs9 committed Mar 2, 2023
1 parent abe8c83 commit cb29508
Show file tree
Hide file tree
Showing 3 changed files with 10 additions and 25 deletions.
7 changes: 1 addition & 6 deletions rcl/include/rcl/context.h
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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.
Expand Down
12 changes: 3 additions & 9 deletions rcl/src/rcl/context.c
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}

Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down
16 changes: 6 additions & 10 deletions rcl/src/rcl/init.c
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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;
}
Expand Down

0 comments on commit cb29508

Please sign in to comment.