Re: [RFC][PATCH v5 03/51] objtool: Make recordmcount into mcount subcmd

[Miroslav Benes]

On Thu, 18 Jun 2020, 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.

I'll try to leave only relevant parts for a question below...

>  sub_cmd_record_mcount =  \
>   if [ $(@) != "scripts/mod/empty.o" ]; then  \
> - $(objtree)/tools/objtool/recordmcount $(RECORDMCOUNT_FLAGS) 
> "$(@)"; \
> + $(objtree)/tools/objtool/objtool mcount record 
> $(RECORDMCOUNT_FLAGS) "$(@)";\
>   fi;

> +int cmd_mcount(int argc, const char **argv)
> +{
> + argc--; argv++;
> + if (argc <= 0)
> + usage_with_options(mcount_usage, mcount_options);
> +
> + if (!strncmp(argv[0], "record", 6)) {
> + argc = parse_options(argc, argv,
> +  mcount_options, mcount_usage, 0);
> + if (argc < 1)
> + usage_with_options(mcount_usage, mcount_options);
> +
> + return record_mcount(argc, argv);
> + }
> +
> + usage_with_options(mcount_usage, mcount_options);
> +
> + return 0;
> +}

> -int main(int argc, char *argv[])
> +int record_mcount(int argc, const char **argv)
>  {
>   const char ftrace[] = "/ftrace.o";
>   int ftrace_size = sizeof(ftrace) - 1;
>   int n_error = 0;  /* gcc-4.3.0 false positive complaint */
> - int c;
>   int i;
>  
> - while ((c = getopt(argc, argv, "w")) >= 0) {
> - switch (c) {
> - case 'w':
> - warn_on_notrace_sect = 1;
> - break;
> - default:
> - fprintf(stderr, "usage: recordmcount [-w] file.o...\n");
> - return 0;
> - }
> - }
> -
> - if ((argc - optind) < 1) {
> - fprintf(stderr, "usage: recordmcount [-w] file.o...\n");
> - return 0;
> - }
> -
>   /* Process each file in turn, allowing deep failure. */
> - for (i = optind; i < argc; i++) {
> - char *file = argv[i];
> + for (i = 0; i < argc; i++) {
> + const char *file = argv[i];
>   int len;

Do you expect that mcount subcmd would be called on more than one object 
file at a time? I don't see a reason for that with all the Makefile 
changes, but I may be missing something (Kbuild is a maze for me).

Because if not, I think it would be nice to make record_mcount() more 
similar to what we have for check(). After Julien's changes 
(20200608071203.4055-1-jthie...@redhat.com) at least. So that 
record_mcount() could accept struct objtool_file and work directly on 
that.

It would also impact several other patches in the series. For example, 
is there a need for a private 'struct elf *lf' in mcount.c?

Thanks
Miroslav

[RFC][PATCH v5 03/51] objtool: Make recordmcount into mcount subcmd

[Matt Helsley]

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| 15 --
 arch/arm64/include/asm/ftrace.h |  2 +-
 arch/x86/include/asm/ftrace.h   |  2 +-
 kernel/trace/Kconfig|  9 +-
 scripts/Makefile.build  | 19 +++--
 scripts/sorttable.h |  2 +-
 tools/objtool/Build |  4 +--
 tools/objtool/Makefile  | 20 ++---
 tools/objtool/builtin-mcount.c  | 50 +
 tools/objtool/builtin.h |  1 +
 tools/objtool/objtool.c |  1 +
 tools/objtool/objtool.h |  1 +
 tools/objtool/recordmcount.c| 36 +++-
 tools/objtool/weak.c|  5 
 16 files changed, 104 insertions(+), 71 deletions(-)
 create mode 100644 tools/objtool/builtin-mcount.c

diff --git a/Documentation/dontdiff b/Documentation/dontdiff
index ef9519c32c55..82cc4e3bb713 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 24ec4ec2d98d..eefb966e5832 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 e20c30f82c58..3842d7258b43 100644
--- a/Makefile
+++ b/Makefile
@@ -841,6 +841,7 @@ ifdef CONFIG_FTRACE_MCOUNT_RECORD
   ifeq ($(call cc-option-yn,-mrecord-mcount),y)
 CC_FLAGS_FTRACE+= -mrecord-mcount
 export CC_USING_RECORD_MCOUNT := 1
+undefine CONFIG_OBJTOOL_SUBCMD_MCOUNT
   endif
   ifdef CONFIG_HAVE_NOP_MCOUNT
 ifeq ($(call cc-option-yn, -mnop-mcount),y)
@@ -848,7 +849,7 @@ ifdef CONFIG_FTRACE_MCOUNT_RECORD
   CC_FLAGS_USING   += -DCC_USING_NOP_MCOUNT
 endif
   endif
-endif
+endif # CONFIG_FTRACE_MCOUNT_RECORD
 ifdef CONFIG_HAVE_FENTRY
   ifeq ($(call cc-option-yn, -mfentry),y)
 CC_FLAGS_FTRACE+= -mfentry
@@ -858,14 +859,7 @@ endif
 export CC_FLAGS_FTRACE
 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
-   objtool_target := tools/objtool FORCE
-   endif
-endif
-endif
+endif # CONFIG_FUNCTION_TRACER
 
 # We trigger additional mismatches with less inlining
 ifdef CONFIG_DEBUG_SECTION_MISMATCH
@@ -1196,6 +1190,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 CONFIG_OBJTOOL_SUBCMD_MCOUNT
+   @echo "error: Cannot generate tracing metadata for 
CONFIG_DYNAMIC_FT