diff mbox

Staging/comedi: Fixes static analysis warning raised by sparse

Message ID 20140615193227.GA24515@ravnborg.org (mailing list archive)
State Not Applicable, archived
Headers show

Commit Message

Sam Ravnborg June 15, 2014, 7:32 p.m. UTC 
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




--
To unsubscribe from this list: send the line "unsubscribe linux-sparse" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Dan Carpenter June 16, 2014, 7:40 a.m. UTC | #1 
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
--
To unsubscribe from this list: send the line "unsubscribe linux-sparse" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Sam Ravnborg June 16, 2014, 8:06 a.m. UTC | #2 
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
--
To unsubscribe from this list: send the line "unsubscribe linux-sparse" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

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);