atomic: Define inline functions for atomic_load/store
ClosedPublic
Actions

Authored by markj on Jul 15 2022, 10:26 PM.

Details

Reviewers

mjg
jrtc27
kib
imp
rpokala

Commits

rG30253da1a982: atomic: Add some type checking to plain atomic_load/store helpers

Summary

This way we get some type checking.

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

markj created this revision.Jul 15 2022, 10:26 PM

Herald added a subscriber: imp. · View Herald TranscriptJul 15 2022, 10:26 PM

markj requested review of this revision.Jul 15 2022, 10:26 PM

Harbormaster completed remote builds in B46452: Diff 108215.Jul 15 2022, 10:26 PM

nice

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

rpokala accepted this revision.Jul 16 2022, 12:43 AM

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

Harbormaster completed remote builds in B46463: Diff 108285.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.

kib accepted this revision.Jul 18 2022, 5:13 PM

This revision is now accepted and ready to land.Jul 18 2022, 5:13 PM

jrtc27 added inline comments.Jul 18 2022, 5:39 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?

jrtc27 added inline comments.Jul 18 2022, 5:40 PM

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.