tools/perf/util/synthetic-events.c | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-)
The sample data is 64-bit aligned basically but raw data starts with
32-bit length field and data follows. In perf_event__synthesize_sample
it treats the sample data as a 64-bit array. And it needs some trick
to update the raw data properly.
But it seems some compilers are not happy with this and the program dies
siliently. I found the sample parsing test failed without any messages
on affected systems.
Let's update the code to use a 32-bit pointer directly and make sure the
result is 64-bit aligned again. No functional changes intended.
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
tools/perf/util/synthetic-events.c | 14 +++++++++-----
1 file changed, 9 insertions(+), 5 deletions(-)
diff --git a/tools/perf/util/synthetic-events.c b/tools/perf/util/synthetic-events.c
index a58444c4aed1f1ea..385383ef6cf1edaf 100644
--- a/tools/perf/util/synthetic-events.c
+++ b/tools/perf/util/synthetic-events.c
@@ -1686,12 +1686,16 @@ int perf_event__synthesize_sample(union perf_event *event, u64 type, u64 read_fo
}
if (type & PERF_SAMPLE_RAW) {
- u.val32[0] = sample->raw_size;
- *array = u.val64;
- array = (void *)array + sizeof(u32);
+ u32 *array32 = (void *)array;
+
+ *array32 = sample->raw_size;
+ array32++;
+
+ memcpy(array32, sample->raw_data, sample->raw_size);
+ array = (void *)(array32 + (sample->raw_size / sizeof(u32)));
- memcpy(array, sample->raw_data, sample->raw_size);
- array = (void *)array + sample->raw_size;
+ /* make sure the array is 64-bit aligned */
+ BUG_ON(((long)array) / sizeof(u64));
}
if (type & PERF_SAMPLE_BRANCH_STACK) {
--
2.47.0.338.g60cca15819-googOn Wed, Nov 27, 2024 at 1:26 PM Namhyung Kim <namhyung@kernel.org> wrote:
>
> The sample data is 64-bit aligned basically but raw data starts with
> 32-bit length field and data follows. In perf_event__synthesize_sample
> it treats the sample data as a 64-bit array. And it needs some trick
> to update the raw data properly.
>
> But it seems some compilers are not happy with this and the program dies
> siliently. I found the sample parsing test failed without any messages
> on affected systems.
>
> Let's update the code to use a 32-bit pointer directly and make sure the
> result is 64-bit aligned again. No functional changes intended.
>
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> ---
> tools/perf/util/synthetic-events.c | 14 +++++++++-----
> 1 file changed, 9 insertions(+), 5 deletions(-)
>
> diff --git a/tools/perf/util/synthetic-events.c b/tools/perf/util/synthetic-events.c
> index a58444c4aed1f1ea..385383ef6cf1edaf 100644
> --- a/tools/perf/util/synthetic-events.c
> +++ b/tools/perf/util/synthetic-events.c
> @@ -1686,12 +1686,16 @@ int perf_event__synthesize_sample(union perf_event *event, u64 type, u64 read_fo
> }
>
> if (type & PERF_SAMPLE_RAW) {
> - u.val32[0] = sample->raw_size;
> - *array = u.val64;
> - array = (void *)array + sizeof(u32);
> + u32 *array32 = (void *)array;
> +
> + *array32 = sample->raw_size;
> + array32++;
> +
> + memcpy(array32, sample->raw_data, sample->raw_size);
> + array = (void *)(array32 + (sample->raw_size / sizeof(u32)));
>
> - memcpy(array, sample->raw_data, sample->raw_size);
> - array = (void *)array + sample->raw_size;
> + /* make sure the array is 64-bit aligned */
> + BUG_ON(((long)array) / sizeof(u64));
I think you intended:
BUG_ON(((long)array) % sizeof(u64));
Thanks,
Ian
> }
>
> if (type & PERF_SAMPLE_BRANCH_STACK) {
> --
> 2.47.0.338.g60cca15819-goog
>
On Thu, Dec 12, 2024 at 03:56:31PM -0800, Ian Rogers wrote:
> On Wed, Nov 27, 2024 at 1:26 PM Namhyung Kim <namhyung@kernel.org> wrote:
> >
> > The sample data is 64-bit aligned basically but raw data starts with
> > 32-bit length field and data follows. In perf_event__synthesize_sample
> > it treats the sample data as a 64-bit array. And it needs some trick
> > to update the raw data properly.
> >
> > But it seems some compilers are not happy with this and the program dies
> > siliently. I found the sample parsing test failed without any messages
> > on affected systems.
> >
> > Let's update the code to use a 32-bit pointer directly and make sure the
> > result is 64-bit aligned again. No functional changes intended.
> >
> > Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> > ---
> > tools/perf/util/synthetic-events.c | 14 +++++++++-----
> > 1 file changed, 9 insertions(+), 5 deletions(-)
> >
> > diff --git a/tools/perf/util/synthetic-events.c b/tools/perf/util/synthetic-events.c
> > index a58444c4aed1f1ea..385383ef6cf1edaf 100644
> > --- a/tools/perf/util/synthetic-events.c
> > +++ b/tools/perf/util/synthetic-events.c
> > @@ -1686,12 +1686,16 @@ int perf_event__synthesize_sample(union perf_event *event, u64 type, u64 read_fo
> > }
> >
> > if (type & PERF_SAMPLE_RAW) {
> > - u.val32[0] = sample->raw_size;
> > - *array = u.val64;
> > - array = (void *)array + sizeof(u32);
> > + u32 *array32 = (void *)array;
> > +
> > + *array32 = sample->raw_size;
> > + array32++;
> > +
> > + memcpy(array32, sample->raw_data, sample->raw_size);
> > + array = (void *)(array32 + (sample->raw_size / sizeof(u32)));
> >
> > - memcpy(array, sample->raw_data, sample->raw_size);
> > - array = (void *)array + sample->raw_size;
> > + /* make sure the array is 64-bit aligned */
> > + BUG_ON(((long)array) / sizeof(u64));
>
> I think you intended:
>
> BUG_ON(((long)array) % sizeof(u64));
Yep, fixed in v2.
https://lore.kernel.org/r/20241128010325.946897-1-namhyung@kernel.org
Thanks,
Namhyung
On Wed, Nov 27, 2024 at 1:26 PM Namhyung Kim <namhyung@kernel.org> wrote:
>
> The sample data is 64-bit aligned basically but raw data starts with
> 32-bit length field and data follows. In perf_event__synthesize_sample
> it treats the sample data as a 64-bit array. And it needs some trick
> to update the raw data properly.
>
> But it seems some compilers are not happy with this and the program dies
> siliently. I found the sample parsing test failed without any messages
> on affected systems.
>
> Let's update the code to use a 32-bit pointer directly and make sure the
> result is 64-bit aligned again. No functional changes intended.
>
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>
Reviewed-by: Ian Rogers <irogers@google.com>
Thanks,
Ian
> ---
> tools/perf/util/synthetic-events.c | 14 +++++++++-----
> 1 file changed, 9 insertions(+), 5 deletions(-)
>
> diff --git a/tools/perf/util/synthetic-events.c b/tools/perf/util/synthetic-events.c
> index a58444c4aed1f1ea..385383ef6cf1edaf 100644
> --- a/tools/perf/util/synthetic-events.c
> +++ b/tools/perf/util/synthetic-events.c
> @@ -1686,12 +1686,16 @@ int perf_event__synthesize_sample(union perf_event *event, u64 type, u64 read_fo
> }
>
> if (type & PERF_SAMPLE_RAW) {
> - u.val32[0] = sample->raw_size;
> - *array = u.val64;
> - array = (void *)array + sizeof(u32);
> + u32 *array32 = (void *)array;
> +
> + *array32 = sample->raw_size;
> + array32++;
> +
> + memcpy(array32, sample->raw_data, sample->raw_size);
> + array = (void *)(array32 + (sample->raw_size / sizeof(u32)));
>
> - memcpy(array, sample->raw_data, sample->raw_size);
> - array = (void *)array + sample->raw_size;
> + /* make sure the array is 64-bit aligned */
> + BUG_ON(((long)array) / sizeof(u64));
> }
>
> if (type & PERF_SAMPLE_BRANCH_STACK) {
> --
> 2.47.0.338.g60cca15819-goog
>
On Wed, Nov 27, 2024 at 04:51:15PM -0800, Ian Rogers wrote: > On Wed, Nov 27, 2024 at 1:26 PM Namhyung Kim <namhyung@kernel.org> wrote: > > The sample data is 64-bit aligned basically but raw data starts with > > 32-bit length field and data follows. In perf_event__synthesize_sample > > it treats the sample data as a 64-bit array. And it needs some trick > > to update the raw data properly. > > But it seems some compilers are not happy with this and the program dies > > siliently. I found the sample parsing test failed without any messages > > on affected systems. > > Let's update the code to use a 32-bit pointer directly and make sure the > > result is 64-bit aligned again. No functional changes intended. > > Signed-off-by: Namhyung Kim <namhyung@kernel.org> > Reviewed-by: Ian Rogers <irogers@google.com> Looks good, applied to perf-tools-next since this is something that is not new nor looks urgent. I think that since we have multiple maintainers, one for not urgent stuff/development and the other for the current window/urgent stuff, that we should express the expectation about where a patch should be processed, by having on the subject the tree the submitter thinks should take the patch, i.e. for this one: [PATCH next] perf tools: Avoid unaligned pointer operations While for urgent stuff we could do: [PATCH urgent] perf tools: Avoid unaligned pointer operations wdyt? - Arnaldo
On Thu, Dec 12, 2024 at 03:41:02PM -0300, Arnaldo Carvalho de Melo wrote: > On Wed, Nov 27, 2024 at 04:51:15PM -0800, Ian Rogers wrote: > > On Wed, Nov 27, 2024 at 1:26 PM Namhyung Kim <namhyung@kernel.org> wrote: > > > The sample data is 64-bit aligned basically but raw data starts with > > > 32-bit length field and data follows. In perf_event__synthesize_sample > > > it treats the sample data as a 64-bit array. And it needs some trick > > > to update the raw data properly. > > > > But it seems some compilers are not happy with this and the program dies > > > siliently. I found the sample parsing test failed without any messages > > > on affected systems. > > > > Let's update the code to use a 32-bit pointer directly and make sure the > > > result is 64-bit aligned again. No functional changes intended. > > > > Signed-off-by: Namhyung Kim <namhyung@kernel.org> > > > Reviewed-by: Ian Rogers <irogers@google.com> > > Looks good, applied to perf-tools-next since this is something that is > not new nor looks urgent. > > I think that since we have multiple maintainers, one for not urgent > stuff/development and the other for the current window/urgent stuff, > that we should express the expectation about where a patch should be > processed, by having on the subject the tree the submitter thinks should > take the patch, i.e. for this one: > > [PATCH next] perf tools: Avoid unaligned pointer operations > > While for urgent stuff we could do: > > [PATCH urgent] perf tools: Avoid unaligned pointer operations > > wdyt? Looks good. It'd be really great if contributors can do this. But I also think 'next' should be the default so only 'urgent' would be specified if needed. Thanks, Namhyung
On Thu, Dec 12, 2024 at 1:01 PM Namhyung Kim <namhyung@kernel.org> wrote:
>
> On Thu, Dec 12, 2024 at 03:41:02PM -0300, Arnaldo Carvalho de Melo wrote:
> > On Wed, Nov 27, 2024 at 04:51:15PM -0800, Ian Rogers wrote:
> > > On Wed, Nov 27, 2024 at 1:26 PM Namhyung Kim <namhyung@kernel.org> wrote:
> > > > The sample data is 64-bit aligned basically but raw data starts with
> > > > 32-bit length field and data follows. In perf_event__synthesize_sample
> > > > it treats the sample data as a 64-bit array. And it needs some trick
> > > > to update the raw data properly.
> >
> > > > But it seems some compilers are not happy with this and the program dies
> > > > siliently. I found the sample parsing test failed without any messages
> > > > on affected systems.
> >
> > > > Let's update the code to use a 32-bit pointer directly and make sure the
> > > > result is 64-bit aligned again. No functional changes intended.
> >
> > > > Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> >
> > > Reviewed-by: Ian Rogers <irogers@google.com>
> >
> > Looks good, applied to perf-tools-next since this is something that is
> > not new nor looks urgent.
> >
> > I think that since we have multiple maintainers, one for not urgent
> > stuff/development and the other for the current window/urgent stuff,
> > that we should express the expectation about where a patch should be
> > processed, by having on the subject the tree the submitter thinks should
> > take the patch, i.e. for this one:
> >
> > [PATCH next] perf tools: Avoid unaligned pointer operations
> >
> > While for urgent stuff we could do:
> >
> > [PATCH urgent] perf tools: Avoid unaligned pointer operations
> >
> > wdyt?
>
> Looks good. It'd be really great if contributors can do this.
>
> But I also think 'next' should be the default so only 'urgent' would be
> specified if needed.
Fwiw, I needed this fix, forgot about this change, and wrote my own by
just sinking the unaligned array computation (that causes undefined
behavior) into where it was used:
```
--- a/tools/perf/util/synthetic-events.c
+++ b/tools/perf/util/synthetic-events.c
@@ -1690,10 +1690,9 @@ int perf_event__synthesize_sample(union
perf_event *event, u64 type, u64 read_fo
if (type & PERF_SAMPLE_RAW) {
u.val32[0] = sample->raw_size;
*array = u.val64;
- array = (void *)array + sizeof(u32);
- memcpy(array, sample->raw_data, sample->raw_size);
- array = (void *)array + sample->raw_size;
+ memcpy((void *)array + sizeof(u32), sample->raw_data,
sample->raw_size);
+ array = (void *)array + sizeof(u32) + sample->raw_size;
}
if (type & PERF_SAMPLE_BRANCH_STACK) {
````
Namhyung's change is better because of the BUG_ON. Perhaps that BUG_ON
should appear after all the void* math that can create unaligned u64
pointers in this function.
Thanks,
Ian
© 2016 - 2026 Red Hat, Inc.