Re: [PATCH v5 4/4] perf tools: Add test for weight annotation

[Andi Kleen <ak@linux.intel.com>]

> > +# mem-loads:pu requests a precise user PEBS event whose sample weight should
> > +# be populated by -W. Unsupported PEBS/weight PMUs are skipped below.
> > +if ! perf record -W -e mem-loads:pu -o "${perfdata}" -- perf test -w thloop \
> > +	> /dev/null 2> "${record_log}"; then
> > +	echo "[SKIP] precise PEBS weight sampling is unavailable"
> > +	exit 2
> > +fi
> 
> [Severity: Medium]
> If the inner workload (perf test -w thloop) crashes or otherwise fails to
> execute, does this conditional block improperly catch the failure and report 
> it as an unsupported hardware feature? 
> 
> It looks like any non-zero exit status from the child process will result 
> in a skipped test, which might mask underlying environment or test suite bugs.


There are other tests for this, so this is acceptable.

> 
> [ ... ]
> > +	echo "[SKIP] PEBS weight sampling produced no nonzero weights"
> > +	exit 2
> > +fi
> > +
> > +perf annotate --stdio -i "${perfdata}" --symbol "${testsym}" \
> > +	> "${annotate_out}" 2> "${record_log}"
> > +
> > +grep -q 'Percent Weight' "${annotate_out}"
> 
> [Severity: Medium]
> Since set -e is enabled at the top of the script, will this isolated grep 
> command cause an abrupt and silent exit if the 'Percent Weight' header is 
> missing from the annotation output? 
> 
> If the string is missing, grep will exit with code 1, which might instantly 
> abort the script without printing diagnostic output, making regressions harder 
> to debug.

it will exit with 1 which makes perf test fail I believe.




-Andi