include/trace/events/nilfs2.h | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)
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
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
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.
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
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
© 2016 - 2024 Red Hat, Inc.