Re: [PATCH] perf tools: Handle old data in PERF_RECORD_ATTR

[Ian Rogers]

On Sun, Aug 6, 2023 at 11:16 PM Namhyung Kim <namhyung@xxxxxxxxxx> wrote:
>
> The PERF_RECORD_ATTR is used for a pipe mode to describe an event with
> attribute and IDs.  The ID table comes after the attr and it calculate
> size of the table using the total record size and the attr size.
>
>   n_ids = (total_record_size - end_of_the_attr_field) / sizeof(u64)
>
> This is fine for most use cases, but sometimes it saves the pipe output
> in a file and then process it later.  And it becomes a problem if there
> is a change in attr size between the record and report.
>
>   $ perf record -o- > perf-pipe.data  # old version
>   $ perf report -i- < perf-pipe.data  # new version
>
> For example, if the attr size is 128 and it has 4 IDs, then it would
> save them in 168 byte like below:
>
>    8 byte: perf event header { .type = PERF_RECORD_ATTR, .size = 168 },
>  128 byte: perf event attr { .size = 128, ... },
>   32 byte: event IDs [] = { 1234, 1235, 1236, 1237 },
>
> But when report later, it thinks the attr size is 136 then it only read
> the last 3 entries as ID.
>
>    8 byte: perf event header { .type = PERF_RECORD_ATTR, .size = 168 },
>  136 byte: perf event attr { .size = 136, ... },
>   24 byte: event IDs [] = { 1235, 1236, 1237 },  // 1234 is missing
>
> So it should use the recorded version of the attr.  The attr has the
> size field already then it should honor the size when reading data.
>
> Signed-off-by: Namhyung Kim <namhyung@xxxxxxxxxx>

Reviewed-by: Ian Rogers <irogers@xxxxxxxxxx>

Thanks,
Ian

> ---
>  tools/perf/util/header.c | 11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)
>
> diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c
> index 52fbf526fe74..f89321cbfdee 100644
> --- a/tools/perf/util/header.c
> +++ b/tools/perf/util/header.c
> @@ -4381,7 +4381,8 @@ int perf_event__process_attr(struct perf_tool *tool __maybe_unused,
>                              union perf_event *event,
>                              struct evlist **pevlist)
>  {
> -       u32 i, ids, n_ids;
> +       u32 i, n_ids;
> +       u64 *ids;
>         struct evsel *evsel;
>         struct evlist *evlist = *pevlist;
>
> @@ -4397,9 +4398,8 @@ int perf_event__process_attr(struct perf_tool *tool __maybe_unused,
>
>         evlist__add(evlist, evsel);
>
> -       ids = event->header.size;
> -       ids -= (void *)&event->attr.id - (void *)event;
> -       n_ids = ids / sizeof(u64);
> +       n_ids = event->header.size - sizeof(event->header) - event->attr.attr.size;
> +       n_ids = n_ids / sizeof(u64);
>         /*
>          * We don't have the cpu and thread maps on the header, so
>          * for allocating the perf_sample_id table we fake 1 cpu and
> @@ -4408,8 +4408,9 @@ int perf_event__process_attr(struct perf_tool *tool __maybe_unused,
>         if (perf_evsel__alloc_id(&evsel->core, 1, n_ids))
>                 return -ENOMEM;
>
> +       ids = (void *)&event->attr.attr + event->attr.attr.size;
>         for (i = 0; i < n_ids; i++) {
> -               perf_evlist__id_add(&evlist->core, &evsel->core, 0, i, event->attr.id[i]);
> +               perf_evlist__id_add(&evlist->core, &evsel->core, 0, i, ids[i]);
>         }
>
>         return 0;
> --
> 2.41.0.640.ga95def55d0-goog
>