Re: include/asm-generic/unaligned.h:119:16: sparse: sparse: cast truncates bits from constant value (aa01a0 becomes a0)

Re: include/asm-generic/unaligned.h:119:16: sparse: sparse: cast truncates bits from constant value (aa01a0 becomes a0)

[Dmitry Torokhov]

On Sun, Jan 07, 2024 at 01:41:34AM +0800, kernel test robot wrote:
> tree:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master
> head:   95c8a35f1c017327eab3b6a2ff5c04255737c856
> commit: 66603243f5283f7f28c795f09e7c2167233df0bd Input: add driver for Hynitron cstxxx touchscreens
> date:   1 year, 2 months ago
> config: x86_64-randconfig-x051-20230705 (https://download.01.org/0day-ci/archive/20240107/202401070147.gqwVulOn-lkp@intel.com/config)
> compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
> reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240107/202401070147.gqwVulOn-lkp@intel.com/reproduce)
> 
> 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/202401070147.gqwVulOn-lkp@intel.com/
> 
> sparse warnings: (new ones prefixed by >>)
>    drivers/input/touchscreen/hynitron_cstxxx.c: note: in included file (through arch/x86/include/generated/asm/unaligned.h):
> >> include/asm-generic/unaligned.h:119:16: sparse: sparse: cast truncates bits from constant value (aa01a0 becomes a0)
>    include/asm-generic/unaligned.h:120:20: sparse: sparse: cast truncates bits from constant value (aa01 becomes 1)
> >> include/asm-generic/unaligned.h:119:16: sparse: sparse: cast truncates bits from constant value (ab00d0 becomes d0)
>    include/asm-generic/unaligned.h:120:20: sparse: sparse: cast truncates bits from constant value (ab00 becomes 0)
> 
> vim +119 include/asm-generic/unaligned.h
> 
> 803f4e1eab7a89 Arnd Bergmann 2021-05-08  116  
> 803f4e1eab7a89 Arnd Bergmann 2021-05-08  117  static inline void __put_unaligned_le24(const u32 val, u8 *p)
> 803f4e1eab7a89 Arnd Bergmann 2021-05-08  118  {
> 803f4e1eab7a89 Arnd Bergmann 2021-05-08 @119  	*p++ = val;
> 803f4e1eab7a89 Arnd Bergmann 2021-05-08  120  	*p++ = val >> 8;
> 803f4e1eab7a89 Arnd Bergmann 2021-05-08  121  	*p++ = val >> 16;
> 803f4e1eab7a89 Arnd Bergmann 2021-05-08  122  }
> 803f4e1eab7a89 Arnd Bergmann 2021-05-08  123  

This is not really a kernel/driver bug, just sparse being over-eager
with truncation detection. I wonder if we could make sparse skip this
check on forced casts like this:

diff --git a/expand.c b/expand.c
index f14e7181..5487e8b3 100644
--- a/expand.c
+++ b/expand.c
@@ -96,6 +96,7 @@ static long long get_longlong(struct expression *expr)
 
 void cast_value(struct expression *expr, struct symbol *newtype, struct expression *old)
 {
+	enum expression_type cast_type = expr->type;
 	struct symbol *oldtype = old->ctype;
 	int old_size = oldtype->bit_size;
 	int new_size = newtype->bit_size;
@@ -133,7 +134,7 @@ Int:
 	expr->value = value & mask;
 
 	// Stop here unless checking for truncation
-	if (!Wcast_truncate || conservative)
+	if (cast_type == EXPR_FORCE_CAST || !Wcast_truncate || conservative)
 		return;
 	
 	// Check if we dropped any bits..

and then in the kernel we would do this:

diff --git a/include/asm-generic/unaligned.h b/include/asm-generic/unaligned.h
index 699650f81970..034237d12d70 100644
--- a/include/asm-generic/unaligned.h
+++ b/include/asm-generic/unaligned.h
@@ -104,9 +104,9 @@ static inline u32 get_unaligned_le24(const void *p)
 
 static inline void __put_unaligned_be24(const u32 val, u8 *p)
 {
-	*p++ = val >> 16;
-	*p++ = val >> 8;
-	*p++ = val;
+	*p++ = (__force u8)(val >> 16);
+	*p++ = (__force u8)(val >> 8);
+	*p++ = (__force u8)val;
 }
 
 static inline void put_unaligned_be24(const u32 val, void *p)
@@ -116,9 +116,9 @@ static inline void put_unaligned_be24(const u32 val, void *p)
 
 static inline void __put_unaligned_le24(const u32 val, u8 *p)
 {
-	*p++ = val;
-	*p++ = val >> 8;
-	*p++ = val >> 16;
+	*p++ = (__force u8)val;
+	*p++ = (__force u8)(val >> 8);
+	*p++ = (__force u8)(val >> 16);
 }
 
 static inline void put_unaligned_le24(const u32 val, void *p)
@@ -128,12 +128,12 @@ static inline void put_unaligned_le24(const u32 val, void *p)
 
 static inline void __put_unaligned_be48(const u64 val, u8 *p)
 {
-	*p++ = val >> 40;
-	*p++ = val >> 32;
-	*p++ = val >> 24;
-	*p++ = val >> 16;
-	*p++ = val >> 8;
-	*p++ = val;
+	*p++ = (__force u8)(val >> 40);
+	*p++ = (__force u8)(val >> 32);
+	*p++ = (__force u8)(val >> 24);
+	*p++ = (__force u8)(val >> 16);
+	*p++ = (__force u8)(val >> 8);
+	*p++ = (__force u8)val;
 }
 
 static inline void put_unaligned_be48(const u64 val, void *p)

What do you think?

Thanks.

-- 
Dmitry

Re: include/asm-generic/unaligned.h:119:16: sparse: sparse: cast truncates bits from constant value (aa01a0 becomes a0)

[Linus Torvalds]

On Sat, 6 Jan 2024 at 16:42, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
>
> This is not really a kernel/driver bug, just sparse being over-eager
> with truncation detection. I wonder if we could make sparse skip this
> check on forced casts like this:

No, please don't.

Just face the fact that using integer casts to mask bits off is a bad idea.

Yes, we could say "explicit casting is ok", since it's really the
hidden implicit casts changing values that sparse complains about, but
your solution is really ugly:

>  static inline void __put_unaligned_be24(const u32 val, u8 *p)
>  {
> -       *p++ = val >> 16;
> -       *p++ = val >> 8;
> -       *p++ = val;
> +       *p++ = (__force u8)(val >> 16);
> +       *p++ = (__force u8)(val >> 8);
> +       *p++ = (__force u8)val;
>  }

That's just disgusting.

The *natural* thing to do is to simply make the masking itself be
explicit - not the cast. IOW, just write it as

        *p++ = (val >> 16) & 0xff;
        *p++ = (val >> 8) & 0xff;
        *p++ = val & 0xff;

and doesn't that look much more natural?

Sure, the compiler will then just notice "you're assigning to a char,
to I don't actually need to do any masking at all", but now sparse
won't complain because there's no "cast silently drops bits" issue any
more.

And while the code is a bit more to read, I think it is actually to
some degree more obvious to a human too what is going on.

No?

              Linus

Re: include/asm-generic/unaligned.h:119:16: sparse: sparse: cast truncates bits from constant value (aa01a0 becomes a0)

[Luc Van Oostenryck]

On Sat, Jan 06, 2024 at 09:54:05PM -0800, Linus Torvalds wrote:
> On Sat, 6 Jan 2024 at 16:42, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
> >
> > This is not really a kernel/driver bug, just sparse being over-eager
> > with truncation detection. I wonder if we could make sparse skip this
> > check on forced casts like this:
> 
> No, please don't.
> 
> Just face the fact that using integer casts to mask bits off is a bad idea.
> 
> ...
> 
> The *natural* thing to do is to simply make the masking itself be
> explicit - not the cast. IOW, just write it as
> 
>         *p++ = (val >> 16) & 0xff;
>         *p++ = (val >> 8) & 0xff;
>         *p++ = val & 0xff;
> 
> ...
> 
> And while the code is a bit more to read, I think it is actually to
> some degree more obvious to a human too what is going on.


I fully agree.

It's kinda sad is that there is more than 800 occurrences of this
"cast truncates bits from constant value" warning and almost all of
them are of the kind:
	"a 32bit constant must be written in 2 steps via 16bit IO registers"

In these cases, no bits are lost, they're just written in the other write,
and real problems, when present, are drown/lost into these 800 harmless ones.
It's in fact the 4th most common warning in the kernel, the top 10 being:
   2858 incorrect type in assignment (different base types)
   2715 cast to restricted type
    923 incorrect type in argument (different address spaces)
    818 cast truncates bits from constant value
    739 restricted type degrades to integer
    549 context imbalance - unexpected unlock
    500 symbol was not declared. Should it be static?
    407 cast removes address space '__iomem' of expression
    344 incompatible types in comparison expression (different address spaces)
    323 context imbalance - different lock contexts for basic block

-- Luc