diff mbox series

[7/9] KVM: selftests: Link selftests directly with lib object files

Message ID 20220429183935.1094599-8-dmatlack@google.com (mailing list archive)
State New, archived
Headers show
Series KVM: selftests: Add nested support to dirty_log_perf_test | expand

Commit Message

David Matlack April 29, 2022, 6:39 p.m. UTC
The linker does obey strong/weak symbols when linking static libraries,
it simply resolves an undefined symbol to the first-encountered symbol.
This means that defining __weak arch-generic functions and then defining
arch-specific strong functions to override them in libkvm will not
always work.

More specifically, if we have:

lib/generic.c:

  void __weak foo(void)
  {
          pr_info("weak\n");
  }

  void bar(void)
  {
          foo();
  }

lib/x86_64/arch.c:

  void foo(void)
  {
          pr_info("strong\n");
  }

And a selftest that calls bar(), it will print "weak". Now if you make
generic.o explicitly depend on arch.o (e.g. add function to arch.c that
is called directly from generic.c) it will print "strong". In other
words, it seems that the linker is free to throw out arch.o when linking
because generic.o does not explicitly depend on it, which causes the
linker to lose the strong symbol.

One solution is to link libkvm.a with --whole-archive so that the linker
doesn't throw away object files it thinks are unnecessary. However that
is a bit difficult to plumb since we are using the common selftests
makefile rules. An easier solution is to drop libkvm.a just link
selftests with all the .o files that were originally in libkvm.a.

Signed-off-by: David Matlack <dmatlack@google.com>
---
 tools/testing/selftests/kvm/Makefile | 13 +++++--------
 1 file changed, 5 insertions(+), 8 deletions(-)

Comments

Peter Xu May 16, 2022, 10:15 p.m. UTC | #1
On Fri, Apr 29, 2022 at 06:39:33PM +0000, David Matlack wrote:
> The linker does obey strong/weak symbols when linking static libraries,
> it simply resolves an undefined symbol to the first-encountered symbol.
> This means that defining __weak arch-generic functions and then defining
> arch-specific strong functions to override them in libkvm will not
> always work.
> 
> More specifically, if we have:
> 
> lib/generic.c:
> 
>   void __weak foo(void)
>   {
>           pr_info("weak\n");
>   }
> 
>   void bar(void)
>   {
>           foo();
>   }
> 
> lib/x86_64/arch.c:
> 
>   void foo(void)
>   {
>           pr_info("strong\n");
>   }
> 
> And a selftest that calls bar(), it will print "weak". Now if you make
> generic.o explicitly depend on arch.o (e.g. add function to arch.c that
> is called directly from generic.c) it will print "strong". In other
> words, it seems that the linker is free to throw out arch.o when linking
> because generic.o does not explicitly depend on it, which causes the
> linker to lose the strong symbol.
> 
> One solution is to link libkvm.a with --whole-archive so that the linker
> doesn't throw away object files it thinks are unnecessary. However that
> is a bit difficult to plumb since we are using the common selftests
> makefile rules. An easier solution is to drop libkvm.a just link
> selftests with all the .o files that were originally in libkvm.a.
> 
> Signed-off-by: David Matlack <dmatlack@google.com>
> ---
>  tools/testing/selftests/kvm/Makefile | 13 +++++--------
>  1 file changed, 5 insertions(+), 8 deletions(-)
> 
> diff --git a/tools/testing/selftests/kvm/Makefile b/tools/testing/selftests/kvm/Makefile
> index af582d168621..c1eb6acb30de 100644
> --- a/tools/testing/selftests/kvm/Makefile
> +++ b/tools/testing/selftests/kvm/Makefile
> @@ -172,12 +172,13 @@ LDFLAGS += -pthread $(no-pie-option) $(pgste-option)
>  # $(TEST_GEN_PROGS) starts with $(OUTPUT)/
>  include ../lib.mk
>  
> -STATIC_LIBS := $(OUTPUT)/libkvm.a
>  LIBKVM_C := $(filter %.c,$(LIBKVM))
>  LIBKVM_S := $(filter %.S,$(LIBKVM))
>  LIBKVM_C_OBJ := $(patsubst %.c, $(OUTPUT)/%.o, $(LIBKVM_C))
>  LIBKVM_S_OBJ := $(patsubst %.S, $(OUTPUT)/%.o, $(LIBKVM_S))
> -EXTRA_CLEAN += $(LIBKVM_C_OBJ) $(LIBKVM_S_OBJ) $(STATIC_LIBS) cscope.*
> +LIBKVM_OBJS = $(LIBKVM_C_OBJ) $(LIBKVM_S_OBJ)
> +
> +EXTRA_CLEAN += $(LIBKVM_OBJS) cscope.*
>  
>  x := $(shell mkdir -p $(sort $(dir $(LIBKVM_C_OBJ) $(LIBKVM_S_OBJ))))
>  $(LIBKVM_C_OBJ): $(OUTPUT)/%.o: %.c
> @@ -186,13 +187,9 @@ $(LIBKVM_C_OBJ): $(OUTPUT)/%.o: %.c
>  $(LIBKVM_S_OBJ): $(OUTPUT)/%.o: %.S
>  	$(CC) $(CFLAGS) $(CPPFLAGS) $(TARGET_ARCH) -c $< -o $@
>  
> -LIBKVM_OBJS = $(LIBKVM_C_OBJ) $(LIBKVM_S_OBJ)
> -$(OUTPUT)/libkvm.a: $(LIBKVM_OBJS)
> -	$(AR) crs $@ $^
> -
>  x := $(shell mkdir -p $(sort $(dir $(TEST_GEN_PROGS))))
> -all: $(STATIC_LIBS)
> -$(TEST_GEN_PROGS): $(STATIC_LIBS)
> +all: $(LIBKVM_OBJS)

Can this line be dropped alongside?  Default targets should have already
been set in ../lib.mk anyway iiuc, and they all depend on the objs.

> +$(TEST_GEN_PROGS): $(LIBKVM_OBJS)

Never know such a difference, but it does seem true to happen..  Good to
learn this.

Reviewed-by: Peter Xu <peterx@redhat.com>

Thanks,
diff mbox series

Patch

diff --git a/tools/testing/selftests/kvm/Makefile b/tools/testing/selftests/kvm/Makefile
index af582d168621..c1eb6acb30de 100644
--- a/tools/testing/selftests/kvm/Makefile
+++ b/tools/testing/selftests/kvm/Makefile
@@ -172,12 +172,13 @@  LDFLAGS += -pthread $(no-pie-option) $(pgste-option)
 # $(TEST_GEN_PROGS) starts with $(OUTPUT)/
 include ../lib.mk
 
-STATIC_LIBS := $(OUTPUT)/libkvm.a
 LIBKVM_C := $(filter %.c,$(LIBKVM))
 LIBKVM_S := $(filter %.S,$(LIBKVM))
 LIBKVM_C_OBJ := $(patsubst %.c, $(OUTPUT)/%.o, $(LIBKVM_C))
 LIBKVM_S_OBJ := $(patsubst %.S, $(OUTPUT)/%.o, $(LIBKVM_S))
-EXTRA_CLEAN += $(LIBKVM_C_OBJ) $(LIBKVM_S_OBJ) $(STATIC_LIBS) cscope.*
+LIBKVM_OBJS = $(LIBKVM_C_OBJ) $(LIBKVM_S_OBJ)
+
+EXTRA_CLEAN += $(LIBKVM_OBJS) cscope.*
 
 x := $(shell mkdir -p $(sort $(dir $(LIBKVM_C_OBJ) $(LIBKVM_S_OBJ))))
 $(LIBKVM_C_OBJ): $(OUTPUT)/%.o: %.c
@@ -186,13 +187,9 @@  $(LIBKVM_C_OBJ): $(OUTPUT)/%.o: %.c
 $(LIBKVM_S_OBJ): $(OUTPUT)/%.o: %.S
 	$(CC) $(CFLAGS) $(CPPFLAGS) $(TARGET_ARCH) -c $< -o $@
 
-LIBKVM_OBJS = $(LIBKVM_C_OBJ) $(LIBKVM_S_OBJ)
-$(OUTPUT)/libkvm.a: $(LIBKVM_OBJS)
-	$(AR) crs $@ $^
-
 x := $(shell mkdir -p $(sort $(dir $(TEST_GEN_PROGS))))
-all: $(STATIC_LIBS)
-$(TEST_GEN_PROGS): $(STATIC_LIBS)
+all: $(LIBKVM_OBJS)
+$(TEST_GEN_PROGS): $(LIBKVM_OBJS)
 
 cscope: include_paths = $(LINUX_TOOL_INCLUDE) $(LINUX_HDR_PATH) include lib ..
 cscope: