Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Tue, 28 May 2019 09:54:58 -0500
From:   Josh Poimboeuf <jpoimboe@redhat.com>
To:     Matt Helsley <mhelsley@vmware.com>
Cc:     LKML <linux-kernel@vger.kernel.org>,
        Ingo Molnar <mingo@kernel.org>,
        Peter Zijlstra <peterz@infradead.org>,
        Steven Rostedt <rostedt@goodmis.org>
Subject: Re: [RFC][PATCH 10/13] objtool: Make recordmcount into an objtool
 subcmd
Message-ID: <20190528145458.2xmlkzucuekj2km4@treble>
References: <cover.1558569448.git.mhelsley@vmware.com>
 <10da56db6206a99d1b909e56ee48cb4ceb374ef8.1558569448.git.mhelsley@vmware.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <10da56db6206a99d1b909e56ee48cb4ceb374ef8.1558569448.git.mhelsley@vmware.com>
User-Agent: NeoMutt/20180716
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Wed, May 22, 2019 at 05:03:33PM -0700, 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 saving 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.
> 
> 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 <mhelsley@vmware.com>
> ---
>  scripts/Makefile.build         | 15 +++++--
>  tools/objtool/Build            |  3 +-
>  tools/objtool/Makefile         | 18 +++------
>  tools/objtool/builtin-mcount.c | 72 ++++++++++++++++++++++++++++++++++
>  tools/objtool/builtin-mcount.h | 23 +++++++++++
>  tools/objtool/builtin.h        |  6 +++
>  tools/objtool/objtool.c        |  6 +++
>  tools/objtool/recordmcount.c   | 29 +++++---------
>  8 files changed, 134 insertions(+), 38 deletions(-)
>  create mode 100644 tools/objtool/builtin-mcount.c
>  create mode 100644 tools/objtool/builtin-mcount.h
> 
> diff --git a/scripts/Makefile.build b/scripts/Makefile.build
> index f32cfe63bb0e..6a3a5a477cbd 100644
> --- a/scripts/Makefile.build
> +++ b/scripts/Makefile.build
> @@ -173,10 +173,13 @@ cmd_modversions_c =								\
>  	fi
>  endif
>  
> +objtool_dep =
> +
>  ifdef CONFIG_FTRACE_MCOUNT_RECORD
>  ifndef CC_USING_RECORD_MCOUNT
> -# compiler will not generate __mcount_loc use recordmcount or recordmcount.pl
> +# compiler will not generate __mcount_loc use objtool mcount record or recordmcount.pl
>  ifdef BUILD_C_RECORDMCOUNT
> +objtool_dep += $(objtree)/tools/objtool/objtool
>  ifeq ("$(origin RECORDMCOUNT_WARN)", "command line")
>    RECORDMCOUNT_FLAGS = -w
>  endif
> @@ -186,10 +189,12 @@ endif
>  # files, including recordmcount.
>  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;
>  
> -recordmcount_source := $(srctree)/tools/objtool/recordmcount.c \
> +recordmcount_source := $(srctree)/tools/objtool/builtin-mcount.c \
> +		    $(srctree)/tools/objtool/builtin-mcount.h \
> +		    $(srctree)/tools/objtool/recordmcount.c \
>  		    $(srctree)/tools/objtool/recordmcount.h

Is this needed? if any of these files change, then objtool will change,
and which is already covered by the objtool_dep assignment above.

>  else
>  sub_cmd_record_mcount = perl $(srctree)/tools/objtool/recordmcount.pl "$(ARCH)" \
> @@ -203,6 +208,8 @@ endif # BUILD_C_RECORDMCOUNT
>  cmd_record_mcount = $(if $(findstring $(strip $(CC_FLAGS_FTRACE)),$(_c_flags)),	\
>  	$(sub_cmd_record_mcount))
>  endif # CC_USING_RECORD_MCOUNT
> +
> +objtool_dep += $(recordmcount_source)

I don't think this is needed because recordmcount_source is already
listed as a dependency for .o files.

>  endif # CONFIG_FTRACE_MCOUNT_RECORD
>  
>  ifdef CONFIG_STACK_VALIDATION
> @@ -241,7 +248,7 @@ endif # SKIP_STACK_VALIDATION
>  endif # CONFIG_STACK_VALIDATION
>  
>  # Rebuild all objects when objtool changes, or is enabled/disabled.
> -objtool_dep = $(objtool_obj)					\
> +objtool_dep += $(objtool_obj)		\

The backslash should be aligned with the following ones.

>  	      $(wildcard include/config/orc/unwinder.h		\
>  			 include/config/stack/validation.h)

Should we also add an ftrace config dependency here?
Like include/config/ftrace/mcount/record.h?

>  
> diff --git a/tools/objtool/Build b/tools/objtool/Build
> index 78c4a8a2f9e7..7b71534767bd 100644
> --- a/tools/objtool/Build
> +++ b/tools/objtool/Build
> @@ -1,6 +1,7 @@
>  objtool-y += arch/$(SRCARCH)/
>  objtool-y += builtin-check.o
>  objtool-y += builtin-orc.o
> +objtool-$(BUILD_C_RECORDMCOUNT) += builtin-mcount.o recordmcount.o

Can we just build these files unconditionally, even if they're not used?
Thus far, objtool doesn't have any kernel config dependencies like this.
It helps keep things simple and keeps objtool more separate from the
kernel.

So if you build record mcount unconditionally then I think you can also
get rid of the BUILD_C_RECORDMCOUNT export, the CMD_MCOUNT define, and
cmd_nop().

-- 
Josh