Re: [XEN PATCH v7 26/51] build: build everything from the root dir, use obj=$subdir

[Anthony PERARD <anthony.perard@xxxxxxxxxx>]

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