Messages in this thread | | | Date | Sun, 18 Sep 2016 06:57:53 +0200 (CEST) | From | Julia Lawall <> | Subject | Re: Possible code defects: macros and precedence |
| |
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. >
| |