Possible code defects: macros and precedence

Possible code defects: macros and precedence

[Joe Perches]

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)

?

Re: Possible code defects: macros and precedence

[Dan Carpenter]

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

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

[Joe Perches]

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

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

[Joe Perches]

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.

Re: Possible code defects: macros and precedence

[Julia Lawall]

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

Re: Possible code defects: macros and precedence

[Joe Perches]

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.

Re: Possible code defects: macros and precedence

[Joe Perches]

On Sat, 2016-09-17 at 22:24 +0200, Julia Lawall wrote:
> diff -u -p a/lib/sha1.c b/lib/sha1.c
[]
> @@ -49,18 +49,18 @@
>   * the input data, the next mix it from the 512-bit array.
>   */
>  #define SHA_SRC(t) get_unaligned_be32((__u32 *)data + t)
> -#define SHA_MIX(t) rol32(W(t+13) ^ W(t+8) ^ W(t+2) ^ W(t), 1)
> +#define SHA_MIX(t) rol32(W((t)+13) ^ W((t)+8) ^ W((t)+2) ^ W(t), 1)
> 
>  #define SHA_ROUND(t, input, fn, constant, A, B, C, D, E) do { \
>         __u32 TEMP = input(t); setW(t, TEMP); \
>         E += TEMP + rol32(A,5) + (fn) + (constant); \
>         B = ror32(B, 2); } while (0)
> 
> -#define T_0_15(t, A, B, C, D, E)  SHA_ROUND(t, SHA_SRC, (((C^D)&B)^D) , 0x5a827999, A, B, C, D, E )
> -#define T_16_19(t, A, B, C, D, E) SHA_ROUND(t, SHA_MIX, (((C^D)&B)^D) , 0x5a827999, A, B, C, D, E )
> -#define T_20_39(t, A, B, C, D, E) SHA_ROUND(t, SHA_MIX, (B^C^D) , 0x6ed9eba1, A, B, C, D, E )
> -#define T_40_59(t, A, B, C, D, E) SHA_ROUND(t, SHA_MIX, ((B&C)+(D&(B^C))) , 0x8f1bbcdc, A, B, C, D, E )
> -#define T_60_79(t, A, B, C, D, E) SHA_ROUND(t, SHA_MIX, (B^C^D) ,  0xca62c1d6, A, B, C, D, E )
> +#define T_0_15(t, A, B, C, D, E)  SHA_ROUND(t, SHA_SRC, ((((C)^D)&B)^D) , 0x5a827999, A, B, C, D, E )
> +#define T_16_19(t, A, B, C, D, E) SHA_ROUND(t, SHA_MIX, ((((C)^D)&B)^D) , 0x5a827999, A, B, C, D, E )
> +#define T_20_39(t, A, B, C, D, E) SHA_ROUND(t, SHA_MIX, ((B)^C^D) , 0x6ed9eba1, A, B, C, D, E )
> +#define T_40_59(t, A, B, C, D, E) SHA_ROUND(t, SHA_MIX, (((B)&C)+((D)&((B)^C))) , 0x8f1bbcdc, A, B, C, D, E )
> +#define T_60_79(t, A, B, C, D, E) SHA_ROUND(t, SHA_MIX, ((B)^C^D) ,  0xca62c1d6, A, B, C, D, E )

I didn't look at much of the patch.

It looks as if the coccinelle code doesn't do the
transformation on each possible macro argument.

In the transform above, only the first referenced arg
is updated with parentheses and subsequent args are not.

Many or most of the uses of these various macros always
pass a single argument to these macros so there isn't a
real possibility of a precedence defect for those uses.

Is it possible to check the macro users for arguments that
might produce precedence defects and only report those
possible defects?

I also submitted a similar checkpatch addition that looks
for non-comma operators used macro arguments in function
definitions.

The checkpatch test has the same weakness as the coccinelle
test. It doesn't check uses, just the macro definition.

Commits in -next:

>From 75bd22fe82d1fd698894e4a5d21e33ffdf7d4492 Mon Sep 17 00:00:00 2001
From: Joe Perches <joe@perches.com>
Date: Thu, 15 Sep 2016 10:29:22 +1000
Subject: [PATCH] checkpatch: improve MACRO_ARG_PRECEDENCE test

It is possible for a multiple line macro definition to have a false positive
report when an argument is used on a line after a continuation \.

This line might have a leading '+' as the initial character that could be
confused by checkpatch as an operator.

Avoid the leading character on multiple line macro definitions.

Link: http://lkml.kernel.org/r/60229d13399f9b6509db5a32e30d4c16951a60cd.1473836073.git.joe@perches.com
Signed-off-by: Joe Perches <joe@perches.com>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>

>From 0158682614df6006752ac932b2e65475384a87b3 Mon Sep 17 00:00:00 2001
From: Joe Perches <joe@perches.com>
Date: Thu, 15 Sep 2016 10:29:22 +1000
Subject: [PATCH] checkpatch: add --strict test for precedence challenged macro arguments

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

Link: http://lkml.kernel.org/r/47715508972f8d786f435e583ff881dbeee3a114.1473745855.git.joe@perches.com
Signed-off-by: Joe Perches <joe@perches.com>
Cc: Andy Whitcroft <apw@canonical.com>
Cc: Julia Lawall <julia.lawall@lip6.fr>
Cc: Dan Carpenter <dan.carpenter@oracle.com>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>

Re: Possible code defects: macros and precedence

[Julia Lawall]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 4675 bytes --]



On Sat, 17 Sep 2016, Joe Perches wrote:

> On Sat, 2016-09-17 at 22:24 +0200, Julia Lawall wrote:
> > diff -u -p a/lib/sha1.c b/lib/sha1.c
> []
> > @@ -49,18 +49,18 @@
> >   * the input data, the next mix it from the 512-bit array.
> >   */
> >  #define SHA_SRC(t) get_unaligned_be32((__u32 *)data + t)
> > -#define SHA_MIX(t) rol32(W(t+13) ^ W(t+8) ^ W(t+2) ^ W(t), 1)
> > +#define SHA_MIX(t) rol32(W((t)+13) ^ W((t)+8) ^ W((t)+2) ^ W(t), 1)
> >
> >  #define SHA_ROUND(t, input, fn, constant, A, B, C, D, E) do { \
> >         __u32 TEMP = input(t); setW(t, TEMP); \
> >         E += TEMP + rol32(A,5) + (fn) + (constant); \
> >         B = ror32(B, 2); } while (0)
> >
> > -#define T_0_15(t, A, B, C, D, E)  SHA_ROUND(t, SHA_SRC, (((C^D)&B)^D) , 0x5a827999, A, B, C, D, E )
> > -#define T_16_19(t, A, B, C, D, E) SHA_ROUND(t, SHA_MIX, (((C^D)&B)^D) , 0x5a827999, A, B, C, D, E )
> > -#define T_20_39(t, A, B, C, D, E) SHA_ROUND(t, SHA_MIX, (B^C^D) , 0x6ed9eba1, A, B, C, D, E )
> > -#define T_40_59(t, A, B, C, D, E) SHA_ROUND(t, SHA_MIX, ((B&C)+(D&(B^C))) , 0x8f1bbcdc, A, B, C, D, E )
> > -#define T_60_79(t, A, B, C, D, E) SHA_ROUND(t, SHA_MIX, (B^C^D) ,  0xca62c1d6, A, B, C, D, E )
> > +#define T_0_15(t, A, B, C, D, E)  SHA_ROUND(t, SHA_SRC, ((((C)^D)&B)^D) , 0x5a827999, A, B, C, D, E )
> > +#define T_16_19(t, A, B, C, D, E) SHA_ROUND(t, SHA_MIX, ((((C)^D)&B)^D) , 0x5a827999, A, B, C, D, E )
> > +#define T_20_39(t, A, B, C, D, E) SHA_ROUND(t, SHA_MIX, ((B)^C^D) , 0x6ed9eba1, A, B, C, D, E )
> > +#define T_40_59(t, A, B, C, D, E) SHA_ROUND(t, SHA_MIX, (((B)&C)+((D)&((B)^C))) , 0x8f1bbcdc, A, B, C, D, E )
> > +#define T_60_79(t, A, B, C, D, E) SHA_ROUND(t, SHA_MIX, ((B)^C^D) ,  0xca62c1d6, A, B, C, D, E )
>
> I didn't look at much of the patch.
>
> It looks as if the coccinelle code doesn't do the
> transformation on each possible macro argument.
>
> In the transform above, only the first referenced arg
> is updated with parentheses and subsequent args are not.

No, actually it looks like only the left argument is getting transformed.
For example, T_40_59 has a term that started as ((B&C)+(D&(B^C))) and ends
up as (((B)&C)+((D)&((B)^C))).  I can fix this.

> Many or most of the uses of these various macros always
> pass a single argument to these macros so there isn't a
> real possibility of a precedence defect for those uses.
>
> Is it possible to check the macro users for arguments that
> might produce precedence defects and only report those
> possible defects?

It could be possible.  Coccinelle is not always able to associate header
files to C files, if the header files are in strange paths, but perhaps
one could also rely on the names, since some names may be unique in the
kernel.

> I also submitted a similar checkpatch addition that looks
> for non-comma operators used macro arguments in function
> definitions.
>
> The checkpatch test has the same weakness as the coccinelle
> test. It doesn't check uses, just the macro definition.

I wonder if it is really a weakness?  Does anyone care if a macro
definition has more parentheses than what is necessary for the current
usage?

julia

> Commits in -next:
>
> From 75bd22fe82d1fd698894e4a5d21e33ffdf7d4492 Mon Sep 17 00:00:00 2001
> From: Joe Perches <joe@perches.com>
> Date: Thu, 15 Sep 2016 10:29:22 +1000
> Subject: [PATCH] checkpatch: improve MACRO_ARG_PRECEDENCE test
>
> It is possible for a multiple line macro definition to have a false positive
> report when an argument is used on a line after a continuation \.
>
> This line might have a leading '+' as the initial character that could be
> confused by checkpatch as an operator.
>
> Avoid the leading character on multiple line macro definitions.
>
> Link: http://lkml.kernel.org/r/60229d13399f9b6509db5a32e30d4c16951a60cd.1473836073.git.joe@perches.com
> Signed-off-by: Joe Perches <joe@perches.com>
> Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
>
> From 0158682614df6006752ac932b2e65475384a87b3 Mon Sep 17 00:00:00 2001
> From: Joe Perches <joe@perches.com>
> Date: Thu, 15 Sep 2016 10:29:22 +1000
> Subject: [PATCH] checkpatch: add --strict test for precedence challenged macro arguments
>
> Add a test for macro arguents that have a non-comma leading or trailing
> operator where the argument isn't parenthesized to avoid possible precedence
> issues.
>
> Link: http://lkml.kernel.org/r/47715508972f8d786f435e583ff881dbeee3a114.1473745855.git.joe@perches.com
> Signed-off-by: Joe Perches <joe@perches.com>
> Cc: Andy Whitcroft <apw@canonical.com>
> Cc: Julia Lawall <julia.lawall@lip6.fr>
> Cc: Dan Carpenter <dan.carpenter@oracle.com>
> Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
>
>

Re: Possible code defects: macros and precedence

[Joe Perches]

On Sun, 2016-09-18 at 07:09 +0200, Julia Lawall wrote:
> On Sat, 17 Sep 2016, Joe Perches wrote:
> > I also submitted a similar checkpatch addition that looks
> > for non-comma operators used macro arguments in function
> > definitions.
> >
> > The checkpatch test has the same weakness as the coccinelle
> > test. It doesn't check uses, just the macro definition.
> I wonder if it is really a weakness?  Does anyone care if a macro
> definition has more parentheses than what is necessary for the current
> usage

An excess of parentheses can hurt readability a little.

Re: Possible code defects: macros and precedence

[Julia Lawall]

The semantic patch below finds a binary operator in a macro and a binary
operator in the use of the macro, and checks if the priority of the
operator in the macro is higher (lower number) than the priority of the
operator in the use.  If this is the case, it adds parentheses in the use,
which is not what one wants, but serves to show where the problem is.

It doesn't turn up anything, except an occurrence of (u32)-1, which
Coccinelle parses as a subtraction, due to not having any nearby evidence
that u32 is a type.

I didn't make any special effort on the include files, which means that
only local include files and ones with the same name as the C file are
taken into account.  I can try with more aggressive include options.

This only works with the github version of Coccinelle, as it required
quite a lot of improvement to the treatmern of #define.

julia

@initialize:ocaml@
@@

let binoptbl =
    [("*",3);("/",3);("%",3);
      ("+",4);("-",4);
      ("<<",5);(">>",5);
      ("<",6);(">",6);("<=",6);(">=",6);
      ("==",7);("!=",7);
      ("&",8);
      ("^",9);
      ("|",10);
      ("&&",11);
      ("||",12)]

@r@
identifier i,j;
identifier list[n] is;
binary operator b;
expression e;
@@

#define i(is,j,...) (<+... \(j b e \| e b j\) ...+>)

@s@
identifier r.i;
expression list[r.n] es;
binary operator b1;
expression e1,e2;
position p;
@@

 i@p(es,e1 b1 e2,...)

@script:ocaml@
_p << s.p;
b << r.b;
b1 << s.b1;
@@

try
  let p1 = List.assoc b binoptbl in
  let p2 = List.assoc b1 binoptbl in
  if p1 >= p2 then Coccilib.include_match false
with Not_found -> ()

@@
identifier r.i;
expression list[r.n] es;
expression e;
position s.p;
@@

i@p(es,
+(
e
+)
,...)

Re: Possible code defects: macros and precedence

[Joe Perches]

On Tue, 2016-09-20 at 15:14 +0200, Julia Lawall wrote:
> The semantic patch below finds a binary operator in a macro and a binary
> operator in the use of the macro, and checks if the priority of the
> operator in the macro is higher (lower number) than the priority of the
> operator in the use.  If this is the case, it adds parentheses in the use,
> which is not what one wants, but serves to show where the problem is

Thanks, this works on the trivial example I suggested
without an #include

I've tried it on trivial files with --recursive-includes
and it seems to work there too.

I'll investigate some more convoluted uses later.

btw: There are ~15K checkpatch MACRO_ARG_PRECEDENCE
messages in the -next kernel tree.  That's probably
too many for a theoretical and likely not an actual
problem.

cheers, Joe

Re: Possible code defects: macros and precedence

[Joe Perches]

On Tue, 2016-09-20 at 10:07 -0700, Joe Perches wrote:
> On Tue, 2016-09-20 at 15:14 +0200, Julia Lawall wrote:
> > The semantic patch below finds a binary operator in a macro and a binary
> > operator in the use of the macro, and checks if the priority of the
> > operator in the macro is higher (lower number) than the priority of the
> > operator in the use.  If this is the case, it adds parentheses in the use,
> > which is not what one wants, but serves to show where the problem is

> Thanks, this works on the trivial example I suggested
> without an #include
> 
> I've tried it on trivial files with --recursive-includes
> and it seems to work there too.

I tried it on drivers/net with --recursive-includes and got
just 1 hit on an old and probably relatively untested driver.

No hardware, can't test.  It may be correct now.  Who knows...
---
diff --urN a/drivers/net/ethernet/smsc/smc911x.h b/drivers/net/ethernet/smsc/smc911x.h
--- a/drivers/net/ethernet/smsc/smc911x.h
+++ b/drivers/net/ethernet/smsc/smc911x.h
@@ -700,8 +700,8 @@ static const struct chip_id chip_ids[] =
  * capabilities.  Please use those and not the in/out primitives.
  */
 /* FIFO read/write macros */
-#define SMC_PUSH_DATA(lp, p, l)	SMC_outsl( lp, TX_DATA_FIFO, p, (l) >> 2 )
-#define SMC_PULL_DATA(lp, p, l)	SMC_insl ( lp, RX_DATA_FIFO, p, (l) >> 2 )
+#define SMC_PUSH_DATA(lp, p, l)	SMC_outsl( lp, TX_DATA_FIFO, p, ((l) >> 2) )
+#define SMC_PULL_DATA(lp, p, l)	SMC_insl ( lp, RX_DATA_FIFO, p, ((l) >> 2) )
 #define SMC_SET_TX_FIFO(lp, x) 	SMC_outl( x, lp, TX_DATA_FIFO )
 #define SMC_GET_RX_FIFO(lp)	SMC_inl( lp, RX_DATA_FIFO )
 

Re: Possible code defects: macros and precedence

[Joe Perches]

On Tue, 2016-09-20 at 15:14 +0200, Julia Lawall wrote:
> The semantic patch below finds a binary operator in a macro and a binary
> operator in the use of the macro, and checks if the priority of the
> operator in the macro is higher (lower number) than the priority of the
> operator in the use.  If this is the case, it adds parentheses in the use,
> which is not what one wants, but serves to show where the problem is.
> 
> It doesn't turn up anything, except an occurrence of (u32)-1, which
> Coccinelle parses as a subtraction, due to not having any nearby evidence
> that u32 is a type.
> 
> I didn't make any special effort on the include files, which means that
> only local include files and ones with the same name as the C file are
> taken into account.  I can try with more aggressive include options.
> 
> This only works with the github version of Coccinelle, as it required
> quite a lot of improvement to the treatmern of #define.
> 
> julia
> 
> @initialize:ocaml@
> @@
> 
> let binoptbl =
>     [("*",3);("/",3);("%",3);

Shouldn't bitwise negation (~) and not (!) be added at 3?

	("~",3);("!",3);
 
>       ("+",4);("-",4);
>       ("<<",5);(">>",5);
>       ("<",6);(">",6);("<=",6);(">=",6);
>       ("==",7);("!=",7);
>       ("&",8);
>       ("^",9);
>       ("|",10);
>       ("&&",11);
>       ("||",12)]
> 
> @r@
> identifier i,j;
> identifier list[n] is;
> binary operator b;
> expression e;
> @@
> 
> #define i(is,j,...) (<+... \(j b e \| e b j\) ...+>)
> 
> @s@
> identifier r.i;
> expression list[r.n] es;
> binary operator b1;
> expression e1,e2;
> position p;
> @@
> 
> >  i@p(es,e1 b1 e2,...)
> 
> @script:ocaml@
> _p << s.p;
> b << r.b;
> b1 << s.b1;
> @@
> 
> try
>   let p1 = List.assoc b binoptbl in
>   let p2 = List.assoc b1 binoptbl in
>   if p1 >= p2 then Coccilib.include_match false
> with Not_found -> ()
> 
> @@
> identifier r.i;
> expression list[r.n] es;
> expression e;
> position s.p;
> @@
> 
> i@p(es,
> +(
> e
> +)
> ,...)
> 

Re: Possible code defects: macros and precedence

[Julia Lawall]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 2299 bytes --]



On Tue, 20 Sep 2016, Joe Perches wrote:

> On Tue, 2016-09-20 at 15:14 +0200, Julia Lawall wrote:
> > The semantic patch below finds a binary operator in a macro and a binary
> > operator in the use of the macro, and checks if the priority of the
> > operator in the macro is higher (lower number) than the priority of the
> > operator in the use.  If this is the case, it adds parentheses in the use,
> > which is not what one wants, but serves to show where the problem is.
> >
> > It doesn't turn up anything, except an occurrence of (u32)-1, which
> > Coccinelle parses as a subtraction, due to not having any nearby evidence
> > that u32 is a type.
> >
> > I didn't make any special effort on the include files, which means that
> > only local include files and ones with the same name as the C file are
> > taken into account.  I can try with more aggressive include options.
> >
> > This only works with the github version of Coccinelle, as it required
> > quite a lot of improvement to the treatmern of #define.
> >
> > julia
> >
> > @initialize:ocaml@
> > @@
> >
> > let binoptbl =
> >     [("*",3);("/",3);("%",3);
>
> Shouldn't bitwise negation (~) and not (!) be added at 3?
>
> 	("~",3);("!",3);

My semantic patch only covers binary operators at the moment, so I didn't
put the complete table.

I can extend it.

julia

>  
> >       ("+",4);("-",4);
> >       ("<<",5);(">>",5);
> >       ("<",6);(">",6);("<=",6);(">=",6);
> >       ("==",7);("!=",7);
> >       ("&",8);
> >       ("^",9);
> >       ("|",10);
> >       ("&&",11);
> >       ("||",12)]
> >
> > @r@
> > identifier i,j;
> > identifier list[n] is;
> > binary operator b;
> > expression e;
> > @@
> >
> > #define i(is,j,...) (<+... \(j b e \| e b j\) ...+>)
> >
> > @s@
> > identifier r.i;
> > expression list[r.n] es;
> > binary operator b1;
> > expression e1,e2;
> > position p;
> > @@
> >
> > >  i@p(es,e1 b1 e2,...)
> >
> > @script:ocaml@
> > _p << s.p;
> > b << r.b;
> > b1 << s.b1;
> > @@
> >
> > try
> >   let p1 = List.assoc b binoptbl in
> >   let p2 = List.assoc b1 binoptbl in
> >   if p1 >= p2 then Coccilib.include_match false
> > with Not_found -> ()
> >
> > @@
> > identifier r.i;
> > expression list[r.n] es;
> > expression e;
> > position s.p;
> > @@
> >
> > i@p(es,
> > +(
> > e
> > +)
> > ,...)
> >
>

Re: Possible code defects: macros and precedence

[Joe Perches]

On Sat, 2016-09-17 at 22:24 +0200, Julia Lawall wrote:

(A 2.2MB message that (perhaps thankfully) didn't get through to lkml)

> Below is the Coccinelle output for the case where the definition of the
> macro is a single expression.  There is also the case where it is a
> sequence of statements, but that finds very few results.  Note that
> Coccinelle will only match code that it can parse, which is definitely not
> always the case for macros, so some things may be missed.
> 
> There are a huge number of results.  To the extent that you have the
> patience to look through them, do you see anything undesirable?
> 
> thanks,
> julia
> 
> diff -u -p a/lib/lz4/lz4defs.h b/lib/lz4/lz4defs.h
> --- a/lib/lz4/lz4defs.h
> +++ b/lib/lz4/lz4defs.h
> @@ -34,7 +34,7 @@ typedef struct _U64_S { u64 v; } U64_S;
>  #define PUT8(s, d) (A64(d) = A64(s))
> 
>  #define LZ4_READ_LITTLEENDIAN_16(d, s, p)      \
> -       (d = s - A16(p))
> +       (d = (s) - A16(p))
> 
>  #define LZ4_WRITE_LITTLEENDIAN_16(p, v)        \
>         do {    \
> @@ -53,7 +53,7 @@ typedef struct _U64_S { u64 v; } U64_S;
>         put_unaligned(get_unaligned((const u64 *) s), (u64 *) d)
> 
>  #define LZ4_READ_LITTLEENDIAN_16(d, s, p)      \
> -       (d = s - get_unaligned_le16(p))
> +       (d = (s) - get_unaligned_le16(p))
> 
>  #define LZ4_WRITE_LITTLEENDIAN_16(p, v)                        \
>         do {                                            \

Here's the equivalent checkpatch output for that file.
It has a few more instances.
Is what checkpatch suggests unreasonable?

$ ./scripts/checkpatch.pl -f --strict lib/lz4/lz4defs.h --types=macro_arg_precedence
CHECK: Macro argument 's' may be better as '(s)' to avoid precedence issues
#36: FILE: lib/lz4/lz4defs.h:36:
+#define LZ4_READ_LITTLEENDIAN_16(d, s, p)	\
+	(d = s - A16(p))

CHECK: Macro argument 's' may be better as '(s)' to avoid precedence issues
#55: FILE: lib/lz4/lz4defs.h:55:
+#define LZ4_READ_LITTLEENDIAN_16(d, s, p)	\
+	(d = s - get_unaligned_le16(p))

CHECK: Macro argument 'd' may be better as '(d)' to avoid precedence issues
#106: FILE: lib/lz4/lz4defs.h:106:
+#define LZ4_SECURECOPY(s, d, e)			\
+	do {					\
+		if (d < e) {			\
+			LZ4_WILDCOPY(s, d, e);	\
+		}				\
+	} while (0)

CHECK: Macro argument 'e' may be better as '(e)' to avoid precedence issues
#106: FILE: lib/lz4/lz4defs.h:106:
+#define LZ4_SECURECOPY(s, d, e)			\
+	do {					\
+		if (d < e) {			\
+			LZ4_WILDCOPY(s, d, e);	\
+		}				\
+	} while (0)

CHECK: Macro argument 'e' may be better as '(e)' to avoid precedence issues
#147: FILE: lib/lz4/lz4defs.h:147:
+#define LZ4_WILDCOPY(s, d, e)		\
+	do {				\
+		LZ4_COPYPACKET(s, d);	\
+	} while (d < e)

CHECK: Macro argument 'l' may be better as '(l)' to avoid precedence issues
#152: FILE: lib/lz4/lz4defs.h:152:
+#define LZ4_BLINDCOPY(s, d, l)	\
+	do {	\
+		u8 *e = (d) + l;	\
+		LZ4_WILDCOPY(s, d, e);	\
+		d = e;	\
+	} while (0)

total: 0 errors, 0 warnings, 6 checks, 157 lines checked

NOTE: For some of the reported defects, checkpatch may be able to
      mechanically convert to the typical style using --fix or --fix-inplace.

lib/lz4/lz4defs.h has style problems, please review.

NOTE: Used message types: MACRO_ARG_PRECEDENCE

NOTE: If any of the errors are false positives, please report
      them to the maintainer, see CHECKPATCH in MAINTAINERS.

Re: Possible code defects: macros and precedence

[Julia Lawall]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 4031 bytes --]



On Sat, 17 Sep 2016, Joe Perches wrote:

> On Sat, 2016-09-17 at 22:24 +0200, Julia Lawall wrote:
>
> (A 2.2MB message that (perhaps thankfully) didn't get through to lkml)
>
> > Below is the Coccinelle output for the case where the definition of the
> > macro is a single expression.  There is also the case where it is a
> > sequence of statements, but that finds very few results.  Note that
> > Coccinelle will only match code that it can parse, which is definitely not
> > always the case for macros, so some things may be missed.
> >
> > There are a huge number of results.  To the extent that you have the
> > patience to look through them, do you see anything undesirable?
> >
> > thanks,
> > julia
> >
> > diff -u -p a/lib/lz4/lz4defs.h b/lib/lz4/lz4defs.h
> > --- a/lib/lz4/lz4defs.h
> > +++ b/lib/lz4/lz4defs.h
> > @@ -34,7 +34,7 @@ typedef struct _U64_S { u64 v; } U64_S;
> >  #define PUT8(s, d) (A64(d) = A64(s))
> >
> >  #define LZ4_READ_LITTLEENDIAN_16(d, s, p)      \
> > -       (d = s - A16(p))
> > +       (d = (s) - A16(p))
> >
> >  #define LZ4_WRITE_LITTLEENDIAN_16(p, v)        \
> >         do {    \
> > @@ -53,7 +53,7 @@ typedef struct _U64_S { u64 v; } U64_S;
> >         put_unaligned(get_unaligned((const u64 *) s), (u64 *) d)
> >
> >  #define LZ4_READ_LITTLEENDIAN_16(d, s, p)      \
> > -       (d = s - get_unaligned_le16(p))
> > +       (d = (s) - get_unaligned_le16(p))
> >
> >  #define LZ4_WRITE_LITTLEENDIAN_16(p, v)                        \
> >         do {                                            \
>
> Here's the equivalent checkpatch output for that file.
> It has a few more instances.
> Is what checkpatch suggests unreasonable?

Not as far as I can see.  As I mentioned, Coccinelle will only process
what it can parse.  A do ... while with no semicolon at the end is not
correct C (even though it is completely appropriate in the context of a
macro).  Actually, I thought we did something for this case, but maybe it
is not being parsed as what my rule matches.

You did say that checkpatch was giving a lot of noise.  In the end, is it
actually just that there are a lot of changes to make?

julia


> $ ./scripts/checkpatch.pl -f --strict lib/lz4/lz4defs.h --types=macro_arg_precedence
> CHECK: Macro argument 's' may be better as '(s)' to avoid precedence issues
> #36: FILE: lib/lz4/lz4defs.h:36:
> +#define LZ4_READ_LITTLEENDIAN_16(d, s, p)	\
> +	(d = s - A16(p))
>
> CHECK: Macro argument 's' may be better as '(s)' to avoid precedence issues
> #55: FILE: lib/lz4/lz4defs.h:55:
> +#define LZ4_READ_LITTLEENDIAN_16(d, s, p)	\
> +	(d = s - get_unaligned_le16(p))
>
> CHECK: Macro argument 'd' may be better as '(d)' to avoid precedence issues
> #106: FILE: lib/lz4/lz4defs.h:106:
> +#define LZ4_SECURECOPY(s, d, e)			\
> +	do {					\
> +		if (d < e) {			\
> +			LZ4_WILDCOPY(s, d, e);	\
> +		}				\
> +	} while (0)
>
> CHECK: Macro argument 'e' may be better as '(e)' to avoid precedence issues
> #106: FILE: lib/lz4/lz4defs.h:106:
> +#define LZ4_SECURECOPY(s, d, e)			\
> +	do {					\
> +		if (d < e) {			\
> +			LZ4_WILDCOPY(s, d, e);	\
> +		}				\
> +	} while (0)
>
> CHECK: Macro argument 'e' may be better as '(e)' to avoid precedence issues
> #147: FILE: lib/lz4/lz4defs.h:147:
> +#define LZ4_WILDCOPY(s, d, e)		\
> +	do {				\
> +		LZ4_COPYPACKET(s, d);	\
> +	} while (d < e)
>
> CHECK: Macro argument 'l' may be better as '(l)' to avoid precedence issues
> #152: FILE: lib/lz4/lz4defs.h:152:
> +#define LZ4_BLINDCOPY(s, d, l)	\
> +	do {	\
> +		u8 *e = (d) + l;	\
> +		LZ4_WILDCOPY(s, d, e);	\
> +		d = e;	\
> +	} while (0)
>
> total: 0 errors, 0 warnings, 6 checks, 157 lines checked
>
> NOTE: For some of the reported defects, checkpatch may be able to
>       mechanically convert to the typical style using --fix or --fix-inplace.
>
> lib/lz4/lz4defs.h has style problems, please review.
>
> NOTE: Used message types: MACRO_ARG_PRECEDENCE
>
> NOTE: If any of the errors are false positives, please report
>       them to the maintainer, see CHECKPATCH in MAINTAINERS.
>

Re: Possible code defects: macros and precedence

[Joe Perches]

On Sun, 2016-09-18 at 06:57 +0200, Julia Lawall wrote:
> You did say that checkpatch was giving a lot of noise.  In the end, is it
> actually just that there are a lot of changes to make

Actually, the first attempt used just the checkpatch $Operators
pattern match which also matches a comma so there were a lot
of false positives.

This current version specifically excludes commas so there
seem to be far fewer or nearly no false positives.

There are a _lot_ of matches though.