From owner-svn-src-all@FreeBSD.ORG Sun Mar 4 00:37:56 2012 Return-Path: Delivered-To: svn-src-all@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [69.147.83.52]) by hub.freebsd.org (Postfix) with ESMTP id 24676106566C; Sun, 4 Mar 2012 00:37:56 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from mail06.syd.optusnet.com.au (mail06.syd.optusnet.com.au [211.29.132.187]) by mx1.freebsd.org (Postfix) with ESMTP id 7C5598FC08; Sun, 4 Mar 2012 00:37:54 +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 mail06.syd.optusnet.com.au (8.13.1/8.13.1) with ESMTP id q240bias011168 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Sun, 4 Mar 2012 11:37:47 +1100 Date: Sun, 4 Mar 2012 11:37:39 +1100 (EST) From: Bruce Evans X-X-Sender: bde@besplex.bde.org To: Dimitry Andric In-Reply-To: <201203031858.q23IwGvG096048@svn.freebsd.org> Message-ID: <20120304102200.D942@besplex.bde.org> References: <201203031858.q23IwGvG096048@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: r232473 - in head: share/mk sys/conf 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, 04 Mar 2012 00:37:56 -0000 On Sat, 3 Mar 2012, Dimitry Andric wrote: > After r232322, it turned out many people (and some ports) are building > kernel modules using their old installed /usr/share/mk/bsd.*.mk files, > instead of the updated ones in their source tree. This leads to errors > like: It is a bug for sys to depend on anything outside of itself for its configuration. I use multiple sys trees spanning up to 7 major FreeBSD versions on several hosts and expect them to work across 5-10 years of differences, not to break every day when someone changes the sys sources. The host's installed files of course can't be changed. I also don't want to create a separate share/mk tree and point to it using -m, though that was necessary with RELENG_4 sources on about RELENG_6 and later hosts, because of incompatibilities caused by fixing the bug that the sys makefiles referred to the bsd.kern.mk file which was outside of the sys tree. (Fixing this bug made it worse in some ways. /usr/share/mk/bsd.kern.mk was moved to sys/conf/kern.mk. The old version should have been left for compatibility. Removing it broke old sys trees on new hosts. It didn't help that it was renamed, so that -m to tree to find it it sys/conf had no chance of working.) Now there are much larger layering bugs in this area :-(. kern.pre.mk now includes bsd.own.mk (a clear layering violation), and a new MK_ feature is used without ifdefing it every other day to break kernel builds. The previous breakage was for MK_CTF. MK_* is also badly designed. A correct design would have used only simple ifdefs. Simple ifdefs are fail-safe since you have to use ifdef to use them. Kernel headers are much more carefully maintained. They started with many out-of-tree references to installed headers, but were eventually fixed, and the bugs don't seem to have come back. > "sys/conf/kmod.mk", line 111: Malformed conditional (${MK_CLANG_IS_CC} == "no" && ${CC:T:Mclang} != "clang") > > Obviously, these errors will go away after a "make installworld", or > alternatively, by using "make buildenv" before attempting to manually > build modules. I never use these (except for performance tests). buildenv is an implementation detail that is not documented in src/Makefile. I also don't want to build or install new toolchains to build kernels. I want my kernel builds from a new sys tree to take seconds, not hours. I sometimes point to an alternative compiler using CC=. BTW, CC=clang works with my -current kernels without the new MK_* foo for clang, except for warnings about tautologous comparisons of unsigned's. -Os in CFLAGS is still horribly broken for CC=gcc, but works with CC=clang. -fno-inline-functions-called-once is necessary for debugging and profiling, but CC=clang doesn't support it. No optimization above -O1 makes a significant difference for kernels, except to waste (possibly insignificant) space (except with CC=clang, -Os actually works, so it saves space). It is sometimes necessary to build but not install a new version of config(8), but this hasn't been necessary for the last 4-5 years of sys trees. Some old sys trees need genassym. I keep genassym in ~/bin for them. Fortunately its ABI was stable, so 1 version works. The only large recent gratuitous incompatibility is that a -current gas can no longer compile 4 year old sources on i386, since it now enforces sizes for accesses to segment registers. > However, since it is apparently an expected use case to build using old > .mk files, change the way we test for clang, so it also works when the > MK_CLANG_IS_CC macro doesn't exist. That is correct. Kernel makefiles shouldn't depend on anything except sys.mk, which should rarely change (MK_CTF mistakes broke it significantly too. These are smaller now). I once tried to make kernel Makefiles completely portable and not depend on depend on sys.mk or on any BSD make feature. The FreeBSD-1 versions were careful to use $@, etc., and to not use ${.TARGET}, etc. BTW, config(8)'s generated makefiles should merge all the included .mk files so that you can actually see what is in them and so that you can edit them. Editing them is useful for removing bugs like the MK_* bugs and for making minor changes to CFLAGS. CFLAGS initialization is very convoluted and broken, so it is hard to control without editing the final CFLAGS setting. This setting is now deep in the source .mk files which are too global to edit to fix just one kernel configuration. I actually fixed the MK_* bugs first by initializing MK*_ in the environment passed to make. This is possible, unlike for CFLAGS, because the initialization is not convuluted. But I got tired of that after forgetting to type the environment change a fe times, and edited the source files. > Note the conditional expressions are becoming rather unreadable now, but > I will attempt to fix that on a followup commit. They also have style bugs (long lines). My fix is missing this bug. > Modified: head/share/mk/bsd.sys.mk > ============================================================================== > --- head/share/mk/bsd.sys.mk Sat Mar 3 18:08:57 2012 (r232472) > +++ head/share/mk/bsd.sys.mk Sat Mar 3 18:58:15 2012 (r232473) > @@ -28,7 +28,7 @@ CFLAGS += -std=${CSTD} > . if defined(WARNS) > . if ${WARNS} >= 1 > CWARNFLAGS += -Wsystem-headers > -. if !defined(NO_WERROR) && ((${MK_CLANG_IS_CC} == "no" && ${CC:T:Mclang} != "clang") || !defined(NO_WERROR.clang)) > +. if !defined(NO_WERROR) && ((${CC:T:Mclang} != "clang" && (!defined(MK_CLANG_IS_CC) || ${MK_CLANG_IS_CC} == "no")) || !defined(NO_WERROR.clang)) > CWARNFLAGS += -Werror > . endif > . endif bsd.sys.mk is in the same layer as bsd.own.mk, so it shouldn't need the new messes. It already had messes (long line are just the largest ones). > @@ -42,7 +42,7 @@ CWARNFLAGS += -W -Wno-unused-parameter - > . if ${WARNS} >= 4 > CWARNFLAGS += -Wreturn-type -Wcast-qual -Wwrite-strings -Wswitch\ > -Wshadow -Wunused-parameter > -. if !defined(NO_WCAST_ALIGN) && ((${MK_CLANG_IS_CC} == "no" && ${CC:T:Mclang} != "clang") || !defined(NO_WCAST_ALIGN.clang)) > +. if !defined(NO_WCAST_ALIGN) && ((${CC:T:Mclang} != "clang" && (!defined(MK_CLANG_IS_CC) || ${MK_CLANG_IS_CC} == "no")) || !defined(NO_WCAST_ALIGN.clang)) > CWARNFLAGS += -Wcast-align > . endif > . endif > @@ -59,7 +59,7 @@ CWARNFLAGS += -Wno-uninitialized > CWARNFLAGS += -Wno-pointer-sign > # Clang has more warnings enabled by default, and when using -Wall, so if WARNS > # is set to low values, these have to be disabled explicitly. > -. if ${MK_CLANG_IS_CC} != "no" || ${CC:T:Mclang} == "clang" > +. if ${CC:T:Mclang} == "clang" || (defined(MK_CLANG_IS_CC) && ${MK_CLANG_IS_CC} != "no") > . if ${WARNS} <= 3 > CWARNFLAGS += -Wno-tautological-compare -Wno-unused-value\ > -Wno-parentheses-equality -Wno-unused-function\ > @@ -84,12 +84,12 @@ WFORMAT = 1 > . if ${WFORMAT} > 0 > #CWARNFLAGS += -Wformat-nonliteral -Wformat-security -Wno-format-extra-args > CWARNFLAGS += -Wformat=2 -Wno-format-extra-args > -. if !defined(NO_WERROR) && ((${MK_CLANG_IS_CC} == "no" && ${CC:T:Mclang} != "clang") || !defined(NO_WERROR.clang)) > +. if !defined(NO_WERROR) && ((${CC:T:Mclang} != "clang" && (!defined(MK_CLANG_IS_CC) || ${MK_CLANG_IS_CC} == "no")) || !defined(NO_WERROR.clang)) > CWARNFLAGS += -Werror > . endif > . endif > . endif > -. if defined(NO_WFORMAT) || ((${MK_CLANG_IS_CC} != "no" || ${CC:T:Mclang} == "clang") && defined(NO_WFORMAT.clang)) > +. if defined(NO_WFORMAT) || ((${CC:T:Mclang} == "clang" || (defined(MK_CLANG_IS_CC) && ${MK_CLANG_IS_CC} != "no")) && defined(NO_WFORMAT.clang)) > CWARNFLAGS += -Wno-format > . endif > .endif > @@ -98,7 +98,7 @@ CWARNFLAGS += -Wno-format > CWARNFLAGS += -Wno-unknown-pragmas > .endif > > -.if ${MK_CLANG_IS_CC} != "no" || ${CC:T:Mclang} == "clang" > +.if ${CC:T:Mclang} == "clang" || (defined(MK_CLANG_IS_CC) && ${MK_CLANG_IS_CC} != "no") > CLANG_NO_IAS = -no-integrated-as > CLANG_OPT_SMALL = -mllvm -stack-alignment=8 -mllvm -inline-threshold=3 \ > -mllvm -enable-load-pre=false > It has many other messes. It doesn't look anything like a BSD mk file (starting with its spaces after "." and its tabs before assignments) so I try not to look at it. Old versions of it only have a few of the long lines. Some of the long lines are made longer by the excessive indentation. > Modified: head/sys/conf/kern.mk > ============================================================================== > --- head/sys/conf/kern.mk Sat Mar 3 18:08:57 2012 (r232472) > +++ head/sys/conf/kern.mk Sat Mar 3 18:58:15 2012 (r232473) > @@ -15,7 +15,7 @@ CWARNFLAGS?= -Wall -Wredundant-decls -Wn > # Disable a few warnings for clang, since there are several places in the > # kernel where fixing them is more trouble than it is worth, or where there is > # a false positive. > -.if ${MK_CLANG_IS_CC} != "no" || ${CC:T:Mclang} == "clang" > +.if ${CC:T:Mclang} == "clang" || (defined(MK_CLANG_IS_CC) && ${MK_CLANG_IS_CC} != "no") My version just defines things if they are undefined: % Index: kern.mk % =================================================================== % RCS file: /home/ncvs/src/sys/conf/kern.mk,v % retrieving revision 1.93 % diff -u -2 -r1.93 kern.mk % --- kern.mk 29 Feb 2012 22:58:51 -0000 1.93 % +++ kern.mk 2 Mar 2012 09:17:48 -0000 % @@ -16,4 +16,7 @@ % # kernel where fixing them is more trouble than it is worth, or where there is % # a false positive. % +.if !defined(MK_CLANG_IS_CC) % +MK_CLANG_IS_CC= no % +.endif % .if ${MK_CLANG_IS_CC} != "no" || ${CC:T:Mclang} == "clang" % NO_WCONSTANT_CONVERSION= -Wno-constant-conversion % Index: kern.pre.mk % =================================================================== % RCS file: /home/ncvs/src/sys/conf/kern.pre.mk,v % retrieving revision 1.138 % diff -u -2 -r1.138 kern.pre.mk % --- kern.pre.mk 29 Feb 2012 22:58:51 -0000 1.138 % +++ kern.pre.mk 2 Mar 2012 09:21:35 -0000 % @@ -6,4 +6,8 @@ % .include % % +.if !defined(MK_CLANG_IS_CC) % +MK_CLANG_IS_CC= no % +.endif % + % # backwards compat option for older systems. % MACHINE_CPUARCH?=${MACHINE_ARCH:C/mipse[lb]/mips/:C/armeb/arm/:C/powerpc64/powerpc/} > Modified: head/sys/conf/kmod.mk I rarely use modules, so I didn't need to touch this. > ============================================================================== > --- head/sys/conf/kmod.mk Sat Mar 3 18:08:57 2012 (r232472) > +++ head/sys/conf/kmod.mk Sat Mar 3 18:58:15 2012 (r232473) > @@ -108,7 +108,7 @@ CFLAGS+= -I. -I@ > # for example. > CFLAGS+= -I@/contrib/altq > > -.if ${MK_CLANG_IS_CC} == "no" && ${CC:T:Mclang} != "clang" > +.if ${CC:T:Mclang} != "clang" && (!defined(MK_CLANG_IS_CC) || ${MK_CLANG_IS_CC} == "no") Is any change necessary here? Normally CC is not clang, so the first test should prevent evaluation of MK_LANG_IS_CC just was well as the defined() test. When CC is clang, the defined() test is strictly needed for portability, but people who set CC=clang probably actually need to use the new feature. > CFLAGS+= -finline-limit=${INLINE_LIMIT} > CFLAGS+= --param inline-unit-growth=100 > CFLAGS+= --param large-function-growth=1000 > Bruce