Re: [PATCH 1/3] btrfs: zlib: fix and simplify the inline extent decompression

[David Sterba <dsterba@suse.cz>]

On Wed, Jan 10, 2024 at 01:04:06PM +1030, Qu Wenruo wrote:
> >>>>
> >>>> If you fix the issue in a separate patch/commit (i.e. not just a new version of
> >>>> the same patch/commit), kindly add following tags
> >>>> | Reported-by: kernel test robot <lkp@intel.com>
> >>>> | Closes: https://lore.kernel.org/oe-kbuild-all/202401091012.pLm6PcKG-lkp@intel.com/
> >>>>
> >>>> All warnings (new ones prefixed by >>):
> >>>>
> >>>>>> fs/btrfs/zlib.c:402:15: warning: format specifies type 'unsigned long' but the argument has type 'size_t' (aka 'unsigned int') [-Wformat]
> >>>>        401 |                 pr_warn_ratelimited("BTRFS: infalte failed, decompressed=%lu expected=%lu\n",
> >>>>            |                                                                                       ~~~
> >>>>            |                                                                                       %zu
> >>>>        402 |                                         to_copy, destlen);
> >>>>            |                                                  ^~~~~~~
> >>>
> >>> Valid report but I can't reproduce it. Built with clang 17 and
> >>> explicitly enabled -Wformat. We have additional warnings enabled per
> >>> directory fs/btrfs/ so we can add -Wformat, I'd like to know what I'm
> >>> missing, we've had fixups for the size_t printk format so it would make
> >>> sense to catch it early.
> >>
> >> I guess it's due to the platform? (The report is from 32bit system).
> > 
> > Ah I see, I build on 64bit platform but should the Wformat warning also
> > point out mismatch there? The size_t type is an alias of unsigned long
> > so it is not an error but when size_t and %zu don't match could be a
> > platform-independent error, no? This would save us reports and followup
> > fixups roundtrips.
> 
> size_t is defined differently, in include/uapi/asm-generic/posix_types.h:
> 
> /*
>   * Most 32 bit architectures use "unsigned int" size_t,
>   * and all 64 bit architectures use "unsigned long" size_t.
>   */
> #ifndef __kernel_size_t
> #if __BITS_PER_LONG != 64
> typedef unsigned int    __kernel_size_t;
> typedef int             __kernel_ssize_t;
> typedef int             __kernel_ptrdiff_t;
> #else
> typedef __kernel_ulong_t __kernel_size_t;
> typedef __kernel_long_t __kernel_ssize_t;
> typedef __kernel_long_t __kernel_ptrdiff_t;
> #endif
> #endif
> 
> Thus this is the reason why we need @zu for size_t to handle the 
> difference, and since for 64bit it's just unsigned long, thus compiler 
> won't give any warning.

So it's the int/long difference and kernel does it in a special way due
to absence of the standard libc headers.

> (That's also why I tend to not use size_t at all, and why I like rust's 
> explicit sized type, and we may want to go that path to prefer 
> u8/u16/u32/u64 when possible)

Yeah, from what I've seen so far is that size_t brings us only problems,
I would not mind conversions to explicit types like u32 or u64. We do
that in many places, I think it's namely on the interface boundaries
where we provide a callback with a size_t type, but from that down it
could be u64.

Quick grep for size_t returns 300+ lines and 100+ of that is ssize_t,
that's still a lot so the conversions should be targeted.