From owner-svn-src-all@FreeBSD.ORG Sat Feb 14 09:47:09 2015 Return-Path: Delivered-To: svn-src-all@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [8.8.178.115]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by hub.freebsd.org (Postfix) with ESMTPS id A2DD859E; Sat, 14 Feb 2015 09:47:09 +0000 (UTC) Received: from mail108.syd.optusnet.com.au (mail108.syd.optusnet.com.au [211.29.132.59]) by mx1.freebsd.org (Postfix) with ESMTP id 6654EAB8; Sat, 14 Feb 2015 09:47:09 +0000 (UTC) Received: from c211-30-166-197.carlnfd1.nsw.optusnet.com.au (c211-30-166-197.carlnfd1.nsw.optusnet.com.au [211.30.166.197]) by mail108.syd.optusnet.com.au (Postfix) with ESMTPS id C550A1A3965; Sat, 14 Feb 2015 20:46:59 +1100 (AEDT) Date: Sat, 14 Feb 2015 20:46:58 +1100 (EST) From: Bruce Evans X-X-Sender: bde@besplex.bde.org To: Gleb Smirnoff Subject: Re: svn commit: r278737 - head/usr.sbin/flowctl In-Reply-To: <201502132357.t1DNvKda075915@svn.freebsd.org> Message-ID: <20150214193210.N945@besplex.bde.org> References: <201502132357.t1DNvKda075915@svn.freebsd.org> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed X-Optus-CM-Score: 0 X-Optus-CM-Analysis: v=2.1 cv=Za4kaKlA c=1 sm=1 tr=0 a=KA6XNC2GZCFrdESI5ZmdjQ==:117 a=PO7r1zJSAAAA:8 a=kj9zAlcOel0A:10 a=JzwRw_2MAAAA:8 a=6I5d2MoRAAAA:8 a=Qn7aHUtcSh8-iNL0V9sA:9 a=CjuIK1q_8ugA:10 Cc: svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.18-1 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: Sat, 14 Feb 2015 09:47:09 -0000 On Fri, 13 Feb 2015, Gleb Smirnoff wrote: > Author: glebius > Date: Fri Feb 13 23:57:20 2015 > New Revision: 278737 > URL: https://svnweb.freebsd.org/changeset/base/278737 > > Log: > Use less ugly code to allocate buffer of SORCVBUF_SIZE. Less ugly, but wrong. The version that used alloca() was correct. > Modified: head/usr.sbin/flowctl/flowctl.c > ============================================================================== > --- head/usr.sbin/flowctl/flowctl.c Fri Feb 13 23:43:59 2015 (r278736) > +++ head/usr.sbin/flowctl/flowctl.c Fri Feb 13 23:57:20 2015 (r278737) > @@ -222,10 +222,12 @@ ctl_show(int argc, char **argv) > static void > do_show(int version, void (*func)(struct ngnf_show_header *)) > { > - struct ng_mesg ng_mesg[SORCVBUF_SIZE]; > + char buf[SORCVBUF_SIZE]; alloca(), like malloc(), gave a buffer suitably aligned for any object. This only gives a buffer suitably aligned for char objects. It may accidentally be suitably aligned for other objects. The accident often happens because objects on the stack are usually given larger alignment than necessary. Depending on this is unportable at best. > + struct ng_mesg *ng_mesg; > struct ngnf_show_header req, *resp; > int token, nread; > > + ng_mesg = (struct ng_mesg *)buf; The new bug is detected at high warning levels. WARNS >= 4 gives -Wcast-align unless the MK option to break this warning is configured. The bug is detected by -Wcast0align even on amd64. The bug is not detected by default because flowctl has many other warnings at the default WARNS of 6 (mainly -Wcast-align and -Wsign-compare ones), so it breaks the warnings using WARNGS?=2. I think arches with strict alignment requirements have a warning about this without -Wcast-align, but couldn't find one on ia64. Certainly, related warnings turned up on ia64 when they didn't on amd64 with the same WARNS. The runtime bug can be fixed using __aligned(__ALIGN_MUMBLE). This exposes a bug in -Wcast-align -- it still warns although the char buffer is obviously aligned. Compilers should also know that the buffer is suitably aligned when they just allocated it on the stack and the alignment happens to be enough. But in this case, compilers should also know that the suitable alignment is only accidental, and still warn unless portability warnings are supressed. This and the non-spurious warning can be broken using another cast: ng_mesg = (struct ng_mesg *)(void *)buf; This depends on the compiler being too stupid to remember the alignment of the original char buffer. This fixed version is still worse than the old one using alloca(), because it is longer and more complicated. alloca(), like malloc(), returns a void * so that the compiler cannot know the alignment of the buffer (except it can for alloca() because it just allocated the buffer -- it must do a (void *) cast internally and forget the actual alignment, just like the above cast does but with more intentional forgetfulness). We are basically using a home made alloca(N) for the easier case where N is constant. Using VLAs and also the C99 feature of declarations anwhere, and extensions like __aligned(), we can almost implement a full alloca() using the fixed version of this change: /* * XXX need extended statement-expression so that __buf doesn't go out * of scope after the right brace. */ #define my_alloca(n) __extension__ ({ /* XXX need unique name. */ \ char __buf[__roundup2((n), MUMBLE)] __aligned(MUMBLE); \ \ (void *)__buf; \ }) Bruce