svn commit: r279338 - head/sys/arm/include

Bruce Evans brde at optusnet.com.au
Sat Feb 28 01:38:12 UTC 2015


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


More information about the svn-src-head mailing list