Date: Fri, 3 Dec 2010 22:37:16 +0300 From: Eygene Ryabinkin <rea@freebsd.org> To: Bruce Evans <brde@optusnet.com.au> Cc: svn-src-head@FreeBSD.org, svn-src-all@FreeBSD.org, src-committers@FreeBSD.org, Bruce Cran <brucec@FreeBSD.org> Subject: Re: svn commit: r216143 - in head: share/man/man9 sys/amd64/include sys/arm/include sys/i386/include sys/ia64/include sys/mips/include sys/pc98/include sys/powerpc/include sys/sparc64/include sys/sun4v... Message-ID: <cEbDex9rRdSmKUpxrYLwdIVvssg@QsmfhJNucgI88DfvPJdT1/nyboE> In-Reply-To: <20101204045754.T4046@besplex.bde.org> References: <201012030709.oB379NOH058721@svn.freebsd.org> <20101204045754.T4046@besplex.bde.org>
next in thread | previous in thread | raw e-mail | index | archive | help
Sat, Dec 04, 2010 at 05:01:55AM +1100, Bruce Evans wrote:
> On Fri, 3 Dec 2010, Bruce Cran wrote:
> > Log:
> > Revert r216134. This checkin broke platforms where bus_space are macros:
> > they need to be a single statement, and do { } while (0) doesn't
> > work in this situation so revert until a solution can be devised.
>
> Surprising that do-while doesn't work.
Prior to the revert, something like "a = bus_space_read_multi_1(...)"
will generate improper code like "a = KASSERT(); __bs_nonsigle(XXX);"
and making "do { KASSERT(); __bs_nonsingle(XXX); } while(0)" won't
help either, since we can't generally assign the compound statement
to the lvalue.
> > Modified: head/sys/arm/include/bus.h
> > ==============================================================================
> > --- head/sys/arm/include/bus.h Fri Dec 3 07:01:07 2010 (r216142)
> > +++ head/sys/arm/include/bus.h Fri Dec 3 07:09:23 2010 (r216143)
> > ...
> > @@ -321,29 +318,21 @@ struct bus_space {
> > * Bus read multiple operations.
> > */
> > #define bus_space_read_multi_1(t, h, o, a, c) \
> > - KASSERT(c != 0, ("bus_space_read_multi_1: count == 0")); \
> > __bs_nonsingle(rm,1,(t),(h),(o),(a),(c))
>
> I just noticed the following possibly more serious problems for the macro
> versions:
>
> - the `c' arg is missing parentheses in the KASSERT()
> - the `c' arg is now evaluated twice. This turns safe macros into unsafe
> ones.
Perhaps we can define the macros as
{{{
#define bus_space_read_multi_1(t, h, o, a, c) ({\
size_t count = (c); \
KASSERT(count != 0, ("bus_space_read_multi_1: count == 0")); \
__bs_nonsingle(rm,1,(t),(h),(o),(a),count); \
})
}}}
This will both allow to avoid unsafety and will make this statement
to be the correct assignment for any compiler that supports the
"braced-groups within expressions" GNU extension. GNU C, Clang
and Intel C both support it (but not with -pedantic -ansi -Werror
flag combo).
But, probably, the inline function will be better here from the
portability point of view, since it is supported by the C standard
and braced-groups -- aren't.
So, the question is "why these statements were made to be
macros at some platforms?".
--
Eygene Ryabinkin ,,,^..^,,,
[ Life's unfair - but root password helps! | codelabs.ru ]
[ 82FE 06BC D497 C0DE 49EC 4FF0 16AF 9EAE 8152 ECFB | freebsd.org ]
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?cEbDex9rRdSmKUpxrYLwdIVvssg>