From: Jan Beulich <jbeulich@suse.com>
To: Anthony PERARD <anthony.perard@citrix.com>
Cc: "Stefano Stabellini" <sstabellini@kernel.org>,
"Julien Grall" <julien@xen.org>, "Wei Liu" <wl@xen.org>,
"Konrad Rzeszutek Wilk" <konrad.wilk@oracle.com>,
"George Dunlap" <George.Dunlap@eu.citrix.com>,
"Andrew Cooper" <andrew.cooper3@citrix.com>,
"Ian Jackson" <ian.jackson@eu.citrix.com>,
xen-devel@lists.xenproject.org,
"Volodymyr Babchuk" <Volodymyr_Babchuk@epam.com>,
"Roger Pau Monné" <roger.pau@citrix.com>
Subject: Re: [Xen-devel] [XEN PATCH v2 12/12] xen/build: have the root Makefile generates the CFLAGS
Date: Thu, 30 Jan 2020 15:33:15 +0100 [thread overview]
Message-ID: <e704f28b-818f-8f92-53a8-c9c804805aff@suse.com> (raw)
In-Reply-To: <20200117105358.607910-13-anthony.perard@citrix.com>
On 17.01.2020 11:53, Anthony PERARD wrote:
> Instead of generating the CFLAGS in Rules.mk everytime we enter a new
> subdirectory, we are going to generate most of them a single time, and
> export the result in the environment so that Rules.mk can use it. The
> only flags left to generates are the one that depends on the targets,
> but the variable $(c_flags) takes care of that.
>
> Arch specific CFLAGS are generated by a new file "arch/*/arch.mk"
> which is included by the root Makefile.
>
> In order to allow some variable that are specific to one arch and
> needs to be regenerated for each target, there's a new variable
> $(arch_ccflags).
And simply adding to c_flags is considered bad? Or does not work?
> --- a/xen/Makefile
> +++ b/xen/Makefile
> @@ -113,6 +113,76 @@ $(KCONFIG_CONFIG):
> %/auto.conf %/auto.conf.cmd: $(KCONFIG_CONFIG)
> $(MAKE) -f $(BASEDIR)/tools/kconfig/Makefile.kconfig ARCH=$(ARCH) SRCARCH=$(SRCARCH) HOSTCC="$(HOSTCC)" HOSTCXX="$(HOSTCXX)" syncconfig
>
> +ifeq ($(CONFIG_DEBUG),y)
> +CFLAGS += -O1
> +else
> +CFLAGS += -O2
> +endif
Why does this start with +=, not := (or = )?
> +ifeq ($(CONFIG_FRAME_POINTER),y)
> +CFLAGS += -fno-omit-frame-pointer
> +else
> +CFLAGS += -fomit-frame-pointer
> +endif
> +
> +CFLAGS += -nostdinc -fno-builtin -fno-common
> +CFLAGS += -Werror -Wredundant-decls -Wno-pointer-arith
> +$(call cc-option-add,CFLAGS,CC,-Wvla)
> +CFLAGS += -pipe -D__XEN__ -include $(BASEDIR)/include/xen/config.h
> +CFLAGS-$(CONFIG_DEBUG_INFO) += -g
> +
> +ifneq ($(CONFIG_CC_IS_CLANG),y)
> +# Clang doesn't understand this command line argument, and doesn't appear to
> +# have an suitable alternative. The resulting compiled binary does function,
> +# but has an excessively large symbol table.
> +CFLAGS += -Wa,--strip-local-absolute
> +endif
> +
> +AFLAGS-y += -D__ASSEMBLY__
Why not just AFLAGS? I think in a overhaul like what you do,
anomalies like this one would better be eliminated. The -y
forms should be added into the base variables (like you do ...
> +CFLAGS += $(CFLAGS-y)
... here), but be added to only via CFLAGS-$(variable)
constructs. Or otherwise there should be only CFLAGS-y, and
no plain CFLAGS at all.
> +# allow extra CFLAGS externally via EXTRA_CFLAGS_XEN_CORE
> +CFLAGS += $(EXTRA_CFLAGS_XEN_CORE)
> +
> +# Most CFLAGS are safe for assembly files:
> +# -std=gnu{89,99} gets confused by #-prefixed end-of-line comments
> +# -flto makes no sense and annoys clang
> +AFLAGS += $(AFLAGS-y) $(filter-out -std=gnu% -flto,$(CFLAGS))
> +
> +# LDFLAGS are only passed directly to $(LD)
> +LDFLAGS += $(LDFLAGS_DIRECT)
> +
> +LDFLAGS += $(LDFLAGS-y)
These two could be folded.
> +ifeq ($(CONFIG_COVERAGE),y)
> +ifeq ($(CONFIG_CC_IS_CLANG),y)
> + COV_FLAGS := -fprofile-instr-generate -fcoverage-mapping
> +else
> + COV_FLAGS := -fprofile-arcs -ftest-coverage
> +endif
> +else
> +COV_FLAGS :=
> +endif
COV_FLAGS gets propagated through the environment, despite being
invariant. Can't this stay in Rules.mk?
> --- a/xen/arch/x86/Rules.mk
> +++ b/xen/arch/x86/Rules.mk
> @@ -1,89 +1,10 @@
> ########################################
> # x86-specific definitions
>
> -XEN_IMG_OFFSET := 0x200000
> -
> -CFLAGS += -I$(BASEDIR)/include
> -CFLAGS += -I$(BASEDIR)/include/asm-x86/mach-generic
> -CFLAGS += -I$(BASEDIR)/include/asm-x86/mach-default
> -CFLAGS += -DXEN_IMG_OFFSET=$(XEN_IMG_OFFSET)
> -CFLAGS += '-D__OBJECT_LABEL__=$(subst /,$$,$(subst -,_,$(subst $(BASEDIR)/,,$(CURDIR))/$@))'
> -
> -# Prevent floating-point variables from creeping into Xen.
> -CFLAGS += -msoft-float
> -
> -ifeq ($(CONFIG_CC_IS_CLANG),y)
> -# Note: Any test which adds -no-integrated-as will cause subsequent tests to
> -# succeed, and not trigger further additions.
> -#
> -# The tests to select whether the integrated assembler is usable need to happen
> -# before testing any assembler features, or else the result of the tests would
> -# be stale if the integrated assembler is not used.
> -
> -# Older clang's built-in assembler doesn't understand .skip with labels:
> -# https://bugs.llvm.org/show_bug.cgi?id=27369
> -$(call as-option-add,CFLAGS,CC,".L0: .L1: .skip (.L1 - .L0)",,\
> - -no-integrated-as)
> -
> -# Check whether clang asm()-s support .include.
> -$(call as-option-add,CFLAGS,CC,".include \"asm/indirect_thunk_asm.h\"",,\
> - -no-integrated-as)
> -
> -# Check whether clang keeps .macro-s between asm()-s:
> -# https://bugs.llvm.org/show_bug.cgi?id=36110
> -$(call as-option-add,CFLAGS,CC,\
> - ".macro FOO;.endm"$$(close); asm volatile $$(open)".macro FOO;.endm",\
> - -no-integrated-as)
> -endif
> -
> -$(call cc-options-add,CFLAGS,CC,$(EMBEDDED_EXTRA_CFLAGS))
> -$(call cc-option-add,CFLAGS,CC,-Wnested-externs)
> -$(call as-option-add,CFLAGS,CC,"vmcall",-DHAVE_AS_VMX)
> -$(call as-option-add,CFLAGS,CC,"crc32 %eax$$(comma)%eax",-DHAVE_AS_SSE4_2)
> -$(call as-option-add,CFLAGS,CC,"invept (%rax)$$(comma)%rax",-DHAVE_AS_EPT)
> -$(call as-option-add,CFLAGS,CC,"rdrand %eax",-DHAVE_AS_RDRAND)
> -$(call as-option-add,CFLAGS,CC,"rdfsbase %rax",-DHAVE_AS_FSGSBASE)
> -$(call as-option-add,CFLAGS,CC,"xsaveopt (%rax)",-DHAVE_AS_XSAVEOPT)
> -$(call as-option-add,CFLAGS,CC,"rdseed %eax",-DHAVE_AS_RDSEED)
> -$(call as-option-add,CFLAGS,CC,"clwb (%rax)",-DHAVE_AS_CLWB)
> -$(call as-option-add,CFLAGS,CC,".equ \"x\"$$(comma)1", \
> - -U__OBJECT_LABEL__ -DHAVE_AS_QUOTED_SYM \
> - '-D__OBJECT_LABEL__=$(subst $(BASEDIR)/,,$(CURDIR))/$$@')
> -$(call as-option-add,CFLAGS,CC,"invpcid (%rax)$$(comma)%rax",-DHAVE_AS_INVPCID)
> -
> -# GAS's idea of true is -1. Clang's idea is 1
> -$(call as-option-add,CFLAGS,CC,\
> - ".if ((1 > 0) < 0); .error \"\";.endif",,-DHAVE_AS_NEGATIVE_TRUE)
> -
> -# Check to see whether the assmbler supports the .nop directive.
> -$(call as-option-add,CFLAGS,CC,\
> - ".L1: .L2: .nops (.L2 - .L1)$$(comma)9",-DHAVE_AS_NOPS_DIRECTIVE)
> -
> -CFLAGS += -mno-red-zone -fpic -fno-asynchronous-unwind-tables
> -
> -# Xen doesn't use SSE interally. If the compiler supports it, also skip the
> -# SSE setup for variadic function calls.
> -CFLAGS += -mno-sse $(call cc-option,$(CC),-mskip-rax-setup)
> -
> -# Compile with thunk-extern, indirect-branch-register if avaiable.
> -ifeq ($(CONFIG_INDIRECT_THUNK),y)
> -CFLAGS += -mindirect-branch=thunk-extern -mindirect-branch-register
> -CFLAGS += -fno-jump-tables
> +ifdef HAVE_AS_QUOTED_SYM
> +arch_ccflags += -DHAVE_AS_QUOTED_SYM \
> + '-D__OBJECT_LABEL__=$(subst $(BASEDIR)/,,$(CURDIR))/$@'
> +else
> +arch_ccflags += '-D__OBJECT_LABEL__=$(subst /,$$,$(subst -,_,$(subst $(BASEDIR)/,,$(CURDIR))/$@))'
> endif
Why does HAVE_AS_QUOTED_SYM need a make / environment variable to
propagate? Can't this be as-option-add against arch_ccflags (or
c_flags), in arch.mk or Rules.mk? Or can't arch.mk have
$(call as-option-add,CFLAGS,CC,".equ \"x\"$$(comma)1",-DHAVE_AS_QUOTED_SYM)
and then here you simply check CFLAGS for this specific -D option?
> --- /dev/null
> +++ b/xen/arch/x86/arch.mk
> @@ -0,0 +1,87 @@
> +########################################
> +# x86-specific definitions
> +
> +export XEN_IMG_OFFSET := 0x200000
> +
> +CFLAGS += -I$(BASEDIR)/include
> +CFLAGS += -I$(BASEDIR)/include/asm-x86/mach-generic
> +CFLAGS += -I$(BASEDIR)/include/asm-x86/mach-default
> +CFLAGS += -DXEN_IMG_OFFSET=$(XEN_IMG_OFFSET)
> +
> +# Prevent floating-point variables from creeping into Xen.
> +CFLAGS += -msoft-float
> +
> +ifeq ($(CONFIG_CC_IS_CLANG),y)
> +# Note: Any test which adds -no-integrated-as will cause subsequent tests to
> +# succeed, and not trigger further additions.
> +#
> +# The tests to select whether the integrated assembler is usable need to happen
> +# before testing any assembler features, or else the result of the tests would
> +# be stale if the integrated assembler is not used.
> +
> +# Older clang's built-in assembler doesn't understand .skip with labels:
> +# https://bugs.llvm.org/show_bug.cgi?id=27369
> +$(call as-option-add,CFLAGS,CC,".L0: .L1: .skip (.L1 - .L0)",,\
> + -no-integrated-as)
> +
> +# Check whether clang asm()-s support .include.
> +$(call as-option-add,CFLAGS,CC,".include \"asm/indirect_thunk_asm.h\"",,\
> + -no-integrated-as)
> +
> +# Check whether clang keeps .macro-s between asm()-s:
> +# https://bugs.llvm.org/show_bug.cgi?id=36110
> +$(call as-option-add,CFLAGS,CC,\
> + ".macro FOO;.endm"$$(close); asm volatile $$(open)".macro FOO;.endm",\
> + -no-integrated-as)
> +endif
> +
> +$(call cc-options-add,CFLAGS,CC,$(EMBEDDED_EXTRA_CFLAGS))
> +$(call cc-option-add,CFLAGS,CC,-Wnested-externs)
> +$(call as-option-add,CFLAGS,CC,"vmcall",-DHAVE_AS_VMX)
> +$(call as-option-add,CFLAGS,CC,"crc32 %eax$$(comma)%eax",-DHAVE_AS_SSE4_2)
> +$(call as-option-add,CFLAGS,CC,"invept (%rax)$$(comma)%rax",-DHAVE_AS_EPT)
> +$(call as-option-add,CFLAGS,CC,"rdrand %eax",-DHAVE_AS_RDRAND)
> +$(call as-option-add,CFLAGS,CC,"rdfsbase %rax",-DHAVE_AS_FSGSBASE)
> +$(call as-option-add,CFLAGS,CC,"xsaveopt (%rax)",-DHAVE_AS_XSAVEOPT)
> +$(call as-option-add,CFLAGS,CC,"rdseed %eax",-DHAVE_AS_RDSEED)
> +$(call as-option-add,CFLAGS,CC,"clwb (%rax)",-DHAVE_AS_CLWB)
> +ifeq ($(call as-insn,$(CC) $(CFLAGS),".equ \"x\"$(comma)1",y),y)
> + export HAVE_AS_QUOTED_SYM := y
> +endif
> +$(call as-option-add,CFLAGS,CC,"invpcid (%rax)$$(comma)%rax",-DHAVE_AS_INVPCID)
> +
> +# GAS's idea of true is -1. Clang's idea is 1
> +$(call as-option-add,CFLAGS,CC,\
> + ".if ((1 > 0) < 0); .error \"\";.endif",,-DHAVE_AS_NEGATIVE_TRUE)
> +
> +# Check to see whether the assmbler supports the .nop directive.
> +$(call as-option-add,CFLAGS,CC,\
> + ".L1: .L2: .nops (.L2 - .L1)$$(comma)9",-DHAVE_AS_NOPS_DIRECTIVE)
> +
> +CFLAGS += -mno-red-zone -fpic -fno-asynchronous-unwind-tables
> +
> +# Xen doesn't use SSE interally. If the compiler supports it, also skip the
> +# SSE setup for variadic function calls.
> +CFLAGS += -mno-sse $(call cc-option,$(CC),-mskip-rax-setup)
> +
> +# Compile with thunk-extern, indirect-branch-register if avaiable.
> +ifeq ($(CONFIG_INDIRECT_THUNK),y)
> +CFLAGS += -mindirect-branch=thunk-extern -mindirect-branch-register
> +CFLAGS += -fno-jump-tables
> +endif
CFLAGS-$(CONFIG_INDIRECT_THUNK) += ... ?
> +# If supported by the compiler, reduce stack alignment to 8 bytes. But allow
> +# this to be overridden elsewhere.
> +$(call cc-option-add,CFLAGS-stack-boundary,CC,-mpreferred-stack-boundary=3)
> +export CFLAGS-stack-boundary
I find such random export unfortunate, but I can see why - within
the targeted model - this is the least bad alternative.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
next prev parent reply other threads:[~2020-01-30 14:33 UTC|newest]
Thread overview: 50+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-01-17 10:53 [Xen-devel] [XEN PATCH v2 00/12] xen: Build system improvements Anthony PERARD
2020-01-17 10:53 ` [Xen-devel] [XEN PATCH v2 01/12] xen/build: Remove left over -DMAX_PHYS_IRQS Anthony PERARD
2020-01-17 11:03 ` Jan Beulich
2020-01-17 10:53 ` [Xen-devel] [XEN PATCH v2 02/12] xen/build: Use obj-y += subdir/ instead of subdir-y Anthony PERARD
2020-01-29 14:19 ` Jan Beulich
2020-01-30 16:54 ` Anthony PERARD
2020-01-31 8:35 ` Jan Beulich
2020-02-03 11:31 ` Anthony PERARD
2020-01-17 10:53 ` [Xen-devel] [XEN PATCH v2 03/12] xen/build: use $(clean) shorthand for clean targets Anthony PERARD
2020-01-29 14:21 ` Jan Beulich
2020-01-17 10:53 ` [Xen-devel] [XEN PATCH v2 04/12] xen/build: extract clean target from Rules.mk Anthony PERARD
2020-01-29 14:30 ` Jan Beulich
2020-01-30 18:10 ` Anthony PERARD
2020-01-31 8:50 ` Jan Beulich
2020-01-17 10:53 ` [Xen-devel] [XEN PATCH v2 05/12] xen/include: remove include of Config.mk Anthony PERARD
2020-01-29 15:28 ` Jan Beulich
2020-01-29 15:33 ` Jan Beulich
2020-01-30 18:34 ` Anthony PERARD
2020-01-31 8:56 ` Jan Beulich
2020-01-17 10:53 ` [Xen-devel] [XEN PATCH v2 06/12] xen/test/livepatch: " Anthony PERARD
2020-01-17 17:17 ` Ross Lagerwall
2020-01-17 10:53 ` [Xen-devel] [XEN PATCH v2 07/12] xen/build: run targets csopes, tags, .. without Rules.mk Anthony PERARD
2020-01-30 11:29 ` Jan Beulich
2020-01-17 10:53 ` [Xen-devel] [XEN PATCH v2 08/12] xen/build: make tests in test/ directly Anthony PERARD
2020-01-30 11:31 ` Jan Beulich
2020-01-17 10:53 ` [Xen-devel] [XEN PATCH v2 09/12] xen/build: include include/config/auto.conf in main Makefile Anthony PERARD
2020-01-30 13:06 ` Jan Beulich
2020-02-03 11:45 ` Anthony PERARD
2020-02-03 12:20 ` Jan Beulich
2020-01-17 10:53 ` [Xen-devel] [XEN PATCH v2 10/12] xen/build: use new $(c_flags) and $(a_flags) instead of $(CFLAGS) Anthony PERARD
2020-01-30 13:29 ` Jan Beulich
2020-02-03 12:17 ` Anthony PERARD
2020-02-03 12:34 ` Jan Beulich
2020-01-17 10:53 ` [Xen-devel] [XEN PATCH v2 11/12] xen/build: introduce ccflags-y and CFLAGS_$@ Anthony PERARD
2020-01-30 13:39 ` Jan Beulich
2020-02-03 14:23 ` Anthony PERARD
2020-02-03 15:20 ` Jan Beulich
2020-01-17 10:53 ` [Xen-devel] [XEN PATCH v2 12/12] xen/build: have the root Makefile generates the CFLAGS Anthony PERARD
2020-01-30 14:33 ` Jan Beulich [this message]
2020-02-03 13:57 ` Anthony PERARD
2020-02-03 15:26 ` Jan Beulich
2020-01-21 13:59 ` [Xen-devel] [XEN PATCH v2.1 13/12] Makefile: Fix install-tests Anthony PERARD
2020-01-30 11:37 ` Jan Beulich
2020-02-03 14:29 ` Anthony PERARD
2020-02-03 15:21 ` Jan Beulich
2020-01-21 13:59 ` [Xen-devel] [XEN PATCH v2.1 14/12] squash! xen/build: introduce ccflags-y and CFLAGS_$@ Anthony PERARD
2020-01-21 13:59 ` [Xen-devel] [XEN PATCH v2.1 15/12] squash! xen/build: have the root Makefile generates the CFLAGS Anthony PERARD
2020-01-30 13:32 ` Jan Beulich
2020-02-03 14:32 ` Anthony PERARD
2020-01-21 14:08 ` [Xen-devel] [XEN PATCH v2 00/12] xen: Build system improvements Anthony PERARD
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=e704f28b-818f-8f92-53a8-c9c804805aff@suse.com \
--to=jbeulich@suse.com \
--cc=George.Dunlap@eu.citrix.com \
--cc=Volodymyr_Babchuk@epam.com \
--cc=andrew.cooper3@citrix.com \
--cc=anthony.perard@citrix.com \
--cc=ian.jackson@eu.citrix.com \
--cc=julien@xen.org \
--cc=konrad.wilk@oracle.com \
--cc=roger.pau@citrix.com \
--cc=sstabellini@kernel.org \
--cc=wl@xen.org \
--cc=xen-devel@lists.xenproject.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).