Re: [PATCH v3 2/8] lib: collect library files in an archive

[Bertrand Marquis <Bertrand.Marquis@xxxxxxx>]

Hi,

> On 23 Nov 2020, at 15:21, Jan Beulich <jbeulich@xxxxxxxx> wrote:
> 
> In order to (subsequently) drop odd things like CONFIG_NEEDS_LIST_SORT
> just to avoid bloating binaries when only some arch-es and/or
> configurations need generic library routines, combine objects under lib/
> into an archive, which the linker then can pick the necessary objects
> out of.
> 
> Note that we can't use thin archives just yet, until we've raised the
> minimum required binutils version suitably.
> 
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
Reviewed-by: Bertrand Marquis <bertrand.marquis@xxxxxxx>

with one remark...


> ---
> xen/Rules.mk          | 29 +++++++++++++++++++++++++----
> xen/arch/arm/Makefile |  6 +++---
> xen/arch/x86/Makefile |  8 ++++----
> xen/lib/Makefile      |  3 ++-
> 4 files changed, 34 insertions(+), 12 deletions(-)
> 
> diff --git a/xen/Rules.mk b/xen/Rules.mk
> index d5e5eb33de39..aba6ca2a90f5 100644
> --- a/xen/Rules.mk
> +++ b/xen/Rules.mk
> @@ -41,12 +41,16 @@ ALL_OBJS-y               += $(BASEDIR)/xsm/built_in.o
> ALL_OBJS-y               += $(BASEDIR)/arch/$(TARGET_ARCH)/built_in.o
> ALL_OBJS-$(CONFIG_CRYPTO)   += $(BASEDIR)/crypto/built_in.o
> 
> +ALL_LIBS-y               := $(BASEDIR)/lib/lib.a
> +
> # Initialise some variables
> +lib-y :=
> targets :=
> CFLAGS-y :=
> AFLAGS-y :=
> 
> ALL_OBJS := $(ALL_OBJS-y)
> +ALL_LIBS := $(ALL_LIBS-y)
> 
> SPECIAL_DATA_SECTIONS := rodata $(foreach a,1 2 4 8 16, \
>                                             $(foreach w,1 2 4, \
> @@ -60,7 +64,14 @@ include Makefile
> # ---------------------------------------------------------------------------
> 
> quiet_cmd_ld = LD      $@
> -cmd_ld = $(LD) $(XEN_LDFLAGS) -r -o $@ $(real-prereqs)
> +cmd_ld = $(LD) $(XEN_LDFLAGS) -r -o $@ $(filter-out %.a,$(real-prereqs)) \
> +               --start-group $(filter %.a,$(real-prereqs)) --end-group

This might be a good idea to add a comment to explain why the start/end-group
is needed so that someone does not change this back in the future.

Something like: put libraries between start/end group to have unused symbols 
removed.

Cheers
Bertrand

> +
> +# Archive
> +# ---------------------------------------------------------------------------
> +
> +quiet_cmd_ar = AR      $@
> +cmd_ar = rm -f $@; $(AR) cPrs $@ $(real-prereqs)
> 
> # Objcopy
> # ---------------------------------------------------------------------------
> @@ -86,6 +97,10 @@ obj-y    := $(patsubst %/, %/built_in.o, $(obj-y))
> # tell kbuild to descend
> subdir-obj-y := $(filter %/built_in.o, $(obj-y))
> 
> +# Libraries are always collected in one lib file.
> +# Filter out objects already built-in
> +lib-y := $(filter-out $(obj-y), $(sort $(lib-y)))
> +
> $(filter %.init.o,$(obj-y) $(obj-bin-y) $(extra-y)): CFLAGS-y += 
> -DINIT_SECTIONS_ONLY
> 
> ifeq ($(CONFIG_COVERAGE),y)
> @@ -129,7 +144,7 @@ include $(BASEDIR)/arch/$(TARGET_ARCH)/Rules.mk
> c_flags += $(CFLAGS-y)
> a_flags += $(CFLAGS-y) $(AFLAGS-y)
> 
> -built_in.o: $(obj-y) $(extra-y)
> +built_in.o: $(obj-y) $(if $(strip $(lib-y)),lib.a) $(extra-y)
> ifeq ($(strip $(obj-y)),)
>       $(CC) $(c_flags) -c -x c /dev/null -o $@
> else
> @@ -140,8 +155,14 @@ else
> endif
> endif
> 
> +lib.a: $(lib-y) FORCE
> +     $(call if_changed,ar)
> +
> targets += built_in.o
> -targets += $(filter-out $(subdir-obj-y), $(obj-y)) $(extra-y)
> +ifneq ($(strip $(lib-y)),)
> +targets += lib.a
> +endif
> +targets += $(filter-out $(subdir-obj-y), $(obj-y) $(lib-y)) $(extra-y)
> targets += $(MAKECMDGOALS)
> 
> built_in_bin.o: $(obj-bin-y) $(extra-y)
> @@ -155,7 +176,7 @@ endif
> PHONY += FORCE
> FORCE:
> 
> -%/built_in.o: FORCE
> +%/built_in.o %/lib.a: FORCE
>       $(MAKE) -f $(BASEDIR)/Rules.mk -C $* built_in.o
> 
> %/built_in_bin.o: FORCE
> diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile
> index 296c5e68bbc3..612a83b315c8 100644
> --- a/xen/arch/arm/Makefile
> +++ b/xen/arch/arm/Makefile
> @@ -90,14 +90,14 @@ endif
> 
> ifeq ($(CONFIG_LTO),y)
> # Gather all LTO objects together
> -prelink_lto.o: $(ALL_OBJS)
> -     $(LD_LTO) -r -o $@ $^
> +prelink_lto.o: $(ALL_OBJS) $(ALL_LIBS)
> +     $(LD_LTO) -r -o $@ $(filter-out %.a,$^) --start-group $(filter %.a,$^) 
> --end-group
> 
> # Link it with all the binary objects
> prelink.o: $(patsubst %/built_in.o,%/built_in_bin.o,$(ALL_OBJS)) prelink_lto.o
>       $(call if_changed,ld)
> else
> -prelink.o: $(ALL_OBJS) FORCE
> +prelink.o: $(ALL_OBJS) $(ALL_LIBS) FORCE
>       $(call if_changed,ld)
> endif
> 
> diff --git a/xen/arch/x86/Makefile b/xen/arch/x86/Makefile
> index 9b368632fb43..8f2180485b2b 100644
> --- a/xen/arch/x86/Makefile
> +++ b/xen/arch/x86/Makefile
> @@ -132,8 +132,8 @@ EFI_OBJS-$(XEN_BUILD_EFI) := efi/relocs-dummy.o
> 
> ifeq ($(CONFIG_LTO),y)
> # Gather all LTO objects together
> -prelink_lto.o: $(ALL_OBJS)
> -     $(LD_LTO) -r -o $@ $^
> +prelink_lto.o: $(ALL_OBJS) $(ALL_LIBS)
> +     $(LD_LTO) -r -o $@ $(filter-out %.a,$^) --start-group $(filter %.a,$^) 
> --end-group
> 
> # Link it with all the binary objects
> prelink.o: $(patsubst %/built_in.o,%/built_in_bin.o,$(ALL_OBJS)) 
> prelink_lto.o $(EFI_OBJS-y) FORCE
> @@ -142,10 +142,10 @@ prelink.o: $(patsubst 
> %/built_in.o,%/built_in_bin.o,$(ALL_OBJS)) prelink_lto.o $
> prelink-efi.o: $(patsubst %/built_in.o,%/built_in_bin.o,$(ALL_OBJS)) 
> prelink_lto.o FORCE
>       $(call if_changed,ld)
> else
> -prelink.o: $(ALL_OBJS) $(EFI_OBJS-y) FORCE
> +prelink.o: $(ALL_OBJS) $(ALL_LIBS) $(EFI_OBJS-y) FORCE
>       $(call if_changed,ld)
> 
> -prelink-efi.o: $(ALL_OBJS) FORCE
> +prelink-efi.o: $(ALL_OBJS) $(ALL_LIBS) FORCE
>       $(call if_changed,ld)
> endif
> 
> diff --git a/xen/lib/Makefile b/xen/lib/Makefile
> index 53b1da025e0d..b8814361d63e 100644
> --- a/xen/lib/Makefile
> +++ b/xen/lib/Makefile
> @@ -1,2 +1,3 @@
> -obj-y += ctype.o
> obj-$(CONFIG_X86) += x86/
> +
> +lib-y += ctype.o
> 
>