Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Wed, 24 Feb 2021 14:43:44 +0100
From:   Jiri Olsa <jolsa@redhat.com>
To:     Nicholas Fraser <nfraser@codeweavers.com>
Cc:     Peter Zijlstra <peterz@infradead.org>,
        Ingo Molnar <mingo@redhat.com>,
        Arnaldo Carvalho de Melo <acme@kernel.org>,
        Mark Rutland <mark.rutland@arm.com>,
        Alexander Shishkin <alexander.shishkin@linux.intel.com>,
        Namhyung Kim <namhyung@kernel.org>,
        Ian Rogers <irogers@google.com>, linux-kernel@vger.kernel.org,
        Ulrich Czekalla <uczekalla@codeweavers.com>,
        Huw Davies <huw@codeweavers.com>
Subject: Re: [PATCH 2/2] perf buildid-cache: Add test for PE executable
Message-ID: <YDZYEKJ7w+XgqA7S@krava>
References: <295f5380-93a1-78fa-884b-afd4319b96d7@codeweavers.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <295f5380-93a1-78fa-884b-afd4319b96d7@codeweavers.com>
Precedence: bulk

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
>