tree: https://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git locking/core
head: ac605bee0bfab40fd5d11964705e907d2d5a32de
commit: 8bf705d130396e69c04cd8e6e010244ad2ce71f4 [9/11] locking/atomic/x86: Switch atomic.h to use atomic-instrumented.h
reproduce:
# apt-get install sparse
git checkout 8bf705d130396e69c04cd8e6e010244ad2ce71f4
make ARCH=x86_64 allmodconfig
make C=1 CF=-D__CHECK_ENDIAN__
sparse warnings: (new ones prefixed by >>)
kernel/locking/qspinlock.c:418:22: sparse: incorrect type in assignment (different modifiers) @@ expected struct mcs_spinlock *prev @@ got struct struct mcs_spinlock *prev @@
kernel/locking/qspinlock.c:418:22: expected struct mcs_spinlock *prev
kernel/locking/qspinlock.c:418:22: got struct mcs_spinlock [pure] *
kernel/locking/qspinlock_paravirt.h:519:1: sparse: symbol '__pv_queued_spin_unlock_slowpath' was not declared. Should it be static?
kernel/locking/qspinlock.c:418:22: sparse: incorrect type in assignment (different modifiers) @@ expected struct mcs_spinlock *prev @@ got struct struct mcs_spinlock *prev @@
kernel/locking/qspinlock.c:418:22: expected struct mcs_spinlock *prev
kernel/locking/qspinlock.c:418:22: got struct mcs_spinlock [pure] *
>> include/asm-generic/atomic-instrumented.h:288:24: sparse: cast truncates bits from constant value (100 becomes 0)
vim +288 include/asm-generic/atomic-instrumented.h
b06ed71a6 Dmitry Vyukov 2018-01-29 282
b06ed71a6 Dmitry Vyukov 2018-01-29 283 static __always_inline unsigned long
b06ed71a6 Dmitry Vyukov 2018-01-29 284 cmpxchg_size(volatile void *ptr, unsigned long old, unsigned long new, int size)
b06ed71a6 Dmitry Vyukov 2018-01-29 285 {
b06ed71a6 Dmitry Vyukov 2018-01-29 286 switch (size) {
b06ed71a6 Dmitry Vyukov 2018-01-29 287 case 1:
b06ed71a6 Dmitry Vyukov 2018-01-29 @288 return arch_cmpxchg((u8 *)ptr, (u8)old, (u8)new);
b06ed71a6 Dmitry Vyukov 2018-01-29 289 case 2:
b06ed71a6 Dmitry Vyukov 2018-01-29 290 return arch_cmpxchg((u16 *)ptr, (u16)old, (u16)new);
b06ed71a6 Dmitry Vyukov 2018-01-29 291 case 4:
b06ed71a6 Dmitry Vyukov 2018-01-29 292 return arch_cmpxchg((u32 *)ptr, (u32)old, (u32)new);
b06ed71a6 Dmitry Vyukov 2018-01-29 293 case 8:
b06ed71a6 Dmitry Vyukov 2018-01-29 294 BUILD_BUG_ON(sizeof(unsigned long) != 8);
b06ed71a6 Dmitry Vyukov 2018-01-29 295 return arch_cmpxchg((u64 *)ptr, (u64)old, (u64)new);
b06ed71a6 Dmitry Vyukov 2018-01-29 296 }
b06ed71a6 Dmitry Vyukov 2018-01-29 297 BUILD_BUG();
b06ed71a6 Dmitry Vyukov 2018-01-29 298 return 0;
b06ed71a6 Dmitry Vyukov 2018-01-29 299 }
b06ed71a6 Dmitry Vyukov 2018-01-29 300
:::::: The code at line 288 was first introduced by commit
:::::: b06ed71a624ba088a3e3e3ac7d4185f48c7c1660 locking/atomic, asm-generic: Add asm-generic/atomic-instrumented.h
:::::: TO: Dmitry Vyukov <[email protected]>
:::::: CC: Ingo Molnar <[email protected]>
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
On Mon, Mar 12, 2018 at 5:52 PM, kbuild test robot
<[email protected]> wrote:
> tree: https://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git locking/core
> head: ac605bee0bfab40fd5d11964705e907d2d5a32de
> commit: 8bf705d130396e69c04cd8e6e010244ad2ce71f4 [9/11] locking/atomic/x86: Switch atomic.h to use atomic-instrumented.h
> reproduce:
> # apt-get install sparse
> git checkout 8bf705d130396e69c04cd8e6e010244ad2ce71f4
> make ARCH=x86_64 allmodconfig
> make C=1 CF=-D__CHECK_ENDIAN__
>
>
> sparse warnings: (new ones prefixed by >>)
>
> kernel/locking/qspinlock.c:418:22: sparse: incorrect type in assignment (different modifiers) @@ expected struct mcs_spinlock *prev @@ got struct struct mcs_spinlock *prev @@
> kernel/locking/qspinlock.c:418:22: expected struct mcs_spinlock *prev
> kernel/locking/qspinlock.c:418:22: got struct mcs_spinlock [pure] *
> kernel/locking/qspinlock_paravirt.h:519:1: sparse: symbol '__pv_queued_spin_unlock_slowpath' was not declared. Should it be static?
> kernel/locking/qspinlock.c:418:22: sparse: incorrect type in assignment (different modifiers) @@ expected struct mcs_spinlock *prev @@ got struct struct mcs_spinlock *prev @@
> kernel/locking/qspinlock.c:418:22: expected struct mcs_spinlock *prev
> kernel/locking/qspinlock.c:418:22: got struct mcs_spinlock [pure] *
>>> include/asm-generic/atomic-instrumented.h:288:24: sparse: cast truncates bits from constant value (100 becomes 0)
>
> vim +288 include/asm-generic/atomic-instrumented.h
>
> b06ed71a6 Dmitry Vyukov 2018-01-29 282
> b06ed71a6 Dmitry Vyukov 2018-01-29 283 static __always_inline unsigned long
> b06ed71a6 Dmitry Vyukov 2018-01-29 284 cmpxchg_size(volatile void *ptr, unsigned long old, unsigned long new, int size)
> b06ed71a6 Dmitry Vyukov 2018-01-29 285 {
> b06ed71a6 Dmitry Vyukov 2018-01-29 286 switch (size) {
> b06ed71a6 Dmitry Vyukov 2018-01-29 287 case 1:
> b06ed71a6 Dmitry Vyukov 2018-01-29 @288 return arch_cmpxchg((u8 *)ptr, (u8)old, (u8)new);
> b06ed71a6 Dmitry Vyukov 2018-01-29 289 case 2:
> b06ed71a6 Dmitry Vyukov 2018-01-29 290 return arch_cmpxchg((u16 *)ptr, (u16)old, (u16)new);
> b06ed71a6 Dmitry Vyukov 2018-01-29 291 case 4:
> b06ed71a6 Dmitry Vyukov 2018-01-29 292 return arch_cmpxchg((u32 *)ptr, (u32)old, (u32)new);
> b06ed71a6 Dmitry Vyukov 2018-01-29 293 case 8:
> b06ed71a6 Dmitry Vyukov 2018-01-29 294 BUILD_BUG_ON(sizeof(unsigned long) != 8);
> b06ed71a6 Dmitry Vyukov 2018-01-29 295 return arch_cmpxchg((u64 *)ptr, (u64)old, (u64)new);
> b06ed71a6 Dmitry Vyukov 2018-01-29 296 }
> b06ed71a6 Dmitry Vyukov 2018-01-29 297 BUILD_BUG();
> b06ed71a6 Dmitry Vyukov 2018-01-29 298 return 0;
> b06ed71a6 Dmitry Vyukov 2018-01-29 299 }
> b06ed71a6 Dmitry Vyukov 2018-01-29 300
>
> :::::: The code at line 288 was first introduced by commit
> :::::: b06ed71a624ba088a3e3e3ac7d4185f48c7c1660 locking/atomic, asm-generic: Add asm-generic/atomic-instrumented.h
>
> :::::: TO: Dmitry Vyukov <[email protected]>
> :::::: CC: Ingo Molnar <[email protected]>
I will take a look tomorrow.
On Mon, Mar 12, 2018 at 6:17 PM, Dmitry Vyukov <[email protected]> wrote:
> On Mon, Mar 12, 2018 at 5:52 PM, kbuild test robot
> <[email protected]> wrote:
>> tree: https://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git locking/core
>> head: ac605bee0bfab40fd5d11964705e907d2d5a32de
>> commit: 8bf705d130396e69c04cd8e6e010244ad2ce71f4 [9/11] locking/atomic/x86: Switch atomic.h to use atomic-instrumented.h
>> reproduce:
>> # apt-get install sparse
>> git checkout 8bf705d130396e69c04cd8e6e010244ad2ce71f4
>> make ARCH=x86_64 allmodconfig
>> make C=1 CF=-D__CHECK_ENDIAN__
>>
>>
>> sparse warnings: (new ones prefixed by >>)
>>
>> kernel/locking/qspinlock.c:418:22: sparse: incorrect type in assignment (different modifiers) @@ expected struct mcs_spinlock *prev @@ got struct struct mcs_spinlock *prev @@
>> kernel/locking/qspinlock.c:418:22: expected struct mcs_spinlock *prev
>> kernel/locking/qspinlock.c:418:22: got struct mcs_spinlock [pure] *
>> kernel/locking/qspinlock_paravirt.h:519:1: sparse: symbol '__pv_queued_spin_unlock_slowpath' was not declared. Should it be static?
>> kernel/locking/qspinlock.c:418:22: sparse: incorrect type in assignment (different modifiers) @@ expected struct mcs_spinlock *prev @@ got struct struct mcs_spinlock *prev @@
>> kernel/locking/qspinlock.c:418:22: expected struct mcs_spinlock *prev
>> kernel/locking/qspinlock.c:418:22: got struct mcs_spinlock [pure] *
>>>> include/asm-generic/atomic-instrumented.h:288:24: sparse: cast truncates bits from constant value (100 becomes 0)
>>
>> vim +288 include/asm-generic/atomic-instrumented.h
>>
>> b06ed71a6 Dmitry Vyukov 2018-01-29 282
>> b06ed71a6 Dmitry Vyukov 2018-01-29 283 static __always_inline unsigned long
>> b06ed71a6 Dmitry Vyukov 2018-01-29 284 cmpxchg_size(volatile void *ptr, unsigned long old, unsigned long new, int size)
>> b06ed71a6 Dmitry Vyukov 2018-01-29 285 {
>> b06ed71a6 Dmitry Vyukov 2018-01-29 286 switch (size) {
>> b06ed71a6 Dmitry Vyukov 2018-01-29 287 case 1:
>> b06ed71a6 Dmitry Vyukov 2018-01-29 @288 return arch_cmpxchg((u8 *)ptr, (u8)old, (u8)new);
>> b06ed71a6 Dmitry Vyukov 2018-01-29 289 case 2:
>> b06ed71a6 Dmitry Vyukov 2018-01-29 290 return arch_cmpxchg((u16 *)ptr, (u16)old, (u16)new);
>> b06ed71a6 Dmitry Vyukov 2018-01-29 291 case 4:
>> b06ed71a6 Dmitry Vyukov 2018-01-29 292 return arch_cmpxchg((u32 *)ptr, (u32)old, (u32)new);
>> b06ed71a6 Dmitry Vyukov 2018-01-29 293 case 8:
>> b06ed71a6 Dmitry Vyukov 2018-01-29 294 BUILD_BUG_ON(sizeof(unsigned long) != 8);
>> b06ed71a6 Dmitry Vyukov 2018-01-29 295 return arch_cmpxchg((u64 *)ptr, (u64)old, (u64)new);
>> b06ed71a6 Dmitry Vyukov 2018-01-29 296 }
>> b06ed71a6 Dmitry Vyukov 2018-01-29 297 BUILD_BUG();
>> b06ed71a6 Dmitry Vyukov 2018-01-29 298 return 0;
>> b06ed71a6 Dmitry Vyukov 2018-01-29 299 }
>> b06ed71a6 Dmitry Vyukov 2018-01-29 300
>>
>> :::::: The code at line 288 was first introduced by commit
>> :::::: b06ed71a624ba088a3e3e3ac7d4185f48c7c1660 locking/atomic, asm-generic: Add asm-generic/atomic-instrumented.h
>>
>> :::::: TO: Dmitry Vyukov <[email protected]>
>> :::::: CC: Ingo Molnar <[email protected]>
>
>
> I will take a look tomorrow.
It seems that this is due to this guy:
static __always_inline int trylock_clear_pending(struct qspinlock *lock)
{
struct __qspinlock *l = (void *)lock;
return !READ_ONCE(l->locked) &&
(cmpxchg_acquire(&l->locked_pending, _Q_PENDING_VAL,
_Q_LOCKED_VAL) == _Q_PENDING_VAL);
}
_Q_PENDING_VAL is 0x100. However, locked_pending is 2 bytes. So it
seems that compiler checks all switch cases, this inevitably will lead
to such warnings.
Any suggestion on how to resolve this? Leave as is?
Off the top of my head I can think of the following solution:
switch (size) {
case 1:
return arch_cmpxchg((u8 *)ptr, (u8)(old * (size !=
1)), (u8)(new * (size != 1)));
case 2:
return arch_cmpxchg((u16 *)ptr, (u16)(old * (size !=
2)), (u16)(new * (size != 2)));
But it's too ugly.
FTR, the following was helpful to find invocations with particular size:
#define cmpxchg(ptr, old, new) \
({ \
+ BUILD_BUG_ON(sizeof(*(ptr)) == 1); \
((__typeof__(*(ptr)))cmpxchg_size((ptr), (unsigned long)(old), \
(unsigned long)(new), sizeof(*(ptr)))); \
})
On Tue, Mar 13, 2018 at 11:49:17AM +0300, Dmitry Vyukov wrote:
> On Mon, Mar 12, 2018 at 6:17 PM, Dmitry Vyukov <[email protected]> wrote:
> > On Mon, Mar 12, 2018 at 5:52 PM, kbuild test robot
> >> kernel/locking/qspinlock.c:418:22: sparse: incorrect type in assignment (different modifiers) @@ expected struct mcs_spinlock *prev @@ got struct struct mcs_spinlock *prev @@
> >> kernel/locking/qspinlock.c:418:22: expected struct mcs_spinlock *prev
> >> kernel/locking/qspinlock.c:418:22: got struct mcs_spinlock [pure] *
> >> b06ed71a6 Dmitry Vyukov 2018-01-29 283 static __always_inline unsigned long
> >> b06ed71a6 Dmitry Vyukov 2018-01-29 284 cmpxchg_size(volatile void *ptr, unsigned long old, unsigned long new, int size)
> >> b06ed71a6 Dmitry Vyukov 2018-01-29 285 {
> >> b06ed71a6 Dmitry Vyukov 2018-01-29 286 switch (size) {
> >> b06ed71a6 Dmitry Vyukov 2018-01-29 287 case 1:
> >> b06ed71a6 Dmitry Vyukov 2018-01-29 @288 return arch_cmpxchg((u8 *)ptr, (u8)old, (u8)new);
> >> b06ed71a6 Dmitry Vyukov 2018-01-29 289 case 2:
> >> b06ed71a6 Dmitry Vyukov 2018-01-29 290 return arch_cmpxchg((u16 *)ptr, (u16)old, (u16)new);
> >> b06ed71a6 Dmitry Vyukov 2018-01-29 291 case 4:
> >> b06ed71a6 Dmitry Vyukov 2018-01-29 292 return arch_cmpxchg((u32 *)ptr, (u32)old, (u32)new);
> >> b06ed71a6 Dmitry Vyukov 2018-01-29 293 case 8:
> >> b06ed71a6 Dmitry Vyukov 2018-01-29 294 BUILD_BUG_ON(sizeof(unsigned long) != 8);
> >> b06ed71a6 Dmitry Vyukov 2018-01-29 295 return arch_cmpxchg((u64 *)ptr, (u64)old, (u64)new);
> >> b06ed71a6 Dmitry Vyukov 2018-01-29 296 }
> >> b06ed71a6 Dmitry Vyukov 2018-01-29 297 BUILD_BUG();
> >> b06ed71a6 Dmitry Vyukov 2018-01-29 298 return 0;
> >> b06ed71a6 Dmitry Vyukov 2018-01-29 299 }
> >> b06ed71a6 Dmitry Vyukov 2018-01-29 300
> It seems that this is due to this guy:
>
> static __always_inline int trylock_clear_pending(struct qspinlock *lock)
> {
> struct __qspinlock *l = (void *)lock;
>
> return !READ_ONCE(l->locked) &&
> (cmpxchg_acquire(&l->locked_pending, _Q_PENDING_VAL,
> _Q_LOCKED_VAL) == _Q_PENDING_VAL);
> }
>
> _Q_PENDING_VAL is 0x100. However, locked_pending is 2 bytes. So it
> seems that compiler checks all switch cases, this inevitably will lead
> to such warnings.
>
> Any suggestion on how to resolve this? Leave as is?
I'm not sure I understand what it thinks is wrong. Can't we fix sparse
to not be stupid? The actual compilers don't seem to a have a problem
with this.
> Off the top of my head I can think of the following solution:
>
> switch (size) {
> case 1:
> return arch_cmpxchg((u8 *)ptr, (u8)(old * (size !=
> 1)), (u8)(new * (size != 1)));
> case 2:
> return arch_cmpxchg((u16 *)ptr, (u16)(old * (size !=
> 2)), (u16)(new * (size != 2)));
>
> But it's too ugly.
Yes agreed, that's horrendous.
On Tue, Mar 13, 2018 at 1:46 PM, Peter Zijlstra <[email protected]> wrote:
> On Tue, Mar 13, 2018 at 11:49:17AM +0300, Dmitry Vyukov wrote:
>> On Mon, Mar 12, 2018 at 6:17 PM, Dmitry Vyukov <[email protected]> wrote:
>> > On Mon, Mar 12, 2018 at 5:52 PM, kbuild test robot
>> >> kernel/locking/qspinlock.c:418:22: sparse: incorrect type in assignment (different modifiers) @@ expected struct mcs_spinlock *prev @@ got struct struct mcs_spinlock *prev @@
>> >> kernel/locking/qspinlock.c:418:22: expected struct mcs_spinlock *prev
>> >> kernel/locking/qspinlock.c:418:22: got struct mcs_spinlock [pure] *
>
>> >> b06ed71a6 Dmitry Vyukov 2018-01-29 283 static __always_inline unsigned long
>> >> b06ed71a6 Dmitry Vyukov 2018-01-29 284 cmpxchg_size(volatile void *ptr, unsigned long old, unsigned long new, int size)
>> >> b06ed71a6 Dmitry Vyukov 2018-01-29 285 {
>> >> b06ed71a6 Dmitry Vyukov 2018-01-29 286 switch (size) {
>> >> b06ed71a6 Dmitry Vyukov 2018-01-29 287 case 1:
>> >> b06ed71a6 Dmitry Vyukov 2018-01-29 @288 return arch_cmpxchg((u8 *)ptr, (u8)old, (u8)new);
>> >> b06ed71a6 Dmitry Vyukov 2018-01-29 289 case 2:
>> >> b06ed71a6 Dmitry Vyukov 2018-01-29 290 return arch_cmpxchg((u16 *)ptr, (u16)old, (u16)new);
>> >> b06ed71a6 Dmitry Vyukov 2018-01-29 291 case 4:
>> >> b06ed71a6 Dmitry Vyukov 2018-01-29 292 return arch_cmpxchg((u32 *)ptr, (u32)old, (u32)new);
>> >> b06ed71a6 Dmitry Vyukov 2018-01-29 293 case 8:
>> >> b06ed71a6 Dmitry Vyukov 2018-01-29 294 BUILD_BUG_ON(sizeof(unsigned long) != 8);
>> >> b06ed71a6 Dmitry Vyukov 2018-01-29 295 return arch_cmpxchg((u64 *)ptr, (u64)old, (u64)new);
>> >> b06ed71a6 Dmitry Vyukov 2018-01-29 296 }
>> >> b06ed71a6 Dmitry Vyukov 2018-01-29 297 BUILD_BUG();
>> >> b06ed71a6 Dmitry Vyukov 2018-01-29 298 return 0;
>> >> b06ed71a6 Dmitry Vyukov 2018-01-29 299 }
>> >> b06ed71a6 Dmitry Vyukov 2018-01-29 300
>
>> It seems that this is due to this guy:
>>
>> static __always_inline int trylock_clear_pending(struct qspinlock *lock)
>> {
>> struct __qspinlock *l = (void *)lock;
>>
>> return !READ_ONCE(l->locked) &&
>> (cmpxchg_acquire(&l->locked_pending, _Q_PENDING_VAL,
>> _Q_LOCKED_VAL) == _Q_PENDING_VAL);
>> }
>>
>> _Q_PENDING_VAL is 0x100. However, locked_pending is 2 bytes. So it
>> seems that compiler checks all switch cases, this inevitably will lead
>> to such warnings.
>>
>> Any suggestion on how to resolve this? Leave as is?
>
> I'm not sure I understand what it thinks is wrong. Can't we fix sparse
> to not be stupid? The actual compilers don't seem to a have a problem
> with this.
That would be best of course. +sparse mailing list and Christopher.
>> Off the top of my head I can think of the following solution:
>>
>> switch (size) {
>> case 1:
>> return arch_cmpxchg((u8 *)ptr, (u8)(old * (size !=
>> 1)), (u8)(new * (size != 1)));
>> case 2:
>> return arch_cmpxchg((u16 *)ptr, (u16)(old * (size !=
>> 2)), (u16)(new * (size != 2)));
>>
>> But it's too ugly.
>
> Yes agreed, that's horrendous.
On Tue, Mar 13, 2018 at 4:08 AM, Dmitry Vyukov <[email protected]> wrote:
> On Tue, Mar 13, 2018 at 1:46 PM, Peter Zijlstra <[email protected]> wrote:
>>
>>> >> b06ed71a6 Dmitry Vyukov 2018-01-29 283 static __always_inline unsigned long
>>> >> b06ed71a6 Dmitry Vyukov 2018-01-29 284 cmpxchg_size(volatile void *ptr, unsigned long old, unsigned long new, int size)
>>> >> b06ed71a6 Dmitry Vyukov 2018-01-29 285 {
>>> >> b06ed71a6 Dmitry Vyukov 2018-01-29 286 switch (size) {
>>> >> b06ed71a6 Dmitry Vyukov 2018-01-29 287 case 1:
>>> >> b06ed71a6 Dmitry Vyukov 2018-01-29 @288 return arch_cmpxchg((u8 *)ptr, (u8)old, (u8)new);
>>> >> b06ed71a6 Dmitry Vyukov 2018-01-29 289 case 2:
>>> >> b06ed71a6 Dmitry Vyukov 2018-01-29 290 return arch_cmpxchg((u16 *)ptr, (u16)old, (u16)new);
>>> >> b06ed71a6 Dmitry Vyukov 2018-01-29 291 case 4:
>>> >> b06ed71a6 Dmitry Vyukov 2018-01-29 292 return arch_cmpxchg((u32 *)ptr, (u32)old, (u32)new);
>>> >> b06ed71a6 Dmitry Vyukov 2018-01-29 293 case 8:
>>> >> b06ed71a6 Dmitry Vyukov 2018-01-29 294 BUILD_BUG_ON(sizeof(unsigned long) != 8);
>>> >> b06ed71a6 Dmitry Vyukov 2018-01-29 295 return arch_cmpxchg((u64 *)ptr, (u64)old, (u64)new);
>>> >> b06ed71a6 Dmitry Vyukov 2018-01-29 296 }
>>> >> b06ed71a6 Dmitry Vyukov 2018-01-29 297 BUILD_BUG();
>>> >> b06ed71a6 Dmitry Vyukov 2018-01-29 298 return 0;
>>> >> b06ed71a6 Dmitry Vyukov 2018-01-29 299 }
>>> >> b06ed71a6 Dmitry Vyukov 2018-01-29 300
>>
>>> It seems that this is due to this guy:
>>>
>>> static __always_inline int trylock_clear_pending(struct qspinlock *lock)
>>> {
>>> struct __qspinlock *l = (void *)lock;
>>>
>>> return !READ_ONCE(l->locked) &&
>>> (cmpxchg_acquire(&l->locked_pending, _Q_PENDING_VAL,
>>> _Q_LOCKED_VAL) == _Q_PENDING_VAL);
>>> }
>>>
>>> _Q_PENDING_VAL is 0x100. However, locked_pending is 2 bytes. So it
>>> seems that compiler checks all switch cases, this inevitably will lead
>>> to such warnings.
So you are saying cmpxchg_size() was call with a size==2,
however sparse did not do the constant propagation or evaluation
properly. So that it did not eliminate the other branch of size==1
case statement.
Can you do a "sparse -E" on the original file, then save the
result to a new file. That will take care of the pre-processing.
Then run the sparse on the pre-processed file to get a smaller
test case?
Having a smaller test case would make it easier to reproduce
what exactly IR was issued during that warning.
>>> Off the top of my head I can think of the following solution:
>>>
>>> switch (size) {
>>> case 1:
>>> return arch_cmpxchg((u8 *)ptr, (u8)(old * (size !=
>>> 1)), (u8)(new * (size != 1)));
>>> case 2:
>>> return arch_cmpxchg((u16 *)ptr, (u16)(old * (size !=
>>> 2)), (u16)(new * (size != 2)));
>>>
>>> But it's too ugly.
>>
>> Yes agreed, that's horrendous.
Let's not do that. If it is a sparse problem, let's try to fix this sparse.
Chris
On Tue, Mar 13, 2018 at 02:08:13PM +0300, Dmitry Vyukov wrote:
> On Tue, Mar 13, 2018 at 1:46 PM, Peter Zijlstra <[email protected]> wrote:
> > On Tue, Mar 13, 2018 at 11:49:17AM +0300, Dmitry Vyukov wrote:
> >> On Mon, Mar 12, 2018 at 6:17 PM, Dmitry Vyukov <[email protected]> wrote:
> >> > On Mon, Mar 12, 2018 at 5:52 PM, kbuild test robot
> >> >> kernel/locking/qspinlock.c:418:22: sparse: incorrect type in assignment (different modifiers) @@ expected struct mcs_spinlock *prev @@ got struct struct mcs_spinlock *prev @@
> >> >> kernel/locking/qspinlock.c:418:22: expected struct mcs_spinlock *prev
> >> >> kernel/locking/qspinlock.c:418:22: got struct mcs_spinlock [pure] *
> >
> >> >> b06ed71a6 Dmitry Vyukov 2018-01-29 283 static __always_inline unsigned long
> >> >> b06ed71a6 Dmitry Vyukov 2018-01-29 284 cmpxchg_size(volatile void *ptr, unsigned long old, unsigned long new, int size)
> >> >> b06ed71a6 Dmitry Vyukov 2018-01-29 285 {
> >> >> b06ed71a6 Dmitry Vyukov 2018-01-29 286 switch (size) {
> >> >> b06ed71a6 Dmitry Vyukov 2018-01-29 287 case 1:
> >> >> b06ed71a6 Dmitry Vyukov 2018-01-29 @288 return arch_cmpxchg((u8 *)ptr, (u8)old, (u8)new);
> >> >> b06ed71a6 Dmitry Vyukov 2018-01-29 289 case 2:
> >> >> b06ed71a6 Dmitry Vyukov 2018-01-29 290 return arch_cmpxchg((u16 *)ptr, (u16)old, (u16)new);
> >> >> b06ed71a6 Dmitry Vyukov 2018-01-29 291 case 4:
> >> >> b06ed71a6 Dmitry Vyukov 2018-01-29 292 return arch_cmpxchg((u32 *)ptr, (u32)old, (u32)new);
> >> >> b06ed71a6 Dmitry Vyukov 2018-01-29 293 case 8:
> >> >> b06ed71a6 Dmitry Vyukov 2018-01-29 294 BUILD_BUG_ON(sizeof(unsigned long) != 8);
> >> >> b06ed71a6 Dmitry Vyukov 2018-01-29 295 return arch_cmpxchg((u64 *)ptr, (u64)old, (u64)new);
> >> >> b06ed71a6 Dmitry Vyukov 2018-01-29 296 }
> >> >> b06ed71a6 Dmitry Vyukov 2018-01-29 297 BUILD_BUG();
> >> >> b06ed71a6 Dmitry Vyukov 2018-01-29 298 return 0;
> >> >> b06ed71a6 Dmitry Vyukov 2018-01-29 299 }
> >> >> b06ed71a6 Dmitry Vyukov 2018-01-29 300
> >
> >> It seems that this is due to this guy:
> >>
> >> static __always_inline int trylock_clear_pending(struct qspinlock *lock)
> >> {
> >> struct __qspinlock *l = (void *)lock;
> >>
> >> return !READ_ONCE(l->locked) &&
> >> (cmpxchg_acquire(&l->locked_pending, _Q_PENDING_VAL,
> >> _Q_LOCKED_VAL) == _Q_PENDING_VAL);
> >> }
> >>
> >> _Q_PENDING_VAL is 0x100. However, locked_pending is 2 bytes. So it
> >> seems that compiler checks all switch cases, this inevitably will lead
> >> to such warnings.
> >>
> >> Any suggestion on how to resolve this? Leave as is?
> >
> > I'm not sure I understand what it thinks is wrong. Can't we fix sparse
> > to not be stupid? The actual compilers don't seem to a have a problem
> > with this.
The issue here is that sparse has a whole class of warnings that are
given very early (here at expansion of constant expressions), before
eliminating code from branches that are never taken (which, surprise,
need itself to have constant expressions already expanded).
It's often annoying like the case here.
OTOH, I don't think it's always a bad thing. Sometimes we want to
have warnings even from code we know will not be executed (in this
config but maybe it will in another one).
Several solution would be possible:
1) removing all those early warnings (but then we'll loose more legitimate ones)
2) add an option to not issues those early warnings (ame as above).
3) move those warnings (and thus the expansion of the concerned constants)
after trivial dead code elimination
4) add a very early pass to eliminate never taken branches
5) something else ?
1) & 2) are easy but not really acceptable.
3) can be good for some cases but not here, I think.
4) seems to way to go
-- Luc Van Oostenryck
On Tue, Mar 13, 2018 at 07:00:17PM +0100, Luc Van Oostenryck wrote:
> The issue here is that sparse has a whole class of warnings that are
> given very early (here at expansion of constant expressions), before
> eliminating code from branches that are never taken (which, surprise,
> need itself to have constant expressions already expanded).
>
> It's often annoying like the case here.
> OTOH, I don't think it's always a bad thing. Sometimes we want to
> have warnings even from code we know will not be executed (in this
> config but maybe it will in another one).
Is that really a valid concern with all the automated randconfig
building going on today?
On Tue, Mar 13, 2018 at 07:08:06PM +0100, Peter Zijlstra wrote:
> On Tue, Mar 13, 2018 at 07:00:17PM +0100, Luc Van Oostenryck wrote:
> > The issue here is that sparse has a whole class of warnings that are
> > given very early (here at expansion of constant expressions), before
> > eliminating code from branches that are never taken (which, surprise,
> > need itself to have constant expressions already expanded).
> >
> > It's often annoying like the case here.
> > OTOH, I don't think it's always a bad thing. Sometimes we want to
> > have warnings even from code we know will not be executed (in this
> > config but maybe it will in another one).
>
> Is that really a valid concern with all the automated randconfig
> building going on today?
I don't think so, for the kernel at least. For other uses it may.
But don't take me wrongly: I don't want to defend those warnings here,
I just want to say that the situation is not totally black & white.
One easy-short-term solution that wouldn't make things ugly would be
to use a mask instead of a cast:
static __always_inline unsigned long
cmpxchg_size(volatile void *ptr, unsigned long old, unsigned long new, int size)
{
switch (size) {
case 1:
- return arch_cmpxchg((u8 *)ptr, (u8)old, (u8)new);
+ return arch_cmpxchg((u8 *)ptr, old & 0xff, new & 0xff);
case 2:
- return arch_cmpxchg((u16 *)ptr, (u16)old, (u16)new);
+ return arch_cmpxchg((u16 *)ptr, old & 0xffff, new & 0xffff);
-- Luc