diff mbox series

[2/3] kbuild: remove PROVIDE() for kallsyms symbols

Message ID 20240522114755.318238-3-masahiroy@kernel.org (mailing list archive)
State Not Applicable
Headers show
Series kbuild: remove PROVIDE() and refactor vmlinux_link steps | expand

Checks

Context Check Description
netdev/tree_selection success Not a local patch

Commit Message

Masahiro Yamada May 22, 2024, 11:47 a.m. UTC
This reimplements commit 951bcae6c5a0 ("kallsyms: Avoid weak references
for kallsyms symbols").

I am not a big fan of PROVIDE() because it always satisfies the linker
even in situations that should result in a link error. In other words,
it can potentially shift a compile-time error into a run-time error.

Duplicating kallsyms_* in vmlinux.lds.h also reduces maintainability.

As an alternative solution, this commit prepends one more kallsyms step.

    KSYMS   .tmp_vmlinux.kallsyms0.S          # added
    AS      .tmp_vmlinux.kallsyms0.o          # added
    LD      .tmp_vmlinux.btf
    BTF     .btf.vmlinux.bin.o
    LD      .tmp_vmlinux.kallsyms1
    NM      .tmp_vmlinux.kallsyms1.syms
    KSYMS   .tmp_vmlinux.kallsyms1.S
    AS      .tmp_vmlinux.kallsyms1.o
    LD      .tmp_vmlinux.kallsyms2
    NM      .tmp_vmlinux.kallsyms2.syms
    KSYMS   .tmp_vmlinux.kallsyms2.S
    AS      .tmp_vmlinux.kallsyms2.o
    LD      vmlinux

Step 0 takes /dev/null as input, and generates .tmp_vmlinux.kallsyms0.o,
which has a valid kallsyms format with the empty symbol list, and can be
linked to vmlinux. Since it is really small, the added compile-time cost
is negligible.

Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
---

 include/asm-generic/vmlinux.lds.h | 19 -------------------
 kernel/kallsyms_internal.h        |  5 -----
 scripts/kallsyms.c                |  6 ------
 scripts/link-vmlinux.sh           | 10 ++++++++--
 4 files changed, 8 insertions(+), 32 deletions(-)

Comments

Ard Biesheuvel May 23, 2024, 9:31 a.m. UTC | #1
On Wed, 22 May 2024 at 13:48, Masahiro Yamada <masahiroy@kernel.org> wrote:
>
> This reimplements commit 951bcae6c5a0 ("kallsyms: Avoid weak references
> for kallsyms symbols").
>
> I am not a big fan of PROVIDE() because it always satisfies the linker
> even in situations that should result in a link error. In other words,
> it can potentially shift a compile-time error into a run-time error.
>

I don't disagree. However, I did realize that, in this particular
case, we could at least make the preliminary symbol definitions
conditional on CONFIG_KALLSYMS rather than always providing them.

This approach is also fine with me, though.


> Duplicating kallsyms_* in vmlinux.lds.h also reduces maintainability.
>
> As an alternative solution, this commit prepends one more kallsyms step.
>
>     KSYMS   .tmp_vmlinux.kallsyms0.S          # added
>     AS      .tmp_vmlinux.kallsyms0.o          # added
>     LD      .tmp_vmlinux.btf
>     BTF     .btf.vmlinux.bin.o
>     LD      .tmp_vmlinux.kallsyms1
>     NM      .tmp_vmlinux.kallsyms1.syms
>     KSYMS   .tmp_vmlinux.kallsyms1.S
>     AS      .tmp_vmlinux.kallsyms1.o
>     LD      .tmp_vmlinux.kallsyms2
>     NM      .tmp_vmlinux.kallsyms2.syms
>     KSYMS   .tmp_vmlinux.kallsyms2.S
>     AS      .tmp_vmlinux.kallsyms2.o
>     LD      vmlinux
>
> Step 0 takes /dev/null as input, and generates .tmp_vmlinux.kallsyms0.o,
> which has a valid kallsyms format with the empty symbol list, and can be
> linked to vmlinux. Since it is really small, the added compile-time cost
> is negligible.
>

OK, so the number of linker invocations is the same, right? The
difference is that the kallsyms symbol references are satisfied by a
dummy object?

That seems reasonable to me.

For the series,

Acked-by: Ard Biesheuvel <ardb@kernel.org>
Masahiro Yamada May 25, 2024, 4:43 p.m. UTC | #2
On Thu, May 23, 2024 at 6:32 PM Ard Biesheuvel <ardb@kernel.org> wrote:
>
> On Wed, 22 May 2024 at 13:48, Masahiro Yamada <masahiroy@kernel.org> wrote:
> >
> > This reimplements commit 951bcae6c5a0 ("kallsyms: Avoid weak references
> > for kallsyms symbols").
> >
> > I am not a big fan of PROVIDE() because it always satisfies the linker
> > even in situations that should result in a link error. In other words,
> > it can potentially shift a compile-time error into a run-time error.
> >
>
> I don't disagree. However, I did realize that, in this particular
> case, we could at least make the preliminary symbol definitions
> conditional on CONFIG_KALLSYMS rather than always providing them.


Fair enough. I am fine with dropping this statement.




>
> This approach is also fine with me, though.
>
>
> > Duplicating kallsyms_* in vmlinux.lds.h also reduces maintainability.
> >
> > As an alternative solution, this commit prepends one more kallsyms step.
> >
> >     KSYMS   .tmp_vmlinux.kallsyms0.S          # added
> >     AS      .tmp_vmlinux.kallsyms0.o          # added
> >     LD      .tmp_vmlinux.btf
> >     BTF     .btf.vmlinux.bin.o
> >     LD      .tmp_vmlinux.kallsyms1
> >     NM      .tmp_vmlinux.kallsyms1.syms
> >     KSYMS   .tmp_vmlinux.kallsyms1.S
> >     AS      .tmp_vmlinux.kallsyms1.o
> >     LD      .tmp_vmlinux.kallsyms2
> >     NM      .tmp_vmlinux.kallsyms2.syms
> >     KSYMS   .tmp_vmlinux.kallsyms2.S
> >     AS      .tmp_vmlinux.kallsyms2.o
> >     LD      vmlinux
> >
> > Step 0 takes /dev/null as input, and generates .tmp_vmlinux.kallsyms0.o,
> > which has a valid kallsyms format with the empty symbol list, and can be
> > linked to vmlinux. Since it is really small, the added compile-time cost
> > is negligible.
> >
>
> OK, so the number of linker invocations is the same, right? The
> difference is that the kallsyms symbol references are satisfied by a
> dummy object?


Correct.

In 3/3, I even reduce the number of link steps
when both CONFIG_DEBUG_INFO_BTF and CONFIG_KALLSYMS are enabled.





>
> That seems reasonable to me.
>
> For the series,
>
> Acked-by: Ard Biesheuvel <ardb@kernel.org>
>
diff mbox series

Patch

diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm-generic/vmlinux.lds.h
index 5703526d6ebf..62b4cb0462e6 100644
--- a/include/asm-generic/vmlinux.lds.h
+++ b/include/asm-generic/vmlinux.lds.h
@@ -451,30 +451,11 @@ 
 #endif
 #endif
 
-/*
- * Some symbol definitions will not exist yet during the first pass of the
- * link, but are guaranteed to exist in the final link. Provide preliminary
- * definitions that will be superseded in the final link to avoid having to
- * rely on weak external linkage, which requires a GOT when used in position
- * independent code.
- */
-#define PRELIMINARY_SYMBOL_DEFINITIONS					\
-	PROVIDE(kallsyms_addresses = .);				\
-	PROVIDE(kallsyms_offsets = .);					\
-	PROVIDE(kallsyms_names = .);					\
-	PROVIDE(kallsyms_num_syms = .);					\
-	PROVIDE(kallsyms_relative_base = .);				\
-	PROVIDE(kallsyms_token_table = .);				\
-	PROVIDE(kallsyms_token_index = .);				\
-	PROVIDE(kallsyms_markers = .);					\
-	PROVIDE(kallsyms_seqs_of_names = .);
-
 /*
  * Read only Data
  */
 #define RO_DATA(align)							\
 	. = ALIGN((align));						\
-	PRELIMINARY_SYMBOL_DEFINITIONS					\
 	.rodata           : AT(ADDR(.rodata) - LOAD_OFFSET) {		\
 		__start_rodata = .;					\
 		*(.rodata) *(.rodata.*)					\
diff --git a/kernel/kallsyms_internal.h b/kernel/kallsyms_internal.h
index 85480274fc8f..925f2ab22639 100644
--- a/kernel/kallsyms_internal.h
+++ b/kernel/kallsyms_internal.h
@@ -4,11 +4,6 @@ 
 
 #include <linux/types.h>
 
-/*
- * These will be re-linked against their real values during the second link
- * stage. Preliminary values must be provided in the linker script using the
- * PROVIDE() directive so that the first link stage can complete successfully.
- */
 extern const unsigned long kallsyms_addresses[];
 extern const int kallsyms_offsets[];
 extern const u8 kallsyms_names[];
diff --git a/scripts/kallsyms.c b/scripts/kallsyms.c
index 47978efe4797..fa53b5eef553 100644
--- a/scripts/kallsyms.c
+++ b/scripts/kallsyms.c
@@ -259,12 +259,6 @@  static void shrink_table(void)
 		}
 	}
 	table_cnt = pos;
-
-	/* When valid symbol is not registered, exit to error */
-	if (!table_cnt) {
-		fprintf(stderr, "No valid symbol.\n");
-		exit(1);
-	}
 }
 
 static void read_map(const char *in)
diff --git a/scripts/link-vmlinux.sh b/scripts/link-vmlinux.sh
index b16967d33f1c..fe7db9a265ca 100755
--- a/scripts/link-vmlinux.sh
+++ b/scripts/link-vmlinux.sh
@@ -225,6 +225,11 @@  ${MAKE} -f "${srctree}/scripts/Makefile.build" obj=init init/version-timestamp.o
 kallsymso=
 btf_vmlinux_bin_o=
 
+if is_enabled CONFIG_KALLSYMS; then
+	# kallsyms step 0
+	kallsyms /dev/null .tmp_vmlinux.kallsyms0
+fi
+
 if is_enabled CONFIG_DEBUG_INFO_BTF; then
 	if ! gen_btf .tmp_vmlinux.btf .btf.vmlinux.bin.o ; then
 		echo >&2 "Failed to generate BTF for vmlinux"
@@ -237,9 +242,10 @@  if is_enabled CONFIG_KALLSYMS; then
 
 	# kallsyms support
 	# Generate section listing all symbols and add it into vmlinux
-	# It's a three step process:
+	# It's a four step process:
+	# 0)  Generate a dummy __kallsyms with empty symbol list.
 	# 1)  Link .tmp_vmlinux.kallsyms1 so it has all symbols and sections,
-	#     but __kallsyms is empty.
+	#     with a dummy __kallsyms.
 	#     Running kallsyms on that gives us .tmp_kallsyms1.o with
 	#     the right size
 	# 2)  Link .tmp_vmlinux.kallsyms2 so it now has a __kallsyms section of