Date: Tue, 28 Mar 2006 21:28:02 +0200 From: des@des.no (Dag-Erling =?iso-8859-1?Q?Sm=F8rgrav?=) To: John Baldwin <jhb@freebsd.org> Cc: cvs-src@freebsd.org, src-committers@freebsd.org, cvs-all@freebsd.org Subject: Re: cvs commit: src/sys/i386/include atomic.h Message-ID: <86acbawerh.fsf@xps.des.no> In-Reply-To: <200603281358.26703.jhb@freebsd.org> (John Baldwin's message of "Tue, 28 Mar 2006 13:58:24 -0500") References: <200603281434.k2SEYmaR031447@repoman.freebsd.org> <200603281300.09419.jhb@freebsd.org> <86psk6wilj.fsf@xps.des.no> <200603281358.26703.jhb@freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
--=-=-=
Content-Type: text/plain; charset=iso-8859-1
Content-Transfer-Encoding: quoted-printable
John Baldwin <jhb@freebsd.org> writes:
> On Tuesday 28 March 2006 13:05, Dag-Erling Sm=F8rgrav wrote:
> > John Baldwin <jhb@freebsd.org> writes:
> > > One reason for not having the casts, btw, is that you lose type
> > > checking.
> > Huh? Before my patch, any use of atomic_*_ptr with warnings turned
> > off would result in a slew of warnings because you'd be passing
> > pointers to a function which is declared to take u_int. The only way
> > to make this type safe is to use inline functions instead of the
> > macros I wrote.
>
> s/off/on/ I trust
>
> Not true. The tinderbox would attest to that. Please see code such as
> this: [...]
which uses uintptr_t, not actual pointers, to avoid warnings. In
effect, that code is broken.
Apply the attached patch, see how far a buildkernel gets...
I think the proper thing to do, to cover all your bases, would be to
define a MD atomic_*_intptr family which operated on uintptr_t, and
define an MI atomic_*_ptr family which operates on void * based on
that.
> Even userland uses casts when it uses void * rather than uintptr_t for
> the underlying type. See src/lib/libpthread/sys/lock.c or
> src/lib/libthr/thr_umtx.h.
The latter only works because libthr is built with warnings disabled.
I just finished working on making it build at WARNS level 2; higher
levels will require a major overhaul, because the kernel interface it
uses is fundamentally broken.
DES
--=20
Dag-Erling Sm=F8rgrav - des@des.no
--=-=-=
Content-Type: text/x-patch
Content-Disposition: attachment; filename=i386-atomic.diff
Index: sys/i386/include/atomic.h
===================================================================
RCS file: /home/ncvs/src/sys/i386/include/atomic.h,v
retrieving revision 1.42
diff -u -r1.42 atomic.h
--- sys/i386/include/atomic.h 28 Mar 2006 14:34:48 -0000 1.42
+++ sys/i386/include/atomic.h 28 Mar 2006 19:24:17 -0000
@@ -415,42 +415,116 @@
#define atomic_fetchadd_32 atomic_fetchadd_int
/* Operations on pointers. */
-#define atomic_set_ptr(p, v) \
- atomic_set_int((volatile u_int *)(p), (u_int)(v))
-#define atomic_set_acq_ptr(p, v) \
- atomic_set_acq_int((volatile u_int *)(p), (u_int)(v))
-#define atomic_set_rel_ptr(p, v) \
- atomic_set_rel_int((volatile u_int *)(p), (u_int)(v))
-#define atomic_clear_ptr(p, v) \
- atomic_clear_int((volatile u_int *)(p), (u_int)(v))
-#define atomic_clear_acq_ptr(p, v) \
- atomic_clear_acq_int((volatile u_int *)(p), (u_int)(v))
-#define atomic_clear_rel_ptr(p, v) \
- atomic_clear_rel_int((volatile u_int *)(p), (u_int)(v))
-#define atomic_add_ptr(p, v) \
- atomic_add_int((volatile u_int *)(p), (u_int)(v))
-#define atomic_add_acq_ptr(p, v) \
- atomic_add_acq_int((volatile u_int *)(p), (u_int)(v))
-#define atomic_add_rel_ptr(p, v) \
- atomic_add_rel_int((volatile u_int *)(p), (u_int)(v))
-#define atomic_subtract_ptr(p, v) \
- atomic_subtract_int((volatile u_int *)(p), (u_int)(v))
-#define atomic_subtract_acq_ptr(p, v) \
- atomic_subtract_acq_int((volatile u_int *)(p), (u_int)(v))
-#define atomic_subtract_rel_ptr(p, v) \
- atomic_subtract_rel_int((volatile u_int *)(p), (u_int)(v))
-#define atomic_load_acq_ptr(p) \
- atomic_load_acq_int((volatile u_int *)(p))
-#define atomic_store_rel_ptr(p, v) \
- atomic_store_rel_int((volatile u_int *)(p), (v))
-#define atomic_cmpset_ptr(dst, old, new) \
- atomic_cmpset_int((volatile u_int *)(dst), (u_int)(old), (u_int)(new))
-#define atomic_cmpset_acq_ptr(dst, old, new) \
- atomic_cmpset_acq_int((volatile u_int *)(dst), (u_int)(old), (u_int)(new))
-#define atomic_cmpset_rel_ptr(dst, old, new) \
- atomic_cmpset_rel_int((volatile u_int *)(dst), (u_int)(old), (u_int)(new))
-#define atomic_readandclear_ptr(p) \
- atomic_readandclear_int((volatile u_int *)(p))
+/* Implemented as inline functions for the sake of type safety */
+static __inline void
+atomic_set_ptr(volatile void **p, void *v)
+{
+ atomic_set_int((volatile uintptr_t *)p, (uintptr_t)v);
+}
+
+static __inline void
+atomic_set_acq_ptr(volatile void **p, void *v)
+{
+ atomic_set_acq_int((volatile uintptr_t *)p, (uintptr_t)v);
+}
+
+static __inline void
+atomic_set_rel_ptr(volatile void **p, void *v)
+{
+ atomic_set_rel_int((volatile uintptr_t *)p, (uintptr_t)v);
+}
+
+static __inline void
+atomic_clear_ptr(volatile void **p, void *v)
+{
+ atomic_clear_int((volatile uintptr_t *)p, (uintptr_t)v);
+}
+
+static __inline void
+atomic_clear_acq_ptr(volatile void **p, void *v)
+{
+ atomic_clear_acq_int((volatile uintptr_t *)p, (uintptr_t)v);
+}
+
+static __inline void
+atomic_clear_rel_ptr(volatile void **p, void *v)
+{
+ atomic_clear_rel_int((volatile uintptr_t *)p, (uintptr_t)v);
+}
+
+static __inline void
+atomic_add_ptr(volatile void **p, void *v)
+{
+ atomic_add_int((volatile uintptr_t *)p, (uintptr_t)v);
+}
+
+static __inline void
+atomic_add_acq_ptr(volatile void **p, void *v)
+{
+ atomic_add_acq_int((volatile uintptr_t *)p, (uintptr_t)v);
+}
+
+static __inline void
+atomic_add_rel_ptr(volatile void **p, void *v)
+{
+ atomic_add_rel_int((volatile uintptr_t *)p, (uintptr_t)v);
+}
+
+static __inline void
+atomic_subtract_ptr(volatile void **p, void *v)
+{
+ atomic_subtract_int((volatile uintptr_t *)p, (uintptr_t)v);
+}
+
+static __inline void
+atomic_subtract_acq_ptr(volatile void **p, void *v)
+{
+ atomic_subtract_acq_int((volatile uintptr_t *)p, (uintptr_t)v);
+}
+
+static __inline void
+atomic_subtract_rel_ptr(volatile void **p, void *v)
+{
+ atomic_subtract_rel_int((volatile uintptr_t *)p, (uintptr_t)v);
+}
+
+static __inline void *
+atomic_load_acq_ptr(volatile void **p)
+{
+ return ((void *)(uintptr_t)atomic_load_acq_int((volatile uintptr_t *)p));
+}
+
+static __inline void
+atomic_store_rel_ptr(volatile void **p, void *v)
+{
+ atomic_store_rel_int((volatile uintptr_t *)p, (uintptr_t)v);
+}
+
+static __inline int
+atomic_cmpset_ptr(volatile void **dst, void *old, void *new)
+{
+ return (atomic_cmpset_int((volatile uintptr_t *)dst, (uintptr_t)old, (uintptr_t)new));
+}
+
+static __inline int
+atomic_cmpset_acq_ptr(volatile void **dst, void *old, void *new)
+{
+ return (atomic_cmpset_acq_int((volatile uintptr_t *)dst,
+ (uintptr_t)old, (uintptr_t)new));
+}
+
+static __inline int
+atomic_cmpset_rel_ptr(volatile void **dst, void *old, void *new)
+{
+ return (atomic_cmpset_rel_int((volatile uintptr_t *)dst,
+ (uintptr_t)old, (uintptr_t)new));
+}
+
+static __inline void *
+atomic_readandclear_ptr(volatile void **p)
+{
+ return ((void *)(uintptr_t)atomic_readandclear_int((volatile uintptr_t *)p));
+}
#endif /* !defined(WANT_FUNCTIONS) */
#endif /* ! _MACHINE_ATOMIC_H_ */
--=-=-=--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?86acbawerh.fsf>