Re: [RFC][PATCH v4 01/32] objtool: Prepare to merge recordmcount

[Julien Thierry <jthierry@redhat.com>]

Hi Matt,

On 6/2/20 8:49 PM, Matt Helsley wrote:
> Move recordmcount into the objtool directory. We keep this step separate
> so changes which turn recordmcount into a subcommand of objtool don't
> get obscured.
> 
> Signed-off-by: Matt Helsley <mhelsley@vmware.com>
> ---
>   Documentation/trace/ftrace-design.rst      |  4 ++--
>   Documentation/trace/ftrace.rst             |  2 +-
>   Makefile                                   | 15 +++++++++------
>   scripts/.gitignore                         |  1 -
>   scripts/Makefile                           |  1 -
>   scripts/Makefile.build                     | 11 ++++++-----
>   tools/objtool/.gitignore                   |  1 +
>   tools/objtool/Build                        |  2 ++
>   tools/objtool/Makefile                     | 13 ++++++++++++-
>   {scripts => tools/objtool}/recordmcount.c  |  0
>   {scripts => tools/objtool}/recordmcount.h  |  0
>   {scripts => tools/objtool}/recordmcount.pl |  0
>   12 files changed, 33 insertions(+), 17 deletions(-)
>   rename {scripts => tools/objtool}/recordmcount.c (100%)
>   rename {scripts => tools/objtool}/recordmcount.h (100%)
>   rename {scripts => tools/objtool}/recordmcount.pl (100%)
> 
> diff --git a/Documentation/trace/ftrace-design.rst b/Documentation/trace/ftrace-design.rst
> index a8e22e0db63c..dea8db5e79d0 100644
> --- a/Documentation/trace/ftrace-design.rst
> +++ b/Documentation/trace/ftrace-design.rst
> @@ -261,7 +261,7 @@ You need very few things to get the syscalls tracing in an arch.
>   HAVE_FTRACE_MCOUNT_RECORD
>   -------------------------
>   
> -See scripts/recordmcount.pl for more info.  Just fill in the arch-specific
> +See tools/objtool/recordmcount.pl for more info.  Just fill in the arch-specific
>   details for how to locate the addresses of mcount call sites via objdump.
>   This option doesn't make much sense without also implementing dynamic ftrace.
>   
> @@ -379,7 +379,7 @@ linux/ftrace.h for the functions::
>   	ftrace_make_call()
>   
>   The rec->ip value is the address of the mcount call site that was collected
> -by the scripts/recordmcount.pl during build time.
> +by the tools/objtool/recordmcount.pl during build time.
>   
>   The last function is used to do runtime patching of the active tracer.  This
>   will be modifying the assembly code at the location of the ftrace_call symbol
> diff --git a/Documentation/trace/ftrace.rst b/Documentation/trace/ftrace.rst
> index 3b5614b1d1a5..9adefcc3c7a8 100644
> --- a/Documentation/trace/ftrace.rst
> +++ b/Documentation/trace/ftrace.rst
> @@ -2685,7 +2685,7 @@ 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 scripts directory). This
> +recordmcount program (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
> diff --git a/Makefile b/Makefile
> index 04f5662ae61a..d353a0a65a71 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -844,6 +844,7 @@ 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
> @@ -1023,10 +1024,10 @@ endif
>   export mod_sign_cmd
>   
>   HOST_LIBELF_LIBS = $(shell pkg-config libelf --libs 2>/dev/null || echo -lelf)
> +has_libelf := $(call try-run,\
> +		echo "int main() {}" | $(HOSTCC) -xc -o /dev/null $(HOST_LIBELF_LIBS) -,1,0)
>   

Maybe there could be some build dependency, e.g. CONFIG_OBJTOOL_SUBCMDS 
that sets the "objtool_target" and "has_libelf" when selected.

Then the CONFIG_UNWINDER_ORC, RECORD_MCOUNT and STACK_VALIDATION would 
just had to select that config option.

>   ifdef CONFIG_STACK_VALIDATION
> -  has_libelf := $(call try-run,\
> -		echo "int main() {}" | $(HOSTCC) -xc -o /dev/null $(HOST_LIBELF_LIBS) -,1,0)
>     ifeq ($(has_libelf),1)
>       objtool_target := tools/objtool FORCE
>     else
> @@ -1163,13 +1164,15 @@ uapi-asm-generic:
>   
>   PHONY += prepare-objtool
>   prepare-objtool: $(objtool_target)
> -ifeq ($(SKIP_STACK_VALIDATION),1)
> -ifdef CONFIG_UNWINDER_ORC
> +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
> +  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


I think this would be more readable without the else branch:

	ifneq ($(has_libelf),1)
		ifdef <some objtool command config>
			<warn about unavailability>
		endif
		ifdef <another objtool command config>
			<warn ...>
		endif
		<...>
	endif


Cheers,

-- 
Julien Thierry