* [tip:locking/core 9/11] include/asm-generic/atomic-instrumented.h:288:24: sparse: cast truncates bits from constant value (100 becomes 0)
@ 2018-03-12 14:52 kbuild test robot
2018-03-12 15:17 ` Dmitry Vyukov
0 siblings, 1 reply; 9+ messages in thread
From: kbuild test robot @ 2018-03-12 14:52 UTC (permalink / raw)
To: Dmitry Vyukov; +Cc: kbuild-all, linux-kernel, tipbuild, Ingo Molnar
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 <dvyukov@google.com>
:::::: CC: Ingo Molnar <mingo@kernel.org>
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [tip:locking/core 9/11] include/asm-generic/atomic-instrumented.h:288:24: sparse: cast truncates bits from constant value (100 becomes 0)
2018-03-12 14:52 [tip:locking/core 9/11] include/asm-generic/atomic-instrumented.h:288:24: sparse: cast truncates bits from constant value (100 becomes 0) kbuild test robot
@ 2018-03-12 15:17 ` Dmitry Vyukov
2018-03-13 8:49 ` Dmitry Vyukov
0 siblings, 1 reply; 9+ messages in thread
From: Dmitry Vyukov @ 2018-03-12 15:17 UTC (permalink / raw)
To: kbuild test robot; +Cc: kbuild-all, LKML, tipbuild, Ingo Molnar
On Mon, Mar 12, 2018 at 5:52 PM, kbuild test robot
<fengguang.wu@intel.com> 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 <dvyukov@google.com>
> :::::: CC: Ingo Molnar <mingo@kernel.org>
I will take a look tomorrow.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [tip:locking/core 9/11] include/asm-generic/atomic-instrumented.h:288:24: sparse: cast truncates bits from constant value (100 becomes 0)
2018-03-12 15:17 ` Dmitry Vyukov
@ 2018-03-13 8:49 ` Dmitry Vyukov
2018-03-13 10:46 ` Peter Zijlstra
0 siblings, 1 reply; 9+ messages in thread
From: Dmitry Vyukov @ 2018-03-13 8:49 UTC (permalink / raw)
To: kbuild test robot; +Cc: kbuild-all, LKML, tipbuild, Ingo Molnar, Peter Zijlstra
On Mon, Mar 12, 2018 at 6:17 PM, Dmitry Vyukov <dvyukov@google.com> wrote:
> On Mon, Mar 12, 2018 at 5:52 PM, kbuild test robot
> <fengguang.wu@intel.com> 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 <dvyukov@google.com>
>> :::::: CC: Ingo Molnar <mingo@kernel.org>
>
>
> 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)))); \
})
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [tip:locking/core 9/11] include/asm-generic/atomic-instrumented.h:288:24: sparse: cast truncates bits from constant value (100 becomes 0)
2018-03-13 8:49 ` Dmitry Vyukov
@ 2018-03-13 10:46 ` Peter Zijlstra
2018-03-13 11:08 ` Dmitry Vyukov
0 siblings, 1 reply; 9+ messages in thread
From: Peter Zijlstra @ 2018-03-13 10:46 UTC (permalink / raw)
To: Dmitry Vyukov; +Cc: kbuild test robot, kbuild-all, LKML, tipbuild, Ingo Molnar
On Tue, Mar 13, 2018 at 11:49:17AM +0300, Dmitry Vyukov wrote:
> On Mon, Mar 12, 2018 at 6:17 PM, Dmitry Vyukov <dvyukov@google.com> 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.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [tip:locking/core 9/11] include/asm-generic/atomic-instrumented.h:288:24: sparse: cast truncates bits from constant value (100 becomes 0)
2018-03-13 10:46 ` Peter Zijlstra
@ 2018-03-13 11:08 ` Dmitry Vyukov
2018-03-13 17:29 ` Christopher Li
2018-03-13 18:00 ` Luc Van Oostenryck
0 siblings, 2 replies; 9+ messages in thread
From: Dmitry Vyukov @ 2018-03-13 11:08 UTC (permalink / raw)
To: Peter Zijlstra, linux-sparse, Christopher Li
Cc: kbuild test robot, kbuild-all, LKML, tipbuild, Ingo Molnar
On Tue, Mar 13, 2018 at 1:46 PM, Peter Zijlstra <peterz@infradead.org> 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 <dvyukov@google.com> 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.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [tip:locking/core 9/11] include/asm-generic/atomic-instrumented.h:288:24: sparse: cast truncates bits from constant value (100 becomes 0)
2018-03-13 11:08 ` Dmitry Vyukov
@ 2018-03-13 17:29 ` Christopher Li
2018-03-13 18:00 ` Luc Van Oostenryck
1 sibling, 0 replies; 9+ messages in thread
From: Christopher Li @ 2018-03-13 17:29 UTC (permalink / raw)
To: Dmitry Vyukov
Cc: Peter Zijlstra, Linux-Sparse, kbuild test robot, kbuild-all,
LKML, tipbuild, Ingo Molnar
On Tue, Mar 13, 2018 at 4:08 AM, Dmitry Vyukov <dvyukov@google.com> wrote:
> On Tue, Mar 13, 2018 at 1:46 PM, Peter Zijlstra <peterz@infradead.org> 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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [tip:locking/core 9/11] include/asm-generic/atomic-instrumented.h:288:24: sparse: cast truncates bits from constant value (100 becomes 0)
2018-03-13 11:08 ` Dmitry Vyukov
2018-03-13 17:29 ` Christopher Li
@ 2018-03-13 18:00 ` Luc Van Oostenryck
2018-03-13 18:08 ` Peter Zijlstra
1 sibling, 1 reply; 9+ messages in thread
From: Luc Van Oostenryck @ 2018-03-13 18:00 UTC (permalink / raw)
To: Dmitry Vyukov
Cc: Peter Zijlstra, linux-sparse, Christopher Li, kbuild test robot,
kbuild-all, LKML, tipbuild, Ingo Molnar
On Tue, Mar 13, 2018 at 02:08:13PM +0300, Dmitry Vyukov wrote:
> On Tue, Mar 13, 2018 at 1:46 PM, Peter Zijlstra <peterz@infradead.org> 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 <dvyukov@google.com> 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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [tip:locking/core 9/11] include/asm-generic/atomic-instrumented.h:288:24: sparse: cast truncates bits from constant value (100 becomes 0)
2018-03-13 18:00 ` Luc Van Oostenryck
@ 2018-03-13 18:08 ` Peter Zijlstra
2018-03-13 20:01 ` Luc Van Oostenryck
0 siblings, 1 reply; 9+ messages in thread
From: Peter Zijlstra @ 2018-03-13 18:08 UTC (permalink / raw)
To: Luc Van Oostenryck
Cc: Dmitry Vyukov, linux-sparse, Christopher Li, kbuild test robot,
kbuild-all, LKML, tipbuild, Ingo Molnar
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?
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [tip:locking/core 9/11] include/asm-generic/atomic-instrumented.h:288:24: sparse: cast truncates bits from constant value (100 becomes 0)
2018-03-13 18:08 ` Peter Zijlstra
@ 2018-03-13 20:01 ` Luc Van Oostenryck
0 siblings, 0 replies; 9+ messages in thread
From: Luc Van Oostenryck @ 2018-03-13 20:01 UTC (permalink / raw)
To: Peter Zijlstra
Cc: Dmitry Vyukov, linux-sparse, Christopher Li, kbuild test robot,
kbuild-all, LKML, tipbuild, Ingo Molnar
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
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2018-03-13 20:01 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-03-12 14:52 [tip:locking/core 9/11] include/asm-generic/atomic-instrumented.h:288:24: sparse: cast truncates bits from constant value (100 becomes 0) kbuild test robot
2018-03-12 15:17 ` Dmitry Vyukov
2018-03-13 8:49 ` Dmitry Vyukov
2018-03-13 10:46 ` Peter Zijlstra
2018-03-13 11:08 ` Dmitry Vyukov
2018-03-13 17:29 ` Christopher Li
2018-03-13 18:00 ` Luc Van Oostenryck
2018-03-13 18:08 ` Peter Zijlstra
2018-03-13 20:01 ` Luc Van Oostenryck
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).