* 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 ` [tip:locking/core 9/11] include/asm-generic/atomic-instrumented.h:288:24: sparse: cast truncates bits from constant value (100 becomes 0) Dmitry Vyukov
@ 2018-03-13 17:29 ` Christopher Li
2018-03-13 18:00 ` Luc Van Oostenryck
1 sibling, 0 replies; 5+ 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] 5+ 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 ` [tip:locking/core 9/11] include/asm-generic/atomic-instrumented.h:288:24: sparse: cast truncates bits from constant value (100 becomes 0) 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; 5+ 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] 5+ messages in thread