[PATCH -mm] nilfs2: Use __field_struct() for a bitwise field

[PATCH -mm] nilfs2: Use __field_struct() for a bitwise field

[Ryusuke Konishi]

From: Bart Van Assche <bvanassche@acm.org>

As one can see in include/trace/stages/stage4_event_fields.h, the
implementation of __field() uses the is_signed_type() macro. As one can see
in commit dcf8e5633e2e ("tracing: Define the is_signed_type() macro once"),
there has been an attempt to not make is_signed_type() trigger sparse
warnings for bitwise types. Despite that change, sparse complains when
passing a bitwise type to is_signed_type(). It is not clear to me why.

Follow the example of <trace/events/initcall.h> and suppress the following
sparse warnings by changing __field() into __field_struct():

 fs/nilfs2/segment.c: note: in included file (through
   include/trace/trace_events.h, include/trace/define_trace.h,
   include/trace/events/nilfs2.h):
 ./include/trace/events/nilfs2.h:191:1: warning: cast to restricted
   blk_opf_t
 ./include/trace/events/nilfs2.h:191:1: warning: restricted blk_opf_t
   degrades to integer
 ./include/trace/events/nilfs2.h:191:1: warning: restricted blk_opf_t
   degrades to integer

Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Rasmus Villemoes <linux@rasmusvillemoes.dk>
Reported-by: kernel test robot <lkp@intel.com>
Closes: https://lore.kernel.org/oe-kbuild-all/202401092241.I4mm9OWl-lkp@intel.com/
Reported-by: Ryusuke Konishi <konishi.ryusuke@gmail.com>
Closes: https://lore.kernel.org/all/20240430080019.4242-2-konishi.ryusuke@gmail.com/
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
Signed-off-by: Ryusuke Konishi <konishi.ryusuke@gmail.com>
---
Hi Andrew, Bart has completed a patch that fixes the sparse warnings
related to event trace header, as he kindly shared the link with you
earlier.

Here is the patch (I added a few tags), could you add this to your
tree queue?
I'll send this via email just in case since we don't usually exchange
links.

The patch "nilfs2: use integer type instead of enum req_op for event
tracing header" that I sent is no longer needed, so I will withdraw it.

Thanks,
Ryusuke Konishi

 include/trace/events/nilfs2.h | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/include/trace/events/nilfs2.h b/include/trace/events/nilfs2.h
index 8efc6236f57c..8880c11733dd 100644
--- a/include/trace/events/nilfs2.h
+++ b/include/trace/events/nilfs2.h
@@ -200,7 +200,11 @@ TRACE_EVENT(nilfs2_mdt_submit_block,
 		    __field(struct inode *, inode)
 		    __field(unsigned long, ino)
 		    __field(unsigned long, blkoff)
-		    __field(enum req_op, mode)
+		    /*
+		     * Use field_struct() to avoid is_signed_type() on the
+		     * bitwise type enum req_op.
+		     */
+		    __field_struct(enum req_op, mode)
 	    ),
 
 	    TP_fast_assign(
-- 
2.34.1

Re: [PATCH -mm] nilfs2: Use __field_struct() for a bitwise field

[Linus Torvalds]

On Tue, 7 May 2024 at 07:25, Ryusuke Konishi <konishi.ryusuke@gmail.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).

                 Linus

Re: [PATCH -mm] nilfs2: Use __field_struct() for a bitwise field

[Ryusuke Konishi]

On Wed, May 8, 2024 at 1:25 AM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> On Tue, 7 May 2024 at 07:25, Ryusuke Konishi <konishi.ryusuke@gmail.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).
>
>                  Linus

Linus, thank you very much for your detailed explanation.

I would like to edit the quoted part of his commit message

> >    Despite that change, sparse complains when
> > passing a bitwise type to is_signed_type(). It is not clear to me why.

as follows:

 Despite that change, sparse complains when passing a bitwise type
 to is_signed_type().  The reason is that in its definition below, a
 comparison will be made against bitwise types, which are
 random collections of bits (the casts to bitwise types themselves
 are semantically valid and are not problematic):

  #define is_signed_type(type) (((type)(-1)) < (__force type)1)

 So, as a workaround, fix the warnings by using __field_struct() macro
 that doesn't use is_signed_type() instead of __field().
 ...

I will try to resend the patch later unless there's a misunderstanding or
I'm missing too many points.

Thanks,
Ryusuke Konishi

Re: [PATCH -mm] nilfs2: Use __field_struct() for a bitwise field

[Bart Van Assche]

On 5/7/24 10:25, Linus Torvalds wrote:
> On Tue, 7 May 2024 at 07:25, Ryusuke Konishi <konishi.ryusuke@gmail.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.

Re: [PATCH -mm] nilfs2: Use __field_struct() for a bitwise field

[Linus Torvalds]

On Thu, 16 May 2024 at 14:52, Bart Van Assche <bvanassche@acm.org> wrote:
>
> 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.

Yes.

>    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."

Not really a bug, because "never evaluated" in the above context means
that they don't generate code. The expressions are still obviously
parsed for syntax and validity.

It definitely might be seen as a misfeature, though - the "degrades to
integer" warning is done before code reachability has been determined.
So it's done even for code that is never executed.

So you'd get it even if you had something like

        if (0) .. some bad bitwise expression ...

Sadly, that's fairly deeply ingrained in how sparse deals with the
bitwise types: they degrade to their regular base type as part of the
type evaluation, which happens fairly early on the syntax tree, long
before it has been converted to SSA form and reachability analysis.

It's *fixable* - instead of warning when evaluating the types of the
expression, sparse could leave in a "warning node" into the tree,
linearize it to a "warning instruction" in the SSA form, and only
actually output a warning if that instruction still exists after dead
code elimination etc.

But that kind of fix would be a pretty big change, we don't have that
kind of thing right now at all. So "fixable in theory" is probably not
"practical with the current lack of sparse development".

What would be much easier is probably to hack together a couple of
builtins for type checking: a "__builtin_signed_p()" should not be
hard.

                  Linus