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

rS FreeBSD src repository - subversion

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 46452
Build 43341: arc lint + arc unit

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
99	You've lost the LP64 guard on just this one
102	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
102	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.