Date: Tue, 6 Mar 2012 15:58:36 +1100 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Olivier Houchard <cognet@ci0.org> Cc: svn-src-projects@FreeBSD.org, src-committers@FreeBSD.org, Bruce Evans <brde@optusnet.com.au> Subject: Re: svn commit: r232456 - projects/armv6/sys/arm/include Message-ID: <20120306140646.S940@besplex.bde.org> In-Reply-To: <20120303165737.GA26775@ci0.org> References: <201203031223.q23CN73s081573@svn.freebsd.org> <20120304011922.G5792@besplex.bde.org> <20120303165737.GA26775@ci0.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Sat, 3 Mar 2012, Olivier Houchard wrote:
> On Sun, Mar 04, 2012 at 02:14:46AM +1100, Bruce Evans wrote:
>> On Sat, 3 Mar 2012, Olivier Houchard wrote:
Sorry this reply took so long.
>>> 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).
>
> I understand your concerns. These casts are indeed bogus, and will become even
> more when we'll support 64bits arm, which should come with 64bits long.
Won't they just fail then, and need separate functions which mostly make
the problem go away, as on amd64? The patch seems to add a lot of 64-bit
support, which confused me at first.
> I can't do much for long which should be 64bits even on 32bits machines, that
> is set in stone now, however I can certainly remove the bogus casts.
>
> Would the attached patch be OK for you ?
> It duplicates the various atomic functions to add a _long variant (for armv6
> at least, for armv5 it just introduces _long variants which calls the _32
> version, but at least it should catch any signedness/type error), and it
> removes the bogus casts for the ptr version, and just #defines it to the __32
> version, since that's what uintptr_t is.
This seems to be essentially correct -- don't use blind casts in macros,
and don't duplicate the functions (at least large ones), but provide
wrappers implemented as inline functions. The wrappers do the same thing
as the macros, but in a type-safe way. Perhaps they can be generated
a bit more automatically or otherwise reduced to 1 line each, but I don't
want to use complicated macros for this.
% Index: atomic.h
% ===================================================================
% --- atomic.h (revision 232462)
% +++ atomic.h (working copy)
% @@ -74,6 +74,21 @@
% #endif
% }
%
% +#define ATOMIC_ACQ_REL_LONG(NAME) \
% +static __inline void \
% +atomic_##NAME##_acq_long(__volatile u_long *p, u_long v) \
% +{ \
% + atomic_##NAME##_long(p, v); \
% + __do_dmb(); \
% +} \
% + \
% +static __inline void \
% +atomic_##NAME##_rel_long(__volatile u_long *p, u_long v) \
% +{ \
% + __do_dmb(); \
% + atomic_##NAME##_long(p, v); \
% +}
% +
% #define ATOMIC_ACQ_REL(NAME, WIDTH) \
% static __inline void \
% atomic_##NAME##_acq_##WIDTH(__volatile uint##WIDTH##_t *p, uint##WIDTH##_t v)\
Small functions can be done like this, by duplicating the code for
longs, and putting it in macros, but I think the above already has too
much macro-ization. Code like this is hard to read, and probably not
even much shorter and easier to write, if you organize the wrappers
in the best way.
% ...
% @@ -489,9 +649,64 @@
% #define atomic_subtract_rel_32 atomic_subtract_32
% #define atomic_subtract_acq_32 atomic_subtract_32
% #define atomic_store_rel_32 atomic_store_32
% +#define atomic_store_rel_long atomic_store_long
% #define atomic_load_acq_32 atomic_load_32
% +#define atomic_load_acq_long atomic_load_long
% #undef __with_interrupts_disabled
%
% +static __inline void
% +atomic_add_long(volatile u_long *p, u_long v)
% +{
% +
% + atomic_add_32((volatile uint32_t *)p, (volatile uint32_t)v);
% +}
Most of the previous changes seem to be for adding 64-bit longs. Now I
think we're back in the 32-bit long case, with wrappers like I want
(more verbose than I want).
Casting v seems bogus. How can a function parameter that is passed by
value be volatile? Old arm code has just 2 simimlar volatiles,
both for the non-pointer parameter in 2 cmpset_32's. Maybe it needs
to be volatile in the asm that accesses it, but I doubt this, and
casting it here doesn't make it volatile there. amd64 only uses
volatiles for pointed-to variables.
% ...
% #endif /* _LOCORE */
LOCORE ifdefs in atomic.h are bogus, and were removed on amd64. The
comment on this one is backwards, at least in the main arm tree (the
code says #ifndef, not #ifdef). Adding C functions unused LOCORE part
of the code ensure that it is unusable as well as unused in asm files.
Bruce
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20120306140646.S940>