Re: [Xen-devel] [XEN PATCH v2 12/12] xen/build: have the root Makefile generates the CFLAGS

[Jan Beulich <jbeulich@xxxxxxxx>]

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@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel