Re: [PATCH 5/7] perf test: test_intel_pt.sh: Add jitdump test

[Arnaldo Carvalho de Melo]

Em Mon, Oct 17, 2022 at 03:57:03PM +0300, Adrian Hunter escreveu:
> On 14/10/22 20:28, Arnaldo Carvalho de Melo wrote:
> > Em Fri, Oct 14, 2022 at 08:09:03PM +0300, Adrian Hunter escreveu:
> >> Add a test for decoding self-modifying code using a jitdump file.
> >>
> >> The test creates a workload that uses self-modifying code and generates its
> >> own jitdump file.  The result is processed with perf inject --jit and
> >> checked for decoding errors.
> >>
> >> Note the test will fail without patch "perf inject: Fix GEN_ELF_TEXT_OFFSET
> >> for jit" applied.
> >>
> >> Signed-off-by: Adrian Hunter <adrian.hunter@xxxxxxxxx>
> >> ---
> >>  tools/perf/tests/shell/test_intel_pt.sh | 162 ++++++++++++++++++++++++
> >>  1 file changed, 162 insertions(+)
> >>
> >> diff --git a/tools/perf/tests/shell/test_intel_pt.sh b/tools/perf/tests/shell/test_intel_pt.sh
> >> index 79dde57b561d..e0bf75981b9c 100755
> >> --- a/tools/perf/tests/shell/test_intel_pt.sh
> >> +++ b/tools/perf/tests/shell/test_intel_pt.sh
> >> @@ -22,6 +22,7 @@ outfile="${temp_dir}/test-out.txt"
> >>  errfile="${temp_dir}/test-err.txt"
> >>  workload="${temp_dir}/workload"
> >>  awkscript="${temp_dir}/awkscript"
> >> +jitdump_workload="${temp_dir}/jitdump_workload"
> >>  
> >>  cleanup()
> >>  {
> >> @@ -50,6 +51,13 @@ perf_record_no_decode()
> >>  	perf record -B -N --no-bpf-event "$@"
> >>  }
> >>  
> >> +# perf record for testing should not need BPF events
> >> +perf_record_no_bpf()
> >> +{
> >> +	# Options for no BPF events
> >> +	perf record --no-bpf-event "$@"
> >> +}
> >> +
> >>  have_workload=false
> >>  cat << _end_of_file_ | /usr/bin/cc -o "${workload}" -xc - -pthread && have_workload=true
> >>  #include <time.h>
> >> @@ -269,6 +277,159 @@ test_per_thread()
> >>  	return 0
> >>  }
> >>  
> >> +test_jitdump()
> >> +{
> >> +	echo "--- Test tracing self-modifying code that uses jitdump ---"
> >> +
> >> +	script_path=$(realpath "$0")
> >> +	script_dir=$(dirname "$script_path")
> >> +	jitdump_incl_dir="${script_dir}/../../util"
> >> +	jitdump_h="${jitdump_incl_dir}/jitdump.h"
> > 
> > So this requires one to test this being on the kernel (perf) sources
> > dir? I think we should add this header to some 'perf test' directory to
> > remove this requirement, ok?
> > 
> 
> How about this:

Better, but see below
 
> From: Adrian Hunter <adrian.hunter@xxxxxxxxx>
> Date: Mon, 17 Oct 2022 15:14:25 +0300
> Subject: [PATCH] perf test: test_intel_pt.sh: Install jitdump.h for use by
>  tests
> 
> test_intel_pt.sh builds a workload for testing jitdump and the workload
> includes file jitdump.h.
> 
> Currently, test_intel_pt.sh finds jitdump.h assuming the test is run in
> the kernel source directory, and skips that test if it is not found.
> 
> To improve that situation, amend the build to install the jitdump.h file
> in the test shell directory, and look there first, falling back to the
> original way if that is not found.
> 
> Signed-off-by: Adrian Hunter <adrian.hunter@xxxxxxxxx>
> ---
>  tools/perf/Makefile.perf                |  1 +
>  tools/perf/tests/shell/test_intel_pt.sh | 12 +++++++++---
>  2 files changed, 10 insertions(+), 3 deletions(-)
> 
> diff --git a/tools/perf/Makefile.perf b/tools/perf/Makefile.perf
> index a432e59afc42..c4ec66194465 100644
> --- a/tools/perf/Makefile.perf
> +++ b/tools/perf/Makefile.perf
> @@ -1013,6 +1013,7 @@ install-tests: all install-gtk
>  		$(INSTALL) tests/attr/* -m 644 '$(DESTDIR_SQ)$(perfexec_instdir_SQ)/tests/attr'; \
>  		$(INSTALL) -d -m 755 '$(DESTDIR_SQ)$(perfexec_instdir_SQ)/tests/shell'; \
>  		$(INSTALL) tests/shell/*.sh '$(DESTDIR_SQ)$(perfexec_instdir_SQ)/tests/shell'; \
> +		$(INSTALL) util/jitdump.h -m 644 '$(DESTDIR_SQ)$(perfexec_instdir_SQ)/tests/shell'; \
>  		$(INSTALL) -d -m 755 '$(DESTDIR_SQ)$(perfexec_instdir_SQ)/tests/shell/lib'; \
>  		$(INSTALL) tests/shell/lib/*.sh -m 644 '$(DESTDIR_SQ)$(perfexec_instdir_SQ)/tests/shell/lib'; \
>  		$(INSTALL) tests/shell/lib/*.py -m 644 '$(DESTDIR_SQ)$(perfexec_instdir_SQ)/tests/shell/lib'; \
> diff --git a/tools/perf/tests/shell/test_intel_pt.sh b/tools/perf/tests/shell/test_intel_pt.sh
> index 4c0aabbe33bd..3abf803f96b9 100755
> --- a/tools/perf/tests/shell/test_intel_pt.sh
> +++ b/tools/perf/tests/shell/test_intel_pt.sh
> @@ -284,14 +284,20 @@ test_jitdump()
>  
>  	script_path=$(realpath "$0")
>  	script_dir=$(dirname "$script_path")
> -	jitdump_incl_dir="${script_dir}/../../util"
> +	jitdump_incl_dir="${script_dir}"
>  	jitdump_h="${jitdump_incl_dir}/jitdump.h"
>  
>  	if [ ! -e "${jitdump_h}" ] ; then
> -		echo "SKIP: Include file jitdump.h not found"
> -		return 2
> +		jitdump_incl_dir="${script_dir}/../../util"
> +		jitdump_h="${jitdump_incl_dir}/jitdump.h"
> +		if [ ! -e "${jitdump_h}" ] ; then
> +			echo "SKIP: Include file jitdump.h not found"
> +			return 2
> +		fi
>  	fi
>  
> +	echo "Using include file: ${jitdump_h}"

Shouldn't this appear only with -v?

- Arnaldo

> +
>  	if [ -z "${have_jitdump_workload}" ] ; then
>  		have_jitdump_workload=false
>  		# Create a workload that uses self-modifying code and generates its own jitdump file