On Mon, Oct 11, 2021 at 04:02:22PM +0200, Jan Beulich wrote: > On 24.08.2021 12:50, Anthony PERARD wrote: > > A subdirectory is now built by setting "$(obj)" instead of changing > > directory. "$(obj)" should always be set when using "Rules.mk" and > > thus a shortcut "$(build)" is introduced and should be used. > > > > A new variable "$(need-builtin)" is introduce. It is to be used > > whenever a "built_in.o" is wanted from a subdirectory. "built_in.o" > > isn't the main target anymore, and thus only needs to depends on the > > objects that should be part of "built_in.o". > > How "good" are our chances that we hit a need-builtin variable from > the environment? Its uses are simply using "ifdef".
I think it would be low as Linux is using the same one. If it were define, I think that would mean building a few more object than expected which would not be used. > > --- a/xen/Rules.mk > > +++ b/xen/Rules.mk > > @@ -3,19 +3,29 @@ > > # Makefile and are consumed by Rules.mk > > # > > > > -obj := . > > src := $(obj) > > > > +PHONY := __build > > +__build: > > + > > -include $(BASEDIR)/include/config/auto.conf > > > > include $(XEN_ROOT)/Config.mk > > include $(BASEDIR)/scripts/Kbuild.include > > > > +ifndef obj > > +$(warning kbuild: Rules.mk is included improperly) > > +endif > > Is there a particular reason for this to come only here, rather than > before the include-s (e.g. right at where the assignment to the > variable lived)? Probably not, Linux's Kbuild does check it quite late but I don't know the reason. I can move the check earlier. > > @@ -51,27 +61,54 @@ cmd_objcopy = $(OBJCOPY) $(OBJCOPYFLAGS) $< $@ > > quiet_cmd_binfile = BINFILE $@ > > cmd_binfile = $(SHELL) $(BASEDIR)/tools/binfile $(BINFILE_FLAGS) $@ $(2) > > > > -define gendep > > - ifneq ($(1),$(subst /,:,$(1))) > > - DEPS += $(dir $(1)).$(notdir $(1)).d > > - endif > > -endef > > -$(foreach o,$(filter-out %/,$(obj-y) $(obj-bin-y) $(extra-y)),$(eval > > $(call gendep,$(o)))) > > +# Figure out what we need to build from the various variables > > +# > > =========================================================================== > > + > > +# Libraries are always collected in one lib file. > > +# Filter out objects already built-in > > +lib-y := $(filter-out $(obj-y), $(sort $(lib-y))) > > + > > +# Subdirectories we need to descend into > > +subdir-y := $(subdir-y) $(patsubst %/,%,$(filter %/, $(obj-y))) > > Deliberately or accidentally not += ? Seems to be accidentally. Kbuild does a $(sort ) here, I should probably do the same, just to get rid of duplicates if they are any. > > @@ -156,21 +192,13 @@ endif > > PHONY += FORCE > > FORCE: > > > > -%/built_in.o %/lib.a: FORCE > > - $(MAKE) -f $(BASEDIR)/Rules.mk -C $* built_in.o > > - > > -%/built_in_bin.o: FORCE > > - $(MAKE) -f $(BASEDIR)/Rules.mk -C $* built_in_bin.o > > - > > -SRCPATH := $(patsubst $(BASEDIR)/%,%,$(CURDIR)) > > - > > quiet_cmd_cc_o_c = CC $@ > > ifeq ($(CONFIG_ENFORCE_UNIQUE_SYMBOLS),y) > > cmd_cc_o_c = $(CC) $(c_flags) -c $< -o $(dot-target).tmp -MQ $@ > > ifeq ($(CONFIG_CC_IS_CLANG),y) > > - cmd_objcopy_fix_sym = $(OBJCOPY) --redefine-sym $<=$(SRCPATH)/$< > > $(dot-target).tmp $@ > > + cmd_objcopy_fix_sym = $(OBJCOPY) --redefine-sym $(<F)=$< > > $(dot-target).tmp $@ > > Are you sure about the $< => $(<F) transformation here? Previoiusly it > was present only ... I have to check again. Maybe $< didn't work and it's more obvious with this patch. Or maybe that depends on the version of clang. > > else > > - cmd_objcopy_fix_sym = $(OBJCOPY) --redefine-sym > > $(<F)=$(SRCPATH)/$< $(dot-target).tmp $@ > > + cmd_objcopy_fix_sym = $(OBJCOPY) --redefine-sym $(<F)=$< > > $(dot-target).tmp $@ > > ... here. > > > @@ -251,6 +292,9 @@ existing-targets := $(wildcard $(sort $(targets))) > > > > -include $(foreach f,$(existing-targets),$(dir $(f)).$(notdir $(f)).cmd) > > > > +DEPS:= $(foreach f,$(existing-targets),$(dir $(f)).$(notdir $(f)).d) > > Nit: Preferably blanks on both sides of := or none at all, please. Will fix. > > --- a/xen/arch/x86/Makefile > > +++ b/xen/arch/x86/Makefile > > @@ -81,6 +81,9 @@ endif > > extra-y += asm-macros.i > > extra-y += xen.lds > > > > +# Allows usercopy.c to includes itself > > Nit: include Indeed. > > +$(obj)/usercopy.o: CFLAGS-y += -I. > > This is ugly, but presumably unavoidable. Preferably I would see us > the more specific -iquote though, assuming clang also supports it. clang does have -iquote, so I guess it could be used. That would be the first use of -iquote so I hope nothing would break with it. > > --- a/xen/arch/x86/boot/Makefile > > +++ b/xen/arch/x86/boot/Makefile > > @@ -1,8 +1,8 @@ > > obj-bin-y += head.o > > > > -DEFS_H_DEPS = $(src)/defs.h $(BASEDIR)/include/xen/stdbool.h > > +DEFS_H_DEPS = $(BASEDIR)/$(src)/defs.h $(BASEDIR)/include/xen/stdbool.h > > > > -CMDLINE_DEPS = $(DEFS_H_DEPS) $(src)/video.h > > +CMDLINE_DEPS = $(DEFS_H_DEPS) $(BASEDIR)/$(src)/video.h > > Hmm, new uses of $(BASEDIR) (a few more further down). Why not > $(srctree)? I think I haven't introduce $(abs_srctree) yet, and this needs to be an absolute path (the path is also used by a make which run from a different current directory). Thanks, -- Anthony PERARD
