Re: [PATCH 1/2 v2] perf script: Fix missing '+' indicator when branch counter reaches upper limit

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] The patch incorrectly prints the saturation indicator '+' when branch counter width is 0.
--

commit 7ec212bf30c80539ef5e37c2e9ff148610e094d5
Author: Dapeng Mi <dapeng1.mi@linux.intel.com>

perf script: Fix missing '+' indicator when branch counter reaches upper limit

This commit restores the missing "+" suffix indicator in perf script which
represents that branch counter events may have been lost due to saturation.

> diff --git a/tools/perf/builtin-script.c b/tools/perf/builtin-script.c
> index c8ac9f01a36b..f865c8b2f95f 100644
> --- a/tools/perf/builtin-script.c
> +++ b/tools/perf/builtin-script.c
> @@ -1287,8 +1287,12 @@ static int ip__fprintf_jump(uint64_t ip, struct branch_entry *en,
>  			if (!verbose) {
>  				for (j = 0; j < num; j++)
>  					printed += fprintf(fp, "%s", pos->abbr_name);
> -			} else
> -				printed += fprintf(fp, "%s %d ", pos->name, num);
> +				if (num == mask)

Could this incorrectly print the saturation indicator if the branch counter
width is 0?

Since mask is derived via (1L << width) - 1 earlier in the function:

    perf_env__find_br_cntr_info(evsel__env(evsel), NULL, &width);
    mask = (1L << width) - 1;

A width of 0 results in a mask of 0. In this scenario, num will also evaluate
to 0, making the condition num == mask evaluate to true.

Would it be better to verify that mask > 0 before checking for saturation?

> +					printed += fprintf(fp, "+");
> +			} else {
> +				printed += fprintf(fp, "%s %d%s", pos->name,
> +						   num, num == mask ? "+ " : " ");

Is a similar check needed here to prevent the same false positive in
verbose mode?

> +			}
>  		}
>  		if (numprinted == 0 && !verbose)
>  			printed += fprintf(fp, "-");

In non-verbose mode, a false positive on the mask check introduces a
regression where the output becomes +- since the numprinted == 0 condition
above will also append a "-", contradicting the intention that "-" means no
event occurred.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260429152917.178803-1-thomas.falcon@intel.com?part=1