[PATCH] kbuild: Parameterize kallsyms generation and correct reporting

[PATCH] kbuild: Parameterize kallsyms generation and correct reporting

[Kees Cook]

When kallsyms generation happens, temporary vmlinux outputs are linked
but the quiet make output doesn't report it, giving the impression that
the prior command is taking longer than expected.

Instead, report the KSYM step before the temporary linking. While at it,
this consolidates the repeated "kallsyms generation step" into a single
function and removes the existing copy/pasting.

Signed-off-by: Kees Cook <keescook@chromium.org>
---
 scripts/link-vmlinux.sh | 39 ++++++++++++++++++++-------------------
 1 file changed, 20 insertions(+), 19 deletions(-)

diff --git a/scripts/link-vmlinux.sh b/scripts/link-vmlinux.sh
index 96b6c0233a61..ed52be015523 100755
--- a/scripts/link-vmlinux.sh
+++ b/scripts/link-vmlinux.sh
@@ -170,7 +170,6 @@ gen_btf()
 # Create ${2} .o file with all symbols from the ${1} object file
 kallsyms()
 {
-	info KSYM ${2}
 	local kallsymopt;
 
 	if [ -n "${CONFIG_KALLSYMS_ALL}" ]; then
@@ -277,7 +276,22 @@ info MODINFO modules.builtin.modinfo
 ${OBJCOPY} -j .modinfo -O binary vmlinux.o modules.builtin.modinfo
 
 kallsymso=""
+kallsymso_previous=""
 kallsyms_vmlinux=""
+
+# Perform one step in kallsyms generation, including temporary linking of
+# vmlinux.
+kallsyms_step()
+{
+	kallsymso_previous=${kallsymso}
+	kallsymso=.tmp_kallsyms${1}.o
+	kallsyms_vmlinux=.tmp_vmlinux${1}
+
+	info KSYM ${kallsymso}
+	vmlinux_link "${kallsymso_previous}" ${kallsyms_vmlinux}
+	kallsyms ${kallsyms_vmlinux} ${kallsymso}
+}
+
 if [ -n "${CONFIG_KALLSYMS}" ]; then
 
 	# kallsyms support
@@ -303,28 +317,15 @@ if [ -n "${CONFIG_KALLSYMS}" ]; then
 	# a)  Verify that the System.map from vmlinux matches the map from
 	#     ${kallsymso}.
 
-	kallsymso=.tmp_kallsyms2.o
-	kallsyms_vmlinux=.tmp_vmlinux2
-
-	# step 1
-	vmlinux_link "" .tmp_vmlinux1
-	kallsyms .tmp_vmlinux1 .tmp_kallsyms1.o
-
-	# step 2
-	vmlinux_link .tmp_kallsyms1.o .tmp_vmlinux2
-	kallsyms .tmp_vmlinux2 .tmp_kallsyms2.o
+	kallsyms_step 1
+	kallsyms_step 2
 
 	# step 3
-	size1=$(${CONFIG_SHELL} "${srctree}/scripts/file-size.sh" .tmp_kallsyms1.o)
-	size2=$(${CONFIG_SHELL} "${srctree}/scripts/file-size.sh" .tmp_kallsyms2.o)
+	size1=$(${CONFIG_SHELL} "${srctree}/scripts/file-size.sh" ${kallsymso_previous})
+	size2=$(${CONFIG_SHELL} "${srctree}/scripts/file-size.sh" ${kallsymso})
 
 	if [ $size1 -ne $size2 ] || [ -n "${KALLSYMS_EXTRA_PASS}" ]; then
-		kallsymso=.tmp_kallsyms3.o
-		kallsyms_vmlinux=.tmp_vmlinux3
-
-		vmlinux_link .tmp_kallsyms2.o .tmp_vmlinux3
-
-		kallsyms .tmp_vmlinux3 .tmp_kallsyms3.o
+		kallsyms_step 3
 	fi
 fi
 
-- 
2.17.1


-- 
Kees Cook

Re: [PATCH] kbuild: Parameterize kallsyms generation and correct reporting

[Masahiro Yamada]

On Tue, Aug 13, 2019 at 6:49 AM Kees Cook <keescook@chromium.org> wrote:
>
> When kallsyms generation happens, temporary vmlinux outputs are linked
> but the quiet make output doesn't report it, giving the impression that
> the prior command is taking longer than expected.
>
> Instead, report the KSYM step before the temporary linking. While at it,
> this consolidates the repeated "kallsyms generation step" into a single
> function and removes the existing copy/pasting.
>
> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---

Hmm, I did not notice this.

How about showing the link stage explicitly?
(Is it too verbose?)

  MODINFO modules.builtin.modinfo
  LD      .tmp_vmlinux1
  KSYMS   .tmp_kallsyms1.o
  LD      .tmp_vmlinux2
  KSYMS   .tmp_kallsyms2.o
  LD      vmlinux
  SORTEX  vmlinux


If this verbosity is OK,
you can move 'info LD  ${2}' into vmlinux_link()




Anyway, I like the clean-ups in this patch.

This is just my personal preference, but
may I ask two cosmetic changes?

[1] Could you move kallsyms_step()
    between  kallsyms() and mksysmap() ?
    I want to collect function definitions
    to the top of the script.

[2] Could you shorten 'kallsymso_previous'
    to 'kallsymso_prev' ?




-- 
Best Regards
Masahiro Yamada

Re: [PATCH] kbuild: Parameterize kallsyms generation and correct reporting

[Kees Cook]

On Wed, Aug 14, 2019 at 12:00:05AM +0900, Masahiro Yamada wrote:
> On Tue, Aug 13, 2019 at 6:49 AM Kees Cook <keescook@chromium.org> wrote:
> >
> > When kallsyms generation happens, temporary vmlinux outputs are linked
> > but the quiet make output doesn't report it, giving the impression that
> > the prior command is taking longer than expected.
> >
> > Instead, report the KSYM step before the temporary linking. While at it,
> > this consolidates the repeated "kallsyms generation step" into a single
> > function and removes the existing copy/pasting.
> >
> > Signed-off-by: Kees Cook <keescook@chromium.org>
> > ---
> 
> Hmm, I did not notice this.

He either until I was getting link errors from MODINFO :)

> How about showing the link stage explicitly?
> (Is it too verbose?)
> 
>   MODINFO modules.builtin.modinfo
>   LD      .tmp_vmlinux1
>   KSYMS   .tmp_kallsyms1.o
>   LD      .tmp_vmlinux2
>   KSYMS   .tmp_kallsyms2.o
>   LD      vmlinux
>   SORTEX  vmlinux

I'm fine with this -- it's probably the right thing to do since getting
vmlinux link errors fro KSYMS probably doesn't make sense either. ;)

> If this verbosity is OK,
> you can move 'info LD  ${2}' into vmlinux_link()

Done.

> Anyway, I like the clean-ups in this patch.
> 
> This is just my personal preference, but
> may I ask two cosmetic changes?
> 
> [1] Could you move kallsyms_step()
>     between  kallsyms() and mksysmap() ?
>     I want to collect function definitions
>     to the top of the script.
> 
> [2] Could you shorten 'kallsymso_previous'
>     to 'kallsymso_prev' ?

Sounds good. v2 sent.

-- 
Kees Cook