Message-ID: <CAP-5=fXwAA0PNYAOLNLdHY8g+AyEB0UxmzgX5w-gGj_DZqUxtg@mail.gmail.com>
Date: Thu, 3 Apr 2025 08:20:02 -0700
From: Ian Rogers <irogers@...gle.com>
To: Namhyung Kim <namhyung@...nel.org>
Cc: Leo Yan <leo.yan@....com>, 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>, 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 9:53 PM Namhyung Kim <namhyung@...nel.org> wrote:
>
> On Wed, Apr 02, 2025 at 09:53:57AM -0700, Ian Rogers wrote:
> > On Wed, Apr 2, 2025 at 9:38 AM Leo Yan <leo.yan@....com> wrote:
> > >
> > > Hi Ian,
> > >
> > > On Wed, Apr 02, 2025 at 08:42:58AM -0700, Ian Rogers wrote:
> > >
> > > [...]
> > >
> > > > 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.
> > >
> > > This conclusion is not quite right, see:
> > > https://stackoverflow.com/questions/2524611/how-can-one-print-a-size-t-variable-portably-using-the-printf-family
> > >
> > > > 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.
> > >
> > > I would say in another way.  After we enabled a compiler warning
> > > option, this will give us a chance to improve code by dismissing
> > > the warnings (and avoid potential bugs).
> > >
> > > If we use casts just to silence warnings, we also lose the opportunity
> > > to improve code quality.  The changes in this series that fix type
> > > mismatches are good, but I think the use of casts is not particularly
> > > helpful - we're simply switching from implicit compiler casts to
> > > explicit ones.
> >
> > Right, but I think changing function parameters, return types would
> > turn into an epic refactor and the patch series is already unwieldy.
>
> Yep, it's important to reduce the number and the size of patches from
> the reviewer's pespective. :)
>
>
> > With the casts we can see the behavior is deliberate but that's not to
> > say it couldn't be better. With the warnings gone it allows
> > -Wshorten-64-to-32 to find the truly errant 64 to 32 bit implicit
>
> Do we want to enable this warning?  Is it only available on clang?

It is clang only. libbpf fails with the warning enabled, so enabling
it would break all the BPF stuff which isn't good. That said we could
step toward enabling it, which is what the series does.

> > casts. Anyway, I don't have time to do such a larger refactor so
> > somebody else is going to need to pick that up. I did refactor the
> > cases where I thought it mattered more, but as you say that does lead
> > to a feeling of inconsistency in the series.
>
> I'm curious there are any actual errorenous cases other than the return
> type of comparisons from Leo.

There were other cases of Leo's issue here:
https://web.git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/tree/tools/perf/ui/hist.c?h=perf-tools-next#n290
```
ret = b->callchain->max_depth - a->callchain->max_depth;
```
you can see that file has field_cmp to handle 64 bit comparisons
returning an int, presumably as this problem had been seen before, and
possibly here once the left_count and right_count are made size_ts to
match the hashmap size operation.
https://web.git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/tree/tools/perf/util/metricgroup.c?h=perf-tools-next#n1162

xyarray was a source of differences as the max values are size_t,
which is more than they need to be.
The libperf mmap code was truncating the mask field which didn't look good.
The pipe benchmark, and similar, were truncating the read amount,
which should never be an issue as we shouldn't shove 4GB down a pipe,
but it just isn't good.
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;
```
and
```
@@ -4830,7 +4836,7 @@ static struct syscall_entry
*syscall__sort_stats(struct hashmap *syscall_stats)

               entry[i].stats = ss;
               entry[i].msecs = (u64)st->n * (avg_stats(st) / NSEC_PER_MSEC);
-               entry[i].syscall = pos->key;
+               entry[i].syscall = (int)pos->key;
```
where I think the cast is informative that the top half of the payload
is being discarded, or the key deliberately truncated. In general I
think having the casts makes the behavior of the code more explicit
and less surprising, which is a good thing.

Thanks,
Ian

> Thnaks,
> Namhyung
>

Powered by blists - more mailing lists