Re: [PATCH 4/8] kbuild: simplify dependency generation for CONFIG_TRIM_UNUSED_KSYMS

[Nicolas Pitre]

On Fri, 16 Nov 2018, Masahiro Yamada wrote:

> On Fri, Nov 16, 2018 at 2:13 PM Nicolas Pitre <nicolas.pitre@xxxxxxxxxx> wrote:
> >
> > On Thu, 15 Nov 2018, Masahiro Yamada wrote:
> >
> > > My main motivation of this commit is to clean up scripts/Kbuild.include
> > > and scripts/Makefile.build.
> > >
> > > Currently, CONFIG_TRIM_UNUSED_KSYMS works with a tricky gimmick;
> > > possibly exported symbols are detected by letting $(CPP) replace
> > > EXPORT_SYMBOL* with a special string '=== __KSYM_*===', which is
> > > post-processed by sed, and passed to fixdep. The extra preprocessing
> > > is costly, and hacking cmd_and_fixdep is ugly.
> > >
> > > I came up with a new way to find exported symbols; insert a dummy
> > > symbol __ksym_marker_* to each potentially exported symbol. Those
> > > dummy symbols are picked up by $(NM), post-processed by sed, then
> > > appended to .*.cmd files. I collected the post-process part to a
> > > new shell script scripts/gen_ksymdeps.sh for readability. The dummy
> > > symbols are put into the .discard.* section so that the linker
> > > script rips them off the final vmlinux or modules.
> >
> > Brilliant!  I really like it.
> >
> > Minor comments below.
> >
> > > diff --git a/include/asm-generic/export.h b/include/asm-generic/export.h
> > > index 4d73e6e..294d6ae 100644
> > > --- a/include/asm-generic/export.h
> > > +++ b/include/asm-generic/export.h
> > > @@ -59,16 +59,19 @@ __kcrctab_\name:
> > >  .endm
> > >  #undef __put
> > >
> > > -#if defined(__KSYM_DEPS__)
> > > -
> > > -#define __EXPORT_SYMBOL(sym, val, sec)       === __KSYM_##sym ===
> > > -
> > > -#elif defined(CONFIG_TRIM_UNUSED_KSYMS)
> > > +#if defined(CONFIG_TRIM_UNUSED_KSYMS)
> > >
> > >  #include <linux/kconfig.h>
> > >  #include <generated/autoksyms.h>
> > >
> > > +.macro __ksym_marker sym
> > > +     .section ".discard.ksym","a"
> > > +__ksym_marker_\sym:
> > > +      .previous
> >
> > Does this work as intended? I have vague memories about having problems
> > with sections being discarded when they don't allocate any space.
> 
> What I can tell is, this patch produces the same size kernel
> (after dropping debug info by 'strip' command).
> 
> 
> > > +.endm
> > > +
> > >  #define __EXPORT_SYMBOL(sym, val, sec)                               \
> > > +     __ksym_marker sym;                                      \
> > >       __cond_export_sym(sym, val, sec, __is_defined(__KSYM_##sym))
> > >  #define __cond_export_sym(sym, val, sec, conf)                       \
> > >       ___cond_export_sym(sym, val, sec, conf)
> > > diff --git a/include/linux/export.h b/include/linux/export.h
> > > index ce764a5..0413a3d 100644
> > > --- a/include/linux/export.h
> > > +++ b/include/linux/export.h
> > > @@ -92,22 +92,22 @@ struct kernel_symbol {
> > >   */
> > >  #define __EXPORT_SYMBOL(sym, sec)
> > >
> > > -#elif defined(__KSYM_DEPS__)
> > > +#elif defined(CONFIG_TRIM_UNUSED_KSYMS)
> > > +
> > > +#include <generated/autoksyms.h>
> > >
> > >  /*
> > >   * For fine grained build dependencies, we want to tell the build system
> > >   * about each possible exported symbol even if they're not actually exported.
> > > - * We use a string pattern that is unlikely to be valid code that the build
> > > - * system filters out from the preprocessor output (see ksym_dep_filter
> > > - * in scripts/Kbuild.include).
> > > + * We use a symbol pattern __ksym_marker_<symbol> that the build system filters
> > > + * from the $(NM) output (see scripts/gen_ksymdep.sh). These symbols are
> > > + * discarded in the final link stage.
> > >   */
> > > -#define __EXPORT_SYMBOL(sym, sec)    === __KSYM_##sym ===
> > > -
> > > -#elif defined(CONFIG_TRIM_UNUSED_KSYMS)
> > > -
> > > -#include <generated/autoksyms.h>
> > > +#define __ksym_marker(sym)   \
> > > +     static int __ksym_marker_##sym[0] __section(".discard.ksym") __used
> >
> > Even if this is discarded during the final link, maybe this could save
> > a tiny amount of disk space by using a char instead?
> 
> 
> I am afraid you missed '[0]' after the symbol name.
> This is actually zero-length array.

Ah, indeed.

> No memory allocated for this dummy section.

Right, and that makes it identical to the assembly case.

> As far as I tested, this is working.

Yes, it does.

The problem I was alluding to has to do with symbols generated by the 
linker directly which for some reason behaves differently in that case.

> > > diff --git a/scripts/Makefile.build b/scripts/Makefile.build
> > > index 7f3ca6e..e5ba9b1 100644
> > > --- a/scripts/Makefile.build
> > > +++ b/scripts/Makefile.build
> > > @@ -254,9 +254,18 @@ objtool_dep = $(objtool_obj)                                     \
> > >             $(wildcard include/config/orc/unwinder.h          \
> > >                        include/config/stack/validation.h)
> > >
> > > +ifdef CONFIG_TRIM_UNUSED_KSYMS
> > > +cmd_gen_ksymdeps = \
> > > +     $(CONFIG_SHELL) $(srctree)/scripts/gen_ksymdeps.sh $@ > $(dot-target).tmp; \
> > > +     cat $(dot-target).tmp >> $(dot-target).cmd; \
> > > +     rm -f $(dot-target).tmp;
> >
> > Why don't you append to $(dot-target).cmd directly?
> 
> 
> If scripts/gen_ksymdeps.sh fails for some reasons,
> it will error out immediately thanks to 'set -e' flag.
> 
> Appending incomplete portion might end up with a corrupted .*.cmd file.
> 
> Probably, that would not happen, but I just wanted to ensure it.

Well, strictly speaking, if scripts/gen_ksymdeps.sh fails and its output 
isn't appended at all to the .*.cmd file, then that .*.cmd file is 
already corrupted as it is missing necessary dependencies. Would be 
better to delete the .*.cmd file entirely in that case.


Nicolas