Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1475774 > unrolled thread

Possible code defects: macros and precedence

Back to article view | Back to linux.kernel

Contents

  Possible code defects: macros and precedence Joe Perches <joe@perches.com> - 2016-09-03 20:40 +0200
    Re: Possible code defects: macros and precedence Dan Carpenter <dan.carpenter@oracle.com> - 2016-09-03 22:20 +0200
      [PATCH] checkpatch: Add a --strict test for macro argument reuse and precedence Joe Perches <joe@perches.com> - 2016-09-04 00:30 +0200
        Re: [PATCH] checkpatch: Add a --strict test for macro argument  reuse and precedence Joe Perches <joe@perches.com> - 2016-09-04 16:50 +0200
    Re: Possible code defects: macros and precedence Julia Lawall <julia.lawall@lip6.fr> - 2016-09-04 12:20 +0200
      Re: Possible code defects: macros and precedence Joe Perches <joe@perches.com> - 2016-09-04 17:10 +0200

#1475774 — Possible code defects: macros and precedence

There are many nominally incorrect macro definitions
in linux-kernel source where parentheses are not used
for various macros arguments with calculations.

Does coccinelle or smatch have the ability to detect
potential macro misuse where arguments passed to the
macro are not correctly parenthesized by the macro?

Something like:

	#define A 1
	#define B 2
	#define shift(val) (val << 1)

where a use is:

	int c = shift(A | B)

where the actual result is 5 but the expected result is 6?

Can either tool suggest changing the macro to

	#define shift(val) ((val) << 1)

?

[toc] | [next] | [standalone]

#1475789

No.  I can't think of a way to write a script for that in smatch.  It
works on the pre-processed code.  There is a hack around to tell if code
is inside a macro or not, but you can't tell if code is a macro
parameter.

regards,
dan carpenter

[toc] | [prev] | [next] | [standalone]

#1475799 — [PATCH] checkpatch: Add a --strict test for macro argument reuse and precedence

Add a test for reuse of macro arguments to highlight any possible
side-effects from this reuse.

Avoid this check on token name pasting and when the
argument is used in a typeof or a __builtin.

Add a test for macro arguents that have leading or trailing operators
where the argument isn't parenthesized to avoid possible precedence
issues.

These tests are noisy so make them --strict.

Signed-off-by: Joe Perches <joe@perches.com>
---

This is a slight expansion of a patch I sent a few years back.

https://lkml.org/lkml/2012/11/6/151

This one at least runs.

Maybe it's useful to find some of the possible macro misuses,
but it is _very_ noisy.

 scripts/checkpatch.pl | 47 +++++++++++++++++++++++++++++++++++++++--------
 1 file changed, 39 insertions(+), 8 deletions(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 8946904..921155f 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -4714,7 +4714,17 @@ sub process {
 			$has_flow_statement = 1 if ($ctx =~ /\b(goto|return)\b/);
 			$has_arg_concat = 1 if ($ctx =~ /\#\#/ && $ctx !~ /\#\#\s*(?:__VA_ARGS__|args)\b/);
 
-			$dstat =~ s/^.\s*\#\s*define\s+$Ident(?:\([^\)]*\))?\s*//;
+			$dstat =~ s/^.\s*\#\s*define\s+$Ident(\([^\)]*\))?\s*//;
+			my $define_args = $1;
+			my $define_stmt = $dstat;
+			my @def_args = ();
+
+			if (defined $define_args && $define_args ne "") {
+				$define_args = substr($define_args, 1, length($define_args) - 2);
+				$define_args =~ s/\s*//g;
+				@def_args = split(",", $define_args);
+			}
+
 			$dstat =~ s/$;//g;
 			$dstat =~ s/\\\n.//g;
 			$dstat =~ s/^\s*//s;
@@ -4750,6 +4760,15 @@ sub process {
 				^\[
 			}x;
 			#print "REST<$rest> dstat<$dstat> ctx<$ctx>\n";
+
+			$ctx =~ s/\n*$//;
+			my $herectx = $here . "\n";
+			my $stmt_cnt = statement_rawlines($ctx);
+
+			for (my $n = 0; $n < $stmt_cnt; $n++) {
+				$herectx .= raw_line($linenr, $n) . "\n";
+			}
+
 			if ($dstat ne '' &&
 			    $dstat !~ /^(?:$Ident|-?$Constant),$/ &&			# 10, // foo(),
 			    $dstat !~ /^(?:$Ident|-?$Constant);$/ &&			# foo();
@@ -4765,13 +4784,6 @@ sub process {
 			    $dstat !~ /^\(\{/ &&						# ({...
 			    $ctx !~ /^.\s*#\s*define\s+TRACE_(?:SYSTEM|INCLUDE_FILE|INCLUDE_PATH)\b/)
 			{
-				$ctx =~ s/\n*$//;
-				my $herectx = $here . "\n";
-				my $cnt = statement_rawlines($ctx);
-
-				for (my $n = 0; $n < $cnt; $n++) {
-					$herectx .= raw_line($linenr, $n) . "\n";
-				}
 
 				if ($dstat =~ /;/) {
 					ERROR("MULTISTATEMENT_MACRO_USE_DO_WHILE",
@@ -4780,6 +4792,25 @@ sub process {
 					ERROR("COMPLEX_MACRO",
 					      "Macros with complex values should be enclosed in parentheses\n" . "$herectx");
 				}
+
+			}
+# check if any macro arguments are reused or may have other precedence issues
+			foreach my $arg (@def_args) {
+			        next if ($arg =~ /\.\.\./);
+				my $tmp = $define_stmt;
+				$tmp =~ s/\b(typeof|__typeof__|__builtin\w+|typecheck\s*\(\s*$Type\s*,|\#+)\s*\(*\s*$arg\s*\)*\b//g;
+				$tmp =~ s/\#\#\s*$arg\b//g;
+				$tmp =~ s/\b$arg\s*\#\#//g;
+				my $use_cnt = $tmp =~ s/\b$arg\b//g;
+				if ($use_cnt > 1) {
+					CHK("MACRO_ARG_REUSE",
+					    "Macro argument reuse '$arg' - possible side-effects?\n" . "$herectx");
+				}
+				if ($define_stmt =~ m/\b$arg\b\s*$Operators/m ||
+				    $define_stmt =~ /$Operators\s*$arg\b/m) {
+					CHK("MACRO_ARG_PRECEDENCE",
+					    "Macro argument '$arg' may be better as '($arg)' to avoid precedence issues\n" . "$herectx");
+				}
 			}
 
 # check for macros with flow control, but without ## concatenation
-- 
2.10.0.rc2.1.g053435c

[toc] | [prev] | [next] | [standalone]

#1475941 — Re: [PATCH] checkpatch: Add a --strict test for macro argument reuse and precedence

On Sat, 2016-09-03 at 15:20 -0700, Joe Perches wrote:
> Add a test for reuse of macro arguments to highlight any possible
> side-effects from this reuse.
> 
> Avoid this check on token name pasting and when the
> argument is used in a typeof or a __builtin.
> 
> Add a test for macro arguents that have leading or trailing operators
> where the argument isn't parenthesized to avoid possible precedence
> issues.
> 
> These tests are noisy so make them --strict.

This should have been RFC.  Please do not apply this.

These tests are not just noisy, some false positives it
reports are just silly.

[toc] | [prev] | [next] | [standalone]

#1475906

On Sat, 3 Sep 2016, Joe Perches wrote:

> There are many nominally incorrect macro definitions
> in linux-kernel source where parentheses are not used
> for various macros arguments with calculations.
>
> Does coccinelle or smatch have the ability to detect
> potential macro misuse where arguments passed to the
> macro are not correctly parenthesized by the macro?
>
> Something like:
>
> 	#define A 1
> 	#define B 2
> 	#define shift(val) (val << 1)
>
> where a use is:
>
> 	int c = shift(A | B)
>
> where the actual result is 5 but the expected result is 6?
>
> Can either tool suggest changing the macro to
>
> 	#define shift(val) ((val) << 1)

Coccinelle could do this.  It is possible to match macro parameters, and
it is possible to match binary operators generically.  I can look into it.

julia

[toc] | [prev] | [next] | [standalone]

#1475947

On Sun, 2016-09-04 at 18:10 +0800, Julia Lawall wrote:
> On Sat, 3 Sep 2016, Joe Perches wrote:
> > There are many nominally incorrect macro definitions
> > in linux-kernel source where parentheses are not used
> > for various macros arguments with calculations.
> >
> > Does coccinelle or smatch have the ability to detect
> > potential macro misuse where arguments passed to the
> > macro are not correctly parenthesized by the macro?
> >
> > Something like:
> >
> >       #define A 1
> >       #define B 2
> >       #define shift(val) (val << 1)
> >
> > where a use is:
> >
> >       int c = shift(A | B)
> >
> > where the actual result is 5 but the expected result is 6?
> >
> > Can either tool suggest changing the macro to
> >
> >       #define shift(val) ((val) << 1)
> 
> Coccinelle could do this.  It is possible to match macro parameters, and
> it is possible to match binary operators generically.  I can look into it.

Thanks Julia.

It is not just binary operators though, it is all
operations including dereference where precedence
and associativity operations on the macro argument
might cause an unexpected result.

The possible regex checkpatch rule I sent for this
https://lkml.org/lkml/2016/9/3/271
is _way_ too noisy and stupid.

The $Operator test there includes a comma which
makes the possible macro argument precedence test
output silly.  More work is necessary to make the
checkpatch test more reasonable.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web