linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 2/2] perf buildid-cache: Add test for PE executable
@ 2021-02-19 16:10 Nicholas Fraser
  2021-02-24 13:43 ` Jiri Olsa
  0 siblings, 1 reply; 3+ messages in thread
From: Nicholas Fraser @ 2021-02-19 16:10 UTC (permalink / raw)
  To: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
	Mark Rutland, Alexander Shishkin, Jiri Olsa, Namhyung Kim,
	Nicholas Fraser, Ian Rogers, linux-kernel
  Cc: Ulrich Czekalla, Huw Davies

This builds on the previous changes to tests/shell/buildid.sh, adding
tests for a PE file. It adds it to the build-id cache manually and, if
Wine is available, runs it under "perf record" and verifies that it was
added automatically.

If wine is not installed, only warnings are printed; the test can still
exit 0.

(I welcome ways to make the GUID parsing less awful. checkpatch.pl is
complaining about the line length of the sed command. The re-arranging
could be done via e.g. id=${id:6:2}{id:4:2}... since this style is
already used in the script but that turns out to be longer than the sed
command and anyway it's bash-specific. This uses a hardcoded .exe so we
could also just hardcode its GUID but I'd worry about making the tests
too inflexible.)

Signed-off-by: Nicholas Fraser <nfraser@codeweavers.com>
---
 tools/perf/tests/shell/buildid.sh | 43 +++++++++++++++++++++++++++----
 1 file changed, 38 insertions(+), 5 deletions(-)

diff --git a/tools/perf/tests/shell/buildid.sh b/tools/perf/tests/shell/buildid.sh
index 416af614bbe0..55e2168ef26f 100755
--- a/tools/perf/tests/shell/buildid.sh
+++ b/tools/perf/tests/shell/buildid.sh
@@ -14,18 +14,41 @@ if ! [ -x "$(command -v cc)" ]; then
 	exit 2
 fi
 
+# check what we need to test windows binaries
+add_pe=1
+run_pe=1
+if ! perf version --build-options | grep -q 'libbfd: .* on '; then
+    echo "WARNING: perf not built with libbfd. PE binaries will not be tested."
+    add_pe=0
+    run_pe=0
+fi
+if ! which wine > /dev/null; then
+    echo "WARNING: wine not found. PE binaries will not be run."
+    run_pe=0
+fi
+
 ex_md5=$(mktemp /tmp/perf.ex.MD5.XXX)
 ex_sha1=$(mktemp /tmp/perf.ex.SHA1.XXX)
+ex_pe=$(dirname $0)/../pe-file.exe
 
 echo 'int main(void) { return 0; }' | cc -Wl,--build-id=sha1 -o ${ex_sha1} -x c -
 echo 'int main(void) { return 0; }' | cc -Wl,--build-id=md5 -o ${ex_md5} -x c -
 
-echo "test binaries: ${ex_sha1} ${ex_md5}"
+echo "test binaries: ${ex_sha1} ${ex_md5} ${ex_pe}"
 
 check()
 {
-	id=`readelf -n ${1} 2>/dev/null | grep 'Build ID' | awk '{print $3}'`
-
+	case $1 in
+	*.exe)
+		# the build id must be rearranged into a GUID
+		id=`objcopy -O binary --only-section=.buildid $1 /dev/stdout | \
+			cut -c 33-48 | hexdump -ve '/1 "%02x"' | \
+			sed 's@^\(..\)\(..\)\(..\)\(..\)\(..\)\(..\)\(..\)\(..\)\(.*\)0a$@\4\3\2\1\6\5\8\7\9@'`
+		;;
+	*)
+		id=`readelf -n ${1} 2>/dev/null | grep 'Build ID' | awk '{print $3}'`
+		;;
+	esac
 	echo "build id: ${id}"
 
 	link=${build_id_dir}/.build-id/${id:0:2}/${id:2}
@@ -50,7 +73,7 @@ check()
 		exit 1
 	fi
 
-	${perf} buildid-cache -l | grep $id
+	${perf} buildid-cache -l | grep ${id}
 	if [ $? -ne 0 ]; then
 		echo "failed: ${id} is not reported by \"perf buildid-cache -l\""
 		exit 1
@@ -81,7 +104,7 @@ test_record()
 	build_id_dir=$(mktemp -d /tmp/perf.debug.XXX)
 	perf="perf --buildid-dir ${build_id_dir}"
 
-	${perf} record --buildid-all -o ${data} ${1}
+	${perf} record --buildid-all -o ${data} ${2} ${1}
 	if [ $? -ne 0 ]; then
 		echo "failed: record ${1}"
 		exit 1
@@ -96,12 +119,22 @@ test_record()
 # add binaries manual via perf buildid-cache -a
 test_add ${ex_sha1}
 test_add ${ex_md5}
+if [ $add_pe -eq 1 ]; then
+    test_add ${ex_pe}
+fi
 
 # add binaries via perf record post processing
 test_record ${ex_sha1}
 test_record ${ex_md5}
+if [ $run_pe -eq 1 ]; then
+    test_record ${ex_pe} wine
+fi
 
 # cleanup
 rm ${ex_sha1} ${ex_md5}
 
+if [ $add_pe -eq 0 ] || [ $run_pe -eq 0 ]; then
+    echo "WARNING: some PE tests were skipped. See previous warnings."
+fi
+
 exit ${err}
-- 
2.30.1


^ permalink raw reply related	[flat|nested] 3+ messages in thread

* Re: [PATCH 2/2] perf buildid-cache: Add test for PE executable
  2021-02-19 16:10 [PATCH 2/2] perf buildid-cache: Add test for PE executable Nicholas Fraser
@ 2021-02-24 13:43 ` Jiri Olsa
  2021-02-24 17:40   ` Nicholas Fraser
  0 siblings, 1 reply; 3+ messages in thread
From: Jiri Olsa @ 2021-02-24 13:43 UTC (permalink / raw)
  To: Nicholas Fraser
  Cc: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
	Mark Rutland, Alexander Shishkin, Namhyung Kim, Ian Rogers,
	linux-kernel, Ulrich Czekalla, Huw Davies

On Fri, Feb 19, 2021 at 11:10:34AM -0500, Nicholas Fraser wrote:

SNIP

> +if ! which wine > /dev/null; then
> +    echo "WARNING: wine not found. PE binaries will not be run."
> +    run_pe=0
> +fi
> +
>  ex_md5=$(mktemp /tmp/perf.ex.MD5.XXX)
>  ex_sha1=$(mktemp /tmp/perf.ex.SHA1.XXX)
> +ex_pe=$(dirname $0)/../pe-file.exe
>  
>  echo 'int main(void) { return 0; }' | cc -Wl,--build-id=sha1 -o ${ex_sha1} -x c -
>  echo 'int main(void) { return 0; }' | cc -Wl,--build-id=md5 -o ${ex_md5} -x c -
>  
> -echo "test binaries: ${ex_sha1} ${ex_md5}"
> +echo "test binaries: ${ex_sha1} ${ex_md5} ${ex_pe}"
>  
>  check()
>  {
> -	id=`readelf -n ${1} 2>/dev/null | grep 'Build ID' | awk '{print $3}'`
> -
> +	case $1 in
> +	*.exe)
> +		# the build id must be rearranged into a GUID
> +		id=`objcopy -O binary --only-section=.buildid $1 /dev/stdout | \
> +			cut -c 33-48 | hexdump -ve '/1 "%02x"' | \
> +			sed 's@^\(..\)\(..\)\(..\)\(..\)\(..\)\(..\)\(..\)\(..\)\(.*\)0a$@\4\3\2\1\6\5\8\7\9@'`
> +		;;

wow ;-) could this have some more info on what's going on in here?
what's the .buildid PE section format?

> +	*)
> +		id=`readelf -n ${1} 2>/dev/null | grep 'Build ID' | awk '{print $3}'`
> +		;;
> +	esac
>  	echo "build id: ${id}"
>  
>  	link=${build_id_dir}/.build-id/${id:0:2}/${id:2}
> @@ -50,7 +73,7 @@ check()
>  		exit 1
>  	fi
>  
> -	${perf} buildid-cache -l | grep $id
> +	${perf} buildid-cache -l | grep ${id}
>  	if [ $? -ne 0 ]; then
>  		echo "failed: ${id} is not reported by \"perf buildid-cache -l\""
>  		exit 1
> @@ -81,7 +104,7 @@ test_record()
>  	build_id_dir=$(mktemp -d /tmp/perf.debug.XXX)
>  	perf="perf --buildid-dir ${build_id_dir}"
>  
> -	${perf} record --buildid-all -o ${data} ${1}
> +	${perf} record --buildid-all -o ${data} ${2} ${1}

it could be better just pass $@ and make sure test_record
args are passed in a way that record would accept them

  test_record wine ${ex_pe}


>  	if [ $? -ne 0 ]; then
>  		echo "failed: record ${1}"
>  		exit 1
> @@ -96,12 +119,22 @@ test_record()
>  # add binaries manual via perf buildid-cache -a
>  test_add ${ex_sha1}
>  test_add ${ex_md5}
> +if [ $add_pe -eq 1 ]; then

${add_pe}

> +    test_add ${ex_pe}
> +fi
>  
>  # add binaries via perf record post processing
>  test_record ${ex_sha1}
>  test_record ${ex_md5}
> +if [ $run_pe -eq 1 ]; then

${run_pe}

> +    test_record ${ex_pe} wine

I'm getting lot of wine's output, we should redirect that

every other run I'm getting some small window popup saying it's
updating wine and stuck forever.. could this be prevented?

> +fi
>  
>  # cleanup
>  rm ${ex_sha1} ${ex_md5}
>  
> +if [ $add_pe -eq 0 ] || [ $run_pe -eq 0 ]; then
> +    echo "WARNING: some PE tests were skipped. See previous warnings."
> +fi

there's already a warning for this at the beginning,
I dont think we need another one

thanks,
jirka

> +
>  exit ${err}
> -- 
> 2.30.1
> 


^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH 2/2] perf buildid-cache: Add test for PE executable
  2021-02-24 13:43 ` Jiri Olsa
@ 2021-02-24 17:40   ` Nicholas Fraser
  0 siblings, 0 replies; 3+ messages in thread
From: Nicholas Fraser @ 2021-02-24 17:40 UTC (permalink / raw)
  To: Jiri Olsa
  Cc: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
	Mark Rutland, Alexander Shishkin, Namhyung Kim, Ian Rogers,
	linux-kernel, Ulrich Czekalla, Huw Davies


On 2021-02-24 8:43 a.m., Jiri Olsa wrote:
> On Fri, Feb 19, 2021 at 11:10:34AM -0500, Nicholas Fraser wrote:
>> +		# the build id must be rearranged into a GUID
>> +		id=`objcopy -O binary --only-section=.buildid $1 /dev/stdout | \
>> +			cut -c 33-48 | hexdump -ve '/1 "%02x"' | \
>> +			sed 's@^\(..\)\(..\)\(..\)\(..\)\(..\)\(..\)\(..\)\(..\)\(.*\)0a$@\4\3\2\1\6\5\8\7\9@'`
>> +		;;
> 
> wow ;-) could this have some more info on what's going on in here?
> what's the .buildid PE section format?
> 

The .buildid section is in the PE debug directory format. This has a 28-byte
header:

https://docs.microsoft.com/en-us/windows/win32/debug/pe-format#the-debug-section

This contains a CodeView entry with a GUID only (no path). The GUID is 4 bytes in:

https://github.com/dotnet/runtime/blob/master/docs/design/specs/PE-COFF.md#codeview-debug-directory-entry-type-2

This means the GUID starts at byte 32 (or 33 since cut uses 1-based indexing.)
The snippet of code extracts the .buildid section with objcopy, cuts bytes
33-48, converts them to hex, and re-arranges the bytes to form a GUID (the
first three fields of a GUID are little-endian.)

Technically, bytes 20-24 contain a pointer to the data, which could be
anywhere. In practice the format of this section is fixed in order to support
reproducible builds (so the TimeDateStamp for example is always zero.) This is
something created by the MinGW (and Cygwin?) compilers only; as far as I know
it's not created by MSVC tools. So I don't think we need to do any more
complicated parsing than just pulling out those bytes.

Of course if there is a tool that can pull out the build-id directly we should
use that instead. I don't know of any at the moment though.

I'll supply another patch to fix the other issues here. I'll add a couple more
comments and a better commit message as well.

> 
> I'm getting lot of wine's output, we should redirect that

No problem, I can redirect the output. I left it in because if the output is
hidden it's not clear when the command hangs or fails. I could redirect it to a
temp file instead and leave the temp file in case of failure.

> 
> every other run I'm getting some small window popup saying it's
> updating wine and stuck forever.. could this be prevented?
> 

Hmm. It's possible it's stuck behind a GUI prompt. For example if you don't
have wine-gecko installed it might waiting on a dialog asking to install it.
I'll unset DISPLAY so it can't pop up any dialogs; this should make it fully
non-interactive.

Other than that, it could take some time to set up the wine prefix (the
location where wine stores its runtime configuration) and it might have
something broken in the prefix, for example if it was interrupted while setting
it up. I'll change it to use a temporary wine prefix instead. This means it
will have to do the initialization on every run but it should be more reliable.

^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2021-02-24 17:41 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-02-19 16:10 [PATCH 2/2] perf buildid-cache: Add test for PE executable Nicholas Fraser
2021-02-24 13:43 ` Jiri Olsa
2021-02-24 17:40   ` Nicholas Fraser

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).