[PATCH 1/2] tools/perf/tests: Fix string substitutions in build id test

[PATCH 1/2] tools/perf/tests: Fix string substitutions in build id test

[Athira Rajeev]

The perf test named “build id cache operations” skips with below
error on some distros:

<<>>
 78: build id cache operations                                       :
test child forked, pid 111101
WARNING: wine not found. PE binaries will not be run.
test binaries: /tmp/perf.ex.SHA1.PKz /tmp/perf.ex.MD5.Gt3 ./tests/shell/../pe-file.exe
DEBUGINFOD_URLS=
Adding 4abd406f041feb4f10ecde3fc30fd0639e1a91cb /tmp/perf.ex.SHA1.PKz: Ok
build id: 4abd406f041feb4f10ecde3fc30fd0639e1a91cb
./tests/shell/buildid.sh: 69: ./tests/shell/buildid.sh: Bad substitution
test child finished with -2
build id cache operations: Skip
<<>>

The test script "tests/shell/buildid.sh" uses some of the
string substitution ways which are supported in bash, but not in
"sh" or other shells. Above error on line number 69 that reports
"Bad substitution" is:

<<>>
link=${build_id_dir}/.build-id/${id:0:2}/${id:2}
<<>>

Here the way of getting first two characters from id ie,
${id:0:2} and similarly expressions like ${id:2} is not
recognised in "sh". So the line errors and instead of
hitting failure, the test gets skipped as shown in logs.
So the syntax issue causes test not to be executed in
such cases. Similarly usage : "${@: -1}" [ to pick last
argument passed to a function] in “test_record” doesn’t
work in all distros.

Fix this by using alternative way with "cut" command
to pick "n" characters from the string. Also fix the usage
of “${@: -1}” to work in all cases.

Another usage in “test_record” is:
<<>>
${perf} record --buildid-all -o ${data} $@ &> ${log}
<<>>

This causes the perf record to start in background and
Results in the data file not being created by the time
"check" function is invoked. Below log shows perf record
result getting displayed after the call to "check" function.

<<>>
running: perf record /tmp/perf.ex.SHA1.EAU
build id: 4abd406f041feb4f10ecde3fc30fd0639e1a91cb
link: /tmp/perf.debug.mLT/.build-id/4a/bd406f041feb4f10ecde3fc30fd0639e1a91cb
failed: link /tmp/perf.debug.mLT/.build-id/4a/bd406f041feb4f10ecde3fc30fd0639e1a91cb does not exist
test child finished with -1
build id cache operations: FAILED!
root@machine:~/athira/linux/tools/perf# Couldn't synthesize bpf events.
[ perf record: Woken up 1 times to write data ]
[ perf record: Captured and wrote 0.010 MB /tmp/perf.data.bFF ]
<<>>

Fix this by redirecting output instead of using “&” which
starts the command in background.

Signed-off-by: Athira Rajeev <atrajeev@linux.vnet.ibm.com>
---
 tools/perf/tests/shell/buildid.sh | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/tools/perf/tests/shell/buildid.sh b/tools/perf/tests/shell/buildid.sh
index f05670d1e39e..3512c4423d48 100755
--- a/tools/perf/tests/shell/buildid.sh
+++ b/tools/perf/tests/shell/buildid.sh
@@ -66,7 +66,7 @@ check()
 	esac
 	echo "build id: ${id}"
 
-	link=${build_id_dir}/.build-id/${id:0:2}/${id:2}
+	link=${build_id_dir}/.build-id/$(echo ${id}|cut -c 1-2)/$(echo ${id}|cut -c 3-)
 	echo "link: ${link}"
 
 	if [ ! -h $link ]; then
@@ -74,7 +74,7 @@ check()
 		exit 1
 	fi
 
-	file=${build_id_dir}/.build-id/${id:0:2}/`readlink ${link}`/elf
+	file=${build_id_dir}/.build-id/$(echo ${id}|cut -c 1-2)/`readlink ${link}`/elf
 	echo "file: ${file}"
 
 	if [ ! -x $file ]; then
@@ -117,20 +117,22 @@ test_record()
 {
 	data=$(mktemp /tmp/perf.data.XXX)
 	build_id_dir=$(mktemp -d /tmp/perf.debug.XXX)
-	log=$(mktemp /tmp/perf.log.XXX)
+	log_out=$(mktemp /tmp/perf.log.out.XXX)
+	log_err=$(mktemp /tmp/perf.log.err.XXX)
 	perf="perf --buildid-dir ${build_id_dir}"
+	eval last=\${$#}
 
 	echo "running: perf record $@"
-	${perf} record --buildid-all -o ${data} $@ &> ${log}
+	${perf} record --buildid-all -o ${data} $@ 1>${log_out} 2>${log_err}
 	if [ $? -ne 0 ]; then
 		echo "failed: record $@"
-		echo "see log: ${log}"
+		echo "see log: ${log_err}"
 		exit 1
 	fi
 
-	check ${@: -1}
+	check $last
 
-	rm -f ${log}
+	rm -f ${log_out} ${log_err}
 	rm -rf ${build_id_dir}
 	rm -rf ${data}
 }
-- 
2.17.1

[PATCH 2/2] tools/perf/tests: Fix build id test check for PE file

[Athira Rajeev]

Perf test "build id cache operations" fails for PE
executable.  Logs below from powerpc system.
Same is observed on x86 as well.

<<>>
Adding 5a0fd882b53084224ba47b624c55a469 ./tests/shell/../pe-file.exe: Ok
build id: 5a0fd882b53084224ba47b624c55a469
link: /tmp/perf.debug.w0V/.build-id/5a/0fd882b53084224ba47b624c55a469
file: /tmp/perf.debug.w0V/.build-id/5a/../../root/athira/linux/tools/perf/tests/pe-file.exe/5a0fd882b53084224ba47b624c55a469/elf
failed: file /tmp/perf.debug.w0V/.build-id/5a/../../root/athira/linux/tools/perf/tests/pe-file.exe/5a0fd882b53084224ba47b624c55a469/elf does not exist
test child finished with -1
---- end ----
build id cache operations: FAILED!
<<>>

The test tries to do:

<<>>
mkdir /tmp/perf.debug.TeY1
perf --buildid-dir /tmp/perf.debug.TeY1 buildid-cache -v -a ./tests/shell/../pe-file.exe
<<>>

The option "--buildid-dir" sets the build id cache
directory as /tmp/perf.debug.TeY1. The option given
to buildid-cahe, ie "-a ./tests/shell/../pe-file.exe",
is to add the pe-file.exe to the cache. The testcase,
sets buildid-dir and adds the file: pe-file.exe to build
id cache. To check if the command is run successfully,
"check" function looks for presence of the file in buildid
cache directory. But the check here expects the added
file to be executable. Snippet below:

<<>>
if [ ! -x $file ]; then
	echo "failed: file ${file} does not exist"
	exit 1
fi
<<>>

Buildid test is done for sha1 binary, md5 binary and also
for PE file. The firt two binaries are created at runtime
by compiling with "--build-id" option and hence the check
for sha1/md5 test should use [ ! -x ]. But in case of PE
file, the permission for this input file is rw-r--r--
Hence the file added to build id cache has same permissoin

Original file:
ls tests/pe-file.exe | xargs stat --printf "%n %A \n"
tests/pe-file.exe -rw-r--r--

buildid cache file:

ls /tmp/perf.debug.w0V/.build-id/5a/../../root/athira/linux/tools/perf/tests/pe-file.exe/5a0fd882b53084224ba47b624c55a469/elf | xargs stat --printf "%n %A \n"
/tmp/perf.debug.w0V/.build-id/5a/../../root/athira/linux/tools/perf/tests/pe-file.exe/5a0fd882b53084224ba47b624c55a469/elf -rw-r--r--

Fix the test to match with the permission of original file in
case of FE file. ie if the "tests/pe-file.exe" file is not
having exec permission, just check for existence of the buildid
file using [ ! -e <file> ]

Signed-off-by: Athira Rajeev <atrajeev@linux.vnet.ibm.com>
---
 tools/perf/tests/shell/buildid.sh | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/tools/perf/tests/shell/buildid.sh b/tools/perf/tests/shell/buildid.sh
index 3512c4423d48..75f5117c3417 100755
--- a/tools/perf/tests/shell/buildid.sh
+++ b/tools/perf/tests/shell/buildid.sh
@@ -77,7 +77,20 @@ check()
 	file=${build_id_dir}/.build-id/$(echo ${id}|cut -c 1-2)/`readlink ${link}`/elf
 	echo "file: ${file}"
 
-	if [ ! -x $file ]; then
+	# Check for file permission of orginal file
+	# in case of pe-file.exe file
+	echo $1 | grep ".exe"
+	if [ $? -eq 0 ]; then
+		if [ -x $1  -a ! -x $file ]; then
+			echo "failed: file ${file} executable does not exist"
+			exit 1
+		fi
+
+		if [ ! -x $file -a ! -e $file ]; then
+			echo "failed: file ${file} does not exist"
+			exit 1
+		fi
+	elif [ ! -x $file ]; then
 		echo "failed: file ${file} does not exist"
 		exit 1
 	fi
-- 
2.17.1

Re: [PATCH 1/2] tools/perf/tests: Fix string substitutions in build id test

[Arnaldo Carvalho de Melo]

Em Wed, Sep 21, 2022 at 10:38:38PM +0530, Athira Rajeev escreveu:
> The perf test named “build id cache operations” skips with below
> error on some distros:

I wonder if we shouldn't instead state that bash is needed?

⬢[acme@toolbox perf-urgent]$ head -1 tools/perf/tests/shell/*.sh | grep ^#
#!/bin/sh
#!/bin/bash
#!/bin/sh
#!/bin/sh
#!/bin/sh
#!/bin/sh
#!/bin/sh
#!/bin/sh
#!/bin/sh
#!/bin/sh
#!/bin/bash
#!/bin/sh
#!/bin/sh
#!/bin/sh
#!/bin/bash
#!/bin/sh
#!/bin/bash
#!/bin/sh
#!/bin/sh
#!/bin/sh
#!/bin/sh
#!/bin/sh
#!/bin/sh
#!/bin/sh
#!/bin/sh
#!/bin/sh
⬢[acme@toolbox perf-urgent]$

Opinions?

- Arnaldo
 
> <<>>
>  78: build id cache operations                                       :
> test child forked, pid 111101
> WARNING: wine not found. PE binaries will not be run.
> test binaries: /tmp/perf.ex.SHA1.PKz /tmp/perf.ex.MD5.Gt3 ./tests/shell/../pe-file.exe
> DEBUGINFOD_URLS=
> Adding 4abd406f041feb4f10ecde3fc30fd0639e1a91cb /tmp/perf.ex.SHA1.PKz: Ok
> build id: 4abd406f041feb4f10ecde3fc30fd0639e1a91cb
> ./tests/shell/buildid.sh: 69: ./tests/shell/buildid.sh: Bad substitution
> test child finished with -2
> build id cache operations: Skip
> <<>>
> 
> The test script "tests/shell/buildid.sh" uses some of the
> string substitution ways which are supported in bash, but not in
> "sh" or other shells. Above error on line number 69 that reports
> "Bad substitution" is:
> 
> <<>>
> link=${build_id_dir}/.build-id/${id:0:2}/${id:2}
> <<>>
> 
> Here the way of getting first two characters from id ie,
> ${id:0:2} and similarly expressions like ${id:2} is not
> recognised in "sh". So the line errors and instead of
> hitting failure, the test gets skipped as shown in logs.
> So the syntax issue causes test not to be executed in
> such cases. Similarly usage : "${@: -1}" [ to pick last
> argument passed to a function] in “test_record” doesn’t
> work in all distros.
> 
> Fix this by using alternative way with "cut" command
> to pick "n" characters from the string. Also fix the usage
> of “${@: -1}” to work in all cases.
> 
> Another usage in “test_record” is:
> <<>>
> ${perf} record --buildid-all -o ${data} $@ &> ${log}
> <<>>
> 
> This causes the perf record to start in background and
> Results in the data file not being created by the time
> "check" function is invoked. Below log shows perf record
> result getting displayed after the call to "check" function.
> 
> <<>>
> running: perf record /tmp/perf.ex.SHA1.EAU
> build id: 4abd406f041feb4f10ecde3fc30fd0639e1a91cb
> link: /tmp/perf.debug.mLT/.build-id/4a/bd406f041feb4f10ecde3fc30fd0639e1a91cb
> failed: link /tmp/perf.debug.mLT/.build-id/4a/bd406f041feb4f10ecde3fc30fd0639e1a91cb does not exist
> test child finished with -1
> build id cache operations: FAILED!
> root@machine:~/athira/linux/tools/perf# Couldn't synthesize bpf events.
> [ perf record: Woken up 1 times to write data ]
> [ perf record: Captured and wrote 0.010 MB /tmp/perf.data.bFF ]
> <<>>
> 
> Fix this by redirecting output instead of using “&” which
> starts the command in background.
> 
> Signed-off-by: Athira Rajeev <atrajeev@linux.vnet.ibm.com>
> ---
>  tools/perf/tests/shell/buildid.sh | 16 +++++++++-------
>  1 file changed, 9 insertions(+), 7 deletions(-)
> 
> diff --git a/tools/perf/tests/shell/buildid.sh b/tools/perf/tests/shell/buildid.sh
> index f05670d1e39e..3512c4423d48 100755
> --- a/tools/perf/tests/shell/buildid.sh
> +++ b/tools/perf/tests/shell/buildid.sh
> @@ -66,7 +66,7 @@ check()
>  	esac
>  	echo "build id: ${id}"
>  
> -	link=${build_id_dir}/.build-id/${id:0:2}/${id:2}
> +	link=${build_id_dir}/.build-id/$(echo ${id}|cut -c 1-2)/$(echo ${id}|cut -c 3-)
>  	echo "link: ${link}"
>  
>  	if [ ! -h $link ]; then
> @@ -74,7 +74,7 @@ check()
>  		exit 1
>  	fi
>  
> -	file=${build_id_dir}/.build-id/${id:0:2}/`readlink ${link}`/elf
> +	file=${build_id_dir}/.build-id/$(echo ${id}|cut -c 1-2)/`readlink ${link}`/elf
>  	echo "file: ${file}"
>  
>  	if [ ! -x $file ]; then
> @@ -117,20 +117,22 @@ test_record()
>  {
>  	data=$(mktemp /tmp/perf.data.XXX)
>  	build_id_dir=$(mktemp -d /tmp/perf.debug.XXX)
> -	log=$(mktemp /tmp/perf.log.XXX)
> +	log_out=$(mktemp /tmp/perf.log.out.XXX)
> +	log_err=$(mktemp /tmp/perf.log.err.XXX)
>  	perf="perf --buildid-dir ${build_id_dir}"
> +	eval last=\${$#}
>  
>  	echo "running: perf record $@"
> -	${perf} record --buildid-all -o ${data} $@ &> ${log}
> +	${perf} record --buildid-all -o ${data} $@ 1>${log_out} 2>${log_err}
>  	if [ $? -ne 0 ]; then
>  		echo "failed: record $@"
> -		echo "see log: ${log}"
> +		echo "see log: ${log_err}"
>  		exit 1
>  	fi
>  
> -	check ${@: -1}
> +	check $last
>  
> -	rm -f ${log}
> +	rm -f ${log_out} ${log_err}
>  	rm -rf ${build_id_dir}
>  	rm -rf ${data}
>  }
> -- 
> 2.17.1

-- 

- Arnaldo

Re: [PATCH 1/2] tools/perf/tests: Fix string substitutions in build id test

[Ian Rogers]

On Thu, Sep 22, 2022 at 12:15 PM Arnaldo Carvalho de Melo
<acme@kernel.org> wrote:
>
> Em Wed, Sep 21, 2022 at 10:38:38PM +0530, Athira Rajeev escreveu:
> > The perf test named “build id cache operations” skips with below
> > error on some distros:
>
> I wonder if we shouldn't instead state that bash is needed?
>
> ⬢[acme@toolbox perf-urgent]$ head -1 tools/perf/tests/shell/*.sh | grep ^#
> #!/bin/sh
> #!/bin/bash
> #!/bin/sh
> #!/bin/sh
> #!/bin/sh
> #!/bin/sh
> #!/bin/sh
> #!/bin/sh
> #!/bin/sh
> #!/bin/sh
> #!/bin/bash
> #!/bin/sh
> #!/bin/sh
> #!/bin/sh
> #!/bin/bash
> #!/bin/sh
> #!/bin/bash
> #!/bin/sh
> #!/bin/sh
> #!/bin/sh
> #!/bin/sh
> #!/bin/sh
> #!/bin/sh
> #!/bin/sh
> #!/bin/sh
> #!/bin/sh
> ⬢[acme@toolbox perf-urgent]$
>
> Opinions?

+1 to bash. Perhaps python longer term?  The XML test output generated
by things like kunit is possible to generate from either bash or
python, but in my experience the python stuff feels better built.

Thanks,
Ian

> - Arnaldo
>
> > <<>>
> >  78: build id cache operations                                       :
> > test child forked, pid 111101
> > WARNING: wine not found. PE binaries will not be run.
> > test binaries: /tmp/perf.ex.SHA1.PKz /tmp/perf.ex.MD5.Gt3 ./tests/shell/../pe-file.exe
> > DEBUGINFOD_URLS=
> > Adding 4abd406f041feb4f10ecde3fc30fd0639e1a91cb /tmp/perf.ex.SHA1.PKz: Ok
> > build id: 4abd406f041feb4f10ecde3fc30fd0639e1a91cb
> > ./tests/shell/buildid.sh: 69: ./tests/shell/buildid.sh: Bad substitution
> > test child finished with -2
> > build id cache operations: Skip
> > <<>>
> >
> > The test script "tests/shell/buildid.sh" uses some of the
> > string substitution ways which are supported in bash, but not in
> > "sh" or other shells. Above error on line number 69 that reports
> > "Bad substitution" is:
> >
> > <<>>
> > link=${build_id_dir}/.build-id/${id:0:2}/${id:2}
> > <<>>
> >
> > Here the way of getting first two characters from id ie,
> > ${id:0:2} and similarly expressions like ${id:2} is not
> > recognised in "sh". So the line errors and instead of
> > hitting failure, the test gets skipped as shown in logs.
> > So the syntax issue causes test not to be executed in
> > such cases. Similarly usage : "${@: -1}" [ to pick last
> > argument passed to a function] in “test_record” doesn’t
> > work in all distros.
> >
> > Fix this by using alternative way with "cut" command
> > to pick "n" characters from the string. Also fix the usage
> > of “${@: -1}” to work in all cases.
> >
> > Another usage in “test_record” is:
> > <<>>
> > ${perf} record --buildid-all -o ${data} $@ &> ${log}
> > <<>>
> >
> > This causes the perf record to start in background and
> > Results in the data file not being created by the time
> > "check" function is invoked. Below log shows perf record
> > result getting displayed after the call to "check" function.
> >
> > <<>>
> > running: perf record /tmp/perf.ex.SHA1.EAU
> > build id: 4abd406f041feb4f10ecde3fc30fd0639e1a91cb
> > link: /tmp/perf.debug.mLT/.build-id/4a/bd406f041feb4f10ecde3fc30fd0639e1a91cb
> > failed: link /tmp/perf.debug.mLT/.build-id/4a/bd406f041feb4f10ecde3fc30fd0639e1a91cb does not exist
> > test child finished with -1
> > build id cache operations: FAILED!
> > root@machine:~/athira/linux/tools/perf# Couldn't synthesize bpf events.
> > [ perf record: Woken up 1 times to write data ]
> > [ perf record: Captured and wrote 0.010 MB /tmp/perf.data.bFF ]
> > <<>>
> >
> > Fix this by redirecting output instead of using “&” which
> > starts the command in background.
> >
> > Signed-off-by: Athira Rajeev <atrajeev@linux.vnet.ibm.com>
> > ---
> >  tools/perf/tests/shell/buildid.sh | 16 +++++++++-------
> >  1 file changed, 9 insertions(+), 7 deletions(-)
> >
> > diff --git a/tools/perf/tests/shell/buildid.sh b/tools/perf/tests/shell/buildid.sh
> > index f05670d1e39e..3512c4423d48 100755
> > --- a/tools/perf/tests/shell/buildid.sh
> > +++ b/tools/perf/tests/shell/buildid.sh
> > @@ -66,7 +66,7 @@ check()
> >       esac
> >       echo "build id: ${id}"
> >
> > -     link=${build_id_dir}/.build-id/${id:0:2}/${id:2}
> > +     link=${build_id_dir}/.build-id/$(echo ${id}|cut -c 1-2)/$(echo ${id}|cut -c 3-)
> >       echo "link: ${link}"
> >
> >       if [ ! -h $link ]; then
> > @@ -74,7 +74,7 @@ check()
> >               exit 1
> >       fi
> >
> > -     file=${build_id_dir}/.build-id/${id:0:2}/`readlink ${link}`/elf
> > +     file=${build_id_dir}/.build-id/$(echo ${id}|cut -c 1-2)/`readlink ${link}`/elf
> >       echo "file: ${file}"
> >
> >       if [ ! -x $file ]; then
> > @@ -117,20 +117,22 @@ test_record()
> >  {
> >       data=$(mktemp /tmp/perf.data.XXX)
> >       build_id_dir=$(mktemp -d /tmp/perf.debug.XXX)
> > -     log=$(mktemp /tmp/perf.log.XXX)
> > +     log_out=$(mktemp /tmp/perf.log.out.XXX)
> > +     log_err=$(mktemp /tmp/perf.log.err.XXX)
> >       perf="perf --buildid-dir ${build_id_dir}"
> > +     eval last=\${$#}
> >
> >       echo "running: perf record $@"
> > -     ${perf} record --buildid-all -o ${data} $@ &> ${log}
> > +     ${perf} record --buildid-all -o ${data} $@ 1>${log_out} 2>${log_err}
> >       if [ $? -ne 0 ]; then
> >               echo "failed: record $@"
> > -             echo "see log: ${log}"
> > +             echo "see log: ${log_err}"
> >               exit 1
> >       fi
> >
> > -     check ${@: -1}
> > +     check $last
> >
> > -     rm -f ${log}
> > +     rm -f ${log_out} ${log_err}
> >       rm -rf ${build_id_dir}
> >       rm -rf ${data}
> >  }
> > --
> > 2.17.1
>
> --
>
> - Arnaldo

Re: [PATCH 1/2] tools/perf/tests: Fix string substitutions in build id test

[Adrian Hunter]

On 22/09/22 22:15, Arnaldo Carvalho de Melo wrote:
> Em Wed, Sep 21, 2022 at 10:38:38PM +0530, Athira Rajeev escreveu:
>> The perf test named “build id cache operations” skips with below
>> error on some distros:
> 
> I wonder if we shouldn't instead state that bash is needed?
> 
> ⬢[acme@toolbox perf-urgent]$ head -1 tools/perf/tests/shell/*.sh | grep ^#
> #!/bin/sh
> #!/bin/bash
> #!/bin/sh
> #!/bin/sh
> #!/bin/sh
> #!/bin/sh
> #!/bin/sh
> #!/bin/sh
> #!/bin/sh
> #!/bin/sh
> #!/bin/bash
> #!/bin/sh
> #!/bin/sh
> #!/bin/sh
> #!/bin/bash
> #!/bin/sh
> #!/bin/bash
> #!/bin/sh
> #!/bin/sh
> #!/bin/sh
> #!/bin/sh
> #!/bin/sh
> #!/bin/sh
> #!/bin/sh
> #!/bin/sh
> #!/bin/sh
> ⬢[acme@toolbox perf-urgent]$
> 
> Opinions?
> 

Please don't change tools/perf/tests/shell/test_intel_pt.sh

I started using shellcheck on that with the "perf test: 
test_intel_pt.sh: Add per-thread test" patch set that I sent.

FYI, if you use shellcheck on buildid.sh, it shows up issues even
after changing to bash:

*** Before ***

$ shellcheck -S warning tools/perf/tests/shell/buildid.sh 

In tools/perf/tests/shell/buildid.sh line 69:
        link=${build_id_dir}/.build-id/${id:0:2}/${id:2}
                                       ^-------^ SC2039: In POSIX sh, string indexing is undefined.
                                                 ^-----^ SC2039: In POSIX sh, string indexing is undefined.


In tools/perf/tests/shell/buildid.sh line 77:
        file=${build_id_dir}/.build-id/${id:0:2}/`readlink ${link}`/elf
                                       ^-------^ SC2039: In POSIX sh, string indexing is undefined.


In tools/perf/tests/shell/buildid.sh line 123:
        echo "running: perf record $@"
                                   ^-- SC2145: Argument mixes string and array. Use * or separate argument.


In tools/perf/tests/shell/buildid.sh line 124:
        ${perf} record --buildid-all -o ${data} $@ &> ${log}
                                                ^-- SC2068: Double quote array expansions to avoid re-splitting elements.
                                                   ^-------^ SC2039: In POSIX sh, &> is undefined.


In tools/perf/tests/shell/buildid.sh line 126:
                echo "failed: record $@"
                                     ^-- SC2145: Argument mixes string and array. Use * or separate argument.


In tools/perf/tests/shell/buildid.sh line 131:
        check ${@: -1}
              ^------^ SC2068: Double quote array expansions to avoid re-splitting elements.
              ^------^ SC2039: In POSIX sh, string indexing is undefined.


In tools/perf/tests/shell/buildid.sh line 158:
exit ${err}
     ^----^ SC2154: err is referenced but not assigned.

For more information:
  https://www.shellcheck.net/wiki/SC2068 -- Double quote array expansions to ...
  https://www.shellcheck.net/wiki/SC2145 -- Argument mixes string and array. ...
  https://www.shellcheck.net/wiki/SC2039 -- In POSIX sh, &> is undefined.

*** After ***

$ shellcheck -S warning tools/perf/tests/shell/buildid.sh 

In tools/perf/tests/shell/buildid.sh line 123:
        echo "running: perf record $@"
                                   ^-- SC2145: Argument mixes string and array. Use * or separate argument.


In tools/perf/tests/shell/buildid.sh line 124:
        ${perf} record --buildid-all -o ${data} $@ &> ${log}
                                                ^-- SC2068: Double quote array expansions to avoid re-splitting elements.


In tools/perf/tests/shell/buildid.sh line 126:
                echo "failed: record $@"
                                     ^-- SC2145: Argument mixes string and array. Use * or separate argument.


In tools/perf/tests/shell/buildid.sh line 131:
        check ${@: -1}
              ^------^ SC2068: Double quote array expansions to avoid re-splitting elements.


In tools/perf/tests/shell/buildid.sh line 158:
exit ${err}
     ^----^ SC2154: err is referenced but not assigned.

For more information:
  https://www.shellcheck.net/wiki/SC2068 -- Double quote array expansions to ...
  https://www.shellcheck.net/wiki/SC2145 -- Argument mixes string and array. ...
  https://www.shellcheck.net/wiki/SC2154 -- err is referenced but not assigned.

Re: [PATCH 1/2] tools/perf/tests: Fix string substitutions in build id test

[Athira Rajeev]

Hi All,

Looking for what direction we can take here.
Do we want to change all tests in tools/perf/tests/shell except test_intel_pt.sh to use "bash" or go with
the approach in the patch ? Please share your comments

Thanks
Athira

> On 23-Sep-2022, at 11:54 AM, Adrian Hunter <adrian.hunter@intel.com> wrote:
> 
> On 22/09/22 22:15, Arnaldo Carvalho de Melo wrote:
>> Em Wed, Sep 21, 2022 at 10:38:38PM +0530, Athira Rajeev escreveu:
>>> The perf test named “build id cache operations” skips with below
>>> error on some distros:
>> 
>> I wonder if we shouldn't instead state that bash is needed?
>> 
>> ⬢[acme@toolbox perf-urgent]$ head -1 tools/perf/tests/shell/*.sh | grep ^#
>> #!/bin/sh
>> #!/bin/bash
>> #!/bin/sh
>> #!/bin/sh
>> #!/bin/sh
>> #!/bin/sh
>> #!/bin/sh
>> #!/bin/sh
>> #!/bin/sh
>> #!/bin/sh
>> #!/bin/bash
>> #!/bin/sh
>> #!/bin/sh
>> #!/bin/sh
>> #!/bin/bash
>> #!/bin/sh
>> #!/bin/bash
>> #!/bin/sh
>> #!/bin/sh
>> #!/bin/sh
>> #!/bin/sh
>> #!/bin/sh
>> #!/bin/sh
>> #!/bin/sh
>> #!/bin/sh
>> #!/bin/sh
>> ⬢[acme@toolbox perf-urgent]$
>> 
>> Opinions?
>> 
> 
> Please don't change tools/perf/tests/shell/test_intel_pt.sh
> 
> I started using shellcheck on that with the "perf test: 
> test_intel_pt.sh: Add per-thread test" patch set that I sent.
> 
> FYI, if you use shellcheck on buildid.sh, it shows up issues even
> after changing to bash:
> 
> *** Before ***
> 
> $ shellcheck -S warning tools/perf/tests/shell/buildid.sh 
> 
> In tools/perf/tests/shell/buildid.sh line 69:
>        link=${build_id_dir}/.build-id/${id:0:2}/${id:2}
>                                       ^-------^ SC2039: In POSIX sh, string indexing is undefined.
>                                                 ^-----^ SC2039: In POSIX sh, string indexing is undefined.
> 
> 
> In tools/perf/tests/shell/buildid.sh line 77:
>        file=${build_id_dir}/.build-id/${id:0:2}/`readlink ${link}`/elf
>                                       ^-------^ SC2039: In POSIX sh, string indexing is undefined.
> 
> 
> In tools/perf/tests/shell/buildid.sh line 123:
>        echo "running: perf record $@"
>                                   ^-- SC2145: Argument mixes string and array. Use * or separate argument.
> 
> 
> In tools/perf/tests/shell/buildid.sh line 124:
>        ${perf} record --buildid-all -o ${data} $@ &> ${log}
>                                                ^-- SC2068: Double quote array expansions to avoid re-splitting elements.
>                                                   ^-------^ SC2039: In POSIX sh, &> is undefined.
> 
> 
> In tools/perf/tests/shell/buildid.sh line 126:
>                echo "failed: record $@"
>                                     ^-- SC2145: Argument mixes string and array. Use * or separate argument.
> 
> 
> In tools/perf/tests/shell/buildid.sh line 131:
>        check ${@: -1}
>              ^------^ SC2068: Double quote array expansions to avoid re-splitting elements.
>              ^------^ SC2039: In POSIX sh, string indexing is undefined.
> 
> 
> In tools/perf/tests/shell/buildid.sh line 158:
> exit ${err}
>     ^----^ SC2154: err is referenced but not assigned.
> 
> For more information:
>  https://www.shellcheck.net/wiki/SC2068 -- Double quote array expansions to ...
>  https://www.shellcheck.net/wiki/SC2145 -- Argument mixes string and array. ...
>  https://www.shellcheck.net/wiki/SC2039 -- In POSIX sh, &> is undefined.
> 
> *** After ***
> 
> $ shellcheck -S warning tools/perf/tests/shell/buildid.sh 
> 
> In tools/perf/tests/shell/buildid.sh line 123:
>        echo "running: perf record $@"
>                                   ^-- SC2145: Argument mixes string and array. Use * or separate argument.
> 
> 
> In tools/perf/tests/shell/buildid.sh line 124:
>        ${perf} record --buildid-all -o ${data} $@ &> ${log}
>                                                ^-- SC2068: Double quote array expansions to avoid re-splitting elements.
> 
> 
> In tools/perf/tests/shell/buildid.sh line 126:
>                echo "failed: record $@"
>                                     ^-- SC2145: Argument mixes string and array. Use * or separate argument.
> 
> 
> In tools/perf/tests/shell/buildid.sh line 131:
>        check ${@: -1}
>              ^------^ SC2068: Double quote array expansions to avoid re-splitting elements.
> 
> 
> In tools/perf/tests/shell/buildid.sh line 158:
> exit ${err}
>     ^----^ SC2154: err is referenced but not assigned.
> 
> For more information:
>  https://www.shellcheck.net/wiki/SC2068 -- Double quote array expansions to ...
>  https://www.shellcheck.net/wiki/SC2145 -- Argument mixes string and array. ...
>  https://www.shellcheck.net/wiki/SC2154 -- err is referenced but not assigned.

Re: [PATCH 1/2] tools/perf/tests: Fix string substitutions in build id test

[Athira Rajeev]

> On 28-Sep-2022, at 10:24 AM, Athira Rajeev <atrajeev@linux.vnet.ibm.com> wrote:
> 
> Hi All,
> 
> Looking for what direction we can take here.
> Do we want to change all tests in tools/perf/tests/shell except test_intel_pt.sh to use "bash" or go with
> the approach in the patch ? Please share your comments
> 
> Thanks
> Athira
> 

Hi All,

Looking for what direction we can take here.
Do we want to change all tests in tools/perf/tests/shell except test_intel_pt.sh to use "bash" or go with
the approach in the patch ? Please share your comments

Thanks
Athira

>> On 23-Sep-2022, at 11:54 AM, Adrian Hunter <adrian.hunter@intel.com> wrote:
>> 
>> On 22/09/22 22:15, Arnaldo Carvalho de Melo wrote:
>>> Em Wed, Sep 21, 2022 at 10:38:38PM +0530, Athira Rajeev escreveu:
>>>> The perf test named “build id cache operations” skips with below
>>>> error on some distros:
>>> 
>>> I wonder if we shouldn't instead state that bash is needed?
>>> 
>>> ⬢[acme@toolbox perf-urgent]$ head -1 tools/perf/tests/shell/*.sh | grep ^#
>>> #!/bin/sh
>>> #!/bin/bash
>>> #!/bin/sh
>>> #!/bin/sh
>>> #!/bin/sh
>>> #!/bin/sh
>>> #!/bin/sh
>>> #!/bin/sh
>>> #!/bin/sh
>>> #!/bin/sh
>>> #!/bin/bash
>>> #!/bin/sh
>>> #!/bin/sh
>>> #!/bin/sh
>>> #!/bin/bash
>>> #!/bin/sh
>>> #!/bin/bash
>>> #!/bin/sh
>>> #!/bin/sh
>>> #!/bin/sh
>>> #!/bin/sh
>>> #!/bin/sh
>>> #!/bin/sh
>>> #!/bin/sh
>>> #!/bin/sh
>>> #!/bin/sh
>>> ⬢[acme@toolbox perf-urgent]$
>>> 
>>> Opinions?
>>> 
>> 
>> Please don't change tools/perf/tests/shell/test_intel_pt.sh
>> 
>> I started using shellcheck on that with the "perf test: 
>> test_intel_pt.sh: Add per-thread test" patch set that I sent.
>> 
>> FYI, if you use shellcheck on buildid.sh, it shows up issues even
>> after changing to bash:
>> 
>> *** Before ***
>> 
>> $ shellcheck -S warning tools/perf/tests/shell/buildid.sh 
>> 
>> In tools/perf/tests/shell/buildid.sh line 69:
>>       link=${build_id_dir}/.build-id/${id:0:2}/${id:2}
>>                                      ^-------^ SC2039: In POSIX sh, string indexing is undefined.
>>                                                ^-----^ SC2039: In POSIX sh, string indexing is undefined.
>> 
>> 
>> In tools/perf/tests/shell/buildid.sh line 77:
>>       file=${build_id_dir}/.build-id/${id:0:2}/`readlink ${link}`/elf
>>                                      ^-------^ SC2039: In POSIX sh, string indexing is undefined.
>> 
>> 
>> In tools/perf/tests/shell/buildid.sh line 123:
>>       echo "running: perf record $@"
>>                                  ^-- SC2145: Argument mixes string and array. Use * or separate argument.
>> 
>> 
>> In tools/perf/tests/shell/buildid.sh line 124:
>>       ${perf} record --buildid-all -o ${data} $@ &> ${log}
>>                                               ^-- SC2068: Double quote array expansions to avoid re-splitting elements.
>>                                                  ^-------^ SC2039: In POSIX sh, &> is undefined.
>> 
>> 
>> In tools/perf/tests/shell/buildid.sh line 126:
>>               echo "failed: record $@"
>>                                    ^-- SC2145: Argument mixes string and array. Use * or separate argument.
>> 
>> 
>> In tools/perf/tests/shell/buildid.sh line 131:
>>       check ${@: -1}
>>             ^------^ SC2068: Double quote array expansions to avoid re-splitting elements.
>>             ^------^ SC2039: In POSIX sh, string indexing is undefined.
>> 
>> 
>> In tools/perf/tests/shell/buildid.sh line 158:
>> exit ${err}
>>    ^----^ SC2154: err is referenced but not assigned.
>> 
>> For more information:
>> https://www.shellcheck.net/wiki/SC2068 -- Double quote array expansions to ...
>> https://www.shellcheck.net/wiki/SC2145 -- Argument mixes string and array. ...
>> https://www.shellcheck.net/wiki/SC2039 -- In POSIX sh, &> is undefined.
>> 
>> *** After ***
>> 
>> $ shellcheck -S warning tools/perf/tests/shell/buildid.sh 
>> 
>> In tools/perf/tests/shell/buildid.sh line 123:
>>       echo "running: perf record $@"
>>                                  ^-- SC2145: Argument mixes string and array. Use * or separate argument.
>> 
>> 
>> In tools/perf/tests/shell/buildid.sh line 124:
>>       ${perf} record --buildid-all -o ${data} $@ &> ${log}
>>                                               ^-- SC2068: Double quote array expansions to avoid re-splitting elements.
>> 
>> 
>> In tools/perf/tests/shell/buildid.sh line 126:
>>               echo "failed: record $@"
>>                                    ^-- SC2145: Argument mixes string and array. Use * or separate argument.
>> 
>> 
>> In tools/perf/tests/shell/buildid.sh line 131:
>>       check ${@: -1}
>>             ^------^ SC2068: Double quote array expansions to avoid re-splitting elements.
>> 
>> 
>> In tools/perf/tests/shell/buildid.sh line 158:
>> exit ${err}
>>    ^----^ SC2154: err is referenced but not assigned.
>> 
>> For more information:
>> https://www.shellcheck.net/wiki/SC2068 -- Double quote array expansions to ...
>> https://www.shellcheck.net/wiki/SC2145 -- Argument mixes string and array. ...
>> https://www.shellcheck.net/wiki/SC2154 -- err is referenced but not assigned.
> 

Re: [PATCH 1/2] tools/perf/tests: Fix string substitutions in build id test

[Ian Rogers]

On Sun, Jan 15, 2023 at 9:03 PM Athira Rajeev
<atrajeev@linux.vnet.ibm.com> wrote:
>
>
>
> > On 28-Sep-2022, at 10:24 AM, Athira Rajeev <atrajeev@linux.vnet.ibm.com> wrote:
> >
> > Hi All,
> >
> > Looking for what direction we can take here.
> > Do we want to change all tests in tools/perf/tests/shell except test_intel_pt.sh to use "bash" or go with
> > the approach in the patch ? Please share your comments
> >
> > Thanks
> > Athira
> >
>
> Hi All,
>
> Looking for what direction we can take here.
> Do we want to change all tests in tools/perf/tests/shell except test_intel_pt.sh to use "bash" or go with
> the approach in the patch ? Please share your comments
>
> Thanks
> Athira

I think some of what the patch is doing is good, some of it the
readability becomes a little harder by not being bash. I'm agnostic as
to which approach to take for the fix.

An aside, I noticed that we do run some tests at build time:
https://git.kernel.org/pub/scm/linux/kernel/git/acme/linux.git/tree/lib/Makefile?h=perf/core#n390
So perhaps we can have a shellcheck build option, defaulted off but
enabled as part of Arnaldo's regular test scripts. The shellcheck
build option would run shellcheck to make sure that there weren't
errors in the shell code, which it is far too easy to introduce.

Thanks,
Ian

> >> On 23-Sep-2022, at 11:54 AM, Adrian Hunter <adrian.hunter@intel.com> wrote:
> >>
> >> On 22/09/22 22:15, Arnaldo Carvalho de Melo wrote:
> >>> Em Wed, Sep 21, 2022 at 10:38:38PM +0530, Athira Rajeev escreveu:
> >>>> The perf test named “build id cache operations” skips with below
> >>>> error on some distros:
> >>>
> >>> I wonder if we shouldn't instead state that bash is needed?
> >>>
> >>> ⬢[acme@toolbox perf-urgent]$ head -1 tools/perf/tests/shell/*.sh | grep ^#
> >>> #!/bin/sh
> >>> #!/bin/bash
> >>> #!/bin/sh
> >>> #!/bin/sh
> >>> #!/bin/sh
> >>> #!/bin/sh
> >>> #!/bin/sh
> >>> #!/bin/sh
> >>> #!/bin/sh
> >>> #!/bin/sh
> >>> #!/bin/bash
> >>> #!/bin/sh
> >>> #!/bin/sh
> >>> #!/bin/sh
> >>> #!/bin/bash
> >>> #!/bin/sh
> >>> #!/bin/bash
> >>> #!/bin/sh
> >>> #!/bin/sh
> >>> #!/bin/sh
> >>> #!/bin/sh
> >>> #!/bin/sh
> >>> #!/bin/sh
> >>> #!/bin/sh
> >>> #!/bin/sh
> >>> #!/bin/sh
> >>> ⬢[acme@toolbox perf-urgent]$
> >>>
> >>> Opinions?
> >>>
> >>
> >> Please don't change tools/perf/tests/shell/test_intel_pt.sh
> >>
> >> I started using shellcheck on that with the "perf test:
> >> test_intel_pt.sh: Add per-thread test" patch set that I sent.
> >>
> >> FYI, if you use shellcheck on buildid.sh, it shows up issues even
> >> after changing to bash:
> >>
> >> *** Before ***
> >>
> >> $ shellcheck -S warning tools/perf/tests/shell/buildid.sh
> >>
> >> In tools/perf/tests/shell/buildid.sh line 69:
> >>       link=${build_id_dir}/.build-id/${id:0:2}/${id:2}
> >>                                      ^-------^ SC2039: In POSIX sh, string indexing is undefined.
> >>                                                ^-----^ SC2039: In POSIX sh, string indexing is undefined.
> >>
> >>
> >> In tools/perf/tests/shell/buildid.sh line 77:
> >>       file=${build_id_dir}/.build-id/${id:0:2}/`readlink ${link}`/elf
> >>                                      ^-------^ SC2039: In POSIX sh, string indexing is undefined.
> >>
> >>
> >> In tools/perf/tests/shell/buildid.sh line 123:
> >>       echo "running: perf record $@"
> >>                                  ^-- SC2145: Argument mixes string and array. Use * or separate argument.
> >>
> >>
> >> In tools/perf/tests/shell/buildid.sh line 124:
> >>       ${perf} record --buildid-all -o ${data} $@ &> ${log}
> >>                                               ^-- SC2068: Double quote array expansions to avoid re-splitting elements.
> >>                                                  ^-------^ SC2039: In POSIX sh, &> is undefined.
> >>
> >>
> >> In tools/perf/tests/shell/buildid.sh line 126:
> >>               echo "failed: record $@"
> >>                                    ^-- SC2145: Argument mixes string and array. Use * or separate argument.
> >>
> >>
> >> In tools/perf/tests/shell/buildid.sh line 131:
> >>       check ${@: -1}
> >>             ^------^ SC2068: Double quote array expansions to avoid re-splitting elements.
> >>             ^------^ SC2039: In POSIX sh, string indexing is undefined.
> >>
> >>
> >> In tools/perf/tests/shell/buildid.sh line 158:
> >> exit ${err}
> >>    ^----^ SC2154: err is referenced but not assigned.
> >>
> >> For more information:
> >> https://www.shellcheck.net/wiki/SC2068 -- Double quote array expansions to ...
> >> https://www.shellcheck.net/wiki/SC2145 -- Argument mixes string and array. ...
> >> https://www.shellcheck.net/wiki/SC2039 -- In POSIX sh, &> is undefined.
> >>
> >> *** After ***
> >>
> >> $ shellcheck -S warning tools/perf/tests/shell/buildid.sh
> >>
> >> In tools/perf/tests/shell/buildid.sh line 123:
> >>       echo "running: perf record $@"
> >>                                  ^-- SC2145: Argument mixes string and array. Use * or separate argument.
> >>
> >>
> >> In tools/perf/tests/shell/buildid.sh line 124:
> >>       ${perf} record --buildid-all -o ${data} $@ &> ${log}
> >>                                               ^-- SC2068: Double quote array expansions to avoid re-splitting elements.
> >>
> >>
> >> In tools/perf/tests/shell/buildid.sh line 126:
> >>               echo "failed: record $@"
> >>                                    ^-- SC2145: Argument mixes string and array. Use * or separate argument.
> >>
> >>
> >> In tools/perf/tests/shell/buildid.sh line 131:
> >>       check ${@: -1}
> >>             ^------^ SC2068: Double quote array expansions to avoid re-splitting elements.
> >>
> >>
> >> In tools/perf/tests/shell/buildid.sh line 158:
> >> exit ${err}
> >>    ^----^ SC2154: err is referenced but not assigned.
> >>
> >> For more information:
> >> https://www.shellcheck.net/wiki/SC2068 -- Double quote array expansions to ...
> >> https://www.shellcheck.net/wiki/SC2145 -- Argument mixes string and array. ...
> >> https://www.shellcheck.net/wiki/SC2154 -- err is referenced but not assigned.
> >
>

Re: [PATCH 1/2] tools/perf/tests: Fix string substitutions in build id test

[Athira Rajeev]

> On 16-Jan-2023, at 11:47 AM, Ian Rogers <irogers@google.com> wrote:
> 
> On Sun, Jan 15, 2023 at 9:03 PM Athira Rajeev
> <atrajeev@linux.vnet.ibm.com> wrote:
>> 
>> 
>> 
>>> On 28-Sep-2022, at 10:24 AM, Athira Rajeev <atrajeev@linux.vnet.ibm.com> wrote:
>>> 
>>> Hi All,
>>> 
>>> Looking for what direction we can take here.
>>> Do we want to change all tests in tools/perf/tests/shell except test_intel_pt.sh to use "bash" or go with
>>> the approach in the patch ? Please share your comments
>>> 
>>> Thanks
>>> Athira
>>> 
>> 
>> Hi All,
>> 
>> Looking for what direction we can take here.
>> Do we want to change all tests in tools/perf/tests/shell except test_intel_pt.sh to use "bash" or go with
>> the approach in the patch ? Please share your comments
>> 
>> Thanks
>> Athira
> 

Thanks Ian for the response.

> I think some of what the patch is doing is good, some of it the

Ian, can I take this as an ack for the patch so that Arnaldo can pick this in acme git ?

> readability becomes a little harder by not being bash. I'm agnostic as
> to which approach to take for the fix.

May be we can take this as separate mail thread to get everyone’s opinion on changing all tests in "tools/perf/tests/shell" except test_intel_pt.sh to use “bash" ?

> 
> An aside, I noticed that we do run some tests at build time:
> https://git.kernel.org/pub/scm/linux/kernel/git/acme/linux.git/tree/lib/Makefile?h=perf/core#n390
> So perhaps we can have a shellcheck build option, defaulted off but
> enabled as part of Arnaldo's regular test scripts. The shellcheck
> build option would run shellcheck to make sure that there weren't
> errors in the shell code, which it is far too easy to introduce.

Sure, that is a good option to have. I will check on having “shellcheck" as a build option.

Thanks
Athira

> 
> Thanks,
> Ian
> 
>>>> On 23-Sep-2022, at 11:54 AM, Adrian Hunter <adrian.hunter@intel.com> wrote:
>>>> 
>>>> On 22/09/22 22:15, Arnaldo Carvalho de Melo wrote:
>>>>> Em Wed, Sep 21, 2022 at 10:38:38PM +0530, Athira Rajeev escreveu:
>>>>>> The perf test named “build id cache operations” skips with below
>>>>>> error on some distros:
>>>>> 
>>>>> I wonder if we shouldn't instead state that bash is needed?
>>>>> 
>>>>> ⬢[acme@toolbox perf-urgent]$ head -1 tools/perf/tests/shell/*.sh | grep ^#
>>>>> #!/bin/sh
>>>>> #!/bin/bash
>>>>> #!/bin/sh
>>>>> #!/bin/sh
>>>>> #!/bin/sh
>>>>> #!/bin/sh
>>>>> #!/bin/sh
>>>>> #!/bin/sh
>>>>> #!/bin/sh
>>>>> #!/bin/sh
>>>>> #!/bin/bash
>>>>> #!/bin/sh
>>>>> #!/bin/sh
>>>>> #!/bin/sh
>>>>> #!/bin/bash
>>>>> #!/bin/sh
>>>>> #!/bin/bash
>>>>> #!/bin/sh
>>>>> #!/bin/sh
>>>>> #!/bin/sh
>>>>> #!/bin/sh
>>>>> #!/bin/sh
>>>>> #!/bin/sh
>>>>> #!/bin/sh
>>>>> #!/bin/sh
>>>>> #!/bin/sh
>>>>> ⬢[acme@toolbox perf-urgent]$
>>>>> 
>>>>> Opinions?
>>>>> 
>>>> 
>>>> Please don't change tools/perf/tests/shell/test_intel_pt.sh
>>>> 
>>>> I started using shellcheck on that with the "perf test:
>>>> test_intel_pt.sh: Add per-thread test" patch set that I sent.
>>>> 
>>>> FYI, if you use shellcheck on buildid.sh, it shows up issues even
>>>> after changing to bash:
>>>> 
>>>> *** Before ***
>>>> 
>>>> $ shellcheck -S warning tools/perf/tests/shell/buildid.sh
>>>> 
>>>> In tools/perf/tests/shell/buildid.sh line 69:
>>>>      link=${build_id_dir}/.build-id/${id:0:2}/${id:2}
>>>>                                     ^-------^ SC2039: In POSIX sh, string indexing is undefined.
>>>>                                               ^-----^ SC2039: In POSIX sh, string indexing is undefined.
>>>> 
>>>> 
>>>> In tools/perf/tests/shell/buildid.sh line 77:
>>>>      file=${build_id_dir}/.build-id/${id:0:2}/`readlink ${link}`/elf
>>>>                                     ^-------^ SC2039: In POSIX sh, string indexing is undefined.
>>>> 
>>>> 
>>>> In tools/perf/tests/shell/buildid.sh line 123:
>>>>      echo "running: perf record $@"
>>>>                                 ^-- SC2145: Argument mixes string and array. Use * or separate argument.
>>>> 
>>>> 
>>>> In tools/perf/tests/shell/buildid.sh line 124:
>>>>      ${perf} record --buildid-all -o ${data} $@ &> ${log}
>>>>                                              ^-- SC2068: Double quote array expansions to avoid re-splitting elements.
>>>>                                                 ^-------^ SC2039: In POSIX sh, &> is undefined.
>>>> 
>>>> 
>>>> In tools/perf/tests/shell/buildid.sh line 126:
>>>>              echo "failed: record $@"
>>>>                                   ^-- SC2145: Argument mixes string and array. Use * or separate argument.
>>>> 
>>>> 
>>>> In tools/perf/tests/shell/buildid.sh line 131:
>>>>      check ${@: -1}
>>>>            ^------^ SC2068: Double quote array expansions to avoid re-splitting elements.
>>>>            ^------^ SC2039: In POSIX sh, string indexing is undefined.
>>>> 
>>>> 
>>>> In tools/perf/tests/shell/buildid.sh line 158:
>>>> exit ${err}
>>>>   ^----^ SC2154: err is referenced but not assigned.
>>>> 
>>>> For more information:
>>>> https://www.shellcheck.net/wiki/SC2068 -- Double quote array expansions to ...
>>>> https://www.shellcheck.net/wiki/SC2145 -- Argument mixes string and array. ...
>>>> https://www.shellcheck.net/wiki/SC2039 -- In POSIX sh, &> is undefined.
>>>> 
>>>> *** After ***
>>>> 
>>>> $ shellcheck -S warning tools/perf/tests/shell/buildid.sh
>>>> 
>>>> In tools/perf/tests/shell/buildid.sh line 123:
>>>>      echo "running: perf record $@"
>>>>                                 ^-- SC2145: Argument mixes string and array. Use * or separate argument.
>>>> 
>>>> 
>>>> In tools/perf/tests/shell/buildid.sh line 124:
>>>>      ${perf} record --buildid-all -o ${data} $@ &> ${log}
>>>>                                              ^-- SC2068: Double quote array expansions to avoid re-splitting elements.
>>>> 
>>>> 
>>>> In tools/perf/tests/shell/buildid.sh line 126:
>>>>              echo "failed: record $@"
>>>>                                   ^-- SC2145: Argument mixes string and array. Use * or separate argument.
>>>> 
>>>> 
>>>> In tools/perf/tests/shell/buildid.sh line 131:
>>>>      check ${@: -1}
>>>>            ^------^ SC2068: Double quote array expansions to avoid re-splitting elements.
>>>> 
>>>> 
>>>> In tools/perf/tests/shell/buildid.sh line 158:
>>>> exit ${err}
>>>>   ^----^ SC2154: err is referenced but not assigned.
>>>> 
>>>> For more information:
>>>> https://www.shellcheck.net/wiki/SC2068 -- Double quote array expansions to ...
>>>> https://www.shellcheck.net/wiki/SC2145 -- Argument mixes string and array. ...
>>>> https://www.shellcheck.net/wiki/SC2154 -- err is referenced but not assigned.

Re: [PATCH 1/2] tools/perf/tests: Fix string substitutions in build id test

[Ian Rogers]

On Mon, Jan 16, 2023 at 4:00 AM Athira Rajeev
<atrajeev@linux.vnet.ibm.com> wrote:
>
>
>
> > On 16-Jan-2023, at 11:47 AM, Ian Rogers <irogers@google.com> wrote:
> >
> > On Sun, Jan 15, 2023 at 9:03 PM Athira Rajeev
> > <atrajeev@linux.vnet.ibm.com> wrote:
> >>
> >>
> >>
> >>> On 28-Sep-2022, at 10:24 AM, Athira Rajeev <atrajeev@linux.vnet.ibm.com> wrote:
> >>>
> >>> Hi All,
> >>>
> >>> Looking for what direction we can take here.
> >>> Do we want to change all tests in tools/perf/tests/shell except test_intel_pt.sh to use "bash" or go with
> >>> the approach in the patch ? Please share your comments
> >>>
> >>> Thanks
> >>> Athira
> >>>
> >>
> >> Hi All,
> >>
> >> Looking for what direction we can take here.
> >> Do we want to change all tests in tools/perf/tests/shell except test_intel_pt.sh to use "bash" or go with
> >> the approach in the patch ? Please share your comments
> >>
> >> Thanks
> >> Athira
> >
>
> Thanks Ian for the response.
>
> > I think some of what the patch is doing is good, some of it the
>
> Ian, can I take this as an ack for the patch so that Arnaldo can pick this in acme git ?

Acked-by: Ian Rogers <irogers@google.com>

Thanks,
Ian

> > readability becomes a little harder by not being bash. I'm agnostic as
> > to which approach to take for the fix.
>
> May be we can take this as separate mail thread to get everyone’s opinion on changing all tests in "tools/perf/tests/shell" except test_intel_pt.sh to use “bash" ?
>
> >
> > An aside, I noticed that we do run some tests at build time:
> > https://git.kernel.org/pub/scm/linux/kernel/git/acme/linux.git/tree/lib/Makefile?h=perf/core#n390
> > So perhaps we can have a shellcheck build option, defaulted off but
> > enabled as part of Arnaldo's regular test scripts. The shellcheck
> > build option would run shellcheck to make sure that there weren't
> > errors in the shell code, which it is far too easy to introduce.
>
> Sure, that is a good option to have. I will check on having “shellcheck" as a build option.
>
> Thanks
> Athira
>
> >
> > Thanks,
> > Ian
> >
> >>>> On 23-Sep-2022, at 11:54 AM, Adrian Hunter <adrian.hunter@intel.com> wrote:
> >>>>
> >>>> On 22/09/22 22:15, Arnaldo Carvalho de Melo wrote:
> >>>>> Em Wed, Sep 21, 2022 at 10:38:38PM +0530, Athira Rajeev escreveu:
> >>>>>> The perf test named “build id cache operations” skips with below
> >>>>>> error on some distros:
> >>>>>
> >>>>> I wonder if we shouldn't instead state that bash is needed?
> >>>>>
> >>>>> ⬢[acme@toolbox perf-urgent]$ head -1 tools/perf/tests/shell/*.sh | grep ^#
> >>>>> #!/bin/sh
> >>>>> #!/bin/bash
> >>>>> #!/bin/sh
> >>>>> #!/bin/sh
> >>>>> #!/bin/sh
> >>>>> #!/bin/sh
> >>>>> #!/bin/sh
> >>>>> #!/bin/sh
> >>>>> #!/bin/sh
> >>>>> #!/bin/sh
> >>>>> #!/bin/bash
> >>>>> #!/bin/sh
> >>>>> #!/bin/sh
> >>>>> #!/bin/sh
> >>>>> #!/bin/bash
> >>>>> #!/bin/sh
> >>>>> #!/bin/bash
> >>>>> #!/bin/sh
> >>>>> #!/bin/sh
> >>>>> #!/bin/sh
> >>>>> #!/bin/sh
> >>>>> #!/bin/sh
> >>>>> #!/bin/sh
> >>>>> #!/bin/sh
> >>>>> #!/bin/sh
> >>>>> #!/bin/sh
> >>>>> ⬢[acme@toolbox perf-urgent]$
> >>>>>
> >>>>> Opinions?
> >>>>>
> >>>>
> >>>> Please don't change tools/perf/tests/shell/test_intel_pt.sh
> >>>>
> >>>> I started using shellcheck on that with the "perf test:
> >>>> test_intel_pt.sh: Add per-thread test" patch set that I sent.
> >>>>
> >>>> FYI, if you use shellcheck on buildid.sh, it shows up issues even
> >>>> after changing to bash:
> >>>>
> >>>> *** Before ***
> >>>>
> >>>> $ shellcheck -S warning tools/perf/tests/shell/buildid.sh
> >>>>
> >>>> In tools/perf/tests/shell/buildid.sh line 69:
> >>>>      link=${build_id_dir}/.build-id/${id:0:2}/${id:2}
> >>>>                                     ^-------^ SC2039: In POSIX sh, string indexing is undefined.
> >>>>                                               ^-----^ SC2039: In POSIX sh, string indexing is undefined.
> >>>>
> >>>>
> >>>> In tools/perf/tests/shell/buildid.sh line 77:
> >>>>      file=${build_id_dir}/.build-id/${id:0:2}/`readlink ${link}`/elf
> >>>>                                     ^-------^ SC2039: In POSIX sh, string indexing is undefined.
> >>>>
> >>>>
> >>>> In tools/perf/tests/shell/buildid.sh line 123:
> >>>>      echo "running: perf record $@"
> >>>>                                 ^-- SC2145: Argument mixes string and array. Use * or separate argument.
> >>>>
> >>>>
> >>>> In tools/perf/tests/shell/buildid.sh line 124:
> >>>>      ${perf} record --buildid-all -o ${data} $@ &> ${log}
> >>>>                                              ^-- SC2068: Double quote array expansions to avoid re-splitting elements.
> >>>>                                                 ^-------^ SC2039: In POSIX sh, &> is undefined.
> >>>>
> >>>>
> >>>> In tools/perf/tests/shell/buildid.sh line 126:
> >>>>              echo "failed: record $@"
> >>>>                                   ^-- SC2145: Argument mixes string and array. Use * or separate argument.
> >>>>
> >>>>
> >>>> In tools/perf/tests/shell/buildid.sh line 131:
> >>>>      check ${@: -1}
> >>>>            ^------^ SC2068: Double quote array expansions to avoid re-splitting elements.
> >>>>            ^------^ SC2039: In POSIX sh, string indexing is undefined.
> >>>>
> >>>>
> >>>> In tools/perf/tests/shell/buildid.sh line 158:
> >>>> exit ${err}
> >>>>   ^----^ SC2154: err is referenced but not assigned.
> >>>>
> >>>> For more information:
> >>>> https://www.shellcheck.net/wiki/SC2068 -- Double quote array expansions to ...
> >>>> https://www.shellcheck.net/wiki/SC2145 -- Argument mixes string and array. ...
> >>>> https://www.shellcheck.net/wiki/SC2039 -- In POSIX sh, &> is undefined.
> >>>>
> >>>> *** After ***
> >>>>
> >>>> $ shellcheck -S warning tools/perf/tests/shell/buildid.sh
> >>>>
> >>>> In tools/perf/tests/shell/buildid.sh line 123:
> >>>>      echo "running: perf record $@"
> >>>>                                 ^-- SC2145: Argument mixes string and array. Use * or separate argument.
> >>>>
> >>>>
> >>>> In tools/perf/tests/shell/buildid.sh line 124:
> >>>>      ${perf} record --buildid-all -o ${data} $@ &> ${log}
> >>>>                                              ^-- SC2068: Double quote array expansions to avoid re-splitting elements.
> >>>>
> >>>>
> >>>> In tools/perf/tests/shell/buildid.sh line 126:
> >>>>              echo "failed: record $@"
> >>>>                                   ^-- SC2145: Argument mixes string and array. Use * or separate argument.
> >>>>
> >>>>
> >>>> In tools/perf/tests/shell/buildid.sh line 131:
> >>>>      check ${@: -1}
> >>>>            ^------^ SC2068: Double quote array expansions to avoid re-splitting elements.
> >>>>
> >>>>
> >>>> In tools/perf/tests/shell/buildid.sh line 158:
> >>>> exit ${err}
> >>>>   ^----^ SC2154: err is referenced but not assigned.
> >>>>
> >>>> For more information:
> >>>> https://www.shellcheck.net/wiki/SC2068 -- Double quote array expansions to ...
> >>>> https://www.shellcheck.net/wiki/SC2145 -- Argument mixes string and array. ...
> >>>> https://www.shellcheck.net/wiki/SC2154 -- err is referenced but not assigned.
>

Re: [PATCH 1/2] tools/perf/tests: Fix string substitutions in build id test

[Athira Rajeev]

> On 17-Jan-2023, at 3:30 AM, Ian Rogers <irogers@google.com> wrote:
> 
> On Mon, Jan 16, 2023 at 4:00 AM Athira Rajeev
> <atrajeev@linux.vnet.ibm.com> wrote:
>> 
>> 
>> 
>>> On 16-Jan-2023, at 11:47 AM, Ian Rogers <irogers@google.com> wrote:
>>> 
>>> On Sun, Jan 15, 2023 at 9:03 PM Athira Rajeev
>>> <atrajeev@linux.vnet.ibm.com> wrote:
>>>> 
>>>> 
>>>> 
>>>>> On 28-Sep-2022, at 10:24 AM, Athira Rajeev <atrajeev@linux.vnet.ibm.com> wrote:
>>>>> 
>>>>> Hi All,
>>>>> 
>>>>> Looking for what direction we can take here.
>>>>> Do we want to change all tests in tools/perf/tests/shell except test_intel_pt.sh to use "bash" or go with
>>>>> the approach in the patch ? Please share your comments
>>>>> 
>>>>> Thanks
>>>>> Athira
>>>>> 
>>>> 
>>>> Hi All,
>>>> 
>>>> Looking for what direction we can take here.
>>>> Do we want to change all tests in tools/perf/tests/shell except test_intel_pt.sh to use "bash" or go with
>>>> the approach in the patch ? Please share your comments
>>>> 
>>>> Thanks
>>>> Athira
>>> 
>> 
>> Thanks Ian for the response.
>> 
>>> I think some of what the patch is doing is good, some of it the
>> 
>> Ian, can I take this as an ack for the patch so that Arnaldo can pick this in acme git ?
> 
> Acked-by: Ian Rogers <irogers@google.com>
> 
> Thanks,
> Ian

Thanks Ian.

Hi Arnaldo,

I have re-posted the patch 2 ( in this series ) separately in new patchset here :
https://lore.kernel.org/linux-perf-users/20230116050131.17221-2-atrajeev@linux.vnet.ibm.com/

Ack from Ian is for Patch 1. Shall I rebase to upstream and post this patch 1 separately ?

Thanks
Athira
> 
>>> readability becomes a little harder by not being bash. I'm agnostic as
>>> to which approach to take for the fix.
>> 
>> May be we can take this as separate mail thread to get everyone’s opinion on changing all tests in "tools/perf/tests/shell" except test_intel_pt.sh to use “bash" ?
>> 
>>> 
>>> An aside, I noticed that we do run some tests at build time:
>>> https://git.kernel.org/pub/scm/linux/kernel/git/acme/linux.git/tree/lib/Makefile?h=perf/core#n390
>>> So perhaps we can have a shellcheck build option, defaulted off but
>>> enabled as part of Arnaldo's regular test scripts. The shellcheck
>>> build option would run shellcheck to make sure that there weren't
>>> errors in the shell code, which it is far too easy to introduce.
>> 
>> Sure, that is a good option to have. I will check on having “shellcheck" as a build option.
>> 
>> Thanks
>> Athira
>> 
>>> 
>>> Thanks,
>>> Ian
>>> 
>>>>>> On 23-Sep-2022, at 11:54 AM, Adrian Hunter <adrian.hunter@intel.com> wrote:
>>>>>> 
>>>>>> On 22/09/22 22:15, Arnaldo Carvalho de Melo wrote:
>>>>>>> Em Wed, Sep 21, 2022 at 10:38:38PM +0530, Athira Rajeev escreveu:
>>>>>>>> The perf test named “build id cache operations” skips with below
>>>>>>>> error on some distros:
>>>>>>> 
>>>>>>> I wonder if we shouldn't instead state that bash is needed?
>>>>>>> 
>>>>>>> ⬢[acme@toolbox perf-urgent]$ head -1 tools/perf/tests/shell/*.sh | grep ^#
>>>>>>> #!/bin/sh
>>>>>>> #!/bin/bash
>>>>>>> #!/bin/sh
>>>>>>> #!/bin/sh
>>>>>>> #!/bin/sh
>>>>>>> #!/bin/sh
>>>>>>> #!/bin/sh
>>>>>>> #!/bin/sh
>>>>>>> #!/bin/sh
>>>>>>> #!/bin/sh
>>>>>>> #!/bin/bash
>>>>>>> #!/bin/sh
>>>>>>> #!/bin/sh
>>>>>>> #!/bin/sh
>>>>>>> #!/bin/bash
>>>>>>> #!/bin/sh
>>>>>>> #!/bin/bash
>>>>>>> #!/bin/sh
>>>>>>> #!/bin/sh
>>>>>>> #!/bin/sh
>>>>>>> #!/bin/sh
>>>>>>> #!/bin/sh
>>>>>>> #!/bin/sh
>>>>>>> #!/bin/sh
>>>>>>> #!/bin/sh
>>>>>>> #!/bin/sh
>>>>>>> ⬢[acme@toolbox perf-urgent]$
>>>>>>> 
>>>>>>> Opinions?
>>>>>>> 
>>>>>> 
>>>>>> Please don't change tools/perf/tests/shell/test_intel_pt.sh
>>>>>> 
>>>>>> I started using shellcheck on that with the "perf test:
>>>>>> test_intel_pt.sh: Add per-thread test" patch set that I sent.
>>>>>> 
>>>>>> FYI, if you use shellcheck on buildid.sh, it shows up issues even
>>>>>> after changing to bash:
>>>>>> 
>>>>>> *** Before ***
>>>>>> 
>>>>>> $ shellcheck -S warning tools/perf/tests/shell/buildid.sh
>>>>>> 
>>>>>> In tools/perf/tests/shell/buildid.sh line 69:
>>>>>>     link=${build_id_dir}/.build-id/${id:0:2}/${id:2}
>>>>>>                                    ^-------^ SC2039: In POSIX sh, string indexing is undefined.
>>>>>>                                              ^-----^ SC2039: In POSIX sh, string indexing is undefined.
>>>>>> 
>>>>>> 
>>>>>> In tools/perf/tests/shell/buildid.sh line 77:
>>>>>>     file=${build_id_dir}/.build-id/${id:0:2}/`readlink ${link}`/elf
>>>>>>                                    ^-------^ SC2039: In POSIX sh, string indexing is undefined.
>>>>>> 
>>>>>> 
>>>>>> In tools/perf/tests/shell/buildid.sh line 123:
>>>>>>     echo "running: perf record $@"
>>>>>>                                ^-- SC2145: Argument mixes string and array. Use * or separate argument.
>>>>>> 
>>>>>> 
>>>>>> In tools/perf/tests/shell/buildid.sh line 124:
>>>>>>     ${perf} record --buildid-all -o ${data} $@ &> ${log}
>>>>>>                                             ^-- SC2068: Double quote array expansions to avoid re-splitting elements.
>>>>>>                                                ^-------^ SC2039: In POSIX sh, &> is undefined.
>>>>>> 
>>>>>> 
>>>>>> In tools/perf/tests/shell/buildid.sh line 126:
>>>>>>             echo "failed: record $@"
>>>>>>                                  ^-- SC2145: Argument mixes string and array. Use * or separate argument.
>>>>>> 
>>>>>> 
>>>>>> In tools/perf/tests/shell/buildid.sh line 131:
>>>>>>     check ${@: -1}
>>>>>>           ^------^ SC2068: Double quote array expansions to avoid re-splitting elements.
>>>>>>           ^------^ SC2039: In POSIX sh, string indexing is undefined.
>>>>>> 
>>>>>> 
>>>>>> In tools/perf/tests/shell/buildid.sh line 158:
>>>>>> exit ${err}
>>>>>>  ^----^ SC2154: err is referenced but not assigned.
>>>>>> 
>>>>>> For more information:
>>>>>> https://www.shellcheck.net/wiki/SC2068 -- Double quote array expansions to ...
>>>>>> https://www.shellcheck.net/wiki/SC2145 -- Argument mixes string and array. ...
>>>>>> https://www.shellcheck.net/wiki/SC2039 -- In POSIX sh, &> is undefined.
>>>>>> 
>>>>>> *** After ***
>>>>>> 
>>>>>> $ shellcheck -S warning tools/perf/tests/shell/buildid.sh
>>>>>> 
>>>>>> In tools/perf/tests/shell/buildid.sh line 123:
>>>>>>     echo "running: perf record $@"
>>>>>>                                ^-- SC2145: Argument mixes string and array. Use * or separate argument.
>>>>>> 
>>>>>> 
>>>>>> In tools/perf/tests/shell/buildid.sh line 124:
>>>>>>     ${perf} record --buildid-all -o ${data} $@ &> ${log}
>>>>>>                                             ^-- SC2068: Double quote array expansions to avoid re-splitting elements.
>>>>>> 
>>>>>> 
>>>>>> In tools/perf/tests/shell/buildid.sh line 126:
>>>>>>             echo "failed: record $@"
>>>>>>                                  ^-- SC2145: Argument mixes string and array. Use * or separate argument.
>>>>>> 
>>>>>> 
>>>>>> In tools/perf/tests/shell/buildid.sh line 131:
>>>>>>     check ${@: -1}
>>>>>>           ^------^ SC2068: Double quote array expansions to avoid re-splitting elements.
>>>>>> 
>>>>>> 
>>>>>> In tools/perf/tests/shell/buildid.sh line 158:
>>>>>> exit ${err}
>>>>>>  ^----^ SC2154: err is referenced but not assigned.
>>>>>> 
>>>>>> For more information:
>>>>>> https://www.shellcheck.net/wiki/SC2068 -- Double quote array expansions to ...
>>>>>> https://www.shellcheck.net/wiki/SC2145 -- Argument mixes string and array. ...
>>>>>> https://www.shellcheck.net/wiki/SC2154 -- err is referenced but not assigned.

[PATCH 1/2] tools/perf: Fix the file mode with copyfile while adding file to build-id cache

[Athira Rajeev]

The test "build id cache operations" fails on powerpc
As below:

	Adding 5a0fd882b53084224ba47b624c55a469 ./tests/shell/../pe-file.exe: Ok
	build id: 5a0fd882b53084224ba47b624c55a469
	link: /tmp/perf.debug.ZTu/.build-id/5a/0fd882b53084224ba47b624c55a469
	file: /tmp/perf.debug.ZTu/.build-id/5a/../../root/linux/tools/perf/tests/pe-file.exe/5a0fd882b53084224ba47b624c55a469/elf
	failed: file /tmp/perf.debug.ZTu/.build-id/5a/../../root/linux/tools/perf/tests/pe-file.exe/5a0fd882b53084224ba47b624c55a469/elf does not exist
	test child finished with -1
	---- end ----
	build id cache operations: FAILED!

The failing test is when trying to add pe-file.exe to
build id cache.

Perf buildid-cache can be used to add/remove/manage
files from the build-id cache. "-a" option is used to
add a file to the build-id cache.

Simple command to do so for a PE exe file:
 # ls -ltr tests/pe-file.exe
 -rw-r--r--. 1 root root 75595 Jan 10 23:35 tests/pe-file.exe
 The file is in home directory.

 # mkdir  /tmp/perf.debug.TeY1
 # perf --buildid-dir /tmp/perf.debug.TeY1 buildid-cache -v
   -a tests/pe-file.exe

The above will create ".build-id" folder in build id
directory, which is /tmp/perf.debug.TeY1. Also adds file
to this folder under build id. Example:

 # ls -ltr /tmp/perf.debug.TeY1/.build-id/5a/0fd882b53084224ba47b624c55a469/
 total 76
 -rw-r--r--. 1 root root     0 Jan 11 00:38 probes
 -rwxr-xr-x. 1 root root 75595 Jan 11 00:38 elf

We can see in the results that file mode for original
file and file in build id directory is different. ie,
build id file has executable permission whereas original
file doesn’t have.

The code path and function ( build_id_cache__add ) to
add file to cache is in "util/build-id.c". In
build_id_cache__add() function, it first attempts to link
the original file to destination cache folder. If linking
the file fails ( which can happen if the destination and
source is on a different mount points ), it will copy the
file to destination. Here copyfile() routine explicitly uses
mode as "755" and hence file in the destination will have
executable permission.

Code snippet:

 if (link(realname, filename) && errno != EEXIST &&
                               copyfile(name, filename))

Strace logs:

	172285 link("/home/<user_name>/linux/tools/perf/tests/pe-file.exe", "/tmp/perf.debug.TeY1/home/<user_name>/linux/tools/perf/tests/pe-file.exe/5a0fd882b53084224ba47b624c55a469/elf") = -1 EXDEV (Invalid cross-device link)
	172285 newfstatat(AT_FDCWD, "tests/pe-file.exe", {st_mode=S_IFREG|0644, st_size=75595, ...}, 0) = 0
	172285 openat(AT_FDCWD, "/tmp/perf.debug.TeY1/home/<user_name>/linux/tools/perf/tests/pe-file.exe/5a0fd882b53084224ba47b624c55a469/.elf.KbAnsl", O_RDWR|O_CREAT|O_EXCL, 0600) = 3
	172285 fchmod(3, 0755)                  = 0
	172285 openat(AT_FDCWD, "tests/pe-file.exe", O_RDONLY) = 4
	172285 mmap(NULL, 75595, PROT_READ, MAP_PRIVATE, 4, 0) = 0x7fffa5cd0000
	172285 pwrite64(3, "MZ\220\0\3\0\0\0\4\0\0\0\377\377\0\0\270\0\0\0\0\0\0\0@\0\0\0\0\0\0\0"..., 75595, 0) = 75595

Whereas if the link succeeds, it succeeds in the first
attempt itself and the file in the build-id dir will
have same permission as original file.

Example, above uses /tmp. Instead if we use
"--buildid-dir /home/build", linking will work here
since mount points are same. Hence the destination file
will not have executable permission.

Since the testcase "tests/shell/buildid.sh" always looks
for executable file, test fails in powerpc environment
when test is run from /root.

The patch adds a change in build_id_cache__add to use
copyfile_mode which also passes the file’s original mode as
argument. This way the destination file mode also will
be same as original file.

Signed-off-by: Athira Rajeev <atrajeev@linux.vnet.ibm.com>
---
 tools/perf/util/build-id.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/tools/perf/util/build-id.c b/tools/perf/util/build-id.c
index a839b30c981b..ea9c083ab1e3 100644
--- a/tools/perf/util/build-id.c
+++ b/tools/perf/util/build-id.c
@@ -715,9 +715,13 @@ build_id_cache__add(const char *sbuild_id, const char *name, const char *realnam
 		} else if (nsi && nsinfo__need_setns(nsi)) {
 			if (copyfile_ns(name, filename, nsi))
 				goto out_free;
-		} else if (link(realname, filename) && errno != EEXIST &&
-				copyfile(name, filename))
-			goto out_free;
+		} else if (link(realname, filename) && errno != EEXIST) {
+			struct stat f_stat;
+
+			if (!(stat(name, &f_stat) < 0) &&
+					copyfile_mode(name, filename, f_stat.st_mode))
+				goto out_free;
+		}
 	}
 
 	/* Some binaries are stripped, but have .debug files with their symbol
-- 
2.31.1

[PATCH 2/2] tools/perf/tests: Fix build id test check for PE file

[Athira Rajeev]

Perf test "build id cache operations" fails for PE
executable.  Logs below from powerpc system.
Same is observed on x86 as well.

<<>>
Adding 5a0fd882b53084224ba47b624c55a469 ./tests/shell/../pe-file.exe: Ok
build id: 5a0fd882b53084224ba47b624c55a469
link: /tmp/perf.debug.w0V/.build-id/5a/0fd882b53084224ba47b624c55a469
file: /tmp/perf.debug.w0V/.build-id/5a/../../root/<user>/linux/tools/perf/tests/pe-file.exe/5a0fd882b53084224ba47b624c55a469/elf
failed: file /tmp/perf.debug.w0V/.build-id/5a/../../root/<user>/linux/tools/perf/tests/pe-file.exe/5a0fd882b53084224ba47b624c55a469/elf does not exist
test child finished with -1
---- end ----
build id cache operations: FAILED!
<<>>

The test tries to do:

<<>>
mkdir /tmp/perf.debug.TeY1
perf --buildid-dir /tmp/perf.debug.TeY1 buildid-cache -v -a ./tests/shell/../pe-file.exe
<<>>

The option "--buildid-dir" sets the build id cache
directory as /tmp/perf.debug.TeY1. The option given
to buildid-cahe, ie "-a ./tests/shell/../pe-file.exe",
is to add the pe-file.exe to the cache. The testcase,
sets buildid-dir and adds the file: pe-file.exe to build
id cache. To check if the command is run successfully,
"check" function looks for presence of the file in buildid
cache directory. But the check here expects the added
file to be executable. Snippet below:

<<>>
if [ ! -x $file ]; then
	echo "failed: file ${file} does not exist"
	exit 1
fi
<<>>

Buildid test is done for sha1 binary, md5 binary and also
for PE file. The firt two binaries are created at runtime
by compiling with "--build-id" option and hence the check
for sha1/md5 test should use [ ! -x ]. But in case of PE
file, the permission for this input file is rw-r--r--
Hence the file added to build id cache has same permissoin

Original file:
ls tests/pe-file.exe | xargs stat --printf "%n %A \n"
tests/pe-file.exe -rw-r--r--

buildid cache file:

ls /tmp/perf.debug.w0V/.build-id/5a/../../root/<user>/linux/tools/perf/tests/pe-file.exe/5a0fd882b53084224ba47b624c55a469/elf | xargs stat --printf "%n %A \n"
/tmp/perf.debug.w0V/.build-id/5a/../../root/<user>/linux/tools/perf/tests/pe-file.exe/5a0fd882b53084224ba47b624c55a469/elf -rw-r--r--

Fix the test to match with the permission of original file in
case of FE file. ie if the "tests/pe-file.exe" file is not
having exec permission, just check for existence of the buildid
file using [ ! -e <file> ]

Signed-off-by: Athira Rajeev <atrajeev@linux.vnet.ibm.com>
---
 tools/perf/tests/shell/buildid.sh | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/tools/perf/tests/shell/buildid.sh b/tools/perf/tests/shell/buildid.sh
index f05670d1e39e..22e5b2225abe 100755
--- a/tools/perf/tests/shell/buildid.sh
+++ b/tools/perf/tests/shell/buildid.sh
@@ -77,7 +77,20 @@ check()
 	file=${build_id_dir}/.build-id/${id:0:2}/`readlink ${link}`/elf
 	echo "file: ${file}"
 
-	if [ ! -x $file ]; then
+	# Check for file permission of orginal file
+	# in case of pe-file.exe file
+	echo $1 | grep ".exe"
+	if [ $? -eq 0 ]; then
+		if [ -x $1  -a ! -x $file ]; then
+			echo "failed: file ${file} executable does not exist"
+			exit 1
+		fi
+
+		if [ ! -x $file -a ! -e $file ]; then
+			echo "failed: file ${file} does not exist"
+			exit 1
+		fi
+	elif [ ! -x $file ]; then
 		echo "failed: file ${file} does not exist"
 		exit 1
 	fi
-- 
2.31.1