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

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

[Ian Rogers]

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@google.com>
---
 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));
 
 	return process(tool, &ev, NULL, machine);
-- 
2.46.0.76.ge559c4bf1a-goog

[PATCH v1 2/2] perf test: Expand pipe/inject test

[Ian Rogers]

Test recording of call-graphs and injecting --build-all. Add/expand
trap handler.

Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/tests/shell/pipe_test.sh | 101 ++++++++++++++++++++++------
 1 file changed, 79 insertions(+), 22 deletions(-)

diff --git a/tools/perf/tests/shell/pipe_test.sh b/tools/perf/tests/shell/pipe_test.sh
index a78d35d2cff0..ad10012fdc29 100755
--- a/tools/perf/tests/shell/pipe_test.sh
+++ b/tools/perf/tests/shell/pipe_test.sh
@@ -1,4 +1,4 @@
-#!/bin/sh
+#!/bin/bash
 # perf pipe recording and injection test
 # SPDX-License-Identifier: GPL-2.0
 
@@ -12,30 +12,87 @@ skip_test_missing_symbol ${sym}
 
 data=$(mktemp /tmp/perf.data.XXXXXX)
 prog="perf test -w noploop"
-task="perf"
+err=0
 
-if ! perf record -e task-clock:u -o - ${prog} | perf report -i - --task | grep ${task}; then
-	echo "cannot find the test file in the perf report"
-	exit 1
-fi
+set -e
 
-if ! perf record -e task-clock:u -o - ${prog} | perf inject -b | perf report -i - | grep ${sym}; then
-	echo "cannot find noploop function in pipe #1"
-	exit 1
-fi
+cleanup() {
+  rm -rf "${data}"
+  rm -rf "${data}".old
 
-perf record -e task-clock:u -o - ${prog} | perf inject -b -o ${data}
-if ! perf report -i ${data} | grep ${sym}; then
-	echo "cannot find noploop function in pipe #2"
-	exit 1
-fi
+  trap - EXIT TERM INT
+}
 
-perf record -e task-clock:u -o ${data} ${prog}
-if ! perf inject -b -i ${data} | perf report -i - | grep ${sym}; then
-	echo "cannot find noploop function in pipe #3"
-	exit 1
-fi
+trap_cleanup() {
+  echo "Unexpected signal in ${FUNCNAME[1]}"
+  cleanup
+  exit 1
+}
+trap trap_cleanup EXIT TERM INT
 
+test_record_report() {
+  echo
+  echo "Record+report pipe test"
+
+  task="perf"
+  if ! perf record -e task-clock:u -o - ${prog} | perf report -i - --task | grep -q ${task}
+  then
+    echo "Record+report pipe test [Failed - cannot find the test file in the perf report #1]"
+    err=1
+    return
+  fi
+
+  if ! perf record -g -e task-clock:u -o - ${prog} | perf report -i - --task | grep -q ${task}
+  then
+    echo "Record+report pipe test [Failed - cannot find the test file in the perf report #2]"
+    err=1
+    return
+  fi
+
+  echo "Record+report pipe test [Success]"
+}
+
+test_inject_bids() {
+  inject_opt=$1
+
+  echo
+  echo "Inject ${inject_opt} build-ids test"
+
+  if ! perf record -e task-clock:u -o - ${prog} | perf inject ${inject_opt}| perf report -i - | grep -q ${sym}
+  then
+    echo "Inject build-ids test [Failed - cannot find noploop function in pipe #1]"
+    err=1
+    return
+  fi
+
+  if ! perf record -g -e task-clock:u -o - ${prog} | perf inject ${inject_opt} | perf report -i - | grep -q ${sym}
+  then
+    echo "Inject ${inject_opt} build-ids test [Failed - cannot find noploop function in pipe #2]"
+    err=1
+    return
+  fi
+
+  perf record -e task-clock:u -o - ${prog} | perf inject ${inject_opt} -o ${data}
+  if ! perf report -i ${data} | grep -q ${sym}; then
+    echo "Inject ${inject_opt} build-ids test [Failed - cannot find noploop function in pipe #3]"
+    err=1
+    return
+  fi
+
+  perf record -e task-clock:u -o ${data} ${prog}
+  if ! perf inject ${inject_opt} -i ${data} | perf report -i - | grep -q ${sym}; then
+    echo "Inject ${inject_opt} build-ids test [Failed - cannot find noploop function in pipe #4]"
+    err=1
+    return
+  fi
+
+  echo "Inject ${inject_opt} build-ids test [Success]"
+}
+
+test_record_report
+test_inject_bids -b
+test_inject_bids --buildid-all
+
+cleanup
+exit $err
 
-rm -f ${data} ${data}.old
-exit 0
-- 
2.46.0.76.ge559c4bf1a-goog

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@google.com> 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@google.com>
> ---
>  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
>