Message-ID: <CAK7LNARsJgcEsTdeKxe9MtBeCPp6dCNrOArJRBievo954p33LQ@mail.gmail.com>
Date: Sun, 18 Aug 2024 15:19:36 +0900
From: Masahiro Yamada <masahiroy@...nel.org>
To: Kris Van Hees <kris.van.hees@...cle.com>
Cc: linux-kernel@...r.kernel.org, linux-kbuild@...r.kernel.org, 
	linux-modules@...r.kernel.org, linux-trace-kernel@...r.kernel.org, 
	Nick Alcock <nick.alcock@...cle.com>, Alan Maguire <alan.maguire@...cle.com>, 
	Steven Rostedt <rostedt@...dmis.org>, Luis Chamberlain <mcgrof@...nel.org>, 
	Masami Hiramatsu <mhiramat@...nel.org>, Nick Desaulniers <ndesaulniers@...gle.com>, 
	Jiri Olsa <olsajiri@...il.com>, Elena Zannoni <elena.zannoni@...cle.com>
Subject: Re: [PATCH v6 2/4] kbuild, kconfig: generate offset range data for
 builtin modules

On Fri, Aug 16, 2024 at 12:04 AM Kris Van Hees <kris.van.hees@...cle.com> wrote:


The subject should be:
"kbuild: generate offset range data for builtin modules"


(Drop ", kconfig")




>
> Create file module.builtin.ranges that can be used to find where
> built-in modules are located by their addresses. This will be useful for
> tracing tools to find what functions are for various built-in modules.
>
> The offset range data for builtin modules is generated using:
>  - modules.builtin: associates object files with module names
>  - vmlinux.map: provides load order of sections and offset of first member
>     per section
>  - vmlinux.o.map: provides offset of object file content per section
>  - .*.cmd: build cmd file with KBUILD_MODFILE and KBUILD_MODNAME


I do not see "KBUILD_MODNAME" in the code.
It only checks "KUILD_MODFILE".





>
> The generated data will look like:
>
> .text 00000000-00000000 = _text
> .text 0000baf0-0000cb10 amd_uncore
> .text 0009bd10-0009c8e0 iosf_mbi
> ...
> .text 008e6660-008e9630 snd_soc_wcd_mbhc
> .text 008e9630-008ea610 snd_soc_wcd9335 snd_soc_wcd934x snd_soc_wcd938x



It is good to note that multiple module names appear
in one line, but the instance (snd_soc_wcd933*) no longer
occurs since 11b0b802f8e38d48ca74d520028add81263f003e.


I recommend to replace the output snippet with:


.text 00b9f080-00ba011a intel_skl_int3472_discrete
.text 00ba0120-00ba03c0 intel_skl_int3472_discrete intel_skl_int3472_tps68470
.text 00ba03c0-00ba08d6 intel_skl_int3472_tps68470


This still happens when CONFIG_INTEL_SKL_INT3472=y.





> .text 008ea610-008ea780 snd_soc_wcd9335
> ...
> .data 00000000-00000000 = _sdata
> .data 0000f020-0000f680 amd_uncore
>
> For each ELF section, it lists the offset of the first symbol.  This can
> be used to determine the base address of the section at runtime.
>
> Next, it lists (in strict ascending order) offset ranges in that section
> that cover the symbols of one or more builtin modules.  Multiple ranges
> can apply to a single module, and ranges can be shared between modules.
>
> The CONFIG_BUILTIN_MODULE_RANGES option controls whether offset range data
> is generated for kernel modules that are built into the kernel image.
>
> How it works:
>
>   1. The modules.builtin file is parsed to obtain a list of built-in
>      module names and their associated object names (the .ko file that
>      the module would be in if it were a loadable module, hereafter
>      referred to as <kmodfile>).  This object name can be used to
>      identify objects in the kernel compile because any C or assembler
>      code that ends up into a built-in module will have the option
>      -DKBUILD_MODFILE=<kmodfile> present in its build command, and those
>      can be found in the .<obj>.cmd file in the kernel build tree.
>
>      If an object is part of multiple modules, they will all be listed
>      in the KBUILD_MODFILE option argument.
>
>      This allows us to conclusively determine whether an object in the
>      kernel build belong to any modules, and which.
>
>  2. The vmlinux.map is parsed next to determine the base address of each
>     top level section so that all addresses into the section can be
>     turned into offsets.  This makes it possible to handle sections
>     getting loaded at different addresses at system boot.
>
>     We also determine an 'anchor' symbol at the beginning of each
>     section to make it possible to calculate the true base address of
>     a section at runtime (i.e. symbol address - symbol offset).
>
>     We collect start addresses of sections that are included in the top
>     level section.  This is used when vmlinux is linked using vmlinux.o,
>     because in that case, we need to look at the vmlinux.o linker map to
>     know what object a symbol is found in.
>
>     And finally, we process each symbol that is listed in vmlinux.map
>     (or vmlinux.o.map) based on the following structure:
>
>     vmlinux linked from vmlinux.a:
>
>       vmlinux.map:
>         <top level section>
>           <included section>  -- might be same as top level section)
>             <object>          -- built-in association known
>               <symbol>        -- belongs to module(s) object belongs to
>               ...
>
>     vmlinux linked from vmlinux.o:
>
>       vmlinux.map:
>         <top level section>
>           <included section>  -- might be same as top level section)
>             vmlinux.o         -- need to use vmlinux.o.map
>               <symbol>        -- ignored
>               ...
>
>       vmlinux.o.map:
>         <section>
>             <object>          -- built-in association known
>               <symbol>        -- belongs to module(s) object belongs to
>               ...
>
>  3. As sections, objects, and symbols are processed, offset ranges are
>     constructed in a striaght-forward way:
>
>       - If the symbol belongs to one or more built-in modules:
>           - If we were working on the same module(s), extend the range
>             to include this object
>           - If we were working on another module(s), close that range,
>             and start the new one
>       - If the symbol does not belong to any built-in modules:
>           - If we were working on a module(s) range, close that range
>
> Signed-off-by: Kris Van Hees <kris.van.hees@...cle.com>
> Reviewed-by: Nick Alcock <nick.alcock@...cle.com>
> Reviewed-by: Alan Maguire <alan.maguire@...cle.com>
> Reviewed-by: Steven Rostedt (Google) <rostedt@...dmis.org>
> ---
>     Changes since v5:
>      - Removed unnecessary compatibility info from option description.
>
>     Changes since v4:
>      - Improved commit description to explain the why and how.
>      - Documented dependency on GNU AWK for CONFIG_BUILTIN_MODULE_RANGES.
>      - Improved comments in generate_builtin_ranges.awk
>      - Improved logic in generate_builtin_ranges.awk to handle incorrect
>        object size information in linker maps
>
>     Changes since v3:
>      - Consolidated patches 2 through 5 into a single patch
>      - Move CONFIG_BUILTIN_MODULE_RANGES to Kconfig.debug
>      - Make CONFIG_BUILTIN_MODULE_RANGES select CONFIG_VMLINUX_MAP
>      - Disable CONFIG_BUILTIN_MODULE_RANGES if CONFIG_LTO_CLANG_(FULL|THIN)=y
>      - Support LLVM (lld) compiles in generate_builtin_ranges.awk
>      - Support CONFIG_LD_DEAD_CODE_DATA_ELIMINATION=y
>
>     Changes since v2:
>      - Add explicit dependency on FTRACE for CONFIG_BUILTIN_MODULE_RANGES
>      - 1st arg to generate_builtin_ranges.awk is now modules.builtin.modinfo
>      - Switched from using modules.builtin.objs to parsing .*.cmd files
>      - Parse data from .*.cmd in generate_builtin_ranges.awk
>      - Use $(real-prereqs) rather than $(filter-out ...)
> ---

>  System utilities

> index a30c03a66172..dcdf14ffe031 100644
> --- a/lib/Kconfig.debug
> +++ b/lib/Kconfig.debug
> @@ -571,6 +571,22 @@ config VMLINUX_MAP
>           pieces of code get eliminated with
>           CONFIG_LD_DEAD_CODE_DATA_ELIMINATION.
>
> +config BUILTIN_MODULE_RANGES
> +       bool "Generate address range information for builtin modules"
> +       depends on !LTO_CLANG_FULL
> +       depends on !LTO_CLANG_THIN
> +       select VMLINUX_MAP


I still got

"WARNING: unmet direct dependencies detected for VMLINUX_MAP"


I suggested "depends on VMLINUX_MAP" instead of "select VMLINUX_MAP".



https://lore.kernel.org/linux-kbuild/202405150623.lmS5sVhM-lkp@intel.com/

https://lore.kernel.org/linux-kbuild/CAK7LNAST_SbaN9WQRM_k0xE1MUReJvn9AMSg4A1-9b9xotf67w@mail.gmail.com/








> +       help
> +        When modules are built into the kernel, there will be no module name
> +        associated with its symbols in /proc/kallsyms.  Tracers may want to
> +        identify symbols by module name and symbol name regardless of whether
> +        the module is configured as loadable or not.
> +
> +        This option generates modules.builtin.ranges in the build tree with
> +        offset ranges (per ELF section) for the module(s) they belong to.
> +        It also records an anchor symbol to determine the load address of the
> +        section.
> +
>  config DEBUG_FORCE_WEAK_PER_CPU
>         bool "Force weak per-cpu definitions"
>         depends on DEBUG_KERNEL
> diff --git a/scripts/Makefile.vmlinux b/scripts/Makefile.vmlinux
> index 49946cb96844..7e21162e9de1 100644
> --- a/scripts/Makefile.vmlinux
> +++ b/scripts/Makefile.vmlinux
> @@ -33,6 +33,22 @@ targets += vmlinux
>  vmlinux: scripts/link-vmlinux.sh vmlinux.o $(KBUILD_LDS) FORCE
>         +$(call if_changed_dep,link_vmlinux)
>
> +# module.builtin.ranges
> +# ---------------------------------------------------------------------------
> +ifdef CONFIG_BUILTIN_MODULE_RANGES
> +__default: modules.builtin.ranges
> +
> +quiet_cmd_modules_builtin_ranges = GEN     $@
> +      cmd_modules_builtin_ranges = \
> +       $(srctree)/scripts/generate_builtin_ranges.awk $(real-prereqs) > $@
> +
> +vmlinux.map: vmlinux


This should be:


vmlinux.map: vmlinux
        @:


Otherwise, GNU Make would try to find a pattern rule
to update vmlinux.map.





> +
> +targets += modules.builtin.ranges
> +modules.builtin.ranges: modules.builtin vmlinux.map vmlinux.o.map FORCE
> +       $(call if_changed,modules_builtin_ranges)



Presumably, modules.builtin.ranges should be regenerated when
scripts/generate_builtin_ranges.awk is changed.


Maybe, you can do this:


quiet_cmd_modules_builtin_ranges = GEN     $@
      cmd_modules_builtin_ranges = $(real-prereqs) > $@

targets += modules.builtin.ranges
modules.builtin.ranges: $(srctree)/scripts/generate_builtin_ranges.awk \
                        modules.builtin vmlinux.map vmlinux.o.map FORCE
        $(call if_changed,modules_builtin_ranges)




> +endif
> +
>  # Add FORCE to the prequisites of a target to force it to be always rebuilt.
>  # ---------------------------------------------------------------------------
>
> diff --git a/scripts/Makefile.vmlinux_o b/scripts/Makefile.vmlinux_o
> index 6de297916ce6..252505505e0e 100644
> --- a/scripts/Makefile.vmlinux_o
> +++ b/scripts/Makefile.vmlinux_o
> @@ -45,9 +45,12 @@ objtool-args = $(vmlinux-objtool-args-y) --link
>  # Link of vmlinux.o used for section mismatch analysis
>  # ---------------------------------------------------------------------------
>
> +vmlinux-o-ld-args-$(CONFIG_BUILTIN_MODULE_RANGES)      += -Map=$@...p
> +
>  quiet_cmd_ld_vmlinux.o = LD      $@
>        cmd_ld_vmlinux.o = \
>         $(LD) ${KBUILD_LDFLAGS} -r -o $@ \
> +       $(vmlinux-o-ld-args-y) \
>         $(addprefix -T , $(initcalls-lds)) \
>         --whole-archive vmlinux.a --no-whole-archive \
>         --start-group $(KBUILD_VMLINUX_LIBS) --end-group \
> diff --git a/scripts/generate_builtin_ranges.awk b/scripts/generate_builtin_ranges.awk
> new file mode 100755
> index 000000000000..9b647781d5fe
> --- /dev/null
> +++ b/scripts/generate_builtin_ranges.awk
> @@ -0,0 +1,515 @@
> +#!/usr/bin/gawk -f
> +# SPDX-License-Identifier: GPL-2.0
> +# generate_builtin_ranges.awk: Generate address range data for builtin modules
> +# Written by Kris Van Hees <kris.van.hees@...cle.com>
> +#
> +# Usage: generate_builtin_ranges.awk modules.builtin vmlinux.map \
> +#              vmlinux.o.map > modules.builtin.ranges
> +#
> +
> +# Return the module name(s) (if any) associated with the given object.
> +#
> +# If we have seen this object before, return information from the cache.
> +# Otherwise, retrieve it from the corresponding .cmd file.
> +#
> +function get_module_info(fn, mod, obj, mfn, s) {
> +       if (fn in omod)
> +               return omod[fn];
> +
> +       if (match(fn, /\/[^/]+$/) == 0)
> +               return "";
> +
> +       obj = fn;
> +       mod = "";
> +       mfn = "";
> +       fn = substr(fn, 1, RSTART) "." substr(fn, RSTART + 1) ".cmd";
> +       if (getline s <fn == 1) {
> +               if (match(s, /DKBUILD_MODFILE=['"]+[^'"]+/) > 0) {
> +                       mfn = substr(s, RSTART + 16, RLENGTH - 16);
> +                       gsub(/['"]/, "", mfn);
> +
> +                       mod = mfn;
> +                       gsub(/([^/ ]*\/)+/, "", mod);
> +                       gsub(/-/, "_", mod);
> +               }
> +       }
> +       close(fn);
> +
> +       # A single module (common case) also reflects objects that are not part
> +       # of a module.  Some of those objects have names that are also a module
> +       # name (e.g. core).  We check the associated module file name, and if
> +       # they do not match, the object is not part of a module.
> +       if (mod !~ / /) {
> +               if (!(mod in mods))
> +                       mod = "";
> +               if (mods[mod] != mfn)
> +                       mod = "";
> +       }
> +
> +       # At this point, mod is a single (valid) module name, or a list of
> +       # module names (that do not need validation).
> +       omod[obj] = mod;
> +       close(fn);


Is this "close(fn)" necessary?
I see it a few lines above too.




The code became way simpler since my previous review, but
I think this is still redundant.

You do not need to check both of modname and its path.

I attached a patch for code refactoring.





--
Best Regards

Masahiro Yamada

View attachment "0001-fixup-modules.builtin.ranges.patch" of type "text/x-patch" (3835 bytes)

Powered by blists - more mailing lists