linux-arch.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/3] kbuild: remove PROVIDE() and refactor vmlinux_link steps
@ 2024-05-22 11:47 Masahiro Yamada
  2024-05-22 11:47 ` [PATCH 2/3] kbuild: remove PROVIDE() for kallsyms symbols Masahiro Yamada
  2024-05-28  9:42 ` [PATCH 0/3] kbuild: remove PROVIDE() and refactor vmlinux_link steps Jiri Olsa
  0 siblings, 2 replies; 5+ messages in thread
From: Masahiro Yamada @ 2024-05-22 11:47 UTC (permalink / raw)
  To: linux-kbuild
  Cc: linux-kernel, bpf, Ard Biesheuvel, Masahiro Yamada,
	Arnd Bergmann, Nathan Chancellor, Nicolas Schier, linux-arch


 - Remove PROVIDE() in the linker script
 - Merge temporary vmlinux link steps for BTF and kallsyms



Masahiro Yamada (3):
  kbuild: refactor variables in scripts/link-vmlinux.sh
  kbuild: remove PROVIDE() for kallsyms symbols
  kbuild: merge temp vmlinux for CONFIG_DEBUG_INFO_BTF and
    CONFIG_KALLSYMS

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

-- 
2.40.1


^ permalink raw reply	[flat|nested] 5+ messages in thread

* [PATCH 2/3] kbuild: remove PROVIDE() for kallsyms symbols
  2024-05-22 11:47 [PATCH 0/3] kbuild: remove PROVIDE() and refactor vmlinux_link steps Masahiro Yamada
@ 2024-05-22 11:47 ` Masahiro Yamada
  2024-05-23  9:31   ` Ard Biesheuvel
  2024-05-28  9:42 ` [PATCH 0/3] kbuild: remove PROVIDE() and refactor vmlinux_link steps Jiri Olsa
  1 sibling, 1 reply; 5+ messages in thread
From: Masahiro Yamada @ 2024-05-22 11:47 UTC (permalink / raw)
  To: linux-kbuild
  Cc: linux-kernel, bpf, Ard Biesheuvel, Masahiro Yamada,
	Arnd Bergmann, Nathan Chancellor, Nicolas Schier, linux-arch

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

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


^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [PATCH 2/3] kbuild: remove PROVIDE() for kallsyms symbols
  2024-05-22 11:47 ` [PATCH 2/3] kbuild: remove PROVIDE() for kallsyms symbols Masahiro Yamada
@ 2024-05-23  9:31   ` Ard Biesheuvel
  2024-05-25 16:43     ` Masahiro Yamada
  0 siblings, 1 reply; 5+ messages in thread
From: Ard Biesheuvel @ 2024-05-23  9:31 UTC (permalink / raw)
  To: Masahiro Yamada
  Cc: linux-kbuild, linux-kernel, bpf, Arnd Bergmann,
	Nathan Chancellor, Nicolas Schier, linux-arch

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>

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH 2/3] kbuild: remove PROVIDE() for kallsyms symbols
  2024-05-23  9:31   ` Ard Biesheuvel
@ 2024-05-25 16:43     ` Masahiro Yamada
  0 siblings, 0 replies; 5+ messages in thread
From: Masahiro Yamada @ 2024-05-25 16:43 UTC (permalink / raw)
  To: Ard Biesheuvel
  Cc: linux-kbuild, linux-kernel, bpf, Arnd Bergmann,
	Nathan Chancellor, Nicolas Schier, linux-arch

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


-- 
Best Regards
Masahiro Yamada

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH 0/3] kbuild: remove PROVIDE() and refactor vmlinux_link steps
  2024-05-22 11:47 [PATCH 0/3] kbuild: remove PROVIDE() and refactor vmlinux_link steps Masahiro Yamada
  2024-05-22 11:47 ` [PATCH 2/3] kbuild: remove PROVIDE() for kallsyms symbols Masahiro Yamada
@ 2024-05-28  9:42 ` Jiri Olsa
  1 sibling, 0 replies; 5+ messages in thread
From: Jiri Olsa @ 2024-05-28  9:42 UTC (permalink / raw)
  To: Masahiro Yamada
  Cc: linux-kbuild, linux-kernel, bpf, Ard Biesheuvel, Arnd Bergmann,
	Nathan Chancellor, Nicolas Schier, linux-arch

On Wed, May 22, 2024 at 08:47:52PM +0900, Masahiro Yamada wrote:
> 
>  - Remove PROVIDE() in the linker script
>  - Merge temporary vmlinux link steps for BTF and kallsyms
> 
> 
> 
> Masahiro Yamada (3):
>   kbuild: refactor variables in scripts/link-vmlinux.sh
>   kbuild: remove PROVIDE() for kallsyms symbols
>   kbuild: merge temp vmlinux for CONFIG_DEBUG_INFO_BTF and
>     CONFIG_KALLSYMS

lgtm, fyi I ran bpf CI on top of this change and passed

https://github.com/kernel-patches/bpf/pull/7104

jirka

> 
>  include/asm-generic/vmlinux.lds.h | 19 -------
>  kernel/kallsyms_internal.h        |  5 --
>  scripts/kallsyms.c                |  6 ---
>  scripts/link-vmlinux.sh           | 87 ++++++++++++++++---------------
>  4 files changed, 45 insertions(+), 72 deletions(-)
> 
> -- 
> 2.40.1
> 
> 

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2024-05-28  9:42 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-05-22 11:47 [PATCH 0/3] kbuild: remove PROVIDE() and refactor vmlinux_link steps Masahiro Yamada
2024-05-22 11:47 ` [PATCH 2/3] kbuild: remove PROVIDE() for kallsyms symbols Masahiro Yamada
2024-05-23  9:31   ` Ard Biesheuvel
2024-05-25 16:43     ` Masahiro Yamada
2024-05-28  9:42 ` [PATCH 0/3] kbuild: remove PROVIDE() and refactor vmlinux_link steps Jiri Olsa

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).