Date: Mon, 19 Dec 2011 12:09:21 +1100 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Dimitry Andric <dim@FreeBSD.org> Cc: svn-src-head@FreeBSD.org, svn-src-all@FreeBSD.org, src-committers@FreeBSD.org Subject: Re: svn commit: r228668 - head/usr.bin/netstat Message-ID: <20111219111309.L877@besplex.bde.org> In-Reply-To: <4EEE136F.5080904@FreeBSD.org> References: <201112172232.pBHMW1Bd079555@svn.freebsd.org> <4EED18B5.8000907@FreeBSD.org> <20111218013905.GA20867@zim.MIT.EDU> <4EEE136F.5080904@FreeBSD.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20111219111309.L877>