Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [RFC][PATCH v4 02/32] objtool: Make recordmcount into mcount
 subcmd
To:     Matt Helsley <mhelsley@vmware.com>, linux-kernel@vger.kernel.org,
        Josh Poimboeuf <jpoimboe@redhat.com>,
        Peter Zijlstra <peterz@infradead.org>,
        Steven Rostedt <rostedt@goodmis.org>,
        Sami Tolvanen <samitolvanen@google.com>,
        Kamalesh Babulal <kamalesh@linux.vnet.ibm.com>
References: <cover.1591125127.git.mhelsley@vmware.com>
 <0d44e0b1b8ebce4af8b90f73e351a003ef016005.1591125127.git.mhelsley@vmware.com>
 <fe3e0e4e-4f13-5193-c684-f995c8310e54@redhat.com>
 <20200609183951.GE1284251@rlwimi.vmware.com>
From:   Julien Thierry <jthierry@redhat.com>
Message-ID: <c7011f00-23d2-5d14-d0bb-9d29c4a24c15@redhat.com>
Date:   Tue, 9 Jun 2020 20:11:26 +0100
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101
 Thunderbird/68.5.0
MIME-Version: 1.0
In-Reply-To: <20200609183951.GE1284251@rlwimi.vmware.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk


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@vmware.com>
>>> ---
> ...
>>> 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