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

[Jan Beulich <jbeulich@xxxxxxxx>]

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

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

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

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

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

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

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

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

Jan