Date: Sun, 4 Mar 2012 02:14:46 +1100 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Olivier Houchard <cognet@freebsd.org> Cc: svn-src-projects@freebsd.org, src-committers@freebsd.org Subject: Re: svn commit: r232456 - projects/armv6/sys/arm/include Message-ID: <20120304011922.G5792@besplex.bde.org> In-Reply-To: <201203031223.q23CN73s081573@svn.freebsd.org> References: <201203031223.q23CN73s081573@svn.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20120304011922.G5792>