Re: [kbuild-all] [kvm:queue 27/38] arch/x86/kvm/hyperv.c:186:41: sparse: incorrect type in argument 2 (different modifiers)

Re: [kbuild-all] [kvm:queue 27/38] arch/x86/kvm/hyperv.c:186:41: sparse: incorrect type in argument 2 (different modifiers)

[Fengguang Wu]

[CC sparse people]

On Fri, Sep 18, 2015 at 04:41:56PM +0200, Paolo Bonzini wrote:
> 
> 
> On 18/09/2015 16:40, Roman Kagan wrote:
> > typedef unsigned long __nocast cputime_t;
> > 
> > extern void task_cputime_adjusted(cputime_t *);
> > extern void current_task_runtime_100ns(void);
> > 
> > void current_task_runtime_100ns(void)
> > {
> >         cputime_t utime;
> > 
> >         task_cputime_adjusted(&utime);
> > }
> > %%% gcc -c x.c -Wall -Werror -O2; echo $?
> > 0
> > %%% sparse x.c
> > x.c:16:32: warning: incorrect type in argument 1 (different modifiers)
> > x.c:16:32:    expected unsigned long [nocast] [usertype] *<noident>
> > x.c:16:32:    got unsigned long *<noident>
> > x.c:16:32: warning: implicit cast to nocast type
> > 
> > Looks like a sparse bug to me.
> 
> Indeed...
> 
> Paolo
> _______________________________________________
> kbuild-all mailing list
> kbuild-all@lists.01.org
> https://lists.01.org/mailman/listinfo/kbuild-all

Re: [kbuild-all] [kvm:queue 27/38] arch/x86/kvm/hyperv.c:186:41: sparse: incorrect type in argument 2 (different modifiers)

[Luc Van Oostenryck]

On Fri, Sep 18, 2015 at 11:06:44PM +0800, Fengguang Wu wrote:
> [CC sparse people]
> 
> On Fri, Sep 18, 2015 at 04:41:56PM +0200, Paolo Bonzini wrote:
> > 
> > 
> > On 18/09/2015 16:40, Roman Kagan wrote:
> > > typedef unsigned long __nocast cputime_t;
> > > 
> > > extern void task_cputime_adjusted(cputime_t *);
> > > extern void current_task_runtime_100ns(void);
> > > 
> > > void current_task_runtime_100ns(void)
> > > {
> > >         cputime_t utime;
> > > 
> > >         task_cputime_adjusted(&utime);
> > > }
> > > %%% gcc -c x.c -Wall -Werror -O2; echo $?
> > > 0
> > > %%% sparse x.c
> > > x.c:16:32: warning: incorrect type in argument 1 (different modifiers)
> > > x.c:16:32:    expected unsigned long [nocast] [usertype] *<noident>
> > > x.c:16:32:    got unsigned long *<noident>
> > > x.c:16:32: warning: implicit cast to nocast type
> > > 
> > > Looks like a sparse bug to me.
> > 
> > Indeed...
> > 
> > Paolo

The problem is that the intent and semantic of 'nocast' is not clear at all.
There is an explanation about 'nocast' vs. 'bitwise' here:
	https://git.kernel.org/cgit/devel/sparse/sparse.git/tree/Documentation/sparse.txt
but it doesn't give much info about the exact wanted behaviour.

Since for the kernel 'nocast' is only used for cputime_t & cputime64_t,
I think it should be clarified and fixed if needed.
A patch proposal is following.

Regards,
Luc

[PATCH] Do not drop 'nocast' modifier when taking the address.

[Luc Van Oostenryck]

With the following code:
	typedef unsigned long __nocast cputime_t;

	void task_cputime_adjusted(cputime_t *);

	void current_task_runtime_100ns(void)
	{
	        cputime_t utime;

	        task_cputime_adjusted(&utime);
	}

sparse emits the following message:
	x.c:16:32: warning: incorrect type in argument 1 (different modifiers)
	x.c:16:32:    expected unsigned long [nocast] [usertype] *<noident>
	x.c:16:32:    got unsigned long *<noident>
	x.c:16:32: warning: implicit cast to nocast type

In other words, when taking the address of 'utime', sparse drops the 'nocast'
modifier and then complains that task_cputime_adjusted() is not given a
'nocast' pointer as expected ...

This feels wrong to me.

The proposed fix is to simply not dropping the 'nocast' modifier when
taking the address, like done for a normal type qualifier.
This gives very reasonable behaviour for all the test cases I could
think of:
	- taking the address or dereferencing doesn't drop the nocast
	- arithmetic operations on nocast give a nocast result.
	- implicit to/from cast is OK only if the base type are the same
	- implicit to/from pointer cast is OK only if the base type are the same

This still gives a "leaky" semantic: the nocast modifier can be lost via
an implicit cast to a non-qualified value.

Explicit cast to or from nocast values doesn't give any warning, maybe
it's OK, maybe it's not but it's orthogonal to the current issue.

Is this the wanted semantic for the nocast modifier?

Reported-by: Roman Kagan <rkagan@virtuozzo.com>
Signed-off-by: Luc Van Oostenryck <luc.vanoostenryck@gmail.com>
---
 symbol.h            |   2 +-
 validation/nocast.c | 197 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 198 insertions(+), 1 deletion(-)
 create mode 100644 validation/nocast.c

diff --git a/symbol.h b/symbol.h
index ccb5dcb9..9b3f1604 100644
--- a/symbol.h
+++ b/symbol.h
@@ -247,7 +247,7 @@ struct symbol {
 #define MOD_SIZE	(MOD_CHAR | MOD_SHORT | MOD_LONG_ALL)
 #define MOD_IGNORE (MOD_TOPLEVEL | MOD_STORAGE | MOD_ADDRESSABLE |	\
 	MOD_ASSIGNED | MOD_USERTYPE | MOD_ACCESSED | MOD_EXPLICITLY_SIGNED)
-#define MOD_PTRINHERIT (MOD_VOLATILE | MOD_CONST | MOD_NODEREF | MOD_STORAGE | MOD_NORETURN)
+#define MOD_PTRINHERIT (MOD_VOLATILE | MOD_CONST | MOD_NODEREF | MOD_STORAGE | MOD_NORETURN | MOD_NOCAST)
 
 
 /* Current parsing/evaluation function */
diff --git a/validation/nocast.c b/validation/nocast.c
new file mode 100644
index 00000000..c28676a3
--- /dev/null
+++ b/validation/nocast.c
@@ -0,0 +1,197 @@
+#define	__nocast	__attribute__((nocast))
+typedef unsigned long __nocast ulong_nc_t;
+
+extern void use_val(ulong_nc_t);
+extern void use_ptr(ulong_nc_t *);
+
+/* use address */
+static void good_use_address(void)
+{
+	ulong_nc_t t;
+
+	use_ptr(&t);
+}
+
+static ulong_nc_t *good_ret_address(void)
+{
+	static ulong_nc_t t;
+
+	return &t;
+}
+
+static ulong_nc_t good_deref(ulong_nc_t *t)
+{
+	return *t;
+}
+
+/* assign value */
+static ulong_nc_t t;
+static ulong_nc_t good_assign_self = t;
+static unsigned long good_assign_sametype = t;
+
+/* assign pointer */
+static ulong_nc_t *good_ptr = &t;
+static ulong_nc_t *bad_ptr_to = 1UL;
+static unsigned long *bad_ptr_from = &t;
+
+/* arithmetic operation */
+static ulong_nc_t good_arith(ulong_nc_t t, unsigned int n)
+{
+	return t + n;
+}
+
+/* implicit cast to other types */
+static unsigned long good_ret_samecast(ulong_nc_t t)
+{
+	return t;
+}
+static unsigned long long bad_ret_biggercast(ulong_nc_t t)
+{
+	return t;
+}
+static long bad_ret_signcast(ulong_nc_t t)
+{
+	return t;
+}
+static short bad_ret_smallercast(ulong_nc_t t)
+{
+	return t;
+}
+
+static void assign_val(ulong_nc_t t)
+{
+	ulong_nc_t good_c = t;
+	unsigned long good_ul = t;
+	unsigned long long bad_ull = t;
+	long bad_l = t;
+	short bad_i = t;
+}
+
+static void assign_via_ptr(ulong_nc_t *t)
+{
+	ulong_nc_t good_c = *t;
+	unsigned long good_ul = *t;
+	unsigned long long bad_ull = *t;
+	long bad_l = *t;
+	short bad_i = *t;
+}
+
+static void assign_ptr(ulong_nc_t *t)
+{
+	ulong_nc_t *good_same_type = t;
+	unsigned long *bad_mod = t;
+	unsigned long long __nocast *bad_size = t;
+	short __nocast *bad_i = t;
+	long __nocast *bad_l = t;
+}
+
+/* implicit cast to nocast */
+static void implicit_assign_to(void)
+{
+	ulong_nc_t t;
+	unsigned long ul = 1;
+	unsigned short us = 1;
+	unsigned long long ull = 1;
+	long l = 1;
+
+	t = ul;		/* implicit to nocast from same type: OK? */
+	t = us;
+	t = ull;
+	t = l;
+}
+
+static void bad_implicit_arg_to(void)
+{
+	unsigned long ul = 1;
+	unsigned short us = 1;
+	unsigned long long ull = 1;
+	long l = 1;
+
+	use_val(ul);	/* implicit to nocast from same type: OK? */
+	use_val(us);
+	use_val(ull);
+	use_val(l);
+}
+
+/* implicit cast from nocast */
+static unsigned long good_implicit_ret_ul(ulong_nc_t t)
+{
+	return t;	/* implicit to nocast from same type: OK? */
+}
+
+static unsigned short bad_implicit_ret_us(ulong_nc_t t)
+{
+	return t;
+}
+
+static unsigned long long bad_implicit_ret_ull(ulong_nc_t t)
+{
+	return t;
+}
+
+static long bad_implicit_ret_l(ulong_nc_t t)
+{
+	return t;
+}
+
+/* FIXME: explicit cast: should we complain? */
+static ulong_nc_t good_samecast(ulong_nc_t v)
+{
+	return (ulong_nc_t) v;
+}
+
+static ulong_nc_t bad_tocast(unsigned long v)
+{
+	return (ulong_nc_t) v;
+}
+
+static unsigned long bad_fromcast(ulong_nc_t v)
+{
+	return (unsigned long) v;
+}
+
+/*
+ * check-name: nocast.c
+ *
+ * check-error-start
+nocast.c:34:33: warning: incorrect type in initializer (different base types)
+nocast.c:34:33:    expected unsigned long [nocast] [usertype] *static [toplevel] bad_ptr_to
+nocast.c:34:33:    got unsigned long
+nocast.c:34:33: warning: implicit cast to nocast type
+nocast.c:35:39: warning: incorrect type in initializer (different modifiers)
+nocast.c:35:39:    expected unsigned long *static [toplevel] bad_ptr_from
+nocast.c:35:39:    got unsigned long static [nocast] [toplevel] *<noident>
+nocast.c:35:39: warning: implicit cast from nocast type
+nocast.c:50:16: warning: implicit cast from nocast type
+nocast.c:54:16: warning: implicit cast from nocast type
+nocast.c:58:16: warning: implicit cast from nocast type
+nocast.c:65:38: warning: implicit cast from nocast type
+nocast.c:66:22: warning: implicit cast from nocast type
+nocast.c:67:23: warning: implicit cast from nocast type
+nocast.c:74:38: warning: implicit cast from nocast type
+nocast.c:75:22: warning: implicit cast from nocast type
+nocast.c:76:23: warning: implicit cast from nocast type
+nocast.c:82:34: warning: incorrect type in initializer (different modifiers)
+nocast.c:82:34:    expected unsigned long *bad_mod
+nocast.c:82:34:    got unsigned long [nocast] [usertype] *t
+nocast.c:82:34: warning: implicit cast from nocast type
+nocast.c:83:49: warning: incorrect type in initializer (different type sizes)
+nocast.c:83:49:    expected unsigned long long [nocast] *bad_size
+nocast.c:83:49:    got unsigned long [nocast] [usertype] *t
+nocast.c:83:49: warning: implicit cast to/from nocast type
+nocast.c:84:33: warning: incorrect type in initializer (different type sizes)
+nocast.c:84:33:    expected short [nocast] *bad_i
+nocast.c:84:33:    got unsigned long [nocast] [usertype] *t
+nocast.c:84:33: warning: implicit cast to/from nocast type
+nocast.c:85:32: warning: implicit cast to/from nocast type
+nocast.c:98:13: warning: implicit cast to nocast type
+nocast.c:99:13: warning: implicit cast to nocast type
+nocast.c:100:13: warning: implicit cast to nocast type
+nocast.c:111:17: warning: implicit cast to nocast type
+nocast.c:112:17: warning: implicit cast to nocast type
+nocast.c:113:17: warning: implicit cast to nocast type
+nocast.c:124:16: warning: implicit cast from nocast type
+nocast.c:129:16: warning: implicit cast from nocast type
+nocast.c:134:16: warning: implicit cast from nocast type
+ * check-error-end
+ */
-- 
2.6.4

Re: [PATCH] Do not drop 'nocast' modifier when taking the address.

[Christopher Li]

On Wed, Jan 6, 2016 at 12:25 AM, Luc Van Oostenryck
<luc.vanoostenryck@gmail.com> wrote:
> With the following code:
>         typedef unsigned long __nocast cputime_t;
>
>         void task_cputime_adjusted(cputime_t *);
>
>         void current_task_runtime_100ns(void)
>         {
>                 cputime_t utime;
>
>                 task_cputime_adjusted(&utime);
>         }
>
> sparse emits the following message:
>         x.c:16:32: warning: incorrect type in argument 1 (different modifiers)
>         x.c:16:32:    expected unsigned long [nocast] [usertype] *<noident>
>         x.c:16:32:    got unsigned long *<noident>
>         x.c:16:32: warning: implicit cast to nocast type
>
> In other words, when taking the address of 'utime', sparse drops the 'nocast'
> modifier and then complains that task_cputime_adjusted() is not given a
> 'nocast' pointer as expected ...

I think there is a bug some where else. In the above example,
"cputime_t *" and "&utime" should have the same type regardless
pointer inherent the nocast attribute or not. I haven't fully understand
where the nocast attribute get dropped.

Chris

Re: [PATCH] Do not drop 'nocast' modifier when taking the address.

[Luc Van Oostenryck]

On Wed, Feb 03, 2016 at 04:25:44AM +0800, Christopher Li wrote:
> On Wed, Jan 6, 2016 at 12:25 AM, Luc Van Oostenryck
> <luc.vanoostenryck@gmail.com> wrote:
> > With the following code:
> >         typedef unsigned long __nocast cputime_t;
> >
> >         void task_cputime_adjusted(cputime_t *);
> >
> >         void current_task_runtime_100ns(void)
> >         {
> >                 cputime_t utime;
> >
> >                 task_cputime_adjusted(&utime);
> >         }
> >
> > sparse emits the following message:
> >         x.c:16:32: warning: incorrect type in argument 1 (different modifiers)
> >         x.c:16:32:    expected unsigned long [nocast] [usertype] *<noident>
> >         x.c:16:32:    got unsigned long *<noident>
> >         x.c:16:32: warning: implicit cast to nocast type
> >
> > In other words, when taking the address of 'utime', sparse drops the 'nocast'
> > modifier and then complains that task_cputime_adjusted() is not given a
> > 'nocast' pointer as expected ...
> 
> I think there is a bug some where else. In the above example,
> "cputime_t *" and "&utime" should have the same type regardless
> pointer inherent the nocast attribute or not. I haven't fully understand
> where the nocast attribute get dropped.

The nocast mod is dropped and lost in the function create_pointer().
In the example above, "cputime_t *" has type :
	unsigned long [nocast] [usertype] *
while &utime is just:
	unsigned long *

So, for sparse and its extended notion of type, the type we get when
taking the address of a [variable of some] type X is not the same as
directly using a pointer to the type X.
Which is very fine, just that MOD_NOCAST is dropped while the example
shows that it should not.
OTOH, MOD_STORAGE is kept but I think should be dropped; but that's
another story.


Regards,
Luc

Re: [PATCH] Do not drop 'nocast' modifier when taking the address.

[Christopher Li]

On Wed, Feb 3, 2016 at 11:43 AM, Luc Van Oostenryck
<luc.vanoostenryck@gmail.com> wrote:
pped.
>
> The nocast mod is dropped and lost in the function create_pointer().
> In the example above, "cputime_t *" has type :
>         unsigned long [nocast] [usertype] *
> while &utime is just:
>         unsigned long *

That is my point. Why does "&utime" get drop but "cputime_t *" does not?
They both are pointer of a base type. They both create pointers.

It seems to me the bug is sparse not treating this two case consistently.

> So, for sparse and its extended notion of type, the type we get when
> taking the address of a [variable of some] type X is not the same as
> directly using a pointer to the type X.

In C language type system,  these two should be the same type. It is a
bug in sparse if they are not. I would rather get that bug fixed.

> Which is very fine, just that MOD_NOCAST is dropped while the example
> shows that it should not.

I think that is a separate issue weather MOD_NOCAST should be inherent
from pointer base type. Same with MOD_STORAGE.

Chris

Re: [PATCH] Do not drop 'nocast' modifier when taking the address.

[Luc Van Oostenryck]

On Wed, Feb 03, 2016 at 12:09:16PM +0800, Christopher Li wrote:
> On Wed, Feb 3, 2016 at 11:43 AM, Luc Van Oostenryck
> <luc.vanoostenryck@gmail.com> wrote:
> pped.
> >
> > The nocast mod is dropped and lost in the function create_pointer().
> > In the example above, "cputime_t *" has type :
> >         unsigned long [nocast] [usertype] *
> > while &utime is just:
> >         unsigned long *
> 
> That is my point. Why does "&utime" get drop but "cputime_t *" does not?
> They both are pointer of a base type. They both create pointers.

Well with "cputime_t *" you got the pointer directly, by its own declaration;
with "&utime" you really _create_ one with the "&"/"addressof" operator.

The function "create_pointer()" is only called when evaluating an expression
using the addressof operator or when when an array of a function is degenerated
into a pointer.

> It seems to me the bug is sparse not treating this two case consistently.
> 
> > So, for sparse and its extended notion of type, the type we get when
> > taking the address of a [variable of some] type X is not the same as
> > directly using a pointer to the type X.
> 
> In C language type system,  these two should be the same type. It is a
> bug in sparse if they are not. I would rather get that bug fixed.

They _have_ the same type if we limit ourselves to the pure C type system,
but they differ once we look also at the sparse & gcc extension to the
type system, like the nocast attribute here.
 
Now, whether they should be the same or not is a question of defining the
semantic of the addressof operator on sparse's type extension.

> > Which is very fine, just that MOD_NOCAST is dropped while the example
> > shows that it should not.
> 
> I think that is a separate issue weather MOD_NOCAST should be inherent
> from pointer base type.

I'm not sure to understand you here.

> Same with MOD_STORAGE.
> 
> Chris

Luc

Re: [PATCH] Do not drop 'nocast' modifier when taking the address.

[Christopher Li]

On Wed, Feb 3, 2016 at 5:15 PM, Luc Van Oostenryck
<luc.vanoostenryck@gmail.com> wrote:
> Well with "cputime_t *" you got the pointer directly, by its own declaration;
> with "&utime" you really _create_ one with the "&"/"addressof" operator.
>
> The function "create_pointer()" is only called when evaluating an expression
> using the addressof operator or when when an array of a function is degenerated
> into a pointer.

Yes, I take a closer look. My previous understanding of how the MOD_PTRINHERIT
works was wrong.

Now I see the pointer is created differently. In the parsing stage, it is
created by the "pointer" function. In the evaluation stage it is created
by the "create_pointer" function. The "pointer" function has no treatment for
MOD_PTRINHERIT at all.

Ideally it would be nice to unify the pointer create some how. It is
easier to have
inconsistent modifier other than MOD_NOCAST. As a simply fix, your patch is
fine. Applied.

>
> They _have_ the same type if we limit ourselves to the pure C type system,
> but they differ once we look also at the sparse & gcc extension to the
> type system, like the nocast attribute here.
>
> Now, whether they should be the same or not is a question of defining the
> semantic of the addressof operator on sparse's type extension.

I think sparse should treat them with same semantic. It does not make sense
if they are not the same.

Thanks.

Chris