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:   Fri, 6 Jul 2018 10:15:28 +0200
From:   Hendrik Brueckner <brueckner@linux.ibm.com>
To:     Kim Phillips <kim.phillips@arm.com>
Cc:     Arnaldo Carvalho de Melo <acme@redhat.com>,
        Ravi Bangoria <ravi.bangoria@linux.vnet.ibm.com>,
        Alexander Shishkin <alexander.shishkin@linux.intel.com>,
        Hendrik Brueckner <brueckner@linux.vnet.ibm.com>,
        Jiri Olsa <jolsa@redhat.com>,
        Michael Ellerman <mpe@ellerman.id.au>,
        Namhyung Kim <namhyung@kernel.org>,
        Thomas Richter <tmricht@linux.vnet.ibm.com>,
        Peter Zijlstra <peterz@infradead.org>,
        Ingo Molnar <mingo@redhat.com>, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2 2/3] perf arm64: Generate system call table from
 asm/unistd.h
References: <20180705181216.0cc737d217d9acda567bd519@arm.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20180705181216.0cc737d217d9acda567bd519@arm.com>
User-Agent: Mutt/1.5.21 (2010-09-15)
Message-Id: <20180706081528.GA5120@linux.ibm.com>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Hi Kim,

On Thu, Jul 05, 2018 at 06:12:16PM -0500, Kim Phillips wrote:
> This should speed up accessing new system calls introduced with the
> kernel rather than waiting for libaudit updates to include them.
> 
> Using the existing other arch scripts resulted in this error:
> 
> tools/perf/arch/arm64/entry/syscalls//mksyscalltbl: 25: printf: __NR3264_ftruncate: expected numeric value
> 
> because, unlike other arches, asm-generic's unistd.h does things like:
> 
>  #define __NR_ftruncate __NR3264_ftruncate
> 
> Turning the scripts printf's %d into a %s resulted in this in the
> generated syscalls.c file:
> 
>     static const char *syscalltbl_arm64[] = {
>             [__NR3264_ftruncate] = "ftruncate",
> 
> So we use the host C compiler to fold the macros, and print them out
> from within a temporary C program, in order to get the correct output:
> 
>     static const char *syscalltbl_arm64[] = {
>             [46] = "ftruncate",
> 
> Cc: Ravi Bangoria <ravi.bangoria@linux.vnet.ibm.com>
> Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
> Cc: Hendrik Brueckner <brueckner@linux.vnet.ibm.com>
> Cc: Jiri Olsa <jolsa@redhat.com>
> Cc: Michael Ellerman <mpe@ellerman.id.au>
> Cc: Namhyung Kim <namhyung@kernel.org>
> Cc: Thomas Richter <tmricht@linux.vnet.ibm.com>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Ingo Molnar <mingo@redhat.com>
> Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
> Signed-off-by: Kim Phillips <kim.phillips@arm.com>
> ---
> Changes in V2:
>  - added the "sc" and "nr" local variables, for the sake of completion (Hendrik
>    Brueckner)
>  - Removed the unsafe -u from mktemp, added more X's, and now pipe the
>    C code to the $hostcc command, so we now only require a single temp
>    file that is written by gcc. (Hendrik Brueckner)
>  - used cat << EoHEADER instead of echos for the c file header, to make it more
>    readable (Hendrik Brueckner)
>  - $RM -> rm, to be able to run standalone (Hendrik Brueckner)

Many thanks for incorporating the feedback!

>  - moved the rm into the function that makes the file
>  - converted other echos to printf "%s\n" for more portable printing of
>    backslash characters (bash and dash built-in echo commands differ in
>    how many escape backslashes are needed).  The initial "%s\n" is needed
>    in order to keep the external printf command from interpreting the %ds.
> 
>  tools/perf/arch/arm64/Makefile                | 21 +++++++
>  .../arch/arm64/entry/syscalls/mksyscalltbl    | 61 +++++++++++++++++++
>  2 files changed, 82 insertions(+)
>  create mode 100755 tools/perf/arch/arm64/entry/syscalls/mksyscalltbl
> 
> diff --git a/tools/perf/arch/arm64/Makefile b/tools/perf/arch/arm64/Makefile
> index 91de4860faad..85fdf4949db3 100644
> --- a/tools/perf/arch/arm64/Makefile
> +++ b/tools/perf/arch/arm64/Makefile
> @@ -4,3 +4,24 @@ PERF_HAVE_DWARF_REGS := 1
>  endif
>  PERF_HAVE_JITDUMP := 1
>  PERF_HAVE_ARCH_REGS_QUERY_REGISTER_OFFSET := 1
> +
> +#
> +# Syscall table generation for perf
> +#
> +
> +out    := $(OUTPUT)arch/arm64/include/generated/asm
> +header := $(out)/syscalls.c
> +sysdef := $(srctree)/tools/arch/arm64/include/uapi/asm/unistd.h
> +sysprf := $(srctree)/tools/perf/arch/arm64/entry/syscalls/
> +systbl := $(sysprf)/mksyscalltbl
> +
> +# Create output directory if not already present
> +_dummy := $(shell [ -d '$(out)' ] || mkdir -p '$(out)')
> +
> +$(header): $(sysdef) $(systbl)
> +	$(Q)$(SHELL) '$(systbl)' '$(CC)' '$(HOSTCC)' $(sysdef) > $@
> +
> +clean::
> +	$(call QUIET_CLEAN, arm64) $(RM) $(header)
> +
> +archheaders: $(header)
> diff --git a/tools/perf/arch/arm64/entry/syscalls/mksyscalltbl b/tools/perf/arch/arm64/entry/syscalls/mksyscalltbl
> new file mode 100755
> index 000000000000..f806542eb3cc
> --- /dev/null
> +++ b/tools/perf/arch/arm64/entry/syscalls/mksyscalltbl
> @@ -0,0 +1,61 @@
> +#!/bin/sh
> +# SPDX-License-Identifier: GPL-2.0
> +#
> +# Generate system call table for perf. Derived from
> +# powerpc script.
> +#
> +# Copyright IBM Corp. 2017
> +# Author(s):  Hendrik Brueckner <brueckner@linux.vnet.ibm.com>
> +# Changed by: Ravi Bangoria <ravi.bangoria@linux.vnet.ibm.com>
> +# Changed by: Kim Phillips <kim.phillips@arm.com>
> +
> +gcc=$1
> +hostcc=$2
> +input=$3
> +
> +if ! test -r $input; then
> +	echo "Could not read input file" >&2
> +	exit 1
> +fi
> +
> +create_table_from_c()
> +{
> +	local sc nr last_sc
> +
> +	create_table_exe=`mktemp /tmp/create-table-XXXXXX`
> +
> +	{
> +
> +cat << _EoHEADER
> +#include <stdio.h>
> +#include "$input"
> +int main(int argc, char *argv[])
> +{
> +_EoHEADER

One minor style hint: Using cat <<-_EoHEADER (with a dash) strips any leading
tabs from the what is being concatenated.  With this, using the same indent as
for as the other function statements it possible.  For example,

	cat <<-_EoHEADER
	#include <stdio.h>
	#include "$input"
	int main(int argc, char *argv[])
	{
	_EoHEADER

> +
> +	while read sc nr; do
> +		printf "%s\n" "	printf(\"\\t[%d] = \\\"$sc\\\",\\n\", __NR_$sc);"
> +		last_sc=$sc
> +	done
> +
> +	printf "%s\n" "	printf(\"#define SYSCALLTBL_ARM64_MAX_ID %d\\n\", __NR_$last_sc);"
> +	printf "}\n"
> +
> +	} | $hostcc -o $create_table_exe -x c -
> +
> +	$create_table_exe
> +
> +	rm -f $create_table_exe
> +}
> +
> +create_table()
> +{
> +	echo "static const char *syscalltbl_arm64[] = {"
> +	create_table_from_c
> +	echo "};"
> +}
> +
> +$gcc -E -dM -x c  $input	       \
> +	|sed -ne 's/^#define __NR_//p' \
> +	|sort -t' ' -k2 -nu	       \
> +	|create_table

Overall, this looks good to me.

Reviewed-by: Hendrik Brueckner <brueckner@linux.ibm.com>