Date: Sun, 23 May 2010 18:33:45 +1000 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Poul-Henning Kamp <phk@FreeBSD.org> Cc: svn-src-head@FreeBSD.org, svn-src-all@FreeBSD.org, src-committers@FreeBSD.org Subject: Re: svn commit: r208331 - head/sys/sys Message-ID: <20100523174020.X13588@delplex.bde.org> In-Reply-To: <201005200616.o4K6GDN8010282@svn.freebsd.org> References: <201005200616.o4K6GDN8010282@svn.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Thu, 20 May 2010, Poul-Henning Kamp wrote:
> Log:
> Fix some way-past-brucification complaints from FlexeLint.
Its complaint switch needs work ;-).
> Modified: head/sys/sys/endian.h
> ==============================================================================
> --- head/sys/sys/endian.h Thu May 20 06:05:40 2010 (r208330)
> +++ head/sys/sys/endian.h Thu May 20 06:16:13 2010 (r208331)
> ...
> static __inline uint16_t
> be16dec(const void *pp)
> {
> - unsigned char const *p = (unsigned char const *)pp;
> + uint8_t const *p = (uint8_t const *)pp;
All functions in this file still fail header engineering 101. The
valid application code
#define p you lose
#include <sys/endian.h>
fails messily. At least the i386 <machine/endian.h> in at least old
versions is careful about this, and <sys/endian.h> is not included by
any other header, so the problem is smaller than it would be for
<machine/endian.h>.
> ...
> @@ -102,15 +107,15 @@ be16dec(const void *pp)
> static __inline uint32_t
> be32dec(const void *pp)
> {
> - unsigned char const *p = (unsigned char const *)pp;
> + uint8_t const *p = (uint8_t const *)pp;
>
> - return ((p[0] << 24) | (p[1] << 16) | (p[2] << 8) | p[3]);
> + return (((unsigned)p[0] << 24) | (p[1] << 16) | (p[2] << 8) | p[3]);
> }
This cast is ugly but is not ugly enough to be sufficient. It assumes
>= 32-bit unsigneds. Due to the STDC "value-preserving" extension botch,
there are the following problems:
- p[3] extends to a signed int. No problem since uint8_t is guaranteed
to fit in a signed int.
- p[2] extends to a signed int. Then p[2] << 8 overflows if ints are
16 bits and the top bit of p[2] is set. The above works if ints have
> 16 bits.
- similarly, p[1] << 16 can overflow in more cases than p[2] << 16, but
the above works if ints have > 24 bits.
- similarly, p[0] << 24 can overflow if ints have <= 32 bits, which is
the case on all supported machines, but the above fixes this if
unsigned's have >= 32 bits, which is the case on all supported machines.
The behaviour on overflow is explicitly undefined in C99. In C90, the
specification is too fuzzy to say what happens on overflow, and may
even be too fuzzy to specify what happens on non-overflow of a shift
of a signed type even for left shifting.
So careful code would cast everything to a sufficiently wide _unsigned_
type before shifting. E.g.,
return (((uint32_t)p[0] << 24) | ((uint24_t)/* sic */p[1] << 16) |
((uint16_t)p[2] << 8) | p[3]);
However, this is ugly, and since the undefined behaviour is benign on
all supported machines, we may as well depend on it and omit all the
casts instead of depending on int being >= 24 bits and unsigned being
>= 32 bits on all supported machines. FlexeLint complaining about
the implementation depending on impleentation details is a bug.
> static __inline uint64_t
> le64dec(const void *pp)
> {
> - unsigned char const *p = (unsigned char const *)pp;
> + uint8_t const *p = (uint8_t const *)pp;
>
> return (((uint64_t)le32dec(p + 4) << 32) | le32dec(p));
> }
Similarly in all the `dec' functions, except casts to uint64_t like the
above are necessary on all supported machines, so they were already done.
> @@ -162,16 +167,16 @@ be32enc(void *pp, uint32_t u)
> static __inline void
> be64enc(void *pp, uint64_t u)
> {
> - unsigned char *p = (unsigned char *)pp;
> + uint8_t *p = (uint8_t *)pp;
>
> - be32enc(p, u >> 32);
> - be32enc(p + 4, u & 0xffffffff);
> + be32enc(p, (uint32_t)(u >> 32));
This cast has no effect, since the prototype already does it. If you
want to need such casts, use a K&R compiler.
> + be32enc(p + 4, (uint32_t)(u & 0xffffffffU));
This cast has no effect, as above. Casting u would make more sense
(it would manually optimize for compilers that don't reduce to a 32-bit
and operation automatically).
The explicit unsigned constant is another type of (bogus) cast. It has
no effect, except on machines with < 32-bit unsigned it should cause a
warning that the constant is too larged for its type.
> }
>
> static __inline void
> le16enc(void *pp, uint16_t u)
> {
> - unsigned char *p = (unsigned char *)pp;
> + uint8_t *p = (uint8_t *)pp;
>
> p[0] = u & 0xff;
> p[1] = (u >> 8) & 0xff;
To be bogon for bogon compatible with the above, this should cast down
u and spell 0xff as 0xffU or (uint8_t)u. These cast are equally (not)
needed.
> ...
> #endif /* _SYS_ENDIAN_H_ */
>
Style bugs:
(1) tab instead of space between #endif and comment
(2) backwards comment on #endif.
Bruce
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20100523174020.X13588>