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

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

[Dan Carpenter]

[-- Attachment #1: Type: text/plain, Size: 865 bytes --]

Let's forward this to the Sparse mailing list.

We're seeing a Sparse false positive testing
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.

regards,
dan carpenter


[-- Attachment #2: test_shift.c --]
[-- Type: text/x-csrc, Size: 355 bytes --]

#include <stdio.h>
#include <limits.h>
#include <string.h>

static inline unsigned ni_stc_dma_channel_select_bitfield(unsigned channel)
{
	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);
}

int main(void)
{
	filter(-1);

	return 0;
}

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

[josh]

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
> 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.

> #include <stdio.h>
> #include <limits.h>
> #include <string.h>
> 
> static inline unsigned ni_stc_dma_channel_select_bitfield(unsigned channel)
> {
> 	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);
> }
> 
> int main(void)
> {
> 	filter(-1);
> 
> 	return 0;
> }

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

[Sam Ravnborg]

Hi Josh.

On Wed, Jun 11, 2014 at 02:45:29PM -0700, 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
> > 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.

Something like the appended?

It seems to work for me - and it cured a lot of warnings in math-emu.c on sparc.
If it looks OK I will do a proper patch submission.

	Sam


diff --git a/expand.c b/expand.c
index 0f6720c..4a96de4 100644
--- a/expand.c
+++ b/expand.c
@@ -187,7 +187,7 @@ static int simplify_int_binop(struct expression *expr, struct symbol *ctype)
 		return 0;
 	r = right->value;
 	if (expr->op == SPECIAL_LEFTSHIFT || expr->op == SPECIAL_RIGHTSHIFT) {
-		if (r >= ctype->bit_size) {
+		if (expr->flags & Int_const_expr && r >= ctype->bit_size) {
 			if (conservative)
 				return 0;
 			r = check_shift_count(expr, ctype, r);

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

[Dan Carpenter]

On Sun, Jun 15, 2014 at 09:32:27PM +0200, Sam Ravnborg wrote:
> diff --git a/expand.c b/expand.c
> index 0f6720c..4a96de4 100644
> --- a/expand.c
> +++ b/expand.c
> @@ -187,7 +187,7 @@ static int simplify_int_binop(struct expression *expr, struct symbol *ctype)
>  		return 0;
>  	r = right->value;
>  	if (expr->op == SPECIAL_LEFTSHIFT || expr->op == SPECIAL_RIGHTSHIFT) {
> -		if (r >= ctype->bit_size) {
> +		if (expr->flags & Int_const_expr && r >= ctype->bit_size) {

Thanks!  I had no idea how to start writing a fix for this, but the test
should be:
		if (expr->right->flags & Int_const_expr

Otherwise both sides of the shift have to be const.

>  			if (conservative)
>  				return 0;
>  			r = check_shift_count(expr, ctype, r);
> 

regards,
dan carpenter

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

[Sam Ravnborg]

On Mon, Jun 16, 2014 at 10:40:19AM +0300, Dan Carpenter wrote:
> On Sun, Jun 15, 2014 at 09:32:27PM +0200, Sam Ravnborg wrote:
> > diff --git a/expand.c b/expand.c
> > index 0f6720c..4a96de4 100644
> > --- a/expand.c
> > +++ b/expand.c
> > @@ -187,7 +187,7 @@ static int simplify_int_binop(struct expression *expr, struct symbol *ctype)
> >  		return 0;
> >  	r = right->value;
> >  	if (expr->op == SPECIAL_LEFTSHIFT || expr->op == SPECIAL_RIGHTSHIFT) {
> > -		if (r >= ctype->bit_size) {
> > +		if (expr->flags & Int_const_expr && r >= ctype->bit_size) {
> 
> Thanks!  I had no idea how to start writing a fix for this, but the test
> should be:
> 		if (expr->right->flags & Int_const_expr
> 
> Otherwise both sides of the shift have to be const.
Thanks - will fix.

I will update the test case to check for this, and
then send a proper patch.

	Sam

Re: [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@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

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

[Josh Triplett]

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

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

[Christopher Li]

On Sat, Jun 28, 2014 at 12:20 PM, Josh Triplett <josh@joshtriplett.org> wrote:
> 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.

Ah, I see. I agree that the warning happen on the dead code.

However, the root cause for the warning is not because sparse can not
figure out the value of "channel". Sparse actually correctly does the
inline and figure out "channel" is 4294967295. The root cause
of your complain against sparse is that it happen on the dead code.

I don' think the patch is the right fix for this problem though.
This patch try to silence a warning purely because it can happen
on dead code, otherwise perfectly valid warning.

My point is that, it can happen on dead code is not a valid reason
to weaken the warning. Sparse can give any possible warning on
dead code. e.g. On the dead code path, you can construct an address
space warning. Do we weakling the address space warning purely
because it can happen on the dead code as well? Of course not.
The same apply to any other sparse warnings.

So if there is a bug, It is sparse did not know exactly which part
of the code is dead on AST level. If you enable the "#if 0" inside
expand_if_statement(), sparse can actually correctly expand
"if (channel < 0)" into "return". Of course, it still can't handle jump
into dead code situation, which is the reason that piece of code
is not enabled in the first place. It needs more work. I think that is
a better approach than just random weakening some warnings
on dead code path.

Chris

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

[Christopher Li]

On Sat, Jun 28, 2014 at 8:09 PM, Christopher Li <sparse@chrisli.org> wrote:
> So if there is a bug, It is sparse did not know exactly which part
> of the code is dead on AST level. If you enable the "#if 0" inside

I am thinking about how to fix the dead code issue. Try to do more
than trivial dead code analyses is really not the right place for AST.

Actually, there is a simple way out. We can just move the warning
to the linearize instruction level.

 $ ./test-linearize /tmp/test_shift.c
/tmp/test_shift.c:8:26: warning: shift too big (4294967295) for type int
main:
.L0x7f8f490e5010:
    <entry-point>
    # call      %r1 <- filter, $0xffffffff
    ret.32      $0

You see, once drop into the instruction level, sparse already
know that code is dead.

Since the sparse is already linearizing instruction to perform
the context check. We just need to move the warning from AST
to instruction level.

The warning should be trivial in instruction level. We are looking
for an instruction has larger than type size constant shift
value.

Comments and feed backs, even patches?

Chris

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

[Christopher Li]

On Mon, Jun 30, 2014 at 10:49 AM, Christopher Li <sparse@chrisli.org> wrote:
> The warning should be trivial in instruction level. We are looking
> for an instruction has larger than type size constant shift
> value.

Actually, just try it. Not as trivial as I thought.

The problem is that, in the instruction level, sparse will optimize away
the shift instruction when it see a larger than type size shift.
The finial instruction level don't see the big constant shift any way.
Need to catch it before the instruction get optimized away.

That is unfortunate. If we check it too early, we can't tell it is in the
dead code. If we check it too late, the instruction itself get optimized
away.

Chris