| Message ID | 201908121448.4D023D7@keescook (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | kbuild: Parameterize kallsyms generation and correct reporting | expand |
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' ?
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.
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
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(-)