Skip to content

CMAKE: Bring back checks for atomic CAS#595

Merged
therault merged 1 commit intoICLDisco:masterfrom
devreal:fix-atomic128-lib
Jan 12, 2024
Merged

CMAKE: Bring back checks for atomic CAS#595
therault merged 1 commit intoICLDisco:masterfrom
devreal:fix-atomic128-lib

Conversation

@devreal
Copy link
Copy Markdown
Contributor

@devreal devreal commented Nov 14, 2023

Testing for atomic_is_lock_free() may not be enough as it may not require -latomic that is otherwise needed for 128bit atomics.

Reported by @DSMishler.

Testing for atomic_is_lock_free() may not be enough
as it may not require `-latomic` that is otherwise
needed for 128bit atomics.

Signed-off-by: Joseph Schuchart <schuchart@icl.utk.edu>
@devreal devreal requested a review from a team as a code owner November 14, 2023 18:21
int32_t where = 0;
int32_t where = 0, expected = 0;
if( !atomic_compare_exchange_strong( (_Atomic __int32_t*)&where, &expected, 1 ) )
return -1;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The original code returns 0 or 1. This patch makes it return 0 (success), 1 or -1. I guess 1 and -1 can be conflated to failure in both cases. POSIX specifies only that the 8 lower bits are meaningful (https://pubs.opengroup.org/onlinepubs/9699919799.2018edition/functions/V2_chap02.html#tag_15_13), so -1 is not clear.

May I suggest we use EXIT_SUCCESS and EXIT_FAILURE for all return/exit values, since those are POSIX compliant?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought about harmonizing them but then I realized that it might be helpful to have different return values for the two error cases. That way we can distinguish (in the CMake log) whether the operation is not lock-free or whether we failed to execute it in the first place...

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

return 1 and 2 then, that's probably more clear than 1 and 254

@bosilca
Copy link
Copy Markdown
Contributor

bosilca commented Nov 14, 2023

In what case this is necessary ? On my tests when using the libatomic is necessary, the associated intrinsics are automatically made available, and atomic_is_lock_free return the expected outcome.

@devreal
Copy link
Copy Markdown
Contributor Author

devreal commented Nov 14, 2023

I can only guess here based on @DSMishler issue: atomic_is_lock_free is a macro that may not need to call functions in libatomic even when CAS on 128bit integer would require libatomic to be present. So we never detect that we need to link -latomic.

Either way, these checks don't hurt and may help if a weird compiler may need -latomic for all atomic operations, but not for atomic_is_lock_free.

@abouteiller abouteiller added this to the v4.0 milestone Jan 12, 2024
@therault therault merged commit a23be09 into ICLDisco:master Jan 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants