svn commit: r232456 - projects/armv6/sys/arm/include

[Bruce Evans]

On Sat, 3 Mar 2012, Olivier Houchard wrote:

> Log:
>  Get the right casts for long operations

I think you mean "the wrong casts".  i386 was broken similarly, but amd64
is still correct -- amd64 has no casts at all in its big list of #defines
(but the list is excessively complete, with must cases unused).

> Modified: projects/armv6/sys/arm/include/atomic.h
> ==============================================================================
> --- projects/armv6/sys/arm/include/atomic.h	Sat Mar  3 11:53:35 2012	(r232455)
> +++ projects/armv6/sys/arm/include/atomic.h	Sat Mar  3 12:23:07 2012	(r232456)
> @@ -505,35 +505,47 @@ atomic_store_32(volatile uint32_t *dst,
> 	*dst = src;
> }
>
> -#define	atomic_add_long(p, v) \
> +#define	atomic_set_long(p, v)						\
> +	atomic_set_32((volatile u_int *)(p), (u_int)(v))
> +#define	atomic_set_acq_long(p, v)					\
> +	atomic_set_acq_32((volatile u_int *)(p), (u_int)(v))

Casts like this mostly just defeat type checking.  On amd64,
atomic_set_32 is just atomic_set_int (without even parameters).

There is a minor problem with signedness.  With proper type checking,
the above will create about as many problems as it hides, by turning
a pointer to a signed long or int into a pointer to a u_int.  This
problem already occurs at the inline function level.

There is a problem with int having the same size as long.  It is a bug
for atomic ops on longs to even exist on arches where int has the same
size as long, since atomic ops on longs are useless in MD code on
such arches and unusable in MI code on any arches (since longs should
be 2 words and thus not naturally atomic).  Given this bug, the correct
way to support it is to have separate functions for ints and longs at
the lowest level, the same as when longs are longer than ints except
the contents of the separate functions is essentially indentical when
the type sizes are the same.  Then there is no need to break type checking
by pretending that ints are longs or vice versa.

> #define atomic_clear_ptr		atomic_clear_32

There is a better case for type puns of pointer types.  It's interesting
that you didn't changes anything with _ptr.  i386 uses bogus casts for
pointers too.  E.g.:

% #define	atomic_set_ptr(p, v) \
% 	atomic_set_int((volatile u_int *)(p), (u_int)(v))

It is probably a bug for any atomic ops on pointers to exist.  In
fact, none do, but the ops are spelled with "ptr".  From atomic(9):

%    Types
%      Each atomic operation operates on a specific type.  The type to use is
%      indicated in the function name.  The available types that can be used
%      are:
% 
%            int    unsigned integer
%            long   unsigned long integer
%            ptr    unsigned integer the size of a pointer
%            32     unsigned 32-bit integer
%            64     unsigned 64-bit integer

Note that `ptr' is not a pointer type, but is uintptr_t described verbosely.

The bogus cast in the i386 version rewards broken code that starts with
a pointer type.  Fortunately, there shouldn't be much such code, since
amd64 is still pure and will detect the type mismatch.

% 
%      For example, the function to atomically add two integers is called
%      atomic_add_int().
% 
%      Certain architectures also provide operations for types smaller than
%      ``int''.
% 
%            char   unsigned character
%            short  unsigned short integer
%            8      unsigned 8-bit integer
%            16     unsigned 16-bit integer
% 
%      These must not be used in MI code because the instructions to implement
%      them efficiently may not be available.

It should say this for `long' and `64' too.  long would be non-atomic on
any arch with correctly-sized longs, and `64' already isn't natural
atomic on 32-bit arches.  On i386, `64' is only available for a limited
set of atomic ops (only load/store I think).  This restriction of course
isn't documented above.

% ...
%      The type ``64'' is currently not implemented for any of the atomic opera-
%      tions on the arm, i386, and powerpc architectures.

The above restiction is partly documented below (that is here).  This is wrong
too.  `64' is now implemented for _some_ atomic ops on i386.

% EXAMPLES
%      This example uses the atomic_cmpset_acq_ptr() and atomic_set_ptr() func-
%      tions to obtain a sleep mutex and handle recursion.  Since the mtx_lock
%      member of a struct mtx is a pointer, the ``ptr'' type is used.

This bad example just echoes an old version of the mutex code.  It is so
old that it is wrong.  The mtx_lock member is _not_ a pointer -- it
complies with the earlier documentation and is a uintptr_t.

Bruce