Re: [PATCH v3 13/17] perf pmu-events: Split big_c_string storage into standalone compilation unit

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] Incremental build breakage causing `perf` to be unconditionally relinked on every `make` invocation.
- [Low] Missing variable declaration for `big_c_string` causes a compiler warning/error on Clang.
- [Low] Fragile multiple-output dependency chaining causes an unrecoverable build loop if the secondary file is deleted.
--

commit c21d41481809344c6220821a1680882429693c05
Author: Ian Rogers <irogers@google.com>

perf pmu-events: Split big_c_string storage into standalone compilation unit

This commit refactors jevents.py to output the big_c_string literal into
a separate pmu-events-string.c file. It also updates the perf Makefile
to build this new file, using a dependency chain pattern to avoid multiple
jevents.py executions in parallel builds.

> diff --git a/tools/perf/pmu-events/Build b/tools/perf/pmu-events/Build
> index dc1df2d57ddc1..95172a2a851f5 100644
> --- a/tools/perf/pmu-events/Build
> +++ b/tools/perf/pmu-events/Build

[ ... ]

> @@ -224,6 +234,10 @@ endif
>  # and inputs are dependencies.
>  $(PMU_EVENTS_C): $(JEVENTS_DEPS)
>  	$(call rule_mkdir)
> -	$(Q)$(call echo-cmd,gen)$(PYTHON) $(JEVENTS_PY) $(JEVENTS_ARCH) $(JEVENTS_MODEL) $(OUT_DIR) $@
> +	$(Q)$(call echo-cmd,gen)$(PYTHON) $(JEVENTS_PY) $(JEVENTS_ARCH) $(JEVENTS_MODEL) \
> +            $(OUT_DIR) $(PMU_EVENTS_C) $(PMU_EVENTS_STRING_C)
> +
> +$(PMU_EVENTS_STRING_C): $(PMU_EVENTS_C)
> +	@:

If a user manually deletes pmu-events-string.c, won't Make execute this dummy
recipe, assume the file was recreated, and then fail during compilation?

This could leave the build in an unrecoverable state until pmu-events.c is
also deleted, which seems contrary to the fully self-correcting goal mentioned
in the commit message.

> diff --git a/tools/perf/pmu-events/jevents.py b/tools/perf/pmu-events/jevents.py
> index 3a1bcdcdc685f..70a45e62f5d16 100755
> --- a/tools/perf/pmu-events/jevents.py
> +++ b/tools/perf/pmu-events/jevents.py

[ ... ]

> @@ -1463,10 +1465,20 @@ struct pmu_table_entry {
>      ftw(arch_path, [], preprocess_one_file)
>  
>    _bcs.compute()
> -  _args.output_file.write('static const char *const big_c_string =\n')
> -  for s in _bcs.big_string:
> -    _args.output_file.write(s)
> -  _args.output_file.write(';\n\n')
> +  if not _args.output_string_file:
> +    _args.output_file.write('static const char *const big_c_string =\n')
> +    for s in _bcs.big_string:
> +      _args.output_file.write(s)
> +    _args.output_file.write(';\n\n')
> +  else:
> +    _args.output_string_file.write('/* SPDX-License-Identifier: GPL-2.0 */\n')
> +    _args.output_string_file.write('/* Autogenerated by jevents.py */\n')
> +    _args.output_string_file.write('const char big_c_string[] =\n')
> +    for s in _bcs.big_string:
> +      _args.output_string_file.write(s)
> +    _args.output_string_file.write(';\n')

Will compiling this file with Clang trigger a missing-variable-declarations
warning?

It looks like big_c_string is defined globally here without a prior extern
declaration in the same compilation unit, which could cause a build failure
when -Werror is enabled.

> +    _args.output_string_file.close()

Does closing output_string_file here cause an incremental build breakage?

Since output_file is written to during the subsequent ftw calls that parse
the architecture JSON files, output_string_file will end up with an older
modification timestamp than output_file (which corresponds to pmu-events.c).

Combined with the Makefile rule $(PMU_EVENTS_STRING_C): $(PMU_EVENTS_C)
having a dummy @: recipe, Make will see pmu-events-string.c is older than
pmu-events.c on every invocation, and unconditionally rebuild
pmu-events-string.o and relink the perf binary.

> +    _args.output_file.write('extern const char big_c_string[];\n\n')
>    for arch in archs:
>      arch_path = f'{_args.starting_dir}/{arch}'
>      ftw(arch_path, [], process_one_file)

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260514163409.927816-1-irogers@google.com?part=13