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
_______________________________________________
svn-src-all@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
