Re: [PATCH v4 3/8] perf header: Properly warn/print when libtraceevent/libbpf support is missing

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Returning -1 for unsupported features in `process_*` callbacks causes `perf_header__process_sections()` to abort prematurely, silently skipping all subsequent header features.
- [Medium] `perf record` unconditionally prints errors for BPF features when compiled without `libbpf`.
--

--- Patch [3]: [PATCH v4 3/8] perf header: Properly warn/print when libtraceevent/libbpf support is missing ---
commit e5ac8f49aacee07fccb1639de8607d4e1627acf5
Author: Ian Rogers <[email protected]>

perf header: Properly warn/print when libtraceevent/libbpf support is missing

By removing the features from feat_ops with ifdefs the previous logic
would print "# (null)" when perf processed a feature that lacked
builtin support. Remove the ifdefs from feat_ops and in the relevant
functions print errors/messages about the lack of support.

> diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c
> --- a/tools/perf/util/header.c
> +++ b/tools/perf/util/header.c
[ ... ]
> -#ifdef HAVE_LIBBPF_SUPPORT
> -static int write_bpf_prog_info(struct feat_fd *ff,
> +static int write_bpf_prog_info(struct feat_fd *ff  __maybe_unused,
>  			       struct evlist *evlist __maybe_unused)
>  {
> +#ifdef HAVE_LIBBPF_SUPPORT
>  	struct perf_env *env = &ff->ph->env;
>  	struct rb_root *root;
>  	struct rb_node *next;
[ ... ]
> @@ -1067,11 +1070,16 @@ static int write_bpf_prog_info(struct feat_fd *ff,
>  out:
>  	up_read(&env->bpf_progs.lock);
>  	return ret;
> +#else
> +	pr_err("ERROR: Trying to write bpf_prog_info without libbpf support.\n");
> +	return -1;
> +#endif // HAVE_LIBBPF_SUPPORT
>  }

[Severity: Medium]
Will this cause spurious error messages and failures on every invocation of
perf record when compiled without libbpf?

Previously, when HAVE_LIBBPF_SUPPORT was absent, the feature was missing from
feat_ops and silently skipped. Because record__init_features() unconditionally
sets HEADER_BPF_PROG_INFO, won't perf_header__adds_write() now call this
function, print the error, and return -1 on every run?

[ ... ]
> -static int write_bpf_btf(struct feat_fd *ff,
> +static int write_bpf_btf(struct feat_fd *ff __maybe_unused,
>  			 struct evlist *evlist __maybe_unused)
>  {
> +#ifdef HAVE_LIBBPF_SUPPORT
>  	struct perf_env *env = &ff->ph->env;
>  	struct rb_root *root;
>  	struct rb_node *next;
[ ... ]
> @@ -1100,8 +1108,11 @@ static int write_bpf_btf(struct feat_fd *ff,
>  out:
>  	up_read(&env->bpf_progs.lock);
>  	return ret;
> -}
> +#else
> +	pr_err("ERROR: Trying to write btf data without libbpf support.\n");
> +	return -1;
>  #endif // HAVE_LIBBPF_SUPPORT
> +}

[Severity: Medium]
Similar to the issue above, won't this unconditionally print an error and
fail perf record if libbpf is not supported, since HEADER_BPF_BTF is also
unconditionally set in record__init_features()?

[ ... ]
> -#ifdef HAVE_LIBTRACEEVENT
> -static int process_tracing_data(struct feat_fd *ff, void *data)
> +static int process_tracing_data(struct feat_fd *ff __maybe_unused, void *data __maybe_unused)
>  {
> +#ifdef HAVE_LIBTRACEEVENT
>  	ssize_t ret = trace_report(ff->fd, data, false);
>  
>  	return ret < 0 ? -1 : 0;
> -}
> +#else
> +	pr_err("ERROR: Trying to read tracing data without libtraceevent support.\n");
> +	return -1;
>  #endif
> +}

[Severity: High]
Does returning -1 here cause perf_header__process_sections() to abort
prematurely and silently skip all remaining header features?

When processing features, perf_header__process_sections() iterates over the
added features:

    for_each_set_bit(feat, header->adds_features, HEADER_LAST_FEATURE) {
        err = process(sec++, header, feat, fd, data);
        if (err < 0)
            goto out_free;
    }

If perf encounters a perf.data file containing HEADER_TRACING_DATA but lacks
libtraceevent support, it looks like it will break out of the loop and skip
any features that appear after it in the bitmask (such as COMPRESSED or
CPU_PMU_CAPS), which could cause downstream failures like missing
decompression metadata.

Should this return 0 instead of -1 so that processing can continue?

[ ... ]
> -#ifdef HAVE_LIBBPF_SUPPORT
> -static int process_bpf_prog_info(struct feat_fd *ff, void *data __maybe_unused)
> +static int process_bpf_prog_info(struct feat_fd *ff __maybe_unused, void *data __maybe_unused)
>  {
> +#ifdef HAVE_LIBBPF_SUPPORT
>  	struct bpf_prog_info_node *info_node;
>  	struct perf_env *env = &ff->ph->env;
>  	struct perf_bpil *info_linear;
[ ... ]
> @@ -3402,10 +3422,15 @@ static int process_bpf_prog_info(struct feat_fd *ff, void *data __maybe_unused)
>  	free(info_node);
>  	up_write(&env->bpf_progs.lock);
>  	return err;
> +#else
> +	pr_err("ERROR: Trying to read bpf_prog_info without libbpf support.\n");
> +	return -1;
> +#endif // HAVE_LIBBPF_SUPPORT
>  }

[Severity: High]
Like process_tracing_data() above, won't returning -1 here abort
perf_header__process_sections() and skip the rest of the features in the file
if libbpf is not supported?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=3