From owner-cvs-src@FreeBSD.ORG Tue Nov 20 09:29:29 2007 Return-Path: Delivered-To: cvs-src@FreeBSD.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 3BC9C16A418; Tue, 20 Nov 2007 09:29:29 +0000 (UTC) (envelope-from jb@what-creek.com) Received: from what-creek.com (what-creek.com [66.111.37.70]) by mx1.freebsd.org (Postfix) with ESMTP id E68D113C448; Tue, 20 Nov 2007 09:29:28 +0000 (UTC) (envelope-from jb@what-creek.com) Received: by what-creek.com (Postfix, from userid 102) id 6144273247; Tue, 20 Nov 2007 09:31:55 +0000 (GMT) Date: Tue, 20 Nov 2007 09:31:55 +0000 From: John Birrell To: "Bjoern A. Zeeb" Message-ID: <20071120093155.GA6959@what-creek.com> References: <200711200207.lAK27UmF082244@repoman.freebsd.org> <20071120082329.C53707@maildrop.int.zabbadoz.net> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20071120082329.C53707@maildrop.int.zabbadoz.net> User-Agent: Mutt/1.4.2.3i Cc: cvs-src@FreeBSD.org, src-committers@FreeBSD.org, cvs-all@FreeBSD.org Subject: Re: cvs commit: src/lib/libkse Makefile src/lib/libthr Makefile src/usr.bin/netstat Makefile src/usr.sbin/acpi/acpidb Makefile src/usr.sbin/kldxref Makefile src/usr.sbin/nscd Makefile src/usr.sbin/rpc.yppasswdd Makefile X-BeenThere: cvs-src@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: CVS commit messages for the src tree List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 20 Nov 2007 09:29:29 -0000 On Tue, Nov 20, 2007 at 08:25:54AM +0000, Bjoern A. Zeeb wrote: > On Tue, 20 Nov 2007, John Birrell wrote: > > Hi, > > >jb 2007-11-20 02:07:30 UTC > > > > FreeBSD src repository > > > > Modified files: > > lib/libkse Makefile > > lib/libthr Makefile > > usr.bin/netstat Makefile > > usr.sbin/acpi/acpidb Makefile > > usr.sbin/kldxref Makefile > > usr.sbin/nscd Makefile > > usr.sbin/rpc.yppasswdd Makefile > > Log: > > These are the things that the tinderbox has problems with because it > > doesn't use the default CFLAGS which contain -fno-strict-aliasing. > > > > Until the code is cleaned up, just add -fno-strict-aliasing to the > > CFLAGS of these for the tinderboxes' sake, allowing the rest of the > > tree to have -Werror enabled again. > > I have the feeling that adding -fno-strict-aliasing just for the > tinderbox to be happy is not the right solution, even if it is temporary. > > Isn't lowering WARNS a better temporary solution? My theory is that what is committed to CVS is the most important. I think that I can achieve a better result by leaving the WARNS settings committed by others to CVS rather than bowing to the personal custom CVS flags set by the one person who hosts the tinderboxes on behalf of the project. I appreciate the contribution that the tinderboxes make to the project. I think we _need_ them. The thing that I take issue with is holding off re-enabling the WARNS -Werror checks which used to serve us well simply because there are custom CFLAGS set on the tinderboxes which cause non-fatal errors that nobody _ever_ _looks_ _at_. The errors are non-fatal because -Werror was disabled. Only the tinderbox errors are reported to the mailing lists. So if -Werror is never re-enabled, I have to ask... is it better that you all never see the problem warnings for _all_ things, not just strict aliasing warnings, or that you have the warm fuzzy feeling that if the tinderbox builds don't fail, then the code is wonderful? As an exercise to the readers of this message, please take a look at src/lib/libthr, our preferred threading library. Remove the -fno-strict-aliasing CFLAGS setting that I added. Build the library using "CFLAGS=-02 -pipe" in /etc/make.conf like the tinderbox uses. Notice the compilation error you get in src/lib/libthr/thread/thr_sem.c. You will see that the compiler is complaining about: do { while ((val = (*sem)->count) > 0) { if (atomic_cmpset_acq_int(&(*sem)->count, val, val - 1)) return (0); } if (abstime == NULL) { errno = EINVAL; return (-1); } clock_gettime(CLOCK_REALTIME, &ts); TIMESPEC_SUB(&ts2, abstime, &ts); _thr_cancel_enter(curthread); retval = _thr_umtx_wait((umtx_t *)&(*sem)->count, 0, &ts2); _thr_cancel_leave(curthread); } while (retval == 0); ... actually the cast in _thr_umtx_wait. The problem here is that umtx functions work on u_longs whereas semaphores have u_int32_t as the type for count. On amd64 (on which I am typing this message), sizeof(long) != sizeof(u_int32_t), so we have a problem. By choosing to have -fno-strict-aliasing set by default in CVS we are choosing to ignore this problem. The compiler follows our code and performs the cast on the basis that it'll all be right in the end. But will it? On the tinderbox, des@ has chosen to report strict aliasing failures because he is aware of the problems like the one I describe above. So who is "right"? We have CVS set to default to -fno-strict-aliasing and we choose, therefore to ignore casts which actually shoot us in the foot. I would prefer that the tinderboxes run with the same defaults that we have in CVS, however, from what I have seen today, I think that we may need to rethink our defaults. So back to my goal.... I would like to see -Werror re-instated ASAP, but I don't want to break the tinderboxes. By adding -fno-strict-aliasing to CFLAGS in certain Makefiles, I am effectively flagging these as "work required here". For everything else I can enable -Werror and all subsequent builds will check for regressions from that. -- John Birrell