Skip site navigation (1)Skip section navigation (2)
Date:      Fri, 20 Aug 2010 08:20:57 +1000 (EST)
From:      Bruce Evans <brde@optusnet.com.au>
To:        Rui Paulo <rpaulo@freebsd.org>
Cc:        svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org
Subject:   Re: svn commit: r211505 - head/contrib/gcc
Message-ID:  <20100820075236.L18914@delplex.bde.org>
In-Reply-To: <201008191259.o7JCxv3i004613@svn.freebsd.org>
References:  <201008191259.o7JCxv3i004613@svn.freebsd.org>

next in thread | previous in thread | raw e-mail | index | archive | help
On Thu, 19 Aug 2010, Rui Paulo wrote:

> Log:
>  Remove unneeded casts in inline assembly in contrib/gcc/longlong.h,

These seem to be needed, and some of them valid.  Any lvalue arg that
can be put in a register can be cast to USItype (unsigned int) on i386.
The args are macro args, so they may have any integer type no larger
than USItype originally, and they must be cast to USItype for the asms
to work if the args are smaller than USItype...

>  which are apparently "heinous" GNU extensions, so clang can
>  compile this without using the -fheinous-gnu-extensions option.

But when the args are lvalues, casting them is invalid.  This is
apparently the heinous extension depended on.

>
>  Results in *no* binary change, neither with clang, nor with gcc.

This cannot be tested by compiling a few binaries, since the few binaries
might not include enough examples to give test coverage of cases with
args smaller than USItype.  Perhaps the macros are only used for building
libgcc, but this is unclear.

> Modified: head/contrib/gcc/longlong.h
> ==============================================================================
> --- head/contrib/gcc/longlong.h	Thu Aug 19 12:52:49 2010	(r211504)
> +++ head/contrib/gcc/longlong.h	Thu Aug 19 12:59:57 2010	(r211505)
> @@ -314,38 +314,38 @@ UDItype __umulsidi3 (USItype, USItype);
> #if (defined (__i386__) || defined (__i486__)) && W_TYPE_SIZE == 32
> #define add_ssaaaa(sh, sl, ah, al, bh, bl) \
>   __asm__ ("addl %5,%1\n\tadcl %3,%0"					\
> -	   : "=r" ((USItype) (sh)),					\
> -	     "=&r" ((USItype) (sl))					\

I can only see a problem with casting lvalues (2 here).  The corrected
version might use "USItype _sh = (sh); ... (sh) = _sh;", etc.

> -	   : "%0" ((USItype) (ah)),					\
> -	     "g" ((USItype) (bh)),					\
> -	     "%1" ((USItype) (al)),					\
> -	     "g" ((USItype) (bl)))
> +	   : "=r" (sh),							\
> +	     "=&r" (sl)							\
> +	   : "%0" (ah),							\
> +	     "g" (bh),							\
> +	     "%1" (al),							\
> +	     "g" (bl))
> #define sub_ddmmss(sh, sl, ah, al, bh, bl) \
>   __asm__ ("subl %5,%1\n\tsbbl %3,%0"					\
> -	   : "=r" ((USItype) (sh)),					\
> -	     "=&r" ((USItype) (sl))					\

Most of the asms have 2 problematic lvalues.

> -	   : "0" ((USItype) (ah)),					\

Casting this rvalue shouldn't cause problems, but it is unclear if "0"
can be the same arg as "=r" if they are cast differently.

> -	     "g" ((USItype) (bh)),					\
> -	     "1" ((USItype) (al)),					\
> -	     "g" ((USItype) (bl)))
> +	   : "=r" (sh),							\
> +	     "=&r" (sl)							\
> +	   : "0" (ah),							\
> +	     "g" (bh),							\
> +	     "1" (al),							\
> +	     "g" (bl))
> #define umul_ppmm(w1, w0, u, v) \
>   __asm__ ("mull %3"							\
> -	   : "=a" ((USItype) (w0)),					\
> -	     "=d" ((USItype) (w1))					\
> -	   : "%0" ((USItype) (u)),					\
> -	     "rm" ((USItype) (v)))
> +	   : "=a" (w0),							\
> +	     "=d" (w1)							\
> +	   : "%0" (u),							\
> +	     "rm" (v))
> #define udiv_qrnnd(q, r, n1, n0, dv) \
>   __asm__ ("divl %4"							\
> -	   : "=a" ((USItype) (q)),					\
> -	     "=d" ((USItype) (r))					\
> -	   : "0" ((USItype) (n0)),					\
> -	     "1" ((USItype) (n1)),					\
> -	     "rm" ((USItype) (dv)))
> +	   : "=a" (q),							\
> +	     "=d" (r)							\
> +	   : "0" (n0),							\
> +	     "1" (n1),							\
> +	     "rm" (dv))
> #define count_leading_zeros(count, x) \
>   do {									\
>     USItype __cbtmp;							\
>     __asm__ ("bsrl %1,%0"						\
> -	     : "=r" (__cbtmp) : "rm" ((USItype) (x)));			\
> +	     : "=r" (__cbtmp) : "rm" (x));				\

Here the lvalue is already a temporary variable, and there should be
no problem casting the rvalue.

>     (count) = __cbtmp ^ 31;						\
>   } while (0)
> #define count_trailing_zeros(count, x) \
>

Here the lvalue already wasn't casted, and the rvalue is still casted.

Bruce



Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20100820075236.L18914>