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

[Josh Triplett <josh@joshtriplett.org>]

On Sat, Jun 28, 2014 at 11:07:48AM -0700, Christopher Li wrote:
> On Wed, Jun 11, 2014 at 2:45 PM,  <josh@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.

Except that "filter" has an "int channel" (signed), so it can
successfully test "channel < 0" and return early; it'll never call
ni_stc_dma_channel_select_bitfield(channel) with a negative number.

I do agree that this code should sort out the signedness of its types,
but in this case I don't think the bad shift can actually happen, making
this a false positive.

- Josh Triplett