On Fri, 27 Feb 2015, Ian Lepore wrote:
On Fri, 2015-02-27 at 13:35 +1100, Bruce Evans wrote:
On Thu, 26 Feb 2015, Ian Lepore wrote:
Log:
Add casting to make atomic ops work for pointers. (Apparently nobody has
ever done atomic ops on pointers before now on arm).
Apparently, arm code handled pointers correctly before. des broke
i386 in the same way and didn't back out the changes as requested, but
all other arches including amd64 remained unbroken, so there can't be
any MI code that handles the pointers incorrectly enough to "need" it.
Checking shows no MD code either.
Modified: head/sys/arm/include/atomic.h
==============================================================================
--- head/sys/arm/include/atomic.h Thu Feb 26 22:46:01 2015
(r279337)
+++ head/sys/arm/include/atomic.h Thu Feb 26 23:05:46 2015
(r279338)
@@ -1103,13 +1103,23 @@ atomic_store_long(volatile u_long *dst,
*dst = src;
}
-#define atomic_clear_ptr atomic_clear_32
-#define atomic_set_ptr atomic_set_32
-#define atomic_cmpset_ptr atomic_cmpset_32
-#define atomic_cmpset_rel_ptr atomic_cmpset_rel_32
-#define atomic_cmpset_acq_ptr atomic_cmpset_acq_32
-#define atomic_store_ptr atomic_store_32
-#define atomic_store_rel_ptr atomic_store_rel_32
+#define atomic_clear_ptr(p, v) \
+ atomic_clear_32((volatile uint32_t *)(p), (uint32_t)(v))
+#define atomic_set_ptr(p, v) \
+ atomic_set_32((volatile uint32_t *)(p), (uint32_t)(v))
+#define atomic_cmpset_ptr(p, cmpval, newval) \
+ atomic_cmpset_32((volatile u_int32_t *)(p), (u_int32_t)(cmpval), \
+ (u_int32_t)(newval))
+#define atomic_cmpset_rel_ptr(p, cmpval, newval) \
+ atomic_cmpset_rel_32((volatile u_int32_t *)(p), (u_int32_t)(cmpval), \
+ (u_int32_t)(newval))
+#define atomic_cmpset_acq_ptr(p, cmpval, newval) \
+ atomic_cmpset_acq_32((volatile u_int32_t *)(p), (u_int32_t)(cmpval), \
+ (u_int32_t)(newval))
+#define atomic_store_ptr(p, v) \
+ atomic_store_32((volatile uint32_t *)(p), (uint32_t)(v))
+#define atomic_store_rel_ptr(p, v) \
+ atomic_store_rel_32((volatile uint32_t *)(p), (uint32_t)(v))
#define atomic_add_int atomic_add_32
#define atomic_add_acq_int atomic_add_acq_32
These bogus casts reduce type safety. atomic*ptr is designed and
documented to take only (indirect) uintptr_t * and (direct) uintptr_t
args, not pointer args bogusly cast or otherwise type-punned to these.
Most callers actually use the API correctly. E.g., the mutex cookie
is a uintptr_t, not a pointer. This affected the design of mutexes
a little. The cookie could be either a pointer representing an integer
ot an integer representing a pointer, or a union of these, and the
integer is simplest.
These casts don't even break the warning in most cases where they have
an effect. They share this implementation bug with the i386 ones.
Casting to [volatile] uint32_t * only works if the pointer is already
[volatile] uint32_t * or [volatile] void *. For full breakage, it
us necessary to cast to volatile void * first. Omitting the cast to
void * should only work here because most or all callers ensure that
the pointer already has one of these types, so the cast here has no
effect.
Casting the input arg to uint32_t does work to break the warning,
because uint32_t is the same as uintptr_t and -Wcast-qual is broken
for casting away qualifiers in this way.
Here are all matches with atomic.*ptr in .c files in /sys/:
...
Summary: there are about 97 callers of atomic*ptr(). About 80 of them use
it correctly. Unless I missed something, all of the other 17 already
supply essentially the same bogus casts that this commits adds, as needed
for them to compile on amd64. So the change has no effect now. Similary
on i386. The also can't have any effect in future except in MD arm and
i386 code unless other arches are broken to march. Then the number of
cases with bogus casts could easily expand from 17.
::sigh:: As usual, thousands of words, maybe there's actionable info in
there, but I sure don't have time to ferret it out.
If there's something simple you'd like me to do, please say so. Simply.
Just back out the change. For extra credit, back it out for i386 too.
For too much work, fix the 17 calls with bogus casts.
Bruce
_______________________________________________
svn-src-head@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"
