Re: [PATCH v1 09/48] perf tests: Silence -Wshorten-64-to-32 warnings

Leo Yan
2025-04-29T09:53:40Z

Hi Ian,

On Mon, Apr 28, 2025 at 09:29:37AM -0700, Ian Rogers wrote:

[...]

> Hi Leo,
>
> So I sent out cleaning up the kernel headers separately:
> https://lore.kernel.org/lkml/20250403165702.396388-1-irogers@google.com/
> Arnd commented that it would be nicer to do larger changes, but my
> concern was breaking printf modifiers, etc. It looks like those
> patches have lost momentum, not sure what's going to happen about them
> being merged.
>
> On the fixes vs refactoring. I'm not sure refactoring is the right
> term for the other things in the series. It is basically trying to
> make implicit casts explicit, without doing some boil the ocean
> exercise.

Yeah, I agreed that some "refactoring" is actually fixing bugs.

> I went through the issues in the email this is a reply to:
> https://lore.kernel.org/lkml/CAP-5=fXwAA0PNYAOLNLdHY8g+AyEB0UxmzgX5w-gGj_DZqUxtg@mail.gmail.com/
> Do you want to send the bits you think need prioritizing to the list?

For me, at least two things can be fixed first:

- One is the test case switch-tracking.c is broken on Arm64, your
change for the compar() function would be fine for me.

- You mentioned the issue in ui/hist.c:
ret = b->callchain->max_depth - a->callchain->max_depth;

These two issues are similiar and can fixed in the same format.

> > > There are lots of cases like:
> > > ```
> > > case ARM_SPE_COUNTER:
> > > if (idx == SPE_CNT_PKT_HDR_INDEX_TOTAL_LAT)
> > > - decoder->record.latency = payload;
> > > + decoder->record.latency = (u32)payload;
> >
> > This would be fine. Since Arm SPE implements a counter with a maximum
> > of 16 bits, there will never be an overflow when storing the data in a
> > 32-bit unsigned integer.
>
> The payload was > sizeof(u32), should record.latency be a u16?

Yes, we can change to u16 type for latency.

> My point wasn't so much to complain about adding a u32 cast in the SPE
> code, but that having the cast makes it clear that truncation is
> expected. Adding the casts isn't fixing a bug, but it is making it
> clearer that the code will truncate the payload value (imo).

As Arm SPE has defined the format, I would prefer to refine the code
based on the format:

#define SPE_CNT_PKT_COUNT(v) ((v) & GENMASK_ULL(15, 0))

decoder->record.latency = SPE_CNT_PKT_COUNT(payload);

I can send a minor patch series for this, if not conflict with your
side.

Thanks,
Leo