Re: [PATCH v1 1/2] perf build-id: Align records to 8 rather than 64 bytes

[Ian Rogers]

On Mon, Aug 12, 2024 at 9:34 PM Ian Rogers <irogers@xxxxxxxxxx> wrote:
>
> Events are 8-byte aligned in perf.data files, NAME_ALIGN is 64-bytes
> which is excessive given the alignment needs. Move the addition the
> sizeof so that the alignment considers that the rest of the event is
> 4-byte aligned.
>
> ```
> struct perf_record_header_build_id {
>         struct perf_event_header   header;               /*     0     8 */
>         pid_t                      pid;                  /*     8     4 */
>         union {
>                 __u8               build_id[24];         /*    12    24 */
>                 struct {
>                         __u8       data[20];             /*    12    20 */
>                         __u8       size;                 /*    32     1 */
>                         __u8       reserved1__;          /*    33     1 */
>                         __u16      reserved2__;          /*    34     2 */
>                 };                                       /*    12    24 */
>         };                                               /*    12    24 */
>         char                       filename[];           /*    36     0 */
>
>         /* size: 36, cachelines: 1, members: 4 */
>         /* last cacheline: 36 bytes */
> };
> ```
>
> Signed-off-by: Ian Rogers <irogers@xxxxxxxxxx>
> ---
>  tools/perf/util/build-id.c         | 6 +++---
>  tools/perf/util/synthetic-events.c | 6 +++---
>  2 files changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/tools/perf/util/build-id.c b/tools/perf/util/build-id.c
> index 83a1581e8cf1..8fea17d04468 100644
> --- a/tools/perf/util/build-id.c
> +++ b/tools/perf/util/build-id.c
> @@ -309,8 +309,8 @@ static int write_buildid(const char *name, size_t name_len, struct build_id *bid
>         struct perf_record_header_build_id b;
>         size_t len;
>
> -       len = name_len + 1;
> -       len = PERF_ALIGN(len, NAME_ALIGN);
> +       len = sizeof(b) + name_len + 1;
> +       len = PERF_ALIGN(len, sizeof(u64));
>
>         memset(&b, 0, sizeof(b));
>         memcpy(&b.data, bid->data, bid->size);
> @@ -318,7 +318,7 @@ static int write_buildid(const char *name, size_t name_len, struct build_id *bid
>         misc |= PERF_RECORD_MISC_BUILD_ID_SIZE;
>         b.pid = pid;
>         b.header.misc = misc;
> -       b.header.size = sizeof(b) + len;
> +       b.header.size = len;
>
>         err = do_write(fd, &b, sizeof(b));
>         if (err < 0)
> diff --git a/tools/perf/util/synthetic-events.c b/tools/perf/util/synthetic-events.c
> index 5498048f56ea..a52b85bcb6b6 100644
> --- a/tools/perf/util/synthetic-events.c
> +++ b/tools/perf/util/synthetic-events.c
> @@ -2236,14 +2236,14 @@ int perf_event__synthesize_build_id(struct perf_tool *tool, struct dso *pos, u16
>
>         memset(&ev, 0, sizeof(ev));
>
> -       len = dso__long_name_len(pos) + 1;
> -       len = PERF_ALIGN(len, NAME_ALIGN);
> +       len = sizeof(ev.build_id) + dso__long_name_len(pos) + 1;
> +       len = PERF_ALIGN(len, sizeof(u64));
>         ev.build_id.size = min(dso__bid(pos)->size, sizeof(dso__bid(pos)->data));
>         memcpy(&ev.build_id.build_id, dso__bid(pos)->data, ev.build_id.size);
>         ev.build_id.header.type = PERF_RECORD_HEADER_BUILD_ID;
>         ev.build_id.header.misc = misc | PERF_RECORD_MISC_BUILD_ID_SIZE;
>         ev.build_id.pid = machine->pid;
> -       ev.build_id.header.size = sizeof(ev.build_id) + len;
> +       ev.build_id.header.size = len;
>         memcpy(&ev.build_id.filename, dso__long_name(pos), dso__long_name_len(pos));

So I think there is a bigger issue here and I'm working on a better
fix. The issue is that the synthesized build_id event doesn't have a
sample id inserted afterward (note, no adding of id_hdr_size above).
By over aligning the filename it is quite likely the sample id reading
is just reading zeros in particular for the time stamp. Not having a
sample id means the build_id events are unordered, this will break if
the data file has two dsos with the same filename but different build
ids - as happens when a file is replaced. The better fix is to 8 byte
align the event and insert a sample id.

Thanks,
Ian

>         return process(tool, &ev, NULL, machine);
> --
> 2.46.0.76.ge559c4bf1a-goog
>