From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755959AbcIRFJo (ORCPT ); Sun, 18 Sep 2016 01:09:44 -0400 Received: from mail3-relais-sop.national.inria.fr ([192.134.164.104]:16039 "EHLO mail3-relais-sop.national.inria.fr" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751997AbcIRFJl (ORCPT ); Sun, 18 Sep 2016 01:09:41 -0400 X-IronPort-AV: E=Sophos;i="5.30,354,1470693600"; d="scan'208";a="193794213" Date: Sun, 18 Sep 2016 07:09:36 +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: <1474147658.1954.1.camel@perches.com> Message-ID: References: <1472927739.5018.13.camel@perches.com> <1473001581.5018.37.camel@perches.com> <1474147658.1954.1.camel@perches.com> User-Agent: Alpine 2.10 (DEB 1266 2009-07-14) MIME-Version: 1.0 Content-Type: MULTIPART/MIXED; BOUNDARY="8323329-1921938849-1474175378=: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-1921938849-1474175378=: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: > > 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 > 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 > Signed-off-by: Andrew Morton > > From 0158682614df6006752ac932b2e65475384a87b3 Mon Sep 17 00:00:00 2001 > From: Joe Perches > 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 > Cc: Andy Whitcroft > Cc: Julia Lawall > Cc: Dan Carpenter > Signed-off-by: Andrew Morton > > --8323329-1921938849-1474175378=:3361--