Message-ID: <4a5cf233-a4e6-48ce-b9ba-f1014f452892@acm.org>
Date: Thu, 16 May 2024 15:51:54 -0600
From: Bart Van Assche <bvanassche@....org>
To: Linus Torvalds <torvalds@...ux-foundation.org>
Cc: Andrew Morton <akpm@...ux-foundation.org>, linux-nilfs@...r.kernel.org,
 linux-kernel@...r.kernel.org, Rasmus Villemoes <linux@...musvillemoes.dk>,
 Steven Rostedt <rostedt@...dmis.org>,
 Ryusuke Konishi <konishi.ryusuke@...il.com>
Subject: Re: [PATCH -mm] nilfs2: Use __field_struct() for a bitwise field

On 5/7/24 10:25, Linus Torvalds wrote:
> On Tue, 7 May 2024 at 07:25, Ryusuke Konishi <konishi.ryusuke@...il.com> wrote:
>>
>>     Despite that change, sparse complains when
>> passing a bitwise type to is_signed_type(). It is not clear to me why.
> 
> Bah. The reason is this:
> 
>     #define is_signed_type(type) (((type)(-1)) < (__force type)1)
> 
> Basically, the way "is_signed_type()" works is that it casts a
> negative integer to the type, and checks to see if the value has now
> become a large value.
> 
> Now, it looks odd, because only one of those casts has a "__force" on
> it, but the reason for that is that casting all-ones and all-zeroes is
> ok for bitwise types (think of bitwise types as being a "collection of
> bits" - so all bits set or all bits clear are sane concepts regardless
> of any other semantics).
> 
> So it's not the casts themselves that are problematic: that part works fine.
> 
> But you cannot compare a random collection of bits for greater than or
> lesser than.
> 
> Think of things like byte orders: you can compare two values for
> _equality_ even if they are in the wrong byte order, but you can't
> compare them for "larger than" unless you turn them into the right CPU
> byte order.
> 
> Basically, a "collection of bits" doesn't have an ordering in itself,
> even if equality comparisons are ok.
> 
> So yeah, is_signed_type() doesn't work for bitwise types.
> 
> And I don't see a sane way to make "is_signed_type()" to work for
> bitwise types - the whole concept of signedness of "bunch of bits" is
> kind of nonsensical - so I suspect your workaround is the best we can
> do (alternatively, tracing would have to figure out a different way to
> test for signedness).

(replying to an email from ten days ago)

Thanks Linus for the detailed analysis. I tried the patch below but
unfortunately it is not sufficient to suppress sparse warnings about
bitwise types (all enum req_op values have the type __bitwise __u32):

diff --git a/include/linux/compiler.h b/include/linux/compiler.h
index 8c252e073bd8..940563438b87 100644
--- a/include/linux/compiler.h
+++ b/include/linux/compiler.h
@@ -287,7 +287,14 @@ static inline void *offset_to_ptr(const int *off)
   * Whether 'type' is a signed type or an unsigned type. Supports 
scalar types,
   * bool and also pointer types.
   */
-#define is_signed_type(type) (((type)(-1)) < (__force type)1)
+#define is_signed_type(type)            \
+	(_Generic((__force type)1,      \
+		 unsigned char: 0,      \
+		 unsigned short: 0,     \
+		 unsigned int: 0,       \
+		 unsigned long: 0,      \
+		 unsigned long long: 0, \
+		 default: ((type)(-1)) < (type)1))
  #define is_unsigned_type(type) (!is_signed_type(type))

  /*

It seems like sparse verifies the types of all expressions in a
_Generic() argument list instead of only the expression for which the
type matches. Could this indicate a bug in sparse? On
https://en.cppreference.com/w/c/language/generic I found the
following (I'm not sure whether that website is a good reference):

"The controlling-expression and the expressions of the selections that 
are not chosen are never evaluated."

Thanks,

Bart.

Powered by blists - more mailing lists