svn commit: r228668 - head/usr.bin/netstat

Bruce Evans brde at optusnet.com.au
Mon Dec 19 01:09:25 UTC 2011


PS: (review of the actual patch)

On Sun, 18 Dec 2011, Dimitry Andric wrote:

> A better fix is to add explicit casts to the __bswap macros, as per
> attached patch, which I'm running through a make universe now.

% diff --git a/sys/amd64/include/endian.h b/sys/amd64/include/endian.h
% index de22c8b..60ed226 100644
% --- a/sys/amd64/include/endian.h
% +++ b/sys/amd64/include/endian.h
% @@ -111,16 +111,16 @@ __bswap16_var(__uint16_t _x)
%  }
% 
%  #define	__bswap64(_x)					\
% -	(__builtin_constant_p(_x) ?			\
% -	    __bswap64_const((__uint64_t)(_x)) : __bswap64_var(_x))
% +	((__uint64_t)(__builtin_constant_p(_x) ?	\
% +	    __bswap64_const((__uint64_t)(_x)) : __bswap64_var(_x)))

This has no effect except to enlarge the source code.  The binary
promotions for ?: are null because the rank of uint64_t is larger
than the rank of of u_int.  We can know this since we are the amd64
implementation and the amd64 ABI requires 32-bit ints.  For some
other arches, the ABI is fuzzier, but we only support 32-bit ints
so we know the same in practice.

% 
%  #define	__bswap32(_x)					\
% -	(__builtin_constant_p(_x) ?			\
% -	    __bswap32_const((__uint32_t)(_x)) : __bswap32_var(_x))
% +	((__uint32_t)(__builtin_constant_p(_x) ?	\
% +	    __bswap32_const((__uint32_t)(_x)) : __bswap32_var(_x)))

Similarly.  rank(uint32_t) >= rank(u_int).  Actually equal now.

% 
%  #define	__bswap16(_x)					\
% -	(__builtin_constant_p(_x) ?			\
% -	    __bswap16_const((__uint16_t)(_x)) : __bswap16_var(_x))
% +	((__uint16_t)(__builtin_constant_p(_x) ?	\
% +	    __bswap16_const((__uint16_t)(_x)) : __bswap16_var(_x)))

This one is needed.  It wouldn't be needed with 16-bit ints.

The bogus cast of the return value in __bswap16_const() should be
removed.  See previous mail.

Similarly for all other arches.  See previous mail.

More about the mess here: all of these ?: definitions are MI.  All
of the __bswapN_const() definitions are MI.  They shouldn't be spammed
into ${N_ARCH} endian.h files.  The former remain MI in your fixed
version.  But in my shortened version, omitting some casts is MD.
So the cleaned up version should not be shortened.

> (Note
> that some arches, such as arm and mips already add the explicit casts
> for the expected __bswap return types.)  Would that be OK to commit?

That's part of the mess.  arm and mips are gratuitously different.

Comparing various versions show the following gratuitous differences:
- amd64 to i386:
   - explicit long long constants for i386 only.  Style bug and technical
     problem.
   - i386 has 2 underscores instead of 1 for the variable in __bswap64_var()
   - extra blank line on i386
- amd64 to arm:
   - too many to list.  arm still has the horrible __OPTIMIZE__ ifdefs.
     Its "MI" macros look quite different, and were different in the
     !__OPTIMIZE__ case since they already had the casts, and are
     different in the __OPTIMIZE__ case since they don't have ?: then.
     arm is missing at least 3 rounds of major cleanups.
- amd64 to ia64:
   - ia64 should be closer to amd64 than i386 (no difference except for
     a couple of asms), but is further away.
   - ia64 uses bogus __CC_SUPPORTS___INLINE feature test
   - no optimizations for constants on ia64.  Thus, less complexity and
     the type bugs were missing.
   - no __cplusplus on ia64.  These seem to be just a style bug on amd64.
     All the functions are static inline (spelled __inline for stylistic
     reasons), so I think everything has the same semantics for C++ without
     these ifdefs.
- amd64 to mips:
   - too many to list.  mips is closer to arm than to amd64, but doesn't
     cast the result of ?:.
- amd64 to powerpc:
   - too many to list.  Closer in organization to amd64 than to arm or mips,
     but lexically gratuitously different.  The main differences are:
     - powerpc is missing the cleandown of macro parameter names from
       x to _x, and
     - powerpc is missing the cleanup of casting the macro parameters
       once at the __bswapN() level instead of every time they are
       referenced at lower levels.  After fixing this, it would be
       mostly better than amd64.
     - powerpc is missing cleandown from casts (to __uint64_t) to hard
       long long constants on i386.
- sparc64 to powerpc:
   - almost the same, but shouldn't be:
     - sparc64 still uses __OPTIMIZE__, but in a clean way
     - sparc64 uses casts to __uint64_t but since it is always 64-bit, it
       could use UL suffixes like amd64.

Bruce


More information about the svn-src-head mailing list