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

[Adrian Hunter]

On 17/10/22 16:44, Arnaldo Carvalho de Melo wrote:
> 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?

Yes, but shell scripts do not process -v.  Instead all output is
suppressed by perf test unless -v is used.  So it should be OK?

> 
> - 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