From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755801AbcIRE6C (ORCPT ); Sun, 18 Sep 2016 00:58:02 -0400 Received: from mail2-relais-roc.national.inria.fr ([192.134.164.83]:8915 "EHLO mail2-relais-roc.national.inria.fr" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752526AbcIRE56 (ORCPT ); Sun, 18 Sep 2016 00:57:58 -0400 X-IronPort-AV: E=Sophos;i="5.30,354,1470693600"; d="scan'208";a="237241645" Date: Sun, 18 Sep 2016 06:57:53 +0200 (CEST) From: Julia Lawall X-X-Sender: jll@hadrien To: Joe Perches cc: Dan Carpenter , LKML Subject: Re: Possible code defects: macros and precedence In-Reply-To: <1474149472.1954.6.camel@perches.com> Message-ID: References: <1472927739.5018.13.camel@perches.com> <1473001581.5018.37.camel@perches.com> <1474149472.1954.6.camel@perches.com> User-Agent: Alpine 2.10 (DEB 1266 2009-07-14) MIME-Version: 1.0 Content-Type: MULTIPART/MIXED; BOUNDARY="8323329-1896116547-1474174674=:3361" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org This message is in MIME format. The first part should be readable text, while the remaining parts are likely unreadable without MIME-aware tools. --8323329-1896116547-1474174674=:3361 Content-Type: TEXT/PLAIN; charset=ISO-8859-1 Content-Transfer-Encoding: 8BIT 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. > --8323329-1896116547-1474174674=:3361--