Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1485617 > unrolled thread
| Started by | Joe Perches <joe@perches.com> |
|---|---|
| First post | 2016-09-18 00:00 +0200 |
| Last post | 2016-09-18 11:30 +0200 |
| Articles | 3 — 2 participants |
Back to article view | Back to linux.kernel
This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by
below is the oldest one visible, not the original post.
Re: Possible code defects: macros and precedence Joe Perches <joe@perches.com> - 2016-09-18 00:00 +0200
Re: Possible code defects: macros and precedence Julia Lawall <julia.lawall@lip6.fr> - 2016-09-18 07:00 +0200
Re: Possible code defects: macros and precedence Joe Perches <joe@perches.com> - 2016-09-18 11:30 +0200
| From | Joe Perches <joe@perches.com> |
|---|---|
| Date | 2016-09-18 00:00 +0200 |
| Subject | Re: Possible code defects: macros and precedence |
| Message-ID | <sivOv-7RU-23@gated-at.bofh.it> |
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.
[toc] | [next] | [standalone]
| From | Julia Lawall <julia.lawall@lip6.fr> |
|---|---|
| Date | 2016-09-18 07:00 +0200 |
| Message-ID | <siCmR-3xx-1@gated-at.bofh.it> |
| In reply to | #1485617 |
[Multipart message — attachments visible in raw view] — view raw
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.
>
[toc] | [prev] | [next] | [standalone]
| From | Joe Perches <joe@perches.com> |
|---|---|
| Date | 2016-09-18 11:30 +0200 |
| Message-ID | <siGA9-6ms-9@gated-at.bofh.it> |
| In reply to | #1485687 |
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.
[toc] | [prev] | [standalone]
Back to top | Article view | linux.kernel
csiph-web