Re: [PATCH 51/51] fs/zonefs: Fix sparse warnings in tracing code

Re: [PATCH 51/51] fs/zonefs: Fix sparse warnings in tracing code

[Luc Van Oostenryck]

On Sat, Jun 25, 2022 at 05:44:54PM -0700, Bart Van Assche wrote:
> On 6/25/22 02:23, Christoph Hellwig wrote:
> > On Fri, Jun 24, 2022 at 12:57:56PM -0700, Bart Van Assche wrote:
> > > BTW, I discovered the code in the tracing infrastructure
> > > that makes sparse unhappy:
> > > 
> > > #define is_signed_type(type) (((type)(-1)) < (type)1)
> > > 
> > > Sparse reports four warnings for that expression if 'type' is a bitwise
> > > type. Two of these warnings can be suppressed by changing 'type' into
> > > '__force type'. I have not yet found a way to suppress all the sparse
> > > warnings triggered by the is_signed_type() macro for bitwise types.

Yes, __bitwise is quite strict and only support the bitwise operations
(&, |, ^ and).
 
> > Yeah, that is a bit of a mess.  Rasmus, Steven - any good idea how
> > we can make the trace even macros fit for sparse?  Maybe just drop the
> > is_signed_type check for __CHECKER__ ?

I would strongly advise against this:
-) the macro is sued elsewhere too (for overflow checking)
-) sparse wouldn't check anymore the same code as the one seen by the
   compiler 

What about I would add to sparse something to strip away the bitwise/
recover the underlying type? Something like __unbitwiseof() or
__underlying_typeof() (some better name is needed)?

Implementing directly what's needed here, something like __is_signed_type()
would be possible too but is a bit too specialized and so much less useful.

-- Luc Van Oostenryck

Re: [PATCH 51/51] fs/zonefs: Fix sparse warnings in tracing code

[Bart Van Assche]

On 6/26/22 02:58, Luc Van Oostenryck wrote:
> On Sat, Jun 25, 2022 at 05:44:54PM -0700, Bart Van Assche wrote:
>> On 6/25/22 02:23, Christoph Hellwig wrote:
>>> Yeah, that is a bit of a mess.  Rasmus, Steven - any good idea how
>>> we can make the trace even macros fit for sparse?  Maybe just drop the
>>> is_signed_type check for __CHECKER__ ?
> 
> I would strongly advise against this:
> -) the macro is sued elsewhere too (for overflow checking)
> -) sparse wouldn't check anymore the same code as the one seen by the
>     compiler
> 
> What about I would add to sparse something to strip away the bitwise/
> recover the underlying type? Something like __unbitwiseof() or
> __underlying_typeof() (some better name is needed)?
> 
> Implementing directly what's needed here, something like __is_signed_type()
> would be possible too but is a bit too specialized and so much less useful.

Another question is how to keep the non-sparse build working. Does
anyone want to comment on the following alternatives or propose another
alternative?

(1) sparse implements __strip_bitwise as a macro.

(in compiler.h)

#ifndef __strip_bitwise
#define __strip_bitwise(type) type
#endif

(in trace_events.h)

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

(2) sparse implements __strip_bitwise as an operator that works on types.

#ifdef __CHECKER__
#define is_signed_type(type) ((__strip_bitwise(type))(-1) < (__strip_bitwise(type))1)
#else
#define is_signed_type(type) (((type)(-1)) < (type)1)
#endif

(1) would work better than (2) for kernel developers who are using a
version of sparse that does not support __strip_bitwise().

Thanks,

Bart.

Re: [PATCH 51/51] fs/zonefs: Fix sparse warnings in tracing code

[Luc Van Oostenryck]

On Sun, Jun 26, 2022 at 08:42:27AM -0700, Bart Van Assche wrote:
> On 6/26/22 02:58, Luc Van Oostenryck wrote:
> > On Sat, Jun 25, 2022 at 05:44:54PM -0700, Bart Van Assche wrote:
> > > On 6/25/22 02:23, Christoph Hellwig wrote:
> > > > Yeah, that is a bit of a mess.  Rasmus, Steven - any good idea how
> > > > we can make the trace even macros fit for sparse?  Maybe just drop the
> > > > is_signed_type check for __CHECKER__ ?
> > 
> > I would strongly advise against this:
> > -) the macro is sued elsewhere too (for overflow checking)
> > -) sparse wouldn't check anymore the same code as the one seen by the
> >     compiler
> > 
> > What about I would add to sparse something to strip away the bitwise/
> > recover the underlying type? Something like __unbitwiseof() or
> > __underlying_typeof() (some better name is needed)?
> 
> Another question is how to keep the non-sparse build working. Does
> anyone want to comment on the following alternatives or propose another
> alternative?
> 
> (1) sparse implements __strip_bitwise as a macro.
> 
> (in compiler.h)
> 
> #ifndef __strip_bitwise
> #define __strip_bitwise(type) type
> #endif

...

> (1) would work better than (2) for kernel developers who are using a
> version of sparse that does not support __strip_bitwise().

Yes, sure. I was thinking about using (and adding) __has_feature()
but the goal is the same.

[I prefer this because, internally, an operator is needed anyway and
__has_feature() would be more general (but then it would need to be
protected by its own #ifndef __has_feature).]

-- Luc

Re: [PATCH 51/51] fs/zonefs: Fix sparse warnings in tracing code

[Linus Torvalds]

On Sun, Jun 26, 2022 at 2:58 AM Luc Van Oostenryck
<luc.vanoostenryck@gmail.com> wrote:
>
> What about I would add to sparse something to strip away the bitwise/
> recover the underlying type? Something like __unbitwiseof() or
> __underlying_typeof() (some better name is needed)?

Please no, we don't want to make random macros have to have sparse
logic in them when it's not actually sparse-related.

I think it would be better if sparse just recognized some of these
kinds of situation. In particular:

 (a) for the casting part, I actually suspect we should drop the
warning about castign integers to restricted types.

Note that this is actually one of the main causes of "__force" use in
the kernel, with code like

        VM_FAULT_OOM            = (__force vm_fault_t)0x000001,
        VM_FAULT_SIGBUS         = (__force vm_fault_t)0x000002,
        VM_FAULT_MAJOR          = (__force vm_fault_t)0x000004,
        VM_FAULT_WRITE          = (__force vm_fault_t)0x000008,

and I think that we could/should just say that "explicit casts of
constants are ok".

That would remove two of the four warnings right there, and probably
make bitwise types more convenient in general.

We already treat "0" as special (because for bitwise things, zero is
kind of the universal constant), and we should continue to warn about
_implicit_ casts of restricted types, but I think the use of "__force"
in the kernel does show that the explicit casts are probably a bad
idea.

 (b) I think we could also recognize "comparison of constants" to be
something that doesn't necessarily require a warning.

And here in particular the "compare with zero" and "compare with all
bits set" - which is exactly that "-1" case.

In fact, there's a very good argument that "-1" is as special as zero
is ("all bits set" vs "all bits clear"), so for that (a) case, I think
at a _minimum_ we shouldn't warn about that particular constant.

So I think we could silence this sparse warning entirely, without
really introducing any new syntax, and actually improving on how
bitwise works.

                 Linus

Re: [PATCH 51/51] fs/zonefs: Fix sparse warnings in tracing code

[Linus Torvalds]

On Sun, Jun 26, 2022 at 9:33 AM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
>  (a) for the casting part, I actually suspect we should drop the
> warning about castign integers to restricted types.

Heh. Trying that out, I find we seem to do this at the wrong point anyway.

I removed the check for "value of 0 is ok", and it continues to warn
about casting "-1".

Because it does that before "-1" has even been simplified, so it
actually sees it as an *expression* ("negate the constant 1") rather
than as the *value* -1, and it warns.

That's a bit sad since really -1 and ~0 really both should be ok for
the same reason plain zero is ok - regardless of the whole "do we just
allow it for constants in general" thing.

I think the bitwise thing was always a bit half-baked. It was designed
for detecting little-endian and big-endian issues, and "bitmask"
types. And it's wonderful for the basics of that, but I think it was
never really thought through for this kind of extended use where we
end up having generic macros that do still make sense for them, but
aren't that exact "avoid assigning bitwise values to other values"

                 Linus

Re: [PATCH 51/51] fs/zonefs: Fix sparse warnings in tracing code

[Luc Van Oostenryck]

On Sun, Jun 26, 2022 at 09:50:50AM -0700, Linus Torvalds wrote:
> On Sun, Jun 26, 2022 at 9:33 AM Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
> >
> >  (a) for the casting part, I actually suspect we should drop the
> > warning about castign integers to restricted types.
> 
> Heh. Trying that out, I find we seem to do this at the wrong point anyway.
> 
> I removed the check for "value of 0 is ok", and it continues to warn
> about casting "-1".
> 
> Because it does that before "-1" has even been simplified, so it
> actually sees it as an *expression* ("negate the constant 1") rather
> than as the *value* -1, and it warns.
> 
> That's a bit sad since really -1 and ~0 really both should be ok for
> the same reason plain zero is ok - regardless of the whole "do we just
> allow it for constants in general" thing.

Well, this "there is no negative constants, only minus positive ones" 
is kinda special. Maybe we can special-case this, just doing an early
mini-expansion of PREOP('-', EXPR_VALUE). I'm not sure about all
implications it would have on typechecking though.

> I think the bitwise thing was always a bit half-baked. It was designed
> for detecting little-endian and big-endian issues, and "bitmask"
> types. And it's wonderful for the basics of that, but I think it was
> never really thought through for this kind of extended use where we
> end up having generic macros that do still make sense for them, but
> aren't that exact "avoid assigning bitwise values to other values"

Yes, I fully agree
Also, it's the only 'strong' type that sparse has (IIRC nocast was
too weak), so it's used for everything. Allowing only the bitwise
operators is often too restrictive. I think that often what people
just need is an unique type that doesn't mix with other types
but that can 1) mix with constants (certainly with 0, 1 and -1) and
2) can use the usual arithmetic operations (certainly the compares).

-- Luc

Re: [PATCH 51/51] fs/zonefs: Fix sparse warnings in tracing code

[Luc Van Oostenryck]

On Sun, Jun 26, 2022 at 09:33:57AM -0700, Linus Torvalds wrote:
> On Sun, Jun 26, 2022 at 2:58 AM Luc Van Oostenryck
> <luc.vanoostenryck@gmail.com> wrote:
> >
> > What about I would add to sparse something to strip away the bitwise/
> > recover the underlying type? Something like __unbitwiseof() or
> > __underlying_typeof() (some better name is needed)?
> 
> Please no, we don't want to make random macros have to have sparse
> logic in them when it's not actually sparse-related.
> 
> I think it would be better if sparse just recognized some of these
> kinds of situation. In particular:

Yes, sure, it's ideal.
 
>  (a) for the casting part, I actually suspect we should drop the
> warning about castign integers to restricted types.
> 
> Note that this is actually one of the main causes of "__force" use in
> the kernel, with code like
> 
>         VM_FAULT_OOM            = (__force vm_fault_t)0x000001,
>         VM_FAULT_SIGBUS         = (__force vm_fault_t)0x000002,
>         VM_FAULT_MAJOR          = (__force vm_fault_t)0x000004,
>         VM_FAULT_WRITE          = (__force vm_fault_t)0x000008,

This example is about an enumeration. It's, IMO, a very special case
in its own. Two years ago or so, I had proposed to have 'bitwise'
enums where the cast was not needed. In itself is was very easy to do
but there was a lot of subtle issues about type attributes. I think
I've since solved these issues but on the way I've lost my motivation
for these bitwise enums. I'll take a look at it again.
 
> and I think that we could/should just say that "explicit casts of
> constants are ok".

I'm not convinced, for example when thinking about __be{16,32}.
But on the principle, I fully agree: unneeded casts should be avoided.
 
> That would remove two of the four warnings right there, and probably
> make bitwise types more convenient in general.
> 
> We already treat "0" as special (because for bitwise things, zero is
> kind of the universal constant), and we should continue to warn about
> _implicit_ casts of restricted types, but I think the use of "__force"
> in the kernel does show that the explicit casts are probably a bad
> idea.

Yes.

>  (b) I think we could also recognize "comparison of constants" to be
> something that doesn't necessarily require a warning.
> 
> And here in particular the "compare with zero" and "compare with all
> bits set" - which is exactly that "-1" case.
>
> In fact, there's a very good argument that "-1" is as special as zero
> is ("all bits set" vs "all bits clear"), so for that (a) case, I think
> at a _minimum_ we shouldn't warn about that particular constant.
> 
> So I think we could silence this sparse warning entirely, without
> really introducing any new syntax, and actually improving on how
> bitwise works.

Yes, indeed.

-- Luc 

[PATCH 0/5] allow -1 and compares in bitwise types

[Luc Van Oostenryck]

Allow using -1 and compare operators with bitwise types

This series is an experiment for allowing constants like
-1 or ~0 with bitwise types, as well as using compares
on them.
the motivation for allowing this is to avoid useless casts
and warnings on using some generic macros on bitwise types.
The case of interest here is:
	#define is_signed_type(T) (((T)(-1)) < (T)1)
for which Sparse reports four warnings when 'T' is a bitwise
type. Two of these warnings can be suppressed by using
'__force' in the casts. But the other two can't be worked
around because explicitly reject using any constants other
than 0 with bitwise type.

The changes in the series allow to use the following testcase
without any warnings:
	#define __bitwise __attribute__((bitwise))
	typedef   signed int __bitwise s;
	typedef unsigned int __bitwise u;

	#define is_signed_type(type)  (((type)-1) <= 0)

	int fs(void) { return ((s)-1) <= 0; }
	int fu(void) { return ((u)-1) <= 0; }

and result in a modest but significant decrease in the number
of warnings issued by sparse (x86 {def,all}config):
   -  2736  2735 cast to restricted type
   +   774   792 cast truncates bits from constant value
   -  3183  3152 incorrect type in assignment (different base types)
   -   162   159 incorrect type in initializer (different base types)
   -   789   661 restricted type degrades to integer
   - 13002 12857 Total

@Linus,
This seems to work correctly and I think it correspond to
what you wished (maybe modulo this 'early constant expansion'
I added) and I think that accepting the -1/all-ones is fine.
OTOH, I don't think we can allow the compares because they
don't make any sense for the 'endian' types. Surely we want
to catch things like:
	typedef unsigned int __bitwise __be32;
	__be32 x, y;
	...
	... (x < y)

@Bart,
Is there a good reason why the macro compares against 1 and
not against 0? Is it just because of -Wtype-limits? If so,
is it possible to use '<= 0' like here above?

The series is available for review and testing at:
  https://git.kernel.org/pub/scm/devel/sparse/sparse.git bitwise-ones

Luc Van Oostenryck (5):
  bitwise: add testcases
  bitwise: accept all ones as non-restricted value
  bitwise: allow compares for bitwise types
  bitwise: do not remove the signedness of bitwise types
  bitwise: early expansion of simple constants

 evaluate.c                       | 52 +++++++++++++++++++++++++++++++-
 parse.c                          |  1 -
 show-parse.c                     |  2 +-
 validation/bitwise-cast.c        | 26 ++++++++++++++++
 validation/bitwise-cmp.c         | 31 +++++++++++++++++++
 validation/bitwise-is-signed.c   | 21 +++++++++++++
 validation/linear/bitwise-cmps.c | 17 +++++++++++
 validation/linear/bitwise-cmpu.c | 17 +++++++++++
 8 files changed, 164 insertions(+), 3 deletions(-)
 create mode 100644 validation/bitwise-cmp.c
 create mode 100644 validation/bitwise-is-signed.c
 create mode 100644 validation/linear/bitwise-cmps.c
 create mode 100644 validation/linear/bitwise-cmpu.c

-- 
2.36.1

[PATCH 1/5] bitwise: add testcases

[Luc Van Oostenryck]

Currently bitwise types only support bitwise operations
(&, |, ^ and ~) and the constant 0 (since this value is
invariant for all bitwise operations and endianness conversion).

But the incoming series will relax this a little bit.
So, add a few testcases for it.

Signed-off-by: Luc Van Oostenryck <luc.vanoostenryck@gmail.com>
---
 validation/bitwise-cmp.c         | 32 ++++++++++++++++++++++++++++++++
 validation/bitwise-is-signed.c   | 22 ++++++++++++++++++++++
 validation/linear/bitwise-cmps.c | 18 ++++++++++++++++++
 validation/linear/bitwise-cmpu.c | 18 ++++++++++++++++++
 4 files changed, 90 insertions(+)
 create mode 100644 validation/bitwise-cmp.c
 create mode 100644 validation/bitwise-is-signed.c
 create mode 100644 validation/linear/bitwise-cmps.c
 create mode 100644 validation/linear/bitwise-cmpu.c

diff --git a/validation/bitwise-cmp.c b/validation/bitwise-cmp.c
new file mode 100644
index 000000000000..ca12b5e51e8e
--- /dev/null
+++ b/validation/bitwise-cmp.c
@@ -0,0 +1,32 @@
+#define M 0xffffffff
+
+typedef int __attribute__((bitwise)) b32;
+
+static int eq0(b32 x, b32 y)  { return (x == 0); }
+static int eqm(b32 x, b32 y)  { return (x == M); }
+static int eqx(b32 x, b32 y)  { return (x == y); }
+
+static int ne0(b32 x, b32 y)  { return (x != 0); }
+static int nem(b32 x, b32 y)  { return (x != M); }
+static int nex(b32 x, b32 y)  { return (x != y); }
+
+static int lt0(b32 x, b32 y)  { return (x <  0); }
+static int ltm(b32 x, b32 y)  { return (x <  M); }
+static int ltx(b32 x, b32 y)  { return (x <  y); }
+
+static int lte0(b32 x, b32 y) { return (x <= 0); }
+static int ltem(b32 x, b32 y) { return (x <= M); }
+static int ltex(b32 x, b32 y) { return (x <= y); }
+
+static int gte0(b32 x, b32 y) { return (x >= 0); }
+static int gtem(b32 x, b32 y) { return (x >= M); }
+static int gtex(b32 x, b32 y) { return (x >= y); }
+
+static int gt0(b32 x, b32 y)  { return (x >  0); }
+static int gtm(b32 x, b32 y)  { return (x >  M); }
+static int gtx(b32 x, b32 y)  { return (x >  y); }
+
+/*
+ * check-name: bitwise-cmp
+ * check-known-to-fail
+ */
diff --git a/validation/bitwise-is-signed.c b/validation/bitwise-is-signed.c
new file mode 100644
index 000000000000..dd9c147173cd
--- /dev/null
+++ b/validation/bitwise-is-signed.c
@@ -0,0 +1,22 @@
+#define __bitwise __attribute__((bitwise))
+
+#define is_signed_type(type)  (((type)-1) <= 0)
+
+typedef   signed int __bitwise s;
+typedef unsigned int __bitwise u;
+
+int fos(void);
+int fou(void);
+
+
+int fos(void) { return  is_signed_type(s); }
+int fou(void) { return !is_signed_type(u); }
+
+/*
+ * check-name: bitwise-is-signed
+ * check-command: test-linearize -Wno-decl $file
+ * check-known-to-fail
+ *
+ * check-output-ignore
+ * check-output-returns: 1
+ */
diff --git a/validation/linear/bitwise-cmps.c b/validation/linear/bitwise-cmps.c
new file mode 100644
index 000000000000..6122944a42c6
--- /dev/null
+++ b/validation/linear/bitwise-cmps.c
@@ -0,0 +1,18 @@
+typedef   signed int __attribute__((bitwise)) bs32;
+
+static int ltu(bs32 x, bs32 y)  { return (x <  y); }
+static int lteu(bs32 x, bs32 y) { return (x <= y); }
+static int gteu(bs32 x, bs32 y) { return (x >= y); }
+static int gtu(bs32 x, bs32 y)  { return (x >  y); }
+
+/*
+ * check-name: bitwise-cmps
+ * check-command: test-linearize -Wno-decl $file
+ * check-known-to-fail
+ *
+ * check-output-ignore
+ * check-output-excludes: setb\\.
+ * check-output-excludes: setbe\\.
+ * check-output-excludes: setae\\.
+ * check-output-excludes: seta\\.
+ */
diff --git a/validation/linear/bitwise-cmpu.c b/validation/linear/bitwise-cmpu.c
new file mode 100644
index 000000000000..8932436a7764
--- /dev/null
+++ b/validation/linear/bitwise-cmpu.c
@@ -0,0 +1,18 @@
+typedef unsigned int __attribute__((bitwise)) bu32;
+
+static int ltu(bu32 x, bu32 y)  { return (x <  y); }
+static int lteu(bu32 x, bu32 y) { return (x <= y); }
+static int gteu(bu32 x, bu32 y) { return (x >= y); }
+static int gtu(bu32 x, bu32 y)  { return (x >  y); }
+
+/*
+ * check-name: bitwise-cmpu
+ * check-command: test-linearize -Wno-decl $file
+ * check-known-to-fail
+ *
+ * check-output-ignore
+ * check-output-excludes: setlt\\.
+ * check-output-excludes: setlte\\.
+ * check-output-excludes: setgte\\.
+ * check-output-excludes: setgt\\.
+ */
-- 
2.36.1

[PATCH 2/5] bitwise: accept all ones as non-restricted value

[Luc Van Oostenryck]

Currently, the only value bitwise types can act on is 0
because the this value is anyway invariant for all bitwise
operations and endianness conversions.

But, a bit-pattern of all ones has the same properties and
is also very often used.

So, accept all ones as a valid value for bitwise operations.

Signed-off-by: Luc Van Oostenryck <luc.vanoostenryck@gmail.com>
---
 evaluate.c                |  2 +-
 validation/bitwise-cast.c | 13 +++++++++++++
 2 files changed, 14 insertions(+), 1 deletion(-)

diff --git a/evaluate.c b/evaluate.c
index 61f59ee3908e..bcbcdf1ef0cc 100644
--- a/evaluate.c
+++ b/evaluate.c
@@ -417,7 +417,7 @@ static int restricted_value(struct expression *v, struct symbol *type)
 {
 	if (v->type != EXPR_VALUE)
 		return 1;
-	if (v->value != 0)
+	if (v->value != 0 && v->value != bits_mask(type->bit_size))
 		return 1;
 	return 0;
 }
diff --git a/validation/bitwise-cast.c b/validation/bitwise-cast.c
index 0583461cb745..1075a3e9410c 100644
--- a/validation/bitwise-cast.c
+++ b/validation/bitwise-cast.c
@@ -35,6 +35,19 @@ static __be32 quuy(void)
 	return (__attribute__((force)) __be32) 1730;
 }
 
+/* Implicit casts of all ones, legal */
+static __be32 foo1(void)
+{
+	__be32 x = 0xffffffff;
+	return x;
+}
+
+/* Explicit cast of all ones, legal */
+static __be32 bar1(void)
+{
+	return (__be32)0xffffffff;
+}
+
 /*
  * check-name: conversions to bitwise types
  * check-command: sparse -Wbitwise $file
-- 
2.36.1

Re: [PATCH 2/5] bitwise: accept all ones as non-restricted value

[Ramsay Jones]

On 27/06/2022 20:05, Luc Van Oostenryck wrote:
> Currently, the only value bitwise types can act on is 0
> because the this value is anyway invariant for all bitwise

s/the this/this/
s/anyway//

ATB,
Ramsay Jones

> operations and endianness conversions.
> 
> But, a bit-pattern of all ones has the same properties and
> is also very often used.
> 
> So, accept all ones as a valid value for bitwise operations.
> 
> Signed-off-by: Luc Van Oostenryck <luc.vanoostenryck@gmail.com>
> ---
>  evaluate.c                |  2 +-
>  validation/bitwise-cast.c | 13 +++++++++++++
>  2 files changed, 14 insertions(+), 1 deletion(-)
> 
> diff --git a/evaluate.c b/evaluate.c
> index 61f59ee3908e..bcbcdf1ef0cc 100644
> --- a/evaluate.c
> +++ b/evaluate.c
> @@ -417,7 +417,7 @@ static int restricted_value(struct expression *v, struct symbol *type)
>  {
>  	if (v->type != EXPR_VALUE)
>  		return 1;
> -	if (v->value != 0)
> +	if (v->value != 0 && v->value != bits_mask(type->bit_size))
>  		return 1;
>  	return 0;
>  }
> diff --git a/validation/bitwise-cast.c b/validation/bitwise-cast.c
> index 0583461cb745..1075a3e9410c 100644
> --- a/validation/bitwise-cast.c
> +++ b/validation/bitwise-cast.c
> @@ -35,6 +35,19 @@ static __be32 quuy(void)
>  	return (__attribute__((force)) __be32) 1730;
>  }
>  
> +/* Implicit casts of all ones, legal */
> +static __be32 foo1(void)
> +{
> +	__be32 x = 0xffffffff;
> +	return x;
> +}
> +
> +/* Explicit cast of all ones, legal */
> +static __be32 bar1(void)
> +{
> +	return (__be32)0xffffffff;
> +}
> +
>  /*
>   * check-name: conversions to bitwise types
>   * check-command: sparse -Wbitwise $file

[PATCH 3/5] bitwise: allow compares for bitwise types

[Luc Van Oostenryck]

Currently, bitwise types are restricted to bitwise operations
(&, |, ^ and ~) as well as equality comparisons.

This patch makes the others comparisons valid for bitwise types
too.

Warning: This change make sense in the context of [1] but
         doesn't make sense for the 'main' bitwise types:
         __be32 and friends.
Signed-off-by: Luc Van Oostenryck <luc.vanoostenryck@gmail.com>
---
 evaluate.c                       | 4 ++++
 validation/bitwise-cmp.c         | 1 -
 validation/linear/bitwise-cmps.c | 1 -
 3 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/evaluate.c b/evaluate.c
index bcbcdf1ef0cc..bb8c0caa905a 100644
--- a/evaluate.c
+++ b/evaluate.c
@@ -435,6 +435,10 @@ static int restricted_binop(int op, struct symbol *type)
 		case '^':
 		case '?':
 			return 2;	/* keep fouled */
+		case '<':
+		case '>':
+		case SPECIAL_LTE:
+		case SPECIAL_GTE:
 		case SPECIAL_EQUAL:
 		case SPECIAL_NOTEQUAL:
 			return 3;	/* warn if fouled */
diff --git a/validation/bitwise-cmp.c b/validation/bitwise-cmp.c
index ca12b5e51e8e..8c3e6894072d 100644
--- a/validation/bitwise-cmp.c
+++ b/validation/bitwise-cmp.c
@@ -28,5 +28,4 @@ static int gtx(b32 x, b32 y)  { return (x >  y); }
 
 /*
  * check-name: bitwise-cmp
- * check-known-to-fail
  */
diff --git a/validation/linear/bitwise-cmps.c b/validation/linear/bitwise-cmps.c
index 6122944a42c6..f83ab7fe47db 100644
--- a/validation/linear/bitwise-cmps.c
+++ b/validation/linear/bitwise-cmps.c
@@ -8,7 +8,6 @@ static int gtu(bs32 x, bs32 y)  { return (x >  y); }
 /*
  * check-name: bitwise-cmps
  * check-command: test-linearize -Wno-decl $file
- * check-known-to-fail
  *
  * check-output-ignore
  * check-output-excludes: setb\\.
-- 
2.36.1

Re: [PATCH 3/5] bitwise: allow compares for bitwise types

[Linus Torvalds]

On Mon, Jun 27, 2022 at 12:05 PM Luc Van Oostenryck
<luc.vanoostenryck@gmail.com> wrote:
>
> Currently, bitwise types are restricted to bitwise operations
> (&, |, ^ and ~) as well as equality comparisons.
>
> This patch makes the others comparisons valid for bitwise types
> too.
>
> Warning: This change make sense in the context of [1] but
>          doesn't make sense for the 'main' bitwise types:
>          __be32 and friends.

Yeah, this is wrong.

It will literally break one of the use-cases, which is endianness comparisons.

You cannot compare values in the wrong endianness for greater-than or
less-than, because you will get the wrong answer - the ordering is
different in different byte-orders.

But comparing for equality (and inequality) is fine, and we actually
do that in the kernel (ie you can take a big-endian value, and compare
it with another big-endian value for being equal, without converting
it to the local CPU endianness).

Now, comparing the *constants* 0 and all-ones is fine. They are
smaller than (and larger than) all other values, regardless of any
byte/bit order issues.

So I think that really needs to check that one (or both) sides are the
magic constants.

             Linus

Re: [PATCH 3/5] bitwise: allow compares for bitwise types

[Ramsay Jones]

On 27/06/2022 20:05, Luc Van Oostenryck wrote:
> Currently, bitwise types are restricted to bitwise operations
> (&, |, ^ and ~) as well as equality comparisons.
> 
> This patch makes the others comparisons valid for bitwise types
> too.
> 
> Warning: This change make sense in the context of [1] but

The [1] reference seems to be missing :)

ATB,
Ramsay Jones

>          doesn't make sense for the 'main' bitwise types:
>          __be32 and friends.
> Signed-off-by: Luc Van Oostenryck <luc.vanoostenryck@gmail.com>
> ---
>  evaluate.c                       | 4 ++++
>  validation/bitwise-cmp.c         | 1 -
>  validation/linear/bitwise-cmps.c | 1 -
>  3 files changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/evaluate.c b/evaluate.c
> index bcbcdf1ef0cc..bb8c0caa905a 100644
> --- a/evaluate.c
> +++ b/evaluate.c
> @@ -435,6 +435,10 @@ static int restricted_binop(int op, struct symbol *type)
>  		case '^':
>  		case '?':
>  			return 2;	/* keep fouled */
> +		case '<':
> +		case '>':
> +		case SPECIAL_LTE:
> +		case SPECIAL_GTE:
>  		case SPECIAL_EQUAL:
>  		case SPECIAL_NOTEQUAL:
>  			return 3;	/* warn if fouled */
> diff --git a/validation/bitwise-cmp.c b/validation/bitwise-cmp.c
> index ca12b5e51e8e..8c3e6894072d 100644
> --- a/validation/bitwise-cmp.c
> +++ b/validation/bitwise-cmp.c
> @@ -28,5 +28,4 @@ static int gtx(b32 x, b32 y)  { return (x >  y); }
>  
>  /*
>   * check-name: bitwise-cmp
> - * check-known-to-fail
>   */
> diff --git a/validation/linear/bitwise-cmps.c b/validation/linear/bitwise-cmps.c
> index 6122944a42c6..f83ab7fe47db 100644
> --- a/validation/linear/bitwise-cmps.c
> +++ b/validation/linear/bitwise-cmps.c
> @@ -8,7 +8,6 @@ static int gtu(bs32 x, bs32 y)  { return (x >  y); }
>  /*
>   * check-name: bitwise-cmps
>   * check-command: test-linearize -Wno-decl $file
> - * check-known-to-fail
>   *
>   * check-output-ignore
>   * check-output-excludes: setb\\.

[PATCH 4/5] bitwise: do not remove the signedness of bitwise types

[Luc Van Oostenryck]

When bitwise types were added [1] the signedness modifiers
were removed from them. More exactly, it was MOD_SPECIFIER
which was removed. I suppose this was done because then
MOD_SPECIFIER contained the signedness bits but also MOD_CHAR,
MOD_LONG, ...  and those had to be removed.

But currently MOD_SPECIFIER contains only MOD_SIGNEDNESS
and the signedness info can be useful for bitwise types too.

So, do not removed anymore MOD_SPECIFIER from the bitwise
types' modifiers.

[1] commit 032f492af0ac ("[PATCH] __attribute__((bitwise))")

Signed-off-by: Luc Van Oostenryck <luc.vanoostenryck@gmail.com>
---
 parse.c                          | 1 -
 show-parse.c                     | 2 +-
 validation/linear/bitwise-cmpu.c | 1 -
 3 files changed, 1 insertion(+), 3 deletions(-)

diff --git a/parse.c b/parse.c
index 3d6fef7cb011..14fe9e15448a 100644
--- a/parse.c
+++ b/parse.c
@@ -1586,7 +1586,6 @@ static struct token *declaration_specifiers(struct token *token, struct decl_sta
 		}
 		type = alloc_symbol(token->pos, SYM_BASETYPE);
 		*type = *ctx->ctype.base_type;
-		type->ctype.modifiers &= ~MOD_SPECIFIER;
 		type->ctype.base_type = ctx->ctype.base_type;
 		type->type = SYM_RESTRICT;
 		ctx->ctype.base_type = type;
diff --git a/show-parse.c b/show-parse.c
index e2fc18bb4b3d..2ab2479b8181 100644
--- a/show-parse.c
+++ b/show-parse.c
@@ -318,7 +318,7 @@ deeper:
 		if (as)
 			prepend(name, "%s ", show_as(as));
 
-		if (sym && (sym->type == SYM_BASETYPE || sym->type == SYM_ENUM))
+		if (sym && (sym->type == SYM_BASETYPE || sym->type == SYM_ENUM || sym->type == SYM_RESTRICT))
 			mod &= ~MOD_SPECIFIER;
 		s = modifier_string(mod);
 		len = strlen(s);
diff --git a/validation/linear/bitwise-cmpu.c b/validation/linear/bitwise-cmpu.c
index 8932436a7764..e151b9741fa3 100644
--- a/validation/linear/bitwise-cmpu.c
+++ b/validation/linear/bitwise-cmpu.c
@@ -8,7 +8,6 @@ static int gtu(bu32 x, bu32 y)  { return (x >  y); }
 /*
  * check-name: bitwise-cmpu
  * check-command: test-linearize -Wno-decl $file
- * check-known-to-fail
  *
  * check-output-ignore
  * check-output-excludes: setlt\\.
-- 
2.36.1

[PATCH 5/5] bitwise: early expansion of simple constants

[Luc Van Oostenryck]

C has only positive constants: -1 is really an expression,
the unary '-' operator applied to the constant 1. '-1' as
a constant value only exists after the expansion of constant
expressions.

This is rather unfortunate since it inhibits easy testing
of such constants in the evaluation phase, like here for
restricted_value().

So, expand expressions like +CTE, -CTE or ~CTE before
calling restricted_value().

Signed-off-by: Luc Van Oostenryck <luc.vanoostenryck@gmail.com>
---
 evaluate.c                     | 48 +++++++++++++++++++++++++++++++++-
 validation/bitwise-cast.c      | 13 +++++++++
 validation/bitwise-is-signed.c |  1 -
 3 files changed, 60 insertions(+), 2 deletions(-)

diff --git a/evaluate.c b/evaluate.c
index bb8c0caa905a..33cc85c8d40f 100644
--- a/evaluate.c
+++ b/evaluate.c
@@ -413,11 +413,56 @@ static struct symbol *bad_expr_type(struct expression *expr)
 	return expr->ctype = &bad_ctype;
 }
 
+static bool expand_simple_constant(struct expression *expr, struct symbol *type)
+{
+	unsigned long long val, mask;
+	struct expression *pre;
+	struct symbol *ctype;
+	unsigned size;
+
+	if (expr->type != EXPR_PREOP)
+		return false;
+	pre = expr->unop;
+	if (pre->type != EXPR_VALUE)
+		return false;
+
+	ctype = pre->ctype;
+	if (!ctype || ctype != type)
+		return false;
+
+	size = ctype->bit_size;
+	if (!size)
+		return false;
+
+	mask = sign_bit(size);
+	switch (expr->op) {
+	case '+':
+		val = pre->value;
+		break;
+	case '-':
+		val = pre->value;
+		if (val == mask && !(ctype->ctype.modifiers & MOD_UNSIGNED))
+			return false;
+		val = -val;
+		break;
+	case '~':
+		val = pre->value;
+		val = ~val;
+		break;
+	default:
+		return false;
+	}
+	expr->op = 0;
+	expr->type = EXPR_VALUE;
+	expr->value = val & bits_mask(size);
+	return true;
+}
+
 static int restricted_value(struct expression *v, struct symbol *type)
 {
 	if (v->type != EXPR_VALUE)
 		return 1;
-	if (v->value != 0 && v->value != bits_mask(type->bit_size))
+	if (v->value != 0 && v->value != bits_mask(v->ctype->bit_size))
 		return 1;
 	return 0;
 }
@@ -1919,6 +1964,7 @@ Normal:
 	if (!(class & TYPE_FLOAT)) {
 		ctype = integer_promotion(ctype);
 		expr->unop = cast_to(expr->unop, ctype);
+		expand_simple_constant(expr, ctype);
 	} else if (expr->op != '~') {
 		/* no conversions needed */
 	} else {
diff --git a/validation/bitwise-cast.c b/validation/bitwise-cast.c
index 1075a3e9410c..01af56c73751 100644
--- a/validation/bitwise-cast.c
+++ b/validation/bitwise-cast.c
@@ -48,6 +48,19 @@ static __be32 bar1(void)
 	return (__be32)0xffffffff;
 }
 
+/* Implicit casts of minus one, legal */
+static __be32 foom(void)
+{
+	__be32 x = -1;
+	return x;
+}
+
+/* Explicit cast of minus one, legal */
+static __be32 barm(void)
+{
+	return (__be32)-1;
+}
+
 /*
  * check-name: conversions to bitwise types
  * check-command: sparse -Wbitwise $file
diff --git a/validation/bitwise-is-signed.c b/validation/bitwise-is-signed.c
index dd9c147173cd..99d16bd20f9b 100644
--- a/validation/bitwise-is-signed.c
+++ b/validation/bitwise-is-signed.c
@@ -15,7 +15,6 @@ int fou(void) { return !is_signed_type(u); }
 /*
  * check-name: bitwise-is-signed
  * check-command: test-linearize -Wno-decl $file
- * check-known-to-fail
  *
  * check-output-ignore
  * check-output-returns: 1
-- 
2.36.1

Re: [PATCH 0/5] allow -1 and compares in bitwise types

[Linus Torvalds]

On Mon, Jun 27, 2022 at 12:05 PM Luc Van Oostenryck
<luc.vanoostenryck@gmail.com> wrote:
>
>         #define is_signed_type(type)  (((type)-1) <= 0)

Side note: the reason we *don't* use this form in the kernel is
because broken compilers will complain about compares with zero in
unsigned types.

Using "<=" like you do may be an acceptable way to avoid it (the most
obvious thing is to use "< 0"), but it makes me nervous.

Regardless, I think you need the cast of the zero. I think "type"
might be a pointer, and sparse should be complaining about the horrid
use of a bare 0 as NULL.

Similar issues might happen for enums, where various compilers will
complain about comparing an enum to a non-enum.

So I'm pretty sure you would want casts on both values, instead of
assuming "it's an integer type, I don't need to cast 0".

But yeah, maybe

  #define is_signed_type(type)  (((type)-1) <= (type)0)

works fine and avoids warnings in all the cases.

Famous last words. Warnings can happen for almost anything, and I
wonder if that use of "1" had some other reason.

           Linus

Re: [PATCH 0/5] allow -1 and compares in bitwise types

[Bart Van Assche]

On 6/27/22 12:05, Luc Van Oostenryck wrote:
> Allow using -1 and compare operators with bitwise types
> 
> This series is an experiment for allowing constants like
> -1 or ~0 with bitwise types, as well as using compares
> on them.

Will these changes cause sparse to stop complaining about
the following expression?

	cpu_to_le32(1) < cpu_to_le32(2)

Shouldn't sparse keep complaining about the above expression?

> @Bart,
> Is there a good reason why the macro compares against 1 and
> not against 0?

Maybe to prevent that gcc or clang issues the following
warning: "warning: comparison of unsigned expression in ‘< 0’
is always false"?

Thanks,

Bart.