Re: [PATCH v2 2/2] perf test: Test piped data in inject-callchain.sh

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential
issues to consider:
- [High] perf test: missing Signed-off-by trailer in commit message
- [Medium] perf test: trap_err() overwrites $? and exits 0 on command
  failure
- [Medium] perf test: pipelines ending in cat without pipefail mask perf
  crashes
--

commit e5bf6b4a81ccc0d0065f5b23fedc941ae4553914
Author: Serhei Makarov <serhei@serhei.io>

> perf test: Test piped data in inject-callchain.sh
>
> Expand the test to verify --convert-callchain on piped data.
>
> Also, check that it's now safe to run 'perf inject --convert-callchain'
> on events without DWARF callchain data. In the prior version of the
> code, the up-front evsel check (not possible to do with piping) was
> guarding against a segfault in the prior version of the code.
>
> $ /opt/perf/bin/perf test -vv "DWARF callchain"
>  89: perf inject to convert DWARF callchains to regular ones:

[Severity: High]
This commit message is missing a Signed-off-by trailer.

> diff --git a/tools/perf/tests/shell/inject-callchain.sh b/tools/perf/tests/shell/inject-callchain.sh
> index a1cba8010f954..900459a8dc9e8 100755
> --- a/tools/perf/tests/shell/inject-callchain.sh
> +++ b/tools/perf/tests/shell/inject-callchain.sh
> @@ -23,23 +23,74 @@ trap_cleanup()
>  	exit 1
>  }
>  
> +trap_err()
> +{
> +    local SIG=$?
> +    if (( $? == 139 )); then #SIGSEGV
> +        err=1
> +    fi
> +    echo "Unexpected signal ${SIG} in ${FUNCNAME[1]}"
> +    cleanup
> +    exit ${err}
> +}

[Severity: Medium]
Does trap_err() exit with status 0 when a command fails or crashes?

Running "local SIG=$?" overwrites $? with the return status of the local
builtin (0), so "if (( $? == 139 ))" is always false.  Because err starts at
0 and is never updated for 139 or any other failure, "exit ${err}" causes the
test to pass silently when the trap triggers.

> +
>  trap trap_cleanup EXIT TERM INT
> +trap trap_err ERR
> +
> +echo "recording piped data without DWARF callchain"
> +perf record -F 999 -o - -- perf test -w noploop | cat > ${TESTDATA}.nodwarf
>  
>  echo "recording data with DWARF callchain"
>  perf record -F 999 --call-graph dwarf -o "${TESTDATA}" -- perf test -w noploop
>  
> +echo "recording piped data with DWARF callchain"
> +perf record -F 999 --call-graph dwarf -o - -- perf test -w noploop | cat > ${TESTDATA}.piped
> +
> +echo "check --convert-callchain without DWARF data"
> +# verify that running on non-DWARF data does not produce segfault
> +perf inject -i "${TESTDATA}.nodwarf" --convert-callchain -o "${TESTDATA}.nodwarf.conv"
> +cat "${TESTDATA}.nodwarf" | perf inject --convert-callchain -i - -o "${TESTDATA}.nodwarf.conv2"
> +cat "${TESTDATA}.nodwarf" | perf inject --convert-callchain -i - -o - | cat > ${TESTDATA}.nodwarf.conv3

[Severity: Medium]
Without "set -o pipefail", does piping perf record or perf inject into cat
mask crashes and non-zero exit statuses?  When an upstream command in these
pipelines segfaults, the pipeline still returns 0 from the trailing cat, and
because ${TESTDATA}.nodwarf.conv3 is never inspected afterwards, a crash in
that perf inject invocation goes undetected.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261112201833.78630-1-serhei@serhei.io?part=2