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)
Dec 15 2022, 7:37 AM
Unknown Object (File)
Dec 14 2022, 11:46 PM
Unknown Object (File)
Dec 2 2022, 8:00 AM
Subscribers

Details

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Not Applicable
Unit
Tests Not Applicable

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