svn commit: r233419 - head/sys/x86/include

[Bruce Evans]

On Sat, 24 Mar 2012, Dimitry Andric wrote:

> Log:
>  Fix the following clang warning in sys/dev/dcons/dcons.c, caused by the
>  recent changes in sys/x86/include/endian.h:
>
>    sys/dev/dcons/dcons.c:190:15: error: implicit conversion from '__uint32_t' (aka 'unsigned int') to '__uint16_t' (aka 'unsigned short') changes value from 1684238190 to 28526 [-Werror,-Wconstant-conversion]
> 
>  	  buf->magic = ntohl(DCONS_MAGIC);
>  		       ^~~~~~~~~~~~~~~~~~
>    sys/sys/param.h:306:18: note: expanded from:
>    #define ntohl(x)        __ntohl(x)
>  			  ^
>    ./x86/endian.h:128:20: note: expanded from:
>    #define __ntohl(x)      __bswap32(x)
>  			  ^
>    ./x86/endian.h:78:20: note: expanded from:
>  	      __bswap32_gen((__uint32_t)(x)) : __bswap32_var(x))
>  			    ^
>    ./x86/endian.h:68:26: note: expanded from:
>  	  (((__uint32_t)__bswap16(x) << 16) | __bswap16((x) >> 16))
>  				  ^
>    ./x86/endian.h:75:53: note: expanded from:
>  	      __bswap16_gen((__uint16_t)(x)) : __bswap16_var(x)))
>  					       ~~~~~~~~~~~~~ ^

This warning was discussed before things were committed.  gcc gives a
similar warning, but according to tijl, it can't happen in normal use
with either gcc or clang because -Wno-system-headers suppresses warnings
in system headers.  -Wnosystem-headers is the default in the kernel,
where the above problem happens, so I don't see how it happens.
However, bsd.sys.mk forces the non-default of -Wsystem-headers in
userland at WARNS >= 1, so I don't see why there isn't a problem in
userland if userland actually uses an expression like
ntohl(DCONS_MAGIC).

>  This is because the __bswapXX_gen() macros (for x86) call the regular
>  __bswapXX() macros.  Since the __bswapXX_gen() variants are only called
>  when their arguments are constant, there is no need to do that constancy
>  check recursively.  Also, it causes the above error with clang.

Not true.  The __bswapXX_gen() are called with non-constant args from
__bswap64_var() in the i386 case.  As documented there, it is important
for optimizations that __bswap64_gen() does do the constancy check
recursively.  This was discussed before things were committed, and
again when jhb siggested breaking it similarly to this commit to avoid
a problem on ia64.

>  Fix it by calling __bswap16_gen() from __bswap32_gen(), and similarly,
>  __bswap32_gen() from  __bswap64_gen().

This breaks it.  DIring development, I had a correct fix involving
casting down the arg.

>  While here, add extra parentheses around the __bswap16_gen() macro
>  expansion, to prevent unexpected side effects.

These parentheses were intentionally left out for clarity.
__bswap16_gen() is not user-serving, and all places in endian.h that
use it supply enough parentheses.

> Modified:
>  head/sys/x86/include/endian.h
>
> Modified: head/sys/x86/include/endian.h
> ==============================================================================
> --- head/sys/x86/include/endian.h	Sat Mar 24 06:40:41 2012	(r233418)
> +++ head/sys/x86/include/endian.h	Sat Mar 24 10:07:21 2012	(r233419)
> @@ -63,11 +63,11 @@
> #define	BYTE_ORDER	_BYTE_ORDER
> #endif
>
> -#define	__bswap16_gen(x)	(__uint16_t)((x) << 8 | (x) >> 8)
> +#define	__bswap16_gen(x)	((__uint16_t)((x) << 8 | (x) >> 8))
> #define	__bswap32_gen(x)		\
> -	(((__uint32_t)__bswap16(x) << 16) | __bswap16((x) >> 16))
> +	(((__uint32_t)__bswap16_gen(x) << 16) | __bswap16_gen((x) >> 16))
> #define	__bswap64_gen(x)		\
> -	(((__uint64_t)__bswap32(x) << 32) | __bswap32((x) >> 32))
> +	(((__uint64_t)__bswap32_gen(x) << 32) | __bswap32_gen((x) >> 32))
>
> #ifdef __GNUCLIKE_BUILTIN_CONSTANT_P
> #define	__bswap16(x)				\

Hmm, __bswap32_gen() and __bswap64_gen() were sloppy about leaving out
the parentheses.  They have unnecssary (?) parentheses around the whole
expression.  These parentheses are a bit more needed than for
__bswap16_gen(), since '(cast)(expr)' rarely needs more while 'foo | bar'
often does.   These functions also have unnecessary and
style-conflicting parentheses around the '<<' expression.  Such
parentheses are not used for either the '<<' or the '>>' expression
for __bswap16_gen().  They are needed for parameter passing for the
other 2 '>>' expressions.

Bruce