From owner-svn-src-all@FreeBSD.ORG Sun May 23 08:34:19 2010 Return-Path: Delivered-To: svn-src-all@FreeBSD.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 8A2E41065672; Sun, 23 May 2010 08:34:19 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from mail08.syd.optusnet.com.au (mail08.syd.optusnet.com.au [211.29.132.189]) by mx1.freebsd.org (Postfix) with ESMTP id 2278F8FC1A; Sun, 23 May 2010 08:34:18 +0000 (UTC) Received: from c122-107-114-249.carlnfd1.nsw.optusnet.com.au (c122-107-114-249.carlnfd1.nsw.optusnet.com.au [122.107.114.249]) by mail08.syd.optusnet.com.au (8.13.1/8.13.1) with ESMTP id o4N8XjKY026196 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Sun, 23 May 2010 18:34:16 +1000 Date: Sun, 23 May 2010 18:33:45 +1000 (EST) From: Bruce Evans X-X-Sender: bde@delplex.bde.org To: Poul-Henning Kamp In-Reply-To: <201005200616.o4K6GDN8010282@svn.freebsd.org> Message-ID: <20100523174020.X13588@delplex.bde.org> References: <201005200616.o4K6GDN8010282@svn.freebsd.org> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed Cc: svn-src-head@FreeBSD.org, svn-src-all@FreeBSD.org, src-committers@FreeBSD.org Subject: Re: svn commit: r208331 - head/sys/sys X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sun, 23 May 2010 08:34:19 -0000 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 fails messily. At least the i386 in at least old versions is careful about this, and is not included by any other header, so the problem is smaller than it would be for . > ... > @@ -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