Re: Possible code defects: macros and precedence

[Julia Lawall <julia.lawall@lip6.fr>]

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