Sparse RFC

Sparse RFC

[Darren Jenkins\]

G'day people,

I have been playing around with the sparse code, and was thinking of
adding a few of the checks that my company uses from other code
checkers, starting with checking that macro arguments have brackets
around them. (Because this is the test my/other code most often fails)

I have put together a quick patch to see what everyone thinks of it. It
is obviously not a finished product but I wanted to see peoples
reactions to this type of thing.

I also have a few questions;

1) Do we want to add a Wdescription switch for each test sparse
performs ? or do we want to group tests under Wstyle Wefficiency
WCodingStandard type group switches ?

2) For the macro argument checking would it be better if I checked for
either a bracket or low precedence operator on both sides of the
argument ? like either a comma or a type of assignment operator? it
would not be a foolproof check then but would seem more sensible to me,
and might be more acceptable to users.

Anyway here is the patch 


diff --git a/lib.c b/lib.c
index 79a5c55..27c9fe1 100644
--- a/lib.c
+++ b/lib.c
@@ -204,6 +204,7 @@ int Waddress_space = 1;
 int Wenum_mismatch = 1;
 int Wdo_while = 1;
 int Wuninitialized = 1;
+int Wmacro_argument_brackets = 1;
 
 int dbg_entry = 0;
 int dbg_dead = 0;
diff --git a/lib.h b/lib.h
index 472624d..2657ce3 100644
--- a/lib.h
+++ b/lib.h
@@ -98,6 +98,7 @@ extern int Wshadow;
 extern int Wcast_truncate;
 extern int Wdo_while;
 extern int Wuninitialized;
+extern int Wmacro_argument_brackets;
 
 extern int dbg_entry;
 extern int dbg_dead;
diff --git a/pre-process.c b/pre-process.c
index afae77a..a04a157 100644
--- a/pre-process.c
+++ b/pre-process.c
@@ -1034,7 +1034,9 @@ static struct token *parse_expansion(str
 		} else if (match_op(token->next, SPECIAL_HASHHASH)) {
 			try_arg(token, TOKEN_QUOTED_ARGUMENT, arglist);
 		} else {
-			try_arg(token, TOKEN_MACRO_ARGUMENT, arglist);
+			int argument = try_arg(token, TOKEN_MACRO_ARGUMENT, arglist);
+			if (argument  == 1 && Wmacro_argument_brackets && (!match_op(token->next, ')') || !match_op(last, '(')) )
+				warning(token->pos, "Macro argument is not bracketed in expansion");
 		}
 		if (token_type(token) == TOKEN_ERROR)
 			goto Earg;
diff --git a/token.h b/token.h
diff --git a/tokenize.c b/tokenize.c

 

Re: Sparse RFC

[Derek M Jones]

Darren,

> I have been playing around with the sparse code, and was thinking of
> adding a few of the checks that my company uses from other code
> checkers, starting with checking that macro arguments have brackets
> around them. (Because this is the test my/other code most often fails)

This is a useful check.  However, you need to make sure that
cases where the presence of () have no impact are not flagged
if the parameter is always used in such contexts.  Otherwise the
noise can be excessive.

> 2) For the macro argument checking would it be better if I checked for
> either a bracket or low precedence operator on both sides of the
> argument ? like either a comma or a type of assignment operator? it
> would not be a foolproof check then but would seem more sensible to me,
> and might be more acceptable to users.

Don't make the assumption that everybody knows the precedence
of binary operators.  An experiment I ran last year with experienced
developers (average over 10 years) threw up some unexpected results.
See: http://www.knosof.co.uk/cbook/accu06a.pdf

-- 
Derek M. Jones                              tel: +44 (0) 1252 520 667
Knowledge Software Ltd                      mailto:derek@knosof.co.uk
Applications Standards Conformance Testing    http://www.knosof.co.uk

Re: Sparse RFC

[Linus Torvalds]

On Mon, 26 Mar 2007, Derek M Jones wrote:
> 
> > I have been playing around with the sparse code, and was thinking of
> > adding a few of the checks that my company uses from other code
> > checkers, starting with checking that macro arguments have brackets
> > around them. (Because this is the test my/other code most often fails)
> 
> This is a useful check.  However, you need to make sure that
> cases where the presence of () have no impact are not flagged
> if the parameter is always used in such contexts.  Otherwise the
> noise can be excessive.

Indeed. In the kernel, for example, we sometimes use nested macros for the 
complex cases, and there one of the cases is where the "outer" or inner 
macro adds all required brackets. Ie something like

	#define ALIGN(x,a)              __ALIGN_MASK(x,(typeof(x))(a)-1)
	#define __ALIGN_MASK(x,mask)    (((x)+(mask))&~(mask))

where ALIGN() uses 'x' without any brackets, because it knows the inner 
macro is safe (and a comma expression must have had any brackets of its 
own, otherwise it wouldn't have parsed as a single macro argument anyway!)

> > 2) For the macro argument checking would it be better if I checked for
> > either a bracket or low precedence operator on both sides of the
> > argument ? like either a comma or a type of assignment operator? it
> > would not be a foolproof check then but would seem more sensible to me,
> > and might be more acceptable to users.
> 
> Don't make the assumption that everybody knows the precedence
> of binary operators.  An experiment I ran last year with experienced
> developers (average over 10 years) threw up some unexpected results.
> See: http://www.knosof.co.uk/cbook/accu06a.pdf

Yeah. The only exception is literally the comma expression, because of the 
syntax of macro expansion itself (ie a comma would break a macro argument, 
unless it is bracketed). Pretty much all other precedence rules can pretty 
much be expected to be confused by somebody.

		Linus

Re: Sparse RFC

[Darren Jenkins]

On 3/27/07, Linus Torvalds <torvalds@linux-foundation.org> wrote:
> On Mon, 26 Mar 2007, Derek M Jones wrote:
>
> > This is a useful check.  However, you need to make sure that
> > cases where the presence of () have no impact are not flagged
> > if the parameter is always used in such contexts.  Otherwise the
> > noise can be excessive.
>
> Indeed. In the kernel, for example, we sometimes use nested macros for the
> complex cases, and there one of the cases is where the "outer" or inner
> macro adds all required brackets. Ie something like
>
>         #define ALIGN(x,a)              __ALIGN_MASK(x,(typeof(x))(a)-1)
>         #define __ALIGN_MASK(x,mask)    (((x)+(mask))&~(mask))
>
> where ALIGN() uses 'x' without any brackets, because it knows the inner
> macro is safe (and a comma expression must have had any brackets of its
> own, otherwise it wouldn't have parsed as a single macro argument anyway!)

Hmm it looks like a 'bracket or comma' check wouid correcly ignore
this particular example.

It is obvious however that I need to think about this a bit more.


> > > 2) For the macro argument checking would it be better if I checked for
> > > either a bracket or low precedence operator on both sides of the
> > > argument ? like either a comma or a type of assignment operator? it
> > > would not be a foolproof check then but would seem more sensible to me,
> > > and might be more acceptable to users.
> >
> > Don't make the assumption that everybody knows the precedence
> > of binary operators.  An experiment I ran last year with experienced
> > developers (average over 10 years) threw up some unexpected results.
> > See: http://www.knosof.co.uk/cbook/accu06a.pdf

Yep I am guilty of getting them wrong before.

> Yeah. The only exception is literally the comma expression, because of the
> syntax of macro expansion itself (ie a comma would break a macro argument,
> unless it is bracketed). Pretty much all other precedence rules can pretty
> much be expected to be confused by somebody.

I didn't look into it but I was thinking that something that evaluated
into a comma, after the initial macro expansion would still be
passible/an issue.



Thanks for the feedback guys. I guess I will keep tinkering with it.

Darren J.

Re: Sparse RFC

[Uwe Kleine-König]

Hi Darren,


Derek M Jones wrote:
> Darren,

> >I have been playing around with the sparse code, and was thinking of
> >adding a few of the checks that my company uses from other code
> >checkers, starting with checking that macro arguments have brackets
> >around them. (Because this is the test my/other code most often fails)

> This is a useful check.  However, you need to make sure that
> cases where the presence of () have no impact are not flagged
> if the parameter is always used in such contexts.  Otherwise the
> noise can be excessive.

> >2) For the macro argument checking would it be better if I checked for
> >either a bracket or low precedence operator on both sides of the
> >argument ? like either a comma or a type of assignment operator? it
> >would not be a foolproof check then but would seem more sensible to me,
> >and might be more acceptable to users.
You have to consider ## and #, too.  E.g. look at
include/linux/stringify.h, or SOCKCALL_WRAP in include/linux/net.h

BTW, SOCKCALL_WRAP is funny anyhow, because the last two arguments are
supposed to be function arguments (resp. their type list).

Best regards
Uwe


-- 
Uwe Kleine-König

http://www.google.com/search?q=5%2B7