Skip to contentSkip to conversation

arch/x86/kvm/hyperv.c:186:41: sparse: incorrect type in argument 2 (different modifiers)

PATCH In review kbuild test robot 6 participants 14 messages Sep 18, 2015 – Feb 22, 2016

Subsystems (2)
KVM Sparse checker
lore
Share & tools
lore

kbuild test robot

author build bot
#0 lore raw
tree:   https://git.kernel.org/pub/scm/virt/kvm/kvm.git queue
head:   ed393e4134de0dd02d8ba98ca8ce3ae65d1eb567
commit: 46f4c309534b10ca1026273abe38955d3cff4023 [27/38] kvm/x86: Hyper-V HV_X64_MSR_VP_RUNTIME support
reproduce:
  # apt-get install sparse
  git checkout 46f4c309534b10ca1026273abe38955d3cff4023
  make ARCH=x86_64 allmodconfig
  make C=1 CF=-D__CHECK_ENDIAN__

sparse warnings: (new ones prefixed by >>)

>> arch/x86/kvm/hyperv.c:186:41: sparse: incorrect type in argument 2 (different modifiers)
   arch/x86/kvm/hyperv.c:186:41:    expected unsigned long [nocast] [usertype] *ut
   arch/x86/kvm/hyperv.c:186:41:    got unsigned long *<noident>
>> arch/x86/kvm/hyperv.c:186:41: sparse: implicit cast to nocast type
>> arch/x86/kvm/hyperv.c:186:49: sparse: incorrect type in argument 3 (different modifiers)

arch/x86/kvm/hyperv.c:186:49: expected unsigned long [nocast] [usertype] *st
arch/x86/kvm/hyperv.c:186:49: got unsigned long *<noident>
arch/x86/kvm/hyperv.c:186:49: sparse: implicit cast to nocast type

vim +186 arch/x86/kvm/hyperv.c

170 kvm_make_request(KVM_REQ_HV_RESET, vcpu);
171 }
172 break;
173 default:
174 vcpu_unimpl(vcpu, "Hyper-V uhandled wrmsr: 0x%x data 0x%llx\n",
175 msr, data);
176 return 1;
177 }
178 return 0;
179 }
180
181 /* Calculate cpu time spent by current task in 100ns units */
182 static u64 current_task_runtime_100ns(void)
183 {
184 cputime_t utime, stime;
185

 > 186		task_cputime_adjusted(current, &utime, &stime);

187 return div_u64(cputime_to_nsecs(utime + stime), 100);
188 }
189
190 static int kvm_hv_set_msr(struct kvm_vcpu *vcpu, u32 msr, u64 data, bool host)
191 {
192 struct kvm_vcpu_hv *hv = &vcpu->arch.hyperv;
193
194 switch (msr) {

---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation

#1 lore raw
Quoted 49 lines · kbuild test robot
tree:   https://git.kernel.org/pub/scm/virt/kvm/kvm.git queue
head:   ed393e4134de0dd02d8ba98ca8ce3ae65d1eb567
commit: 46f4c309534b10ca1026273abe38955d3cff4023 [27/38] kvm/x86: Hyper-V HV_X64_MSR_VP_RUNTIME support
reproduce:
   # apt-get install sparse
   git checkout 46f4c309534b10ca1026273abe38955d3cff4023
   make ARCH=x86_64 allmodconfig
   make C=1 CF=-D__CHECK_ENDIAN__

sparse warnings: (new ones prefixed by >>)

Quoted 1 line
Quoted 1 line

arch/x86/kvm/hyperv.c:186:41: sparse: incorrect type in argument 2 (different modifiers)

arch/x86/kvm/hyperv.c:186:41: expected unsigned long [nocast] [usertype] *ut
arch/x86/kvm/hyperv.c:186:41: got unsigned long *<noident>

Quoted 2 lines
Quoted 2 lines

arch/x86/kvm/hyperv.c:186:41: sparse: implicit cast to nocast type arch/x86/kvm/hyperv.c:186:49: sparse: incorrect type in argument 3 (different modifiers)

arch/x86/kvm/hyperv.c:186:49: expected unsigned long [nocast] [usertype] *st
arch/x86/kvm/hyperv.c:186:49: got unsigned long *<noident>
arch/x86/kvm/hyperv.c:186:49: sparse: implicit cast to nocast type

vim +186 arch/x86/kvm/hyperv.c

    170				kvm_make_request(KVM_REQ_HV_RESET, vcpu);
    171			}
    172			break;
    173		default:
    174			vcpu_unimpl(vcpu, "Hyper-V uhandled wrmsr: 0x%x data 0x%llx\n",
    175				    msr, data);
    176			return 1;
    177		}
    178		return 0;
    179	}
    180	
    181	/* Calculate cpu time spent by current task in 100ns units */
    182	static u64 current_task_runtime_100ns(void)
    183	{
    184		cputime_t utime, stime;
    185
Quoted 1 line

186 task_cputime_adjusted(current, &utime, &stime);

    187		return div_u64(cputime_to_nsecs(utime + stime), 100);
    188	}
    189	
    190	static int kvm_hv_set_msr(struct kvm_vcpu *vcpu, u32 msr, u64 data, bool host)
    191	{
    192		struct kvm_vcpu_hv *hv = &vcpu->arch.hyperv;
    193	
    194		switch (msr) {
Quoted 3 lines

---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation

can not get an idea what is this warning about...
For me it looks pretty lame.

Den

maintainer
#2 lore raw
Quoted 12 lines · Denis V. Lunev
Quoted 12 lines
    185
Quoted 1 line

186 task_cputime_adjusted(current, &utime, &stime);

    187        return div_u64(cputime_to_nsecs(utime + stime), 100);
    188    }
    189   
    190    static int kvm_hv_set_msr(struct kvm_vcpu *vcpu, u32 msr,
u64 data, bool host)
    191    {
    192        struct kvm_vcpu_hv *hv = &vcpu->arch.hyperv;
    193   
    194        switch (msr) {
Quoted 7 lines
Quoted 5 lines

---
0-DAY kernel test infrastructure Open Source Technology
Center
https://lists.01.org/pipermail/kbuild-all Intel
Corporation

can not get an idea what is this warning about...
For me it looks pretty lame.

I think it wants you to do

- return div_u64(cputime_to_nsecs(utime + stime), 100);
+ return div_u64(cputime_to_nsecs(utime) +
+ cputime_to_nsecs(stime), 100);

Paolo

#3 lore raw
Quoted 27 lines · Paolo Bonzini
Quoted 12 lines · Denis V. Lunev
Quoted 12 lines
     185
Quoted 1 line

186 task_cputime_adjusted(current, &utime, &stime);

     187        return div_u64(cputime_to_nsecs(utime + stime), 100);
     188    }
     189
     190    static int kvm_hv_set_msr(struct kvm_vcpu *vcpu, u32 msr,
u64 data, bool host)
     191    {
     192        struct kvm_vcpu_hv *hv = &vcpu->arch.hyperv;
     193
     194        switch (msr) {
Quoted 7 lines
Quoted 5 lines

---
0-DAY kernel test infrastructure Open Source Technology
Center
https://lists.01.org/pipermail/kbuild-all Intel
Corporation

can not get an idea what is this warning about...
For me it looks pretty lame.

I think it wants you to do

(quoted diff) +2 -1
-	return div_u64(cputime_to_nsecs(utime + stime), 100);
+	return div_u64(cputime_to_nsecs(utime) +
+		       cputime_to_nsecs(stime), 100);
Quoted 1 line

Paolo

ok, I'll check but warning points here

Quoted 1 line

186 task_cputime_adjusted(current, &utime, &stime);

and says about parameters 2 and 3. There is no parameter 3 in div_u64 call :)

#4 lore raw
In reply to Paolo Bonzini
Quoted 25 lines · Paolo Bonzini
Quoted 12 lines · Denis V. Lunev
Quoted 12 lines
    185
Quoted 1 line

186 task_cputime_adjusted(current, &utime, &stime);

    187        return div_u64(cputime_to_nsecs(utime + stime), 100);
    188    }
    189   
    190    static int kvm_hv_set_msr(struct kvm_vcpu *vcpu, u32 msr,
u64 data, bool host)
    191    {
    192        struct kvm_vcpu_hv *hv = &vcpu->arch.hyperv;
    193   
    194        switch (msr) {
Quoted 7 lines
Quoted 5 lines

---
0-DAY kernel test infrastructure Open Source Technology
Center
https://lists.01.org/pipermail/kbuild-all Intel
Corporation

can not get an idea what is this warning about...
For me it looks pretty lame.

I think it wants you to do

Review

Quoted 3 lines
(quoted diff) +2 -1
-	return div_u64(cputime_to_nsecs(utime + stime), 100);
+	return div_u64(cputime_to_nsecs(utime) +
+		       cputime_to_nsecs(stime), 100);

The warning is pretty specific about the point where it triggered.

I have reduced it to the following standalone reproducer:

%%% cat x.c
#ifdef __CHECKER__

# define __nocast       __attribute__((nocast))
#else
# define __nocast
#endif

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.

Roman.

maintainer
#5 lore raw
Quoted 19 lines · Roman Kagan

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

Quoted 1 line

Looks like a sparse bug to me.

Indeed...

Paolo

author
#6 lore raw

[CC sparse people]

Quoted 31 lines · Paolo Bonzini
Quoted 19 lines · Roman Kagan

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

Quoted 1 line

Looks like a sparse bug to me.

Indeed...

Paolo
_______________________________________________
kbuild-all mailing list
[email protected]
https://lists.01.org/mailman/listinfo/kbuild-all

#7 lore raw
Quoted 30 lines · Fengguang Wu

[CC sparse people]

Quoted 27 lines · Paolo Bonzini
Quoted 19 lines · Roman Kagan

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

Quoted 1 line

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

#8 lore raw

PATCH 1/1 Do not drop 'nocast' modifier when taking the address.

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 <[email protected]>
Signed-off-by Luc Van Oostenryck <[email protected]>
---
 symbol.h            |   2 +-
 validation/nocast.c | 197 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 198 insertions(+), 1 deletion(-)
 create mode 100644 validation/nocast.c
symbol.h +1 -1
index ccb5dcb9..9b3f1604 100644
@@ -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 */
validation/nocast.c new file +197 -0

197 lines not on this page · raw message

Signature · 1 line
2.6.4
maintainer
#9 lore raw

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

Quoted 18 lines · 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

Quoted 3 lines

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

#10 lore raw

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

Quoted 20 lines · Christopher Li
Quoted 18 lines · 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

Quoted 8 lines
Quoted 3 lines

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

maintainer
#11 lore raw

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

On Wed, Feb 3, 2016 at 11:43 AM, Luc Van Oostenryck <[email protected]> wrote:
pped.

Quoted 6 lines

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.

Quoted 3 lines

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.

Quoted 2 lines

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

#12 lore raw

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

Quoted 4 lines · Christopher Li

On Wed, Feb 3, 2016 at 11:43 AM, Luc Van Oostenryck <[email protected]> wrote:
pped.

Quoted 1 line
Quoted 8 lines
Quoted 5 lines

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.

Quoted 8 lines

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

Quoted 3 lines

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.

Quoted 5 lines
Quoted 2 lines

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.

Quoted 3 lines

Same with MOD_STORAGE.

Chris

Luc

maintainer
#13 lore raw

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

Quoted 6 lines · Luc Van Oostenryck

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.

Quoted 7 lines

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