Re: [RFC][PATCH v4 02/32] objtool: Make recordmcount into mcount subcmd
On 6/9/20 7:39 PM, Matt Helsley wrote: On Tue, Jun 09, 2020 at 10:00:59AM +0100, Julien Thierry wrote: Hi Matt, On 6/2/20 8:49 PM, Matt Helsley wrote: Rather than a standalone executable merge recordmcount as a sub command of objtool. This is a small step towards cleaning up recordmcount and eventually sharing ELF code with objtool. For the initial step all that's required is a bit of Makefile changes and invoking the former main() function from recordmcount.c because the subcommand code uses similar function arguments as main when dispatching. objtool ignores some object files that tracing does not, specifically those with OBJECT_FILES_NON_STANDARD Makefile variables. For this reason we keep the recordmcount_dep separate from the objtool_dep. When using objtool mcount we can also, like the other objtool invocations, just depend on the binary rather than the source the binary is built from. Subsequent patches will gradually convert recordmcount to use more and more of libelf/objtool's ELF accessor code. This will both clean up recordmcount to be more easily readable and remove recordmcount's crude accessor wrapping code. Signed-off-by: Matt Helsley --- ... diff --git a/kernel/trace/Kconfig b/kernel/trace/Kconfig index 743647005f64..ae74647b06fa 100644 --- a/kernel/trace/Kconfig +++ b/kernel/trace/Kconfig @@ -59,7 +59,7 @@ config HAVE_NOP_MCOUNT config HAVE_C_RECORDMCOUNT bool help - C version of recordmcount available? + C version of objtool mcount available? The "C version" doesn't make much sense here. "Objtool mcount available?" or "mcount subcommand of objtool available?" perhaps? Agreed, "C version" is nonsense at this point. Looking at the other HAVE_* help messages in that Kconfig suggests: Arch supports objtool mcount subcommand So I've changed it to that. Yes, that seems good. diff --git a/tools/objtool/Makefile b/tools/objtool/Makefile index 285474a77fe9..ffef73f7f47e 100644 --- a/tools/objtool/Makefile +++ b/tools/objtool/Makefile @@ -31,12 +31,6 @@ OBJTOOL_IN := $(OBJTOOL)-in.o LIBELF_FLAGS := $(shell pkg-config libelf --cflags 2>/dev/null) LIBELF_LIBS := $(shell pkg-config libelf --libs 2>/dev/null || echo -lelf) -RECORDMCOUNT := $(OUTPUT)recordmcount -RECORDMCOUNT_IN := $(RECORDMCOUNT)-in.o -ifeq ($(BUILD_C_RECORDMCOUNT),y) -all: $(RECORDMCOUNT) -endif - all: $(OBJTOOL) INCLUDES := -I$(srctree)/tools/include \ @@ -55,13 +49,47 @@ AWK = awk SUBCMD_CHECK := n SUBCMD_ORC := n +SUBCMD_MCOUNT := n ifeq ($(SRCARCH),x86) SUBCMD_CHECK := y SUBCMD_ORC := y + SUBCMD_MCOUNT := y +endif + +ifeq ($(SRCARCH),arm) + SUBCMD_MCOUNT := y +endif + +ifeq ($(SRCARCH),arm64) + SUBCMD_MCOUNT := y +endif + +ifeq ($(SRCARCH),ia64) + SUBCMD_MCOUNT := y +endif + +ifeq ($(SRCARCH),mips) + SUBCMD_MCOUNT := y +endif + +ifeq ($(SRCARCH),powerpc) + SUBCMD_MCOUNT := y +endif + +ifeq ($(SRCARCH),s390) + SUBCMD_MCOUNT := y +endif + +ifeq ($(SRCARCH),sh) + SUBCMD_MCOUNT := y +endif + +ifeq ($(SRCARCH),sparc) + SUBCMD_MCOUNT := y Is there some arch for which MCOUNT is not supported? If not you could just have MCOUNT default to 'y' and avoid adding all those tests (or maybe reduce the numbers and set to 'n' only for arches not supporting it). Yes, there are some which it does not support. For those architectures we keep recordmcount.pl around. It occured to me that with your suggestion to use more CONFIG_ variables we could eliminate this pattern and replace it with these pseudo-patches: +++ b/kernel/trace/Kconfig +config OBJTOOL_SUBCMD_MCOUNT + bool + depends on HAVE_C_RECORDMCOUNT + select OBJTOOL_SUBCMDS + help + Record mcount call locations using objtool and then change the Makefiles to use the CONFIG_ variables rather than have one ifeq block per arch: +++ b/tools/objtool/Makefile +SUBCMD_MCOUNT := $(CONFIG_OBJTOOL_SUBCMD_MCOUNT) Does this seem like a good use of CONFIG_ variables or is it going too far? Definitely seems like a good idea to me! Will be a nice improvement. Cheers, -- Julien Thierry
Re: [RFC][PATCH v4 02/32] objtool: Make recordmcount into mcount subcmd
On Tue, Jun 09, 2020 at 02:52:07PM -0400, Steven Rostedt wrote: > On Tue, 9 Jun 2020 11:39:51 -0700 > Matt Helsley wrote: > > > > > +ifeq ($(SRCARCH),sparc) > > > > + SUBCMD_MCOUNT := y > > > > > > Is there some arch for which MCOUNT is not supported? If not you could > > > just > > > have MCOUNT default to 'y' and avoid adding all those tests (or maybe > > > reduce > > > the numbers and set to 'n' only for arches not supporting it). > > > > Yes, there are some which it does not support. For those architectures > > we keep recordmcount.pl around. > > > > It occured to me that with your suggestion to use more CONFIG_ variables > > we could eliminate this pattern and replace it with these pseudo-patches: > > > > +++ b/kernel/trace/Kconfig > > > > +config OBJTOOL_SUBCMD_MCOUNT > > + bool > > + depends on HAVE_C_RECORDMCOUNT > > + select OBJTOOL_SUBCMDS > > + help > > + Record mcount call locations using objtool > > > > and then change the Makefiles to use the CONFIG_ variables > > rather than have one ifeq block per arch: > > > > +++ b/tools/objtool/Makefile > > > > +SUBCMD_MCOUNT := $(CONFIG_OBJTOOL_SUBCMD_MCOUNT) > > If you can make this work, this is definitely the way to go. I think I can so I'll give it a go! Cheers, -Matt Helsley
Re: [RFC][PATCH v4 02/32] objtool: Make recordmcount into mcount subcmd
On Tue, 9 Jun 2020 11:39:51 -0700 Matt Helsley wrote: > > > +ifeq ($(SRCARCH),sparc) > > > + SUBCMD_MCOUNT := y > > > > Is there some arch for which MCOUNT is not supported? If not you could just > > have MCOUNT default to 'y' and avoid adding all those tests (or maybe reduce > > the numbers and set to 'n' only for arches not supporting it). > > Yes, there are some which it does not support. For those architectures > we keep recordmcount.pl around. > > It occured to me that with your suggestion to use more CONFIG_ variables > we could eliminate this pattern and replace it with these pseudo-patches: > > +++ b/kernel/trace/Kconfig > > +config OBJTOOL_SUBCMD_MCOUNT > + bool > + depends on HAVE_C_RECORDMCOUNT > + select OBJTOOL_SUBCMDS > + help > + Record mcount call locations using objtool > > and then change the Makefiles to use the CONFIG_ variables > rather than have one ifeq block per arch: > > +++ b/tools/objtool/Makefile > > +SUBCMD_MCOUNT := $(CONFIG_OBJTOOL_SUBCMD_MCOUNT) If you can make this work, this is definitely the way to go. -- Steve > > Does this seem like a good use of CONFIG_ variables or is it going too > far? > > I haven't changed to this pattern just yet -- I'm hoping you and Josh > or Peter might weigh in with your preferences.
Re: [RFC][PATCH v4 02/32] objtool: Make recordmcount into mcount subcmd
On Tue, Jun 09, 2020 at 10:00:59AM +0100, Julien Thierry wrote: > Hi Matt, > > On 6/2/20 8:49 PM, Matt Helsley wrote: > > Rather than a standalone executable merge recordmcount as a sub command > > of objtool. This is a small step towards cleaning up recordmcount and > > eventually sharing ELF code with objtool. > > > > For the initial step all that's required is a bit of Makefile changes > > and invoking the former main() function from recordmcount.c because the > > subcommand code uses similar function arguments as main when dispatching. > > > > objtool ignores some object files that tracing does not, specifically > > those with OBJECT_FILES_NON_STANDARD Makefile variables. For this reason > > we keep the recordmcount_dep separate from the objtool_dep. When using > > objtool mcount we can also, like the other objtool invocations, just > > depend on the binary rather than the source the binary is built from. > > > > Subsequent patches will gradually convert recordmcount to use > > more and more of libelf/objtool's ELF accessor code. This will both > > clean up recordmcount to be more easily readable and remove > > recordmcount's crude accessor wrapping code. > > > > Signed-off-by: Matt Helsley > > --- ... > > diff --git a/kernel/trace/Kconfig b/kernel/trace/Kconfig > > index 743647005f64..ae74647b06fa 100644 > > --- a/kernel/trace/Kconfig > > +++ b/kernel/trace/Kconfig > > @@ -59,7 +59,7 @@ config HAVE_NOP_MCOUNT > > config HAVE_C_RECORDMCOUNT > > bool > > help > > - C version of recordmcount available? > > + C version of objtool mcount available? > > The "C version" doesn't make much sense here. "Objtool mcount available?" or > "mcount subcommand of objtool available?" perhaps? Agreed, "C version" is nonsense at this point. Looking at the other HAVE_* help messages in that Kconfig suggests: Arch supports objtool mcount subcommand So I've changed it to that. > > diff --git a/tools/objtool/Makefile b/tools/objtool/Makefile > > index 285474a77fe9..ffef73f7f47e 100644 > > --- a/tools/objtool/Makefile > > +++ b/tools/objtool/Makefile > > @@ -31,12 +31,6 @@ OBJTOOL_IN := $(OBJTOOL)-in.o > > LIBELF_FLAGS := $(shell pkg-config libelf --cflags 2>/dev/null) > > LIBELF_LIBS := $(shell pkg-config libelf --libs 2>/dev/null || echo > > -lelf) > > -RECORDMCOUNT := $(OUTPUT)recordmcount > > -RECORDMCOUNT_IN := $(RECORDMCOUNT)-in.o > > -ifeq ($(BUILD_C_RECORDMCOUNT),y) > > -all: $(RECORDMCOUNT) > > -endif > > - > > all: $(OBJTOOL) > > INCLUDES := -I$(srctree)/tools/include \ > > @@ -55,13 +49,47 @@ AWK = awk > > SUBCMD_CHECK := n > > SUBCMD_ORC := n > > +SUBCMD_MCOUNT := n > > ifeq ($(SRCARCH),x86) > > SUBCMD_CHECK := y > > SUBCMD_ORC := y > > + SUBCMD_MCOUNT := y > > +endif > > + > > +ifeq ($(SRCARCH),arm) > > + SUBCMD_MCOUNT := y > > +endif > > + > > +ifeq ($(SRCARCH),arm64) > > + SUBCMD_MCOUNT := y > > +endif > > + > > +ifeq ($(SRCARCH),ia64) > > + SUBCMD_MCOUNT := y > > +endif > > + > > +ifeq ($(SRCARCH),mips) > > + SUBCMD_MCOUNT := y > > +endif > > + > > +ifeq ($(SRCARCH),powerpc) > > + SUBCMD_MCOUNT := y > > +endif > > + > > +ifeq ($(SRCARCH),s390) > > + SUBCMD_MCOUNT := y > > +endif > > + > > +ifeq ($(SRCARCH),sh) > > + SUBCMD_MCOUNT := y > > +endif > > + > > +ifeq ($(SRCARCH),sparc) > > + SUBCMD_MCOUNT := y > > Is there some arch for which MCOUNT is not supported? If not you could just > have MCOUNT default to 'y' and avoid adding all those tests (or maybe reduce > the numbers and set to 'n' only for arches not supporting it). Yes, there are some which it does not support. For those architectures we keep recordmcount.pl around. It occured to me that with your suggestion to use more CONFIG_ variables we could eliminate this pattern and replace it with these pseudo-patches: +++ b/kernel/trace/Kconfig +config OBJTOOL_SUBCMD_MCOUNT + bool + depends on HAVE_C_RECORDMCOUNT + select OBJTOOL_SUBCMDS + help + Record mcount call locations using objtool and then change the Makefiles to use the CONFIG_ variables rather than have one ifeq block per arch: +++ b/tools/objtool/Makefile +SUBCMD_MCOUNT := $(CONFIG_OBJTOOL_SUBCMD_MCOUNT) Does this seem like a good use of CONFIG_ variables or is it going too far? I haven't changed to this pattern just yet -- I'm hoping you and Josh or Peter might weigh in with your preferences. > > > endif > > -export SUBCMD_CHECK SUBCMD_ORC > > +export SUBCMD_CHECK SUBCMD_ORC SUBCMD_MCOUNT ... > > diff --git a/tools/objtool/builtin.h b/tools/objtool/builtin.h > > index 85c979caa367..9c7331592fa7 100644 > > --- a/tools/objtool/builtin.h > > +++ b/tools/objtool/builtin.h > > @@ -12,5 +12,7 @@ extern bool no_fp, no_unreachable, retpoline, module, > > backtrace, uaccess, stats, > > extern int cmd_check(int argc, const char **argv); > > extern int cmd_orc(int argc, const char **argv); > > +extern bool is_cmd_mcount_available(void); > > This appears to be unused.
Re: [RFC][PATCH v4 02/32] objtool: Make recordmcount into mcount subcmd
Hi Matt, On 6/2/20 8:49 PM, Matt Helsley wrote: Rather than a standalone executable merge recordmcount as a sub command of objtool. This is a small step towards cleaning up recordmcount and eventually sharing ELF code with objtool. For the initial step all that's required is a bit of Makefile changes and invoking the former main() function from recordmcount.c because the subcommand code uses similar function arguments as main when dispatching. objtool ignores some object files that tracing does not, specifically those with OBJECT_FILES_NON_STANDARD Makefile variables. For this reason we keep the recordmcount_dep separate from the objtool_dep. When using objtool mcount we can also, like the other objtool invocations, just depend on the binary rather than the source the binary is built from. Subsequent patches will gradually convert recordmcount to use more and more of libelf/objtool's ELF accessor code. This will both clean up recordmcount to be more easily readable and remove recordmcount's crude accessor wrapping code. Signed-off-by: Matt Helsley --- Documentation/dontdiff | 2 +- Documentation/trace/ftrace.rst | 6 ++-- Makefile| 9 -- arch/arm64/include/asm/ftrace.h | 2 +- arch/x86/include/asm/ftrace.h | 2 +- kernel/trace/Kconfig| 2 +- scripts/Makefile.build | 19 +++-- scripts/sorttable.h | 2 +- tools/objtool/Build | 4 +-- tools/objtool/Makefile | 48 +++ tools/objtool/builtin-mcount.c | 50 + tools/objtool/builtin.h | 2 ++ tools/objtool/objtool.c | 1 + tools/objtool/objtool.h | 1 + tools/objtool/recordmcount.c| 36 +++- tools/objtool/weak.c| 5 16 files changed, 131 insertions(+), 60 deletions(-) create mode 100644 tools/objtool/builtin-mcount.c diff --git a/Documentation/dontdiff b/Documentation/dontdiff index 72fc2e9e2b63..d7e0ec691e02 100644 --- a/Documentation/dontdiff +++ b/Documentation/dontdiff @@ -211,7 +211,7 @@ r420_reg_safe.h r600_reg_safe.h randomize_layout_hash.h randomize_layout_seed.h -recordmcount +objtool relocs rlim_names.h rn50_reg_safe.h diff --git a/Documentation/trace/ftrace.rst b/Documentation/trace/ftrace.rst index 9adefcc3c7a8..6b9fc7cad543 100644 --- a/Documentation/trace/ftrace.rst +++ b/Documentation/trace/ftrace.rst @@ -2684,8 +2684,8 @@ every kernel function, produced by the -pg switch in gcc), starts of pointing to a simple return. (Enabling FTRACE will include the -pg switch in the compiling of the kernel.) -At compile time every C file object is run through the -recordmcount program (located in the tools/objtool directory). This +At compile time every C file object is run through objtool's +mcount subcommand (located in the tools/objtool directory). This program will parse the ELF headers in the C object to find all the locations in the .text section that call mcount. Starting with gcc version 4.6, the -mfentry has been added for x86, which @@ -2699,7 +2699,7 @@ can be traced. A section called "__mcount_loc" is created that holds references to all the mcount/fentry call sites in the .text section. -The recordmcount program re-links this section back into the +Running "objtool mcount" re-links this section back into the original object. The final linking stage of the kernel will add all these references into a single table. diff --git a/Makefile b/Makefile index d353a0a65a71..99a4d8c61bdb 100644 --- a/Makefile +++ b/Makefile @@ -842,12 +842,12 @@ KBUILD_CFLAGS += $(CC_FLAGS_FTRACE) $(CC_FLAGS_USING) KBUILD_AFLAGS += $(CC_FLAGS_USING) ifdef CONFIG_DYNAMIC_FTRACE ifdef CONFIG_HAVE_C_RECORDMCOUNT - BUILD_C_RECORDMCOUNT := y - export BUILD_C_RECORDMCOUNT + USE_OBJTOOL_MCOUNT := y + export USE_OBJTOOL_MCOUNT objtool_target := tools/objtool FORCE endif endif -endif +endif # CONFIG_FUNCTION_TRACER # We trigger additional mismatches with less inlining ifdef CONFIG_DEBUG_SECTION_MISMATCH @@ -1168,6 +1168,9 @@ ifneq ($(has_libelf),1) ifdef CONFIG_UNWINDER_ORC @echo "error: Cannot generate ORC metadata for CONFIG_UNWINDER_ORC=y, please install libelf-dev, libelf-devel or elfutils-libelf-devel" >&2 @false + else ifdef USE_OBJTOOL_MCOUNT + @echo "error: Cannot generate tracing metadata for CONFIG_DYNAMIC_FTRACE, please install libelf-dev, libelf-devel or elfutils-libelf-devel" >&2 + @false else ifeq ($(SKIP_STACK_VALIDATION),1) @echo "warning: Cannot use CONFIG_STACK_VALIDATION=y, please install libelf-dev, libelf-devel or elfutils-libelf-devel" >&2 diff --git a/arch/arm64/include/asm/ftrace.h b/arch/arm64/include/asm/ftrace.h index 91fa4baa1a93..5fd71bf592d5 100644 --- a/arch/arm64/include/asm/ftrace.h +++
