svn commit: r208331 - head/sys/sys

Bruce Evans brde at optusnet.com.au
Sun May 23 08:34:19 UTC 2010


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


More information about the svn-src-head mailing list