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

[Anthony PERARD <anthony.perard@xxxxxxxxxx>]

On Thu, Jan 30, 2020 at 03:33:15PM +0100, Jan Beulich wrote:
> 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?

I could try to add directly to c_flags, it might not be that bad.
arch_ccflags is my invention, Linux doesn't have something similar.

> > --- 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 = )?

Makefile includes Config.mk before these lines, so having += simply add
to the CFLAGS already generated by Config.mk.

> > +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 ...

I wanted to avoid too much modification, and mostly want to move the code
around. So it would be easier to check that the commit doesn't introduce
errors.  So, if your are fine with patch that move code and modify it,
I'll fix some of the oddities. (Or I can have another patch for it.)

> > +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?

I guess that's fine.

> > --- 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?

Sounds good, I'll do that.

> > +ifeq ($(CONFIG_INDIRECT_THUNK),y)
> > +CFLAGS += -mindirect-branch=thunk-extern -mindirect-branch-register
> > +CFLAGS += -fno-jump-tables
> > +endif
> 
> CFLAGS-$(CONFIG_INDIRECT_THUNK) += ... ?

Will change.

Thanks,

-- 
Anthony PERARD

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel