From owner-freebsd-net@FreeBSD.ORG Wed Dec 28 06:59:04 2011 Return-Path: Delivered-To: freebsd-net@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 51A7A106566B for ; Wed, 28 Dec 2011 06:59:04 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from mail07.syd.optusnet.com.au (mail07.syd.optusnet.com.au [211.29.132.188]) by mx1.freebsd.org (Postfix) with ESMTP id DC9F08FC16 for ; Wed, 28 Dec 2011 06:59:03 +0000 (UTC) Received: from c211-30-171-136.carlnfd1.nsw.optusnet.com.au (c211-30-171-136.carlnfd1.nsw.optusnet.com.au [211.30.171.136]) by mail07.syd.optusnet.com.au (8.13.1/8.13.1) with ESMTP id pBS6wxcB013676 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Wed, 28 Dec 2011 17:59:00 +1100 Date: Wed, 28 Dec 2011 17:58:59 +1100 (EST) From: Bruce Evans X-X-Sender: bde@besplex.bde.org To: Bruce Evans In-Reply-To: <20111228162621.G936@besplex.bde.org> Message-ID: <20111228164938.F936@besplex.bde.org> References: <1325015120.17645.7.camel@hitfishpass-lx.corp.yahoo.com> <20111228162621.G936@besplex.bde.org> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed Cc: "freebsd-net@freebsd.org" , Sergey Kandaurov , Sean Bruno Subject: Re: i386 compile sys/dev/ie X-BeenThere: freebsd-net@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Networking and TCP/IP with FreeBSD List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 28 Dec 2011 06:59:04 -0000 On Wed, 28 Dec 2011, Bruce Evans wrote: > On Wed, 28 Dec 2011, Sergey Kandaurov wrote: >> These were used in probe routine and are left from the newbus rewrite. >> I hacked ie a bit to build cleanly. [Not sure if I did this correctly.] > > Use of the __DEVOLATILE() abomination is never correct. It exploits the > bug that -Wcast-qual is broken for casts through [u]intptr_t. PS: I used to maintain some bad fixes in this area for ie, but now have only the following: % .Index: if_ie.c % .=================================================================== % .RCS file: /home/ncvs/src/sys/dev/ie/if_ie.c,v % .retrieving revision 1.99 % .diff -u -2 -r1.99 if_ie.c % .--- if_ie.c 17 Mar 2004 17:50:35 -0000 1.99 % .+++ if_ie.c 31 May 2004 06:57:05 -0000 % .@@ -159,4 +159,7 @@ % . #define IE_BUF_LEN ETHER_MAX_LEN /* length of transmit buffer */ % . % .+/* XXX this driver uses `volatile' and `caddr_t' to a fault. */ % .+typedef volatile char *v_caddr_t; /* core address, pointer to volatile */ % .+ % . /* Forward declaration */ % . struct ie_softc; I never allowed the __DEFOO() abominations in my , and at one time had all uses to them in /usr/src removed and ready to commit (there were about 2 of them. There are hundreds if not thousands now :-(). c_caddr_t and v_caddr_t are as abominable as __DEFOO(), so of course I never allowed them in my . But v_caddr_t is still easy to remove, since it is only used in the ie driver. It is used the break warnings from -Wcast-qual that more natural casts would give (but warnings still result from the implicit conversion for bcopy()): % if_ie.c:static v_caddr_t setup_rfa (struct ie_softc *, v_caddr_t); The very idea of a v_caddr_t is nonsense. caddr_t is a bad old type for a "core address". In most cases, it means the same as "void *", but was used because "void *" didn't exist. In FreeBSD, it must be precisely "char *" to work, since pointer arithmetic is perpetrated on it (the pointer arithmetic defeats its opaqueness). In a few cases, it is used for physical or device memory accesses. In must places, it should be spelled "void *", and in other places it should be replaced by a vm virtual or physical address type, or a bus space type. To this mess, the ie driver, but mercifully no other code in /usr/src, adds v_caddr_t. caddr_t is supposed to be opaque, but if we just cast to that we will get a cast-qual error if we start with one of ie's excessively qualified pointers. Also, "const caddr_t" doesn't work since it puts the const in the wrong place. This is "solved" by looking inside caddr_t to add the const there. ie uses v_caddr_t as follows: First, in the above, some of the excessive qualifiers are in setup_rfa()'s return type and second parameter... % if_ie.c: setup_rfa(sc, (v_caddr_t) sc->rframes[0]); % if_ie.c: setup_rfa(sc, (v_caddr_t) sc->rframes[0]); /* ignore cast-qual */ ... this makes even calling setup_rfa() difficult. We start with a volatile struct ie_mumble **rframes. We can't just cast this to void * or caddr_t since this would cause a -Wcast-qual warning. Casting it to "volatile void *" would be even worse, since it removes a volatile qualifier that is in the (technically) right place and adds it back in a wrong place. So we use v_caddr_t to keep it in the same place. % if_ie.c: bcopy((v_caddr_t) (sc->cbuffs[head] + offset), % if_ie.c: bcopy((v_caddr_t) (sc->cbuffs[head] + offset), % if_ie.c: bcopy((v_caddr_t) (sc->cbuffs[head] + offset), Similarly. cbuffs is a volatile u_char **. We want to aplply bcopy() to (cbuffs[head] + offset), which is a volatile u_char *. We assume that bcopy() is still the 1980's version which takes a caddr_t, although it took a (void *) even in 1992. So we originally tried to bogusly cast the arg to caddr_t. This had no effect, as would the more correct cast to (void *) have done, since the prototype converts to void * anyway. Eventually, -Wcast-qual was used and this bug was detected. (There may be a more serious bug involving char * being incompatble with void *. A grandfather kludge keeps char * equivalent to void * in many context. But it is not clear what happens with complicated qualifiers.) Some time later, the -Wcast-qual was broken using a volatile cast. But the warning for converting away the qualifier by the prototype remained. Omitting the cast would have had the same effect. % if_ie.c: bcopy((v_caddr_t) (sc->rframes[num]), &rfd, Similarly. % if_ie.c:static v_caddr_t % if_ie.c:setup_rfa(struct ie_softc *sc, v_caddr_t ptr) The definition matching the above prototype. % if_ie.c: bcopy((v_caddr_t) sc->mcast_addrs, (v_caddr_t) cmd->ie_mcast_addrs, Similarly. % if_ie.c: bzero((v_caddr_t) sc->xmit_cmds[i], sizeof *sc->xmit_cmds[i]); % if_ie.c: bzero((v_caddr_t) sc->xmit_buffs[i], sizeof *sc->xmit_buffs[i]); Same mistakes for bzero(). My change isolates the v_caddr_t mistake in ie. The c_caddr_t mistake is used 17 times in the kernel and 75 times in /usr/src. Bruce