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

[Dimitry Andric]

On 2012-03-24 18:48, Bruce Evans wrote:
> 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.

Then why was it committed without fixing the warning first?


> 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.

I don't see that option anywhere in either the world or kernel build.
Why would you not want to warn about system headers?


> 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).

It only occurs in certain cases, and I have not been able to exactly
determine when.  It doesn't seem to be related to -Wsystem-headers.


>>  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.

Sure, but there it doesn't matter at all.


> 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.

Isn't it much easier, and less obtuse, to just jam in two bswaps for the
i386?  I never saw the reason for making the bswap macros *more*
complicated instead of less.  If you obfuscate them enough, no compiler
will be able to optimize properly... :)


>>  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.

If it works correctly with clang, please post it.


>>  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.

Let's hope so, until a follow up commit breaks it again.