Date: Sun, 4 Mar 2012 11:37:39 +1100 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Dimitry Andric <dim@freebsd.org> 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 Message-ID: <20120304102200.D942@besplex.bde.org> In-Reply-To: <201203031858.q23IwGvG096048@svn.freebsd.org> References: <201203031858.q23IwGvG096048@svn.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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 <bsd.own.mk>
%
% +.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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20120304102200.D942>