Re: [PATCH v2 05/18] perf test: Add deterministic workload

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Medium] Signed integer overflow in dt_work causes Undefined Behavior
- [Medium] Workload state leakage breaks data determinism on consecutive runs
- [Low] Build failure due to missing `-U_FORTIFY_SOURCE` with `-O0`
- [Low] Global namespace pollution from non-static `dt_work`
--

commit cdb1ed35f7dbaf322c0b18289b38a7da8d21039e
Author: James Clark <james.clark@linaro.org>

perf test: Add deterministic workload

This commit adds a new workload for testing CPU trace decoding. It introduces
a deterministic sequence of multiplications and function calls that can be 
used to verify expected behavior across test runs.

[ ... ]

> diff --git a/tools/perf/tests/workloads/Build b/tools/perf/tests/workloads/Build
> index 3bda6da04a350..599a37c87878f 100644
> --- a/tools/perf/tests/workloads/Build
> +++ b/tools/perf/tests/workloads/Build

[ ... ]

> @@ -22,3 +23,4 @@ CFLAGS_brstack.o          = -g -O0 -fno-inline -U_FORTIFY_SOURCE
>  CFLAGS_datasym.o          = -g -O0 -fno-inline -U_FORTIFY_SOURCE
>  CFLAGS_traploop.o         = -g -O0 -fno-inline -U_FORTIFY_SOURCE
>  CFLAGS_inlineloop.o       = -g -O2
> +CFLAGS_deterministic.o    = -g -O0

[Severity: Low]
Does this code introduce a build regression on systems with glibc?

Since the perf build system globally defines _FORTIFY_SOURCE=2 and enables
-Werror by default, compiling with -O0 without also passing -U_FORTIFY_SOURCE
can trigger a warning from <features.h> ("_FORTIFY_SOURCE requires compiling
with optimization (-O)"), which is then escalated to an error.

> diff --git a/tools/perf/tests/workloads/deterministic.c b/tools/perf/tests/workloads/deterministic.c
> new file mode 100644
> index 0000000000000..3caea8564043c
> --- /dev/null
> +++ b/tools/perf/tests/workloads/deterministic.c
> @@ -0,0 +1,39 @@
> +// SPDX-License-Identifier: GPL-2.0
> +#include <linux/compiler.h>
> +#include "../tests.h"
> +
> +int dt_work = 1234;

[Severity: Low]
Should dt_work be declared static?

It is currently declared at global scope without a unique prefix, which exposes
it to the entire perf tool binary.

[Severity: Medium]
Will dt_work experience undefined behavior due to signed integer overflow?

The variable is initialized to 1234 and is unconditionally multiplied by 7
twelve times during the execution of deterministic(). The 8th multiplication
results in 7,113,764,434, which exceeds the maximum value of a 32-bit signed
integer.

Could this cause crashes if the perf tool is built with undefined behavior
sanitizer? Perhaps dt_work could be unsigned int or unsigned long long
instead.

> +
> +static noinline void function1(void)
> +{
> +	dt_work *= 7;

[ ... ]

> +static int deterministic(int argc __maybe_unused,
> +			 const char **argv __maybe_unused)
> +{

[Severity: Medium]
Does this code break data determinism on consecutive test runs?

If the workload is executed multiple times within the same process (for example,
using perf test -w deterministic -r N), dt_work will start from the final
overflowed value of the previous run rather than being reset to 1234.

> +	dt_work *= 7;
> +	dt_work *= 7;
> +	dt_work *= 7;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260602-james-cs-context-tracking-fix-v2-0-85b5ce6f55c6@linaro.org?part=5