| Message ID | 20200226113355.2532224-15-anthony.perard@citrix.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xen: Build system improvements | expand |
On Wed, Feb 26, 2020 at 11:33:46AM +0000, Anthony PERARD wrote: > From: Anthony PERARD <anthony.perard@gmail.com> > > In a later patch ("xen/build: have the root Makefile generates the > CFLAGS), we want to generate the CFLAGS in xen/Makefile, then export > it and have Rules.mk use a CFLAGS from the environment variables. That > changes the flavor of the CFLAGS and flags intended for one target > (like -D__OBJECT_FILE__ and -M%) gets propagated and duplicated. So we > start by moving such flags out of $(CFLAGS) and into $(c_flags) which > is to be modified by only Rules.mk. > > __OBJECT_FILE__ is only used by arch/x86/mm/*.c files, so having it in > $(c_flags) is enough, we don't need it in $(a_flags). This seem to be used only by source files that are build multiple times with different parameters in order to generate different object files. Is there any harm in having it also in the assembler flags? (in case we require such usage in the future) Or maybe we could even limit __OBJECT_FILE__ to mm/ files that require it only? > > For include/Makefile and as-insn we can keep using CFLAGS, but since > it doesn't have -M* flags anymore there is no need to filter them out. > > The XEN_BUILD_EFI tests in arch/x86/Makefile was filtering out > CFLAGS-y, but according to dd40177c1bc8 ("x86-64/EFI: add CFLAGS to > check compile"), it was done to filter out -MF. CFLAGS doesn't > have those flags anymore, so no filtering is needed. > > This is inspired by the way Kbuild generates CFLAGS for each targets. > > Signed-off-by: Anthony PERARD <anthony.perard@citrix.com> Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>
On 26.02.2020 12:33, Anthony PERARD wrote: > --- a/xen/scripts/Kbuild.include > +++ b/xen/scripts/Kbuild.include > @@ -10,7 +10,7 @@ DEPS_INCLUDE = $(addsuffix .d2, $(basename $(wildcard $(DEPS)))) > # as-insn: Check whether assembler supports an instruction. > # Usage: cflags-y += $(call as-insn,CC FLAGS,"insn",option-yes,option-no) > as-insn = $(if $(shell echo 'void _(void) { asm volatile ( $(2) ); }' \ > - | $(filter-out -M% %.d -include %/include/xen/config.h,$(1)) \ > + | $(filter-out -include %/include/xen/config.h,$(1)) \ > -c -x c -o /dev/null - 2>&1),$(4),$(3)) I'm sorry, while it was me to suggest this change - is this correct? The variable to modify is a parameter of this macro, i.e. things aren't limited to CFLAGS here. If we want to disallow use with e.g. c_flags or anything derived from it, then we should find some way to actually enforce this (like dropping the respective parameter; I'm uncertain though whether we wouldn't regret this if we ever got to the point where we wanted to use a newer insn in a .S file). Jan
On Wed, Mar 04, 2020 at 03:42:36PM +0100, Jan Beulich wrote: > On 26.02.2020 12:33, Anthony PERARD wrote: > > --- a/xen/scripts/Kbuild.include > > +++ b/xen/scripts/Kbuild.include > > @@ -10,7 +10,7 @@ DEPS_INCLUDE = $(addsuffix .d2, $(basename $(wildcard $(DEPS)))) > > # as-insn: Check whether assembler supports an instruction. > > # Usage: cflags-y += $(call as-insn,CC FLAGS,"insn",option-yes,option-no) > > as-insn = $(if $(shell echo 'void _(void) { asm volatile ( $(2) ); }' \ > > - | $(filter-out -M% %.d -include %/include/xen/config.h,$(1)) \ > > + | $(filter-out -include %/include/xen/config.h,$(1)) \ > > -c -x c -o /dev/null - 2>&1),$(4),$(3)) > > I'm sorry, while it was me to suggest this change - is this > correct? The variable to modify is a parameter of this macro, > i.e. things aren't limited to CFLAGS here. If we want to > disallow use with e.g. c_flags or anything derived from it, > then we should find some way to actually enforce this (like > dropping the respective parameter; I'm uncertain though whether > we wouldn't regret this if we ever got to the point where we > wanted to use a newer insn in a .S file). It is probably better to leave the macro as it is for now. I'll drop this hunk. I think it would be nice to have the macro use directly CFLAGS (or XEN_CFLAGS), but since the macro is used within as-option-add, which needs the flags variable name as parameter, we can't really change the way as-insn works. We could come back to that later, remove the use of as-option-add, and have as-insn use CFLAGS (or AFLAGS) directly. That's how the macro as-instr of Linux works, the macro always uses KBUILD_AFLAGS. Thanks,
On Thu, Feb 27, 2020 at 11:22:38AM +0100, Roger Pau Monné wrote: > On Wed, Feb 26, 2020 at 11:33:46AM +0000, Anthony PERARD wrote: > > From: Anthony PERARD <anthony.perard@gmail.com> > > > > In a later patch ("xen/build: have the root Makefile generates the > > CFLAGS), we want to generate the CFLAGS in xen/Makefile, then export > > it and have Rules.mk use a CFLAGS from the environment variables. That > > changes the flavor of the CFLAGS and flags intended for one target > > (like -D__OBJECT_FILE__ and -M%) gets propagated and duplicated. So we > > start by moving such flags out of $(CFLAGS) and into $(c_flags) which > > is to be modified by only Rules.mk. > > > > __OBJECT_FILE__ is only used by arch/x86/mm/*.c files, so having it in > > $(c_flags) is enough, we don't need it in $(a_flags). > > This seem to be used only by source files that are build multiple > times with different parameters in order to generate different object > files. > > Is there any harm in having it also in the assembler flags? (in case > we require such usage in the future) Not really any harm, no, but that can be done later when needed I think. > Or maybe we could even limit __OBJECT_FILE__ to mm/ files that require > it only? That's a possibility, yes. I'll be adding flags to those specific files anyway (GUEST_PAGING_LEVELS, done in a later patch), I could add __OBJECT_FILE__ to the list. > > > > For include/Makefile and as-insn we can keep using CFLAGS, but since > > it doesn't have -M* flags anymore there is no need to filter them out. > > > > The XEN_BUILD_EFI tests in arch/x86/Makefile was filtering out > > CFLAGS-y, but according to dd40177c1bc8 ("x86-64/EFI: add CFLAGS to > > check compile"), it was done to filter out -MF. CFLAGS doesn't > > have those flags anymore, so no filtering is needed. > > > > This is inspired by the way Kbuild generates CFLAGS for each targets. > > > > Signed-off-by: Anthony PERARD <anthony.perard@citrix.com> > > Reviewed-by: Roger Pau Monné <roger.pau@citrix.com> Thanks,
diff --git a/xen/Rules.mk b/xen/Rules.mk index 92a13ca60163..4aa119a90c27 100644 --- a/xen/Rules.mk +++ b/xen/Rules.mk @@ -57,7 +57,6 @@ 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 -CFLAGS += '-D__OBJECT_FILE__="$@"' ifneq ($(CONFIG_CC_IS_CLANG),y) # Clang doesn't understand this command line argument, and doesn't appear to @@ -70,9 +69,6 @@ AFLAGS += -D__ASSEMBLY__ ALL_OBJS := $(ALL_OBJS-y) -# Get gcc to generate the dependencies for us. -CFLAGS-y += -MMD -MF $(@D)/.$(@F).d - CFLAGS += $(CFLAGS-y) # allow extra CFLAGS externally via EXTRA_CFLAGS_XEN_CORE CFLAGS += $(EXTRA_CFLAGS_XEN_CORE) @@ -146,9 +142,12 @@ endif # Always build obj-bin files as binary even if they come from C source. $(obj-bin-y): CFLAGS := $(filter-out -flto,$(CFLAGS)) +c_flags = -MMD -MF $(@D)/.$(@F).d $(CFLAGS) '-D__OBJECT_FILE__="$@"' +a_flags = -MMD -MF $(@D)/.$(@F).d $(AFLAGS) + built_in.o: $(obj-y) $(extra-y) ifeq ($(obj-y),) - $(CC) $(CFLAGS) -c -x c /dev/null -o $@ + $(CC) $(c_flags) -c -x c /dev/null -o $@ else ifeq ($(CONFIG_LTO),y) $(LD_LTO) -r -o $@ $(filter-out $(extra-y),$^) @@ -159,7 +158,7 @@ endif built_in_bin.o: $(obj-bin-y) $(extra-y) ifeq ($(obj-bin-y),) - $(CC) $(AFLAGS) -c -x assembler /dev/null -o $@ + $(CC) $(a_flags) -c -x assembler /dev/null -o $@ else $(LD) $(LDFLAGS) -r -o $@ $(filter-out $(extra-y),$^) endif @@ -178,7 +177,7 @@ SRCPATH := $(patsubst $(BASEDIR)/%,%,$(CURDIR)) %.o: %.c Makefile ifeq ($(CONFIG_ENFORCE_UNIQUE_SYMBOLS),y) - $(CC) $(CFLAGS) -c $< -o $(@D)/.$(@F).tmp -MQ $@ + $(CC) $(c_flags) -c $< -o $(@D)/.$(@F).tmp -MQ $@ ifeq ($(CONFIG_CC_IS_CLANG),y) $(OBJCOPY) --redefine-sym $<=$(SRCPATH)/$< $(@D)/.$(@F).tmp $@ else @@ -186,11 +185,11 @@ else endif rm -f $(@D)/.$(@F).tmp else - $(CC) $(CFLAGS) -c $< -o $@ + $(CC) $(c_flags) -c $< -o $@ endif %.o: %.S Makefile - $(CC) $(AFLAGS) -c $< -o $@ + $(CC) $(a_flags) -c $< -o $@ $(filter %.init.o,$(obj-y) $(obj-bin-y) $(extra-y)): %.init.o: %.o Makefile $(OBJDUMP) -h $< | sed -n '/[0-9]/{s,00*,0,g;p;}' | while read idx name sz rest; do \ @@ -205,12 +204,12 @@ $(filter %.init.o,$(obj-y) $(obj-bin-y) $(extra-y)): %.init.o: %.o Makefile $(OBJCOPY) $(foreach s,$(SPECIAL_DATA_SECTIONS),--rename-section .$(s)=.init.$(s)) $< $@ %.i: %.c Makefile - $(CPP) $(filter-out -Wa$(comma)%,$(CFLAGS)) $< -o $@ + $(CPP) $(filter-out -Wa$(comma)%,$(c_flags)) $< -o $@ %.s: %.c Makefile - $(CC) $(filter-out -Wa$(comma)%,$(CFLAGS)) -S $< -o $@ + $(CC) $(filter-out -Wa$(comma)%,$(c_flags)) -S $< -o $@ %.s: %.S Makefile - $(CPP) $(filter-out -Wa$(comma)%,$(AFLAGS)) $< -o $@ + $(CPP) $(filter-out -Wa$(comma)%,$(a_flags)) $< -o $@ -include $(DEPS_INCLUDE) diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile index 1044c2298a05..7f1427630b96 100644 --- a/xen/arch/arm/Makefile +++ b/xen/arch/arm/Makefile @@ -121,10 +121,10 @@ $(TARGET)-syms: prelink.o xen.lds rm -f $(@D)/.$(@F).[0-9]* asm-offsets.s: $(TARGET_SUBARCH)/asm-offsets.c - $(CC) $(filter-out -flto,$(CFLAGS)) -S -o $@ $< + $(CC) $(filter-out -flto,$(c_flags)) -S -o $@ $< xen.lds: xen.lds.S - $(CC) -P -E -Ui386 $(AFLAGS) -o $@ $< + $(CC) -P -E -Ui386 $(a_flags) -o $@ $< sed -e 's/xen\.lds\.o:/xen\.lds:/g' <.xen.lds.d >.xen.lds.d.new mv -f .xen.lds.d.new .xen.lds.d diff --git a/xen/arch/x86/Makefile b/xen/arch/x86/Makefile index ed709e2373ac..7fbac8ac525d 100644 --- a/xen/arch/x86/Makefile +++ b/xen/arch/x86/Makefile @@ -171,7 +171,7 @@ EFI_LDFLAGS += --major-os-version=2 --minor-os-version=0 EFI_LDFLAGS += --major-subsystem-version=2 --minor-subsystem-version=0 # Check if the compiler supports the MS ABI. -export XEN_BUILD_EFI := $(shell $(CC) $(filter-out $(CFLAGS-y) .%.d,$(CFLAGS)) -c efi/check.c -o efi/check.o 2>/dev/null && echo y) +export XEN_BUILD_EFI := $(shell $(CC) $(CFLAGS) -c efi/check.c -o efi/check.o 2>/dev/null && echo y) # Check if the linker supports PE. XEN_BUILD_PE := $(if $(XEN_BUILD_EFI),$(shell $(LD) -mi386pep --subsystem=10 -o efi/check.efi efi/check.o 2>/dev/null && echo y)) CFLAGS-$(XEN_BUILD_EFI) += -DXEN_BUILD_EFI @@ -226,7 +226,7 @@ efi/boot.init.o efi/runtime.o efi/compat.o efi/buildid.o efi/relocs-dummy.o: $(B efi/boot.init.o efi/runtime.o efi/compat.o efi/buildid.o efi/relocs-dummy.o: ; asm-offsets.s: $(TARGET_SUBARCH)/asm-offsets.c $(BASEDIR)/include/asm-x86/asm-macros.h - $(CC) $(filter-out -Wa$(comma)% -flto,$(CFLAGS)) -S -o $@ $< + $(CC) $(filter-out -Wa$(comma)% -flto,$(c_flags)) -S -o $@ $< asm-macros.i: CFLAGS += -D__ASSEMBLY__ -P @@ -243,7 +243,7 @@ $(BASEDIR)/include/asm-x86/asm-macros.h: asm-macros.i Makefile efi.lds: AFLAGS += -DEFI xen.lds efi.lds: xen.lds.S - $(CC) -P -E -Ui386 $(filter-out -Wa$(comma)%,$(AFLAGS)) -o $@ $< + $(CC) -P -E -Ui386 $(filter-out -Wa$(comma)%,$(a_flags)) -o $@ $< sed -e 's/.*\.lds\.o:/$(@F):/g' <.$(@F).d >.$(@F).d.new mv -f .$(@F).d.new .$(@F).d diff --git a/xen/arch/x86/mm/Makefile b/xen/arch/x86/mm/Makefile index d87dc0aa6eeb..a2431fde6bb4 100644 --- a/xen/arch/x86/mm/Makefile +++ b/xen/arch/x86/mm/Makefile @@ -12,10 +12,10 @@ obj-$(CONFIG_HVM) += p2m-ept.o p2m-pod.o obj-y += paging.o guest_walk_%.o: guest_walk.c Makefile - $(CC) $(CFLAGS) -DGUEST_PAGING_LEVELS=$* -c $< -o $@ + $(CC) $(c_flags) -DGUEST_PAGING_LEVELS=$* -c $< -o $@ guest_walk_%.i: guest_walk.c Makefile - $(CPP) $(filter-out -Wa$(comma)%,$(CFLAGS)) -DGUEST_PAGING_LEVELS=$* -c $< -o $@ + $(CPP) $(filter-out -Wa$(comma)%,$(c_flags)) -DGUEST_PAGING_LEVELS=$* -c $< -o $@ guest_walk_%.s: guest_walk.c Makefile - $(CC) $(filter-out -Wa$(comma)%,$(CFLAGS)) -DGUEST_PAGING_LEVELS=$* -S $< -o $@ + $(CC) $(filter-out -Wa$(comma)%,$(c_flags)) -DGUEST_PAGING_LEVELS=$* -S $< -o $@ diff --git a/xen/arch/x86/mm/hap/Makefile b/xen/arch/x86/mm/hap/Makefile index b14a9aff93d2..22e7ad54bd33 100644 --- a/xen/arch/x86/mm/hap/Makefile +++ b/xen/arch/x86/mm/hap/Makefile @@ -6,10 +6,10 @@ obj-y += nested_hap.o obj-y += nested_ept.o guest_walk_%level.o: guest_walk.c Makefile - $(CC) $(CFLAGS) -DGUEST_PAGING_LEVELS=$* -c $< -o $@ + $(CC) $(c_flags) -DGUEST_PAGING_LEVELS=$* -c $< -o $@ guest_walk_%level.i: guest_walk.c Makefile - $(CPP) $(filter-out -Wa$(comma)%,$(CFLAGS)) -DGUEST_PAGING_LEVELS=$* -c $< -o $@ + $(CPP) $(filter-out -Wa$(comma)%,$(c_flags)) -DGUEST_PAGING_LEVELS=$* -c $< -o $@ guest_walk_%level.s: guest_walk.c Makefile - $(CC) $(filter-out -Wa$(comma)%,$(CFLAGS)) -DGUEST_PAGING_LEVELS=$* -S $< -o $@ + $(CC) $(filter-out -Wa$(comma)%,$(c_flags)) -DGUEST_PAGING_LEVELS=$* -S $< -o $@ diff --git a/xen/arch/x86/mm/shadow/Makefile b/xen/arch/x86/mm/shadow/Makefile index ff03a9937f9b..23d3ff10802c 100644 --- a/xen/arch/x86/mm/shadow/Makefile +++ b/xen/arch/x86/mm/shadow/Makefile @@ -7,10 +7,10 @@ obj-y += none.o endif guest_%.o: multi.c Makefile - $(CC) $(CFLAGS) -DGUEST_PAGING_LEVELS=$* -c $< -o $@ + $(CC) $(c_flags) -DGUEST_PAGING_LEVELS=$* -c $< -o $@ guest_%.i: multi.c Makefile - $(CPP) $(filter-out -Wa$(comma)%,$(CFLAGS)) -DGUEST_PAGING_LEVELS=$* -c $< -o $@ + $(CPP) $(filter-out -Wa$(comma)%,$(c_flags)) -DGUEST_PAGING_LEVELS=$* -c $< -o $@ guest_%.s: multi.c Makefile - $(CC) $(filter-out -Wa$(comma)%,$(CFLAGS)) -DGUEST_PAGING_LEVELS=$* -S $< -o $@ + $(CC) $(filter-out -Wa$(comma)%,$(c_flags)) -DGUEST_PAGING_LEVELS=$* -S $< -o $@ diff --git a/xen/include/Makefile b/xen/include/Makefile index 433bad9055b2..a488a98d8bb7 100644 --- a/xen/include/Makefile +++ b/xen/include/Makefile @@ -64,7 +64,7 @@ compat/%.h: compat/%.i Makefile $(BASEDIR)/tools/compat-build-header.py mv -f $@.new $@ compat/%.i: compat/%.c Makefile - $(CPP) $(filter-out -Wa$(comma)% -M% %.d -include %/include/xen/config.h,$(CFLAGS)) $(cppflags-y) -o $@ $< + $(CPP) $(filter-out -Wa$(comma)% -include %/include/xen/config.h,$(CFLAGS)) $(cppflags-y) -o $@ $< compat/%.c: public/%.h xlat.lst Makefile $(BASEDIR)/tools/compat-build-source.py mkdir -p $(@D) diff --git a/xen/scripts/Kbuild.include b/xen/scripts/Kbuild.include index 806c68824ed5..14bd4e110b45 100644 --- a/xen/scripts/Kbuild.include +++ b/xen/scripts/Kbuild.include @@ -10,7 +10,7 @@ DEPS_INCLUDE = $(addsuffix .d2, $(basename $(wildcard $(DEPS)))) # as-insn: Check whether assembler supports an instruction. # Usage: cflags-y += $(call as-insn,CC FLAGS,"insn",option-yes,option-no) as-insn = $(if $(shell echo 'void _(void) { asm volatile ( $(2) ); }' \ - | $(filter-out -M% %.d -include %/include/xen/config.h,$(1)) \ + | $(filter-out -include %/include/xen/config.h,$(1)) \ -c -x c -o /dev/null - 2>&1),$(4),$(3)) # as-option-add: Conditionally add options to flags