Re: [RFC][PATCH v4 02/32] objtool: Make recordmcount into mcount subcmd

[Julien Thierry]

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 <mhelsley@xxxxxxxxxx>
> > > ---
> ...
> > > 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