Page MenuHomeFreeBSD

atomic: Define inline functions for atomic_load/store
ClosedPublic

Authored by markj on Jul 15 2022, 10:26 PM.
Tags
None
Referenced Files
Unknown Object (File)
Sat, Nov 30, 3:53 PM
Unknown Object (File)
Sat, Nov 23, 10:29 AM
Unknown Object (File)
Fri, Nov 22, 8:08 PM
Unknown Object (File)
Fri, Nov 22, 6:10 PM
Unknown Object (File)
Mon, Nov 18, 4:54 PM
Unknown Object (File)
Mon, Nov 18, 4:01 PM
Unknown Object (File)
Nov 10 2024, 1:27 AM
Unknown Object (File)
Nov 6 2024, 5:48 AM
Subscribers

Details

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 46469
Build 43358: arc lint + arc unit

Event Timeline

This revision is now accepted and ready to land.Jul 15 2022, 11:59 PM

I think kasan builds need similar treatment, otherwise LGTM.

Now that I wrote I think an extra bit would be nice: checking alignment

This depends on specific compiler, its error settings, and user' override of system defaults. In particular, for clang it requires -Werror -Wincompatible-pointer-types or like.

I think a better way would be to do something like this:

#define atomic_load_int(p) ({				\
    _Static_assert(sizeof(*p) == sizeof(int), "XXX");	\
    (*(volatile int *)p);				\
})

Or even better, assuming that the header is used with C11, is to be more portable and use the generic selector, which checks all correctness assumptions, including alignment:

#define atomic_load_int(p) ({				\
    _Static_assert(_Generic(*p,				\
    int: 1, unsigned int: 1, default: 0) == 1, "XXX");	\
     (*(volatile int *)p);				\
})

Provide a _Generic-based version when possible.

This revision now requires review to proceed.Jul 18 2022, 4:34 PM
In D35828#813346, @kib wrote:

This depends on specific compiler, its error settings, and user' override of system defaults. In particular, for clang it requires -Werror -Wincompatible-pointer-types or like.

-Wpointer-sign, I think.

I tried writing an implementation using _Generic. It's not the same as what you suggested, it avoids _Static_assert since the compiler will already complain if _Generic is unmatched.

This revision is now accepted and ready to land.Jul 18 2022, 5:13 PM
sys/sys/atomic_common.h
103

You've lost the LP64 guard on just this one

106

Why not just do:

#define __atomic_load_generic(p, t, ut, n) __atomic_load_ ## n (p)
#define __atomic_store_generic(p, v, t, ut, n) __atomic_load_ ## n (p)

in the !C11 case and avoid duplicating all the per-type defines?

sys/sys/atomic_common.h
106

Uh

#define __atomic_load_generic(p, t, ut, n) __atomic_load_ ## n (p)
#define __atomic_store_generic(p, v, t, ut, n) __atomic_store_ ## n (p, v)

of course...

this breaks buildworld on all archs:

/usr/src/contrib/llvm-project/compiler-rt/lib/builtins/atomic.c:272:1: error: too many arguments provided to function-
like macro invocation
OPTIMISED_CASES
^
/usr/src/contrib/llvm-project/compiler-rt/lib/builtins/atomic.c:259:3: note: expanded from macro 'OPTIMISED_CASES'
  OPTIMISED_CASE(8, IS_LOCK_FREE_8, uint64_t)
  ^
/usr/src/contrib/llvm-project/compiler-rt/lib/builtins/atomic.c:263:37: note: expanded from macro 'OPTIMISED_CASE'
  type __atomic_load_##n(type *src, int model) {                               \
                                    ^
/usr/obj/usr/src/powerpc.powerpc/tmp/usr/include/sys/atomic_common.h:46:9: note: macro '__atomic_load_8' defined here
#define __atomic_load_8(p)      (*(volatile uint8_t *)(p))
        ^
/usr/src/contrib/llvm-project/compiler-rt/lib/builtins/atomic.c:272:1: error: expected ';' after top level declarator
OPTIMISED_CASES
^
/usr/src/contrib/llvm-project/compiler-rt/lib/builtins/atomic.c:259:3: note: expanded from macro 'OPTIMISED_CASES'
  OPTIMISED_CASE(8, IS_LOCK_FREE_8, uint64_t)
  ^
/usr/src/contrib/llvm-project/compiler-rt/lib/builtins/atomic.c:263:48: note: expanded from macro 'OPTIMISED_CASE'
  type __atomic_load_##n(type *src, int model) {                               \
                                               ^
/usr/src/contrib/llvm-project/compiler-rt/lib/builtins/atomic.c:287:1: error: too many arguments provided to function-like macro invocation
OPTIMISED_CASES
^
/usr/src/contrib/llvm-project/compiler-rt/lib/builtins/atomic.c:259:3: note: expanded from macro 'OPTIMISED_CASES'
  OPTIMISED_CASE(8, IS_LOCK_FREE_8, uint64_t)
  ^
/usr/src/contrib/llvm-project/compiler-rt/lib/builtins/atomic.c:276:49: note: expanded from macro 'OPTIMISED_CASE'
  void __atomic_store_##n(type *dest, type val, int model) {                   \
                                                ^
/usr/obj/usr/src/powerpc.powerpc/tmp/usr/include/sys/atomic_common.h:59:9: note: macro '__atomic_store_8' defined here
#define __atomic_store_8(p, v)                  \
        ^
3 errors generated.
markj marked 3 inline comments as done.
  • Apply suggestions.
  • Rename the base primitives to avoid conflicting with libcompiler_rt.
This revision now requires review to proceed.Jul 18 2022, 7:48 PM
This revision is now accepted and ready to land.Jul 18 2022, 9:18 PM