Message-ID: <CAP-5=fVqax8cxdZ4HLBP4AMxL6jADfYNrORC97T6F23mjf3N1w@mail.gmail.com>
Date: Wed, 2 Apr 2025 08:42:58 -0700
From: Ian Rogers <irogers@...gle.com>
To: Leo Yan <leo.yan@....com>
Cc: Yury Norov <yury.norov@...il.com>, Rasmus Villemoes <linux@...musvillemoes.dk>, 
	Peter Zijlstra <peterz@...radead.org>, Ingo Molnar <mingo@...hat.com>, 
	Arnaldo Carvalho de Melo <acme@...nel.org>, Namhyung Kim <namhyung@...nel.org>, 
	Mark Rutland <mark.rutland@....com>, 
	Alexander Shishkin <alexander.shishkin@...ux.intel.com>, Jiri Olsa <jolsa@...nel.org>, 
	Adrian Hunter <adrian.hunter@...el.com>, Kan Liang <kan.liang@...ux.intel.com>, 
	Thomas Gleixner <tglx@...utronix.de>, Darren Hart <dvhart@...radead.org>, 
	Davidlohr Bueso <dave@...olabs.net>, André Almeida <andrealmeid@...lia.com>, 
	John Garry <john.g.garry@...cle.com>, Will Deacon <will@...nel.org>, 
	James Clark <james.clark@...aro.org>, Mike Leach <mike.leach@...aro.org>, 
	Leo Yan <leo.yan@...ux.dev>, Yicong Yang <yangyicong@...ilicon.com>, 
	Jonathan Cameron <jonathan.cameron@...wei.com>, Nathan Chancellor <nathan@...nel.org>, 
	Bill Wendling <morbo@...gle.com>, Justin Stitt <justinstitt@...gle.com>, 
	Josh Poimboeuf <jpoimboe@...nel.org>, Al Viro <viro@...iv.linux.org.uk>, 
	Kyle Meyer <kyle.meyer@....com>, Ben Gainey <ben.gainey@....com>, 
	Athira Rajeev <atrajeev@...ux.vnet.ibm.com>, Kajol Jain <kjain@...ux.ibm.com>, 
	Aditya Gupta <adityag@...ux.ibm.com>, Eder Zulian <ezulian@...hat.com>, 
	Dapeng Mi <dapeng1.mi@...ux.intel.com>, Kuan-Wei Chiu <visitorckw@...il.com>, 
	He Zhe <zhe.he@...driver.com>, Dirk Gouders <dirk@...ders.net>, 
	Brian Geffon <bgeffon@...gle.com>, Ravi Bangoria <ravi.bangoria@....com>, 
	Howard Chu <howardchu95@...il.com>, Charlie Jenkins <charlie@...osinc.com>, 
	Colin Ian King <colin.i.king@...il.com>, Dominique Martinet <asmadeus@...ewreck.org>, 
	Jann Horn <jannh@...gle.com>, Masahiro Yamada <masahiroy@...nel.org>, Arnd Bergmann <arnd@...db.de>, 
	Yang Jihong <yangjihong@...edance.com>, Dmitry Vyukov <dvyukov@...gle.com>, 
	Andi Kleen <ak@...ux.intel.com>, Graham Woodward <graham.woodward@....com>, 
	Ilkka Koskinen <ilkka@...amperecomputing.com>, 
	Anshuman Khandual <anshuman.khandual@....com>, Zhongqiu Han <quic_zhonhan@...cinc.com>, 
	Hao Ge <gehao@...inos.cn>, Tengda Wu <wutengda@...weicloud.com>, 
	Gabriele Monaco <gmonaco@...hat.com>, Chun-Tse Shao <ctshao@...gle.com>, 
	Casey Chen <cachen@...estorage.com>, "Dr. David Alan Gilbert" <linux@...blig.org>, 
	Li Huafei <lihuafei1@...wei.com>, "Steinar H. Gunderson" <sesse@...gle.com>, Levi Yun <yeoreum.yun@....com>, 
	Weilin Wang <weilin.wang@...el.com>, Thomas Falcon <thomas.falcon@...el.com>, 
	Thomas Richter <tmricht@...ux.ibm.com>, Andrew Kreimer <algonell@...il.com>, 
	Krzysztof Łopatowski <krzysztof.m.lopatowski@...il.com>, 
	Christophe Leroy <christophe.leroy@...roup.eu>, 
	Jean-Philippe Romain <jean-philippe.romain@...s.st.com>, Junhao He <hejunhao3@...wei.com>, 
	"Masami Hiramatsu (Google)" <mhiramat@...nel.org>, Xu Yang <xu.yang_2@....com>, 
	Steve Clevenger <scclevenger@...amperecomputing.com>, Zixian Cai <fzczx123@...il.com>, 
	Stephen Brennan <stephen.s.brennan@...cle.com>, Yujie Liu <yujie.liu@...el.com>, 
	linux-kernel@...r.kernel.org, linux-perf-users@...r.kernel.org, 
	linux-arm-kernel@...ts.infradead.org, llvm@...ts.linux.dev
Subject: Re: [PATCH v1 09/48] perf tests: Silence -Wshorten-64-to-32 warnings

On Wed, Apr 2, 2025 at 7:35 AM Leo Yan <leo.yan@....com> wrote:
>
> On Tue, Apr 01, 2025 at 11:23:07AM -0700, Ian Rogers wrote:
>
> [...]
>
> > @@ -478,13 +478,14 @@ static int __cmd_test(struct test_suite **suites, int argc, const char *argv[],
> >       int err = 0;
> >
> >       for (struct test_suite **t = suites; *t; t++) {
> > -             int i, len = strlen(test_description(*t, -1));
> > +             int i;
> > +             int len = (int)strlen(test_description(*t, -1));
>
> Thanks for huge polish.
>
> Just a concern from me.  Throughout this patch, the methodology is not
> consistent.  Some changes update variable types which is fine for me.
>
> But the case above it simply cast size_t to int.  Should we update the
> variable type as 'size_t' at here?

Thanks Leo, I played around with it, but don't mind if someone wants
to do it a different way. I was trying to make the changes minimal.
The problem typically with size_t is we then use the value, for
example, as a printf size modifier and need to introduce lots of casts
back to being an int. When this isn't too great I've done it, but in
this case I think keeping the int, the lack of casts but a cast here
to capture that we expect test descriptions to fit in the size of an
int is the least worst option.

> >               if (width < len)
> >                       width = len;
> >
> >               test_suite__for_each_test_case(*t, i) {
> > -                     len = strlen(test_description(*t, i));
> > +                     len = (int)strlen(test_description(*t, i));
> >                       if (width < len)
> >                               width = len;
> >                       num_tests += runs_per_test;
> > diff --git a/tools/perf/tests/code-reading.c b/tools/perf/tests/code-reading.c
> > index cf6edbe697b2..0c31d5ff77e2 100644
> > --- a/tools/perf/tests/code-reading.c
> > +++ b/tools/perf/tests/code-reading.c
> > @@ -188,7 +188,7 @@ static int objdump_version(void)
> >       char *line = NULL;
> >       const char *fmt;
> >       FILE *f;
> > -     int ret;
> > +     ssize_t ret;
> >
> >       int version_tmp, version_num = 0;
> >       char *version = 0, *token;
> > @@ -295,7 +295,7 @@ static int read_via_objdump(const char *filename, u64 addr, void *buf,
> >       if (len) {
> >               pr_debug("objdump read too few bytes: %zd\n", len);
> >               if (!ret)
> > -                     ret = len;
> > +                     ret = (int)len;
>
> A proper change is to update the function type to:
>
>   size_t read_via_objdump(...)
>
> [...]

Normally the int is negative to error, 0 and more for a successful
positive read of so many bytes. We could switch the return type to
ssize_t but I was trying to avoid changing everything.

> > diff --git a/tools/perf/tests/pmu-events.c b/tools/perf/tests/pmu-events.c
> > index db004d26fcb0..2762794827ce 100644
> > --- a/tools/perf/tests/pmu-events.c
> > +++ b/tools/perf/tests/pmu-events.c
> > @@ -609,7 +609,7 @@ static int __test_uncore_pmu_event_aliases(struct perf_pmu_test_pmu *test_pmu)
> >       pmu_add_sys_aliases(pmu);
> >
> >       /* Count how many aliases we generated */
> > -     alias_count = perf_pmu__num_events(pmu);
> > +     alias_count = (int)perf_pmu__num_events(pmu);
>
> Could we change the variable 'alias_count' to size_t type?
>
> Or in another way, we need to update perf_pmu__num_events() with
> returning int type.
>
> My understanding is rather than using cast, we should polish code for
> using consistent type for both variables and return values.

We can, I was trying to keep the size of the change down. Later the
code will compare against matched_count which is an int, so using a
size_t means a signed vs unsigned compare, so a multi-line change vs 1
cast.

> >       /* Count how many aliases we expect from the known table */
> >       for (table = &test_pmu->aliases[0]; *table; table++)
> > diff --git a/tools/perf/tests/sigtrap.c b/tools/perf/tests/sigtrap.c
> > index a67c756f90b8..b7d7735f8a72 100644
> > --- a/tools/perf/tests/sigtrap.c
> > +++ b/tools/perf/tests/sigtrap.c
> > @@ -154,13 +154,13 @@ sigtrap_handler(int signum __maybe_unused, siginfo_t *info, void *ucontext __may
> >  {
> >       if (!__atomic_fetch_add(&ctx.signal_count, 1, __ATOMIC_RELAXED))
> >               ctx.first_siginfo = *info;
> > -     __atomic_fetch_sub(&ctx.tids_want_signal, syscall(SYS_gettid), __ATOMIC_RELAXED);
> > +     __atomic_fetch_sub(&ctx.tids_want_signal, (pid_t)syscall(SYS_gettid), __ATOMIC_RELAXED);
> >  }
> >
> >  static void *test_thread(void *arg)
> >  {
> >       pthread_barrier_t *barrier = (pthread_barrier_t *)arg;
> > -     pid_t tid = syscall(SYS_gettid);
> > +     pid_t tid = (pid_t)syscall(SYS_gettid);
> >       int i;
> >
> >       pthread_barrier_wait(barrier);
> > diff --git a/tools/perf/tests/switch-tracking.c b/tools/perf/tests/switch-tracking.c
> > index 8df3f9d9ffd2..1f0f8321ae40 100644
> > --- a/tools/perf/tests/switch-tracking.c
> > +++ b/tools/perf/tests/switch-tracking.c
> > @@ -140,8 +140,8 @@ static int process_sample_event(struct evlist *evlist,
> >
> >       evsel = evlist__id2evsel(evlist, sample.id);
> >       if (evsel == switch_tracking->switch_evsel) {
> > -             next_tid = evsel__intval(evsel, &sample, "next_pid");
> > -             prev_tid = evsel__intval(evsel, &sample, "prev_pid");
> > +             next_tid = (int)evsel__intval(evsel, &sample, "next_pid");
> > +             prev_tid = (int)evsel__intval(evsel, &sample, "prev_pid");
>
> Change 'prev_tid' and 'next_tid' to pid_t type?

I agree, but I was trying to do what was minimal and use the types as
they already were. Perhaps we should have multiple of the
evsel__<type>val helpers. Again, I wanted to do what was minimal in
order for the warning to be silenced.

Thanks,
Ian

> Thanks,
> Leo
>
> >               cpu = sample.cpu;
> >               pr_debug3("sched_switch: cpu: %d prev_tid %d next_tid %d\n",
> >                         cpu, prev_tid, next_tid);
> > @@ -262,9 +262,10 @@ static int compar(const void *a, const void *b)
> >  {
> >       const struct event_node *nodea = a;
> >       const struct event_node *nodeb = b;
> > -     s64 cmp = nodea->event_time - nodeb->event_time;
> >
> > -     return cmp;
> > +     if (nodea->event_time == nodeb->event_time)
> > +             return 0;
> > +     return nodea->event_time < nodeb->event_time ? -1 : 1;
> >  }
> >
> >  static int process_events(struct evlist *evlist,
> > @@ -398,7 +399,7 @@ static int test__switch_tracking(struct test_suite *test __maybe_unused, int sub
> >
> >       switch_evsel = evlist__add_sched_switch(evlist, true);
> >       if (IS_ERR(switch_evsel)) {
> > -             err = PTR_ERR(switch_evsel);
> > +             err = (int)PTR_ERR(switch_evsel);
> >               pr_debug("Failed to create event %s\n", sched_switch);
> >               goto out_err;
> >       }
> > diff --git a/tools/perf/tests/vmlinux-kallsyms.c b/tools/perf/tests/vmlinux-kallsyms.c
> > index 74cdbd2ce9d0..fbdb463e965d 100644
> > --- a/tools/perf/tests/vmlinux-kallsyms.c
> > +++ b/tools/perf/tests/vmlinux-kallsyms.c
> > @@ -82,7 +82,7 @@ static bool is_ignored_symbol(const char *name, char type)
> >                       return true;
> >
> >       for (p = ignored_suffixes; *p; p++) {
> > -             int l = strlen(name) - strlen(*p);
> > +             ssize_t l = strlen(name) - strlen(*p);
> >
> >               if (l >= 0 && !strcmp(name + l, *p))
> >                       return true;
> > @@ -313,7 +313,7 @@ static int test__vmlinux_matches_kallsyms(struct test_suite *test __maybe_unused
> >                                * _really_ have a problem.
> >                                */
> >                               s64 skew = mem_end - UM(pair->end);
> > -                             if (llabs(skew) >= page_size)
> > +                             if (llabs(skew) >= (s64)page_size)
> >                                       pr_debug("WARN: %#" PRIx64 ": diff end addr for %s v: %#" PRIx64 " k: %#" PRIx64 "\n",
> >                                                mem_start, sym->name, mem_end,
> >                                                UM(pair->end));
> > diff --git a/tools/perf/tests/wp.c b/tools/perf/tests/wp.c
> > index 6c178985e37f..d5dd17eb5c05 100644
> > --- a/tools/perf/tests/wp.c
> > +++ b/tools/perf/tests/wp.c
> > @@ -31,10 +31,10 @@ volatile u8 data2[3];
> >  #ifndef __s390x__
> >  static int wp_read(int fd, long long *count, int size)
> >  {
> > -     int ret = read(fd, count, size);
> > +     ssize_t ret = read(fd, count, size);
> >
> >       if (ret != size) {
> > -             pr_debug("failed to read: %d\n", ret);
> > +             pr_debug("failed to read: %zd\n", ret);
> >               return -1;
> >       }
> >       return 0;
> > --
> > 2.49.0.504.g3bcea36a83-goog
> >
> >

Powered by blists - more mailing lists