[PATCH] Staging/comedi: Fixes static analysis warning raised by sparse

[Christopher Li]

Sorry for the late reply.

On Wed, Jun 11, 2014 at 2:45 PM,  <josh at joshtriplett.org> wrote:
> On Thu, Jun 12, 2014 at 12:24:25AM +0300, Dan Carpenter wrote:
>> Let's forward this to the Sparse mailing list.
>>
>> We're seeing a Sparse false positive testing

No, this is a valid complain. See my point follows.

>> drivers/staging/comedi/drivers/ni_pcimio.c.
>>
>>   CHECK   drivers/staging/comedi/drivers/ni_pcimio.c
>> drivers/staging/comedi/drivers/ni_stc.h:720:26: warning: shift too big (4294967295) for type int
>> drivers/staging/comedi/drivers/ni_stc.h:720:26: warning: shift too big (4294967295) for type int
>> drivers/staging/comedi/drivers/ni_stc.h:720:26: warning: shift too big (4294967295) for type int
>> drivers/staging/comedi/drivers/ni_stc.h:720:26: warning: shift too big (4294967295) for type int
>>
>> I have created some test code to demonstrate the problem (attached).
>>
>> The check_shift_count() warning is only supposed to be printed for
>> number literals but because of the way inline functions are expanded it
>> still complains even though channel is a variable.
>
> Thanks for the test case; this definitely makes no sense.  I don't think
> Sparse will suddenly develop enough range analysis or reachability
> analysis to handle this case; I think the right answer is to avoid
> giving such warnings for shifts with a non-constant RHS.

Sparse can handle inline function expand and some constant
propagate. In this case, sparse seems doing the right thing.
Sparse actually sees the channel value being  4294967295 (-1).


>> static inline unsigned ni_stc_dma_channel_select_bitfield(unsigned channel)

This is the bug. See this channel is *unsigned*. When -1 pass into
channel, it become a really large number 4294967295.
The code does request C compiler to perform left shift 4294967295 bits.
Which did not make sense.

>> {
>>       if (channel < 4)
>>               return 1 << channel;
>>       return 0;
>> }
>>
>> static inline void filter(int channel)
>> {
>>       if (channel < 0)
>>               return;
>>       ni_stc_dma_channel_select_bitfield(channel);

See this channel is *signed*, with -1 convert to 4294967295.
This is a bug in the kernel source not sparse.

Chris