[PATCH] FS: Fixing return type of unsigned_offsets

[PATCH] FS: Fixing return type of unsigned_offsets

[Pushkar Jambhlekar]

Fixing Sparse warning. It should return bool, instead it returns
int. 

Signed-off-by: Pushkar Jambhlekar <pushkar.iit@gmail.com>
---
 fs/read_write.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/read_write.c b/fs/read_write.c
index 47c1d44..d672830 100644
--- a/fs/read_write.c
+++ b/fs/read_write.c
@@ -33,9 +33,9 @@ const struct file_operations generic_ro_fops = {
 
 EXPORT_SYMBOL(generic_ro_fops);
 
-static inline int unsigned_offsets(struct file *file)
+static inline bool unsigned_offsets(struct file *file)
 {
-	return file->f_mode & FMODE_UNSIGNED_OFFSET;
+	return !!(file->f_mode & FMODE_UNSIGNED_OFFSET);
 }
 
 /**
-- 
2.7.4

Re: [PATCH] FS: Fixing return type of unsigned_offsets

[Joe Perches]

On Thu, 2017-05-11 at 09:57 +0530, Pushkar Jambhlekar wrote:
> Fixing Sparse warning. It should return bool, instead it returns
> int.
[]
> diff --git a/fs/read_write.c b/fs/read_write.c
[]
> @@ -33,9 +33,9 @@ const struct file_operations generic_ro_fops = {
>  
>  EXPORT_SYMBOL(generic_ro_fops);
>  
> -static inline int unsigned_offsets(struct file *file)
> +static inline bool unsigned_offsets(struct file *file)
>  {
> -	return file->f_mode & FMODE_UNSIGNED_OFFSET;
> +	return !!(file->f_mode & FMODE_UNSIGNED_OFFSET);

trivia: the !! isn't necessary as by definition
all non-zero assigns to bool are converted to 1.

Re: [PATCH] FS: Fixing return type of unsigned_offsets

[Pushkar Jambhlekar]

Should I change my implementation, i.e. remove '!!'?

On Thu, May 11, 2017 at 10:09 AM, Joe Perches <joe@perches.com> wrote:
> On Thu, 2017-05-11 at 09:57 +0530, Pushkar Jambhlekar wrote:
>> Fixing Sparse warning. It should return bool, instead it returns
>> int.
> []
>> diff --git a/fs/read_write.c b/fs/read_write.c
> []
>> @@ -33,9 +33,9 @@ const struct file_operations generic_ro_fops = {
>>
>>  EXPORT_SYMBOL(generic_ro_fops);
>>
>> -static inline int unsigned_offsets(struct file *file)
>> +static inline bool unsigned_offsets(struct file *file)
>>  {
>> -     return file->f_mode & FMODE_UNSIGNED_OFFSET;
>> +     return !!(file->f_mode & FMODE_UNSIGNED_OFFSET);
>
> trivia: the !! isn't necessary as by definition
> all non-zero assigns to bool are converted to 1.
>



-- 
Jambhlekar Pushkar Arun

Re: [PATCH] FS: Fixing return type of unsigned_offsets

[Joe Perches]

On Thu, 2017-05-11 at 10:13 +0530, Pushkar Jambhlekar wrote:
> Should I change my implementation, i.e. remove '!!'?

That'd be up to Al.

At least one implementation using similar bit comparisons
in fs/*.c does not use !!

fs/locks.c:static bool lease_breaking(struct file_lock *fl)
fs/locks.c-{
fs/locks.c-�����return fl->fl_flags & (FL_UNLOCK_PENDING | FL_DOWNGRADE_PENDING)
fs/locks.c-}

I didn't look very hard.

Re: [PATCH] FS: Fixing return type of unsigned_offsets

[Joe Perches]

On Thu, 2017-05-11 at 10:13 +0530, Pushkar Jambhlekar wrote:
> Should I change my implementation, i.e. remove '!!'?

That'd be up to Al.

At least one implementation using similar bit comparisons
in fs/*.c does not use !!

fs/locks.c:static bool lease_breaking(struct file_lock *fl)
fs/locks.c-{
fs/locks.c-     return fl->fl_flags & (FL_UNLOCK_PENDING | FL_DOWNGRADE_PENDING)
fs/locks.c-}

I didn't look very hard.

Re: [PATCH] FS: Fixing return type of unsigned_offsets

[Pushkar Jambhlekar]

If I remove '!!', sparse flags warning:

fs/read_write.c:38:29: warning: incorrect type in return expression
(different base types)
fs/read_write.c:38:29:    expected bool
fs/read_write.c:38:29:    got restricted fmode_t

It means explicit conversion is needed.

On Thu, May 11, 2017 at 10:25 AM, Joe Perches <joe@perches.com> wrote:
> On Thu, 2017-05-11 at 10:13 +0530, Pushkar Jambhlekar wrote:
>> Should I change my implementation, i.e. remove '!!'?
>
> That'd be up to Al.
>
> At least one implementation using similar bit comparisons
> in fs/*.c does not use !!
>
> fs/locks.c:static bool lease_breaking(struct file_lock *fl)
> fs/locks.c-{
> fs/locks.c-     return fl->fl_flags & (FL_UNLOCK_PENDING | FL_DOWNGRADE_PENDING)
> fs/locks.c-}
>
> I didn't look very hard.



-- 
Jambhlekar Pushkar Arun

Re: [PATCH] FS: Fixing return type of unsigned_offsets

[Al Viro]

On Thu, May 11, 2017 at 10:34:02AM +0530, Pushkar Jambhlekar wrote:
> If I remove '!!', sparse flags warning:
> 
> fs/read_write.c:38:29: warning: incorrect type in return expression
> (different base types)
> fs/read_write.c:38:29:    expected bool
> fs/read_write.c:38:29:    got restricted fmode_t
> 
> It means explicit conversion is needed.

FVO"needed" equal to "needed to make sparse STFU"?  If anything, that's
sparse being wrong - evaluate.c:check_assignment_type() should do
		if (t == &ctype_bool) {
			if (is_fouled_type(s))
				warning((*rp)->pos, "%s degrades to integer",
					show_typename(s->ctype.base_type));
			goto Cast;
                }
right after
                } else if (!(sclass & TYPE_RESTRICT))
                        goto Cast;

[PATCH] avoid useless warning for 'bool <- restricted type' conversion

[Luc Van Oostenryck]

Conversion to bool is special in C since this conversion
is essentially the result of the comparison with zero.
As such, some operations which are normally unsafe to
do with restricted types, like casting to an unrestricted
type, are in fact safe to do when converting to bool
and issuing a warning in those case is useless, confusing
and causes people to add useless casts in the code in
order to shut up the warning.

Fix this by catching such 'bool <- restricted type' conversion
and avoid such warnings.

CC: Al Viro <viro@zeniv.linux.org.uk>
Originally-by: Al Viro <viro@zeniv.linux.org.uk>
Signed-off-by: Luc Van Oostenryck <luc.vanoostenryck@gmail.com>
---
 evaluate.c                        |  6 ++++++
 validation/bool-cast-restricted.c | 25 +++++++++++++++++++++++++
 2 files changed, 31 insertions(+)
 create mode 100644 validation/bool-cast-restricted.c

diff --git a/evaluate.c b/evaluate.c
index 976857915..3dc26fc09 100644
--- a/evaluate.c
+++ b/evaluate.c
@@ -1345,6 +1345,12 @@ static int check_assignment_types(struct symbol *target, struct expression **rp,
 				return 1;
 		} else if (!(sclass & TYPE_RESTRICT))
 			goto Cast;
+                if (t == &bool_ctype) {
+                        if (is_fouled_type(s))
+                                warning((*rp)->pos, "%s degrades to integer",
+                                        show_typename(s->ctype.base_type));
+                        goto Cast;
+                }
 		*typediff = "different base types";
 		return 0;
 	}
diff --git a/validation/bool-cast-restricted.c b/validation/bool-cast-restricted.c
new file mode 100644
index 000000000..f6776b050
--- /dev/null
+++ b/validation/bool-cast-restricted.c
@@ -0,0 +1,25 @@
+typedef unsigned   int __attribute__((bitwise)) large_t;
+#define	LBIT	((__attribute__((force)) large_t) 1)
+
+_Bool lfoo(large_t x) { return x; }
+_Bool lbar(large_t x) { return ~x; }
+_Bool lbaz(large_t x) { return !x; }
+_Bool lqux(large_t x) { return x & LBIT; }
+
+
+typedef unsigned short __attribute__((bitwise)) small_t;
+#define	SBIT	((__attribute__((force)) small_t) 1)
+
+_Bool sfoo(small_t x) { return x; }
+_Bool sbar(small_t x) { return ~x; }
+_Bool sbaz(small_t x) { return !x; }
+_Bool squx(small_t x) { return x & SBIT; }
+
+/*
+ * check-name: bool-cast-restricted.c
+ * check-command: sparse -Wno-decl $file
+ *
+ * check-error-start
+bool-cast-restricted.c:14:32: warning: restricted small_t degrades to integer
+ * check-error-end
+ */
-- 
2.12.2

Re: [PATCH] avoid useless warning for 'bool <- restricted type' conversion

[Christopher Li]

On Thu, May 11, 2017 at 9:21 AM, Luc Van Oostenryck
<luc.vanoostenryck@gmail.com> wrote:
> Conversion to bool is special in C since this conversion
> is essentially the result of the comparison with zero.
> As such, some operations which are normally unsafe to
> do with restricted types, like casting to an unrestricted
> type, are in fact safe to do when converting to bool
> and issuing a warning in those case is useless, confusing
> and causes people to add useless casts in the code in
> order to shut up the warning.
>
> Fix this by catching such 'bool <- restricted type' conversion
> and avoid such warnings.

The change seems fine. However, have you run the test-suite
with this change? It seems cause some test do not pass.
I assume it is cause by different error message it will output.

Chris

Re: [PATCH] avoid useless warning for 'bool <- restricted type' conversion

[Luc Van Oostenryck]

On Thu, May 11, 2017 at 9:41 PM, Christopher Li <sparse@chrisli.org> wrote:
> On Thu, May 11, 2017 at 9:21 AM, Luc Van Oostenryck
> <luc.vanoostenryck@gmail.com> wrote:
>> Conversion to bool is special in C since this conversion
>> is essentially the result of the comparison with zero.
>> As such, some operations which are normally unsafe to
>> do with restricted types, like casting to an unrestricted
>> type, are in fact safe to do when converting to bool
>> and issuing a warning in those case is useless, confusing
>> and causes people to add useless casts in the code in
>> order to shut up the warning.
>>
>> Fix this by catching such 'bool <- restricted type' conversion
>> and avoid such warnings.
>
> The change seems fine. However, have you run the test-suite
> with this change? It seems cause some test do not pass.
> I assume it is cause by different error message it will output.

Oh my, sorry.
I *have* run the test-suite but then I've forgotten to commit
the change needed for two test cases.

Thanks for noticing this.

-- Luc

[PATCH] avoid useless warning for 'bool <- restricted type' conversion

[Luc Van Oostenryck]

Conversion to bool is special in C since this conversion
is essentially the result of the comparison with zero.
As such, some operations which are normally unsafe to
do with restricted types, like casting to an unrestricted
type, are in fact safe to do when converting to bool
and issuing a warning in those case is useless, confusing
and causes people to add useless casts in the code in
order to shut up the warning.

Fix this by catching such 'bool <- restricted type' conversion
and avoid such warnings.

CC: Al Viro <viro@zeniv.linux.org.uk>
Originally-by: Al Viro <viro@zeniv.linux.org.uk>
Signed-off-by: Luc Van Oostenryck <luc.vanoostenryck@gmail.com>
---
 evaluate.c                        |  6 ++++++
 validation/bool-cast-bad.c        |  3 ---
 validation/bool-cast-implicit.c   |  3 ---
 validation/bool-cast-restricted.c | 25 +++++++++++++++++++++++++
 4 files changed, 31 insertions(+), 6 deletions(-)
 create mode 100644 validation/bool-cast-restricted.c

diff --git a/evaluate.c b/evaluate.c
index 976857915..3dc26fc09 100644
--- a/evaluate.c
+++ b/evaluate.c
@@ -1345,6 +1345,12 @@ static int check_assignment_types(struct symbol *target, struct expression **rp,
 				return 1;
 		} else if (!(sclass & TYPE_RESTRICT))
 			goto Cast;
+                if (t == &bool_ctype) {
+                        if (is_fouled_type(s))
+                                warning((*rp)->pos, "%s degrades to integer",
+                                        show_typename(s->ctype.base_type));
+                        goto Cast;
+                }
 		*typediff = "different base types";
 		return 0;
 	}
diff --git a/validation/bool-cast-bad.c b/validation/bool-cast-bad.c
index b7e7c058d..a0b091e1c 100644
--- a/validation/bool-cast-bad.c
+++ b/validation/bool-cast-bad.c
@@ -15,9 +15,6 @@ static _Bool fstse(struct s a) { return (_Bool)a; }
  * check-command: sparse $file
  *
  * check-error-start
-bool-cast-bad.c:8:41: warning: incorrect type in return expression (different base types)
-bool-cast-bad.c:8:41:    expected bool
-bool-cast-bad.c:8:41:    got restricted le16 [usertype] a
 bool-cast-bad.c:9:42: warning: cast from restricted le16
 bool-cast-bad.c:10:41: warning: incorrect type in return expression (different base types)
 bool-cast-bad.c:10:41:    expected bool
diff --git a/validation/bool-cast-implicit.c b/validation/bool-cast-implicit.c
index ee8b705b9..9d89443b1 100644
--- a/validation/bool-cast-implicit.c
+++ b/validation/bool-cast-implicit.c
@@ -21,8 +21,5 @@ static _Bool fres(le16 a) { return a; }
  * check-output-excludes: cast\\.
  *
  * check-error-start
-bool-cast-implicit.c:15:36: warning: incorrect type in return expression (different base types)
-bool-cast-implicit.c:15:36:    expected bool
-bool-cast-implicit.c:15:36:    got restricted le16 [usertype] a
  * check-error-end
  */
diff --git a/validation/bool-cast-restricted.c b/validation/bool-cast-restricted.c
new file mode 100644
index 000000000..f6776b050
--- /dev/null
+++ b/validation/bool-cast-restricted.c
@@ -0,0 +1,25 @@
+typedef unsigned   int __attribute__((bitwise)) large_t;
+#define	LBIT	((__attribute__((force)) large_t) 1)
+
+_Bool lfoo(large_t x) { return x; }
+_Bool lbar(large_t x) { return ~x; }
+_Bool lbaz(large_t x) { return !x; }
+_Bool lqux(large_t x) { return x & LBIT; }
+
+
+typedef unsigned short __attribute__((bitwise)) small_t;
+#define	SBIT	((__attribute__((force)) small_t) 1)
+
+_Bool sfoo(small_t x) { return x; }
+_Bool sbar(small_t x) { return ~x; }
+_Bool sbaz(small_t x) { return !x; }
+_Bool squx(small_t x) { return x & SBIT; }
+
+/*
+ * check-name: bool-cast-restricted.c
+ * check-command: sparse -Wno-decl $file
+ *
+ * check-error-start
+bool-cast-restricted.c:14:32: warning: restricted small_t degrades to integer
+ * check-error-end
+ */
-- 
2.12.2

Re: [PATCH] FS: Fixing return type of unsigned_offsets

[Luc Van Oostenryck]

On Thu, May 11, 2017 at 8:07 AM, Al Viro <viro@zeniv.linux.org.uk> wrote:
> FVO"needed" equal to "needed to make sparse STFU"?  If anything, that's
> sparse being wrong - evaluate.c:check_assignment_type() should do
>                 if (t == &ctype_bool) {
>                         if (is_fouled_type(s))
>                                 warning((*rp)->pos, "%s degrades to integer",
>                                         show_typename(s->ctype.base_type));
>                         goto Cast;
>                 }
> right after
>                 } else if (!(sclass & TYPE_RESTRICT))
>                         goto Cast;

What about an explicit cast of restricted types to bool?
I think we would want the equivalent of this patch for those too.

-- Luc