Re: [Cocci] coccinelle: Convert comma to semicolons (was Re: [PATCH] checkpatch: Add test for comma use that should be semicolon)
On Thu, Sep 24 2020 at 13:33, Joe Perches wrote: > On Thu, 2020-09-24 at 22:19 +0200, Thomas Gleixner wrote: >> On Sat, Aug 22 2020 at 09:07, Julia Lawall wrote: >> > On Fri, 21 Aug 2020, Joe Perches wrote: >> > > True enough for a general statement, though the coccinelle >> > > script Julia provided does not change a single instance of >> > > for loop expressions with commas. >> > > >> > > As far as I can tell, no logic defect is introduced by the >> > > script at all. >> > >> > The script has a rule to ensure that what is changed is part of a top >> > level statement that has the form e1, e2;. I put that in to avoid >> > transforming cases where the comma is the body of a macro, but it protects >> > against for loop headers as well. >> >> Right. I went through the lot and did not find something dodgy. Except >> for two hunks this still applies. Can someone please send a proper patch >> with changelog/SOB etc. for this? > > Treewide? > > Somebody no doubt would complain, but there > _really should_ be some mechanism for these > trivial and correct treewide changes... There are lots of mechanisms: - Andrew picks such changes up - With a few competent eyeballs on it (reviewers) this can go thorugh the trivial tree as well. It's more than obvious after all. - Send the script to Linus with a proper change log attached and ask him to run it. - In the worst case if nobody feels responsible, I'll take care. All of the above is better than trying to get the attention of a gazillion of maintainters. Thanks, tglx ___ Cocci mailing list Cocci@systeme.lip6.fr https://systeme.lip6.fr/mailman/listinfo/cocci
Re: [Cocci] coccinelle: Convert comma to semicolons (was Re: [PATCH] checkpatch: Add test for comma use that should be semicolon)
On Thu, 2020-09-24 at 22:19 +0200, Thomas Gleixner wrote: > On Sat, Aug 22 2020 at 09:07, Julia Lawall wrote: > > On Fri, 21 Aug 2020, Joe Perches wrote: > > > True enough for a general statement, though the coccinelle > > > script Julia provided does not change a single instance of > > > for loop expressions with commas. > > > > > > As far as I can tell, no logic defect is introduced by the > > > script at all. > > > > The script has a rule to ensure that what is changed is part of a top > > level statement that has the form e1, e2;. I put that in to avoid > > transforming cases where the comma is the body of a macro, but it protects > > against for loop headers as well. > > Right. I went through the lot and did not find something dodgy. Except > for two hunks this still applies. Can someone please send a proper patch > with changelog/SOB etc. for this? Treewide? Somebody no doubt would complain, but there _really should_ be some mechanism for these trivial and correct treewide changes... ___ Cocci mailing list Cocci@systeme.lip6.fr https://systeme.lip6.fr/mailman/listinfo/cocci
Re: [Cocci] coccinelle: Convert comma to semicolons (was Re: [PATCH] checkpatch: Add test for comma use that should be semicolon)
On Thu, 24 Sep 2020, Thomas Gleixner wrote: > On Sat, Aug 22 2020 at 09:07, Julia Lawall wrote: > > On Fri, 21 Aug 2020, Joe Perches wrote: > >> True enough for a general statement, though the coccinelle > >> script Julia provided does not change a single instance of > >> for loop expressions with commas. > >> > >> As far as I can tell, no logic defect is introduced by the > >> script at all. > > > > The script has a rule to ensure that what is changed is part of a top > > level statement that has the form e1, e2;. I put that in to avoid > > transforming cases where the comma is the body of a macro, but it protects > > against for loop headers as well. > > Right. I went through the lot and did not find something dodgy. Except > for two hunks this still applies. Can someone please send a proper patch > with changelog/SOB etc. for this? > > And of course that script really wants to be part of the kernel cocci > checks to catch further instances. I will try to get to it soon. Thanks for checking all the cases. julia > > Thanks, > > tglx > ___ Cocci mailing list Cocci@systeme.lip6.fr https://systeme.lip6.fr/mailman/listinfo/cocci
Re: [Cocci] coccinelle: Convert comma to semicolons (was Re: [PATCH] checkpatch: Add test for comma use that should be semicolon)
On Sat, Aug 22 2020 at 09:07, Julia Lawall wrote: > On Fri, 21 Aug 2020, Joe Perches wrote: >> True enough for a general statement, though the coccinelle >> script Julia provided does not change a single instance of >> for loop expressions with commas. >> >> As far as I can tell, no logic defect is introduced by the >> script at all. > > The script has a rule to ensure that what is changed is part of a top > level statement that has the form e1, e2;. I put that in to avoid > transforming cases where the comma is the body of a macro, but it protects > against for loop headers as well. Right. I went through the lot and did not find something dodgy. Except for two hunks this still applies. Can someone please send a proper patch with changelog/SOB etc. for this? And of course that script really wants to be part of the kernel cocci checks to catch further instances. Thanks, tglx ___ Cocci mailing list Cocci@systeme.lip6.fr https://systeme.lip6.fr/mailman/listinfo/cocci
Re: [Cocci] [PATCH] coccinelle: iterators: Add for_each_child.cocci script
On Thu, 24 Sep 2020, Markus Elfring wrote: > > +@ruletwo depends on patch && !context && !org && !report@ > > How do you think about to combine code from two SmPL rules > by using another SmPL disjunction like the following? What is the goal of doing this? It seems substantially harder to understand than three rules that each take care of a specific case. julia > > @addition_rule depends on patch && !context && !org && !report@ > local idexpression r.n; > expression e,e1; > expression list [r.n1] es; > iterator r.i,i1,i2; > statement S,S2; > @@ > ( > i(es,n,...) { > ... > (of_node_put(n); > |e = n > |return n; > |i1(...,n,...) S > | > +of_node_put(n); > ?return ...; > ) > ... when any > } > | > i(es,n,...) { > ... > (of_node_put(n); > |e = n > |i1(...,n,...) S > | > +of_node_put(n); > ?break; > ) > ... when any > } > ... when != n > when strict > (n = e1; > | > ?i2(...,n,...) S2 > ) > ) > > > Regards, > Markus > ___ > Cocci mailing list > Cocci@systeme.lip6.fr > https://systeme.lip6.fr/mailman/listinfo/cocci > ___ Cocci mailing list Cocci@systeme.lip6.fr https://systeme.lip6.fr/mailman/listinfo/cocci
Re: [Cocci] [PATCH] coccinelle: iterators: Add for_each_child.cocci script
> +@ruletwo depends on patch && !context && !org && !report@ How do you think about to combine code from two SmPL rules by using another SmPL disjunction like the following? @addition_rule depends on patch && !context && !org && !report@ local idexpression r.n; expression e,e1; expression list [r.n1] es; iterator r.i,i1,i2; statement S,S2; @@ ( i(es,n,...) { ... (of_node_put(n); |e = n |return n; |i1(...,n,...) S | +of_node_put(n); ?return ...; ) ... when any } | i(es,n,...) { ... (of_node_put(n); |e = n |i1(...,n,...) S | +of_node_put(n); ?break; ) ... when any } ... when != n when strict (n = e1; | ?i2(...,n,...) S2 ) ) Regards, Markus ___ Cocci mailing list Cocci@systeme.lip6.fr https://systeme.lip6.fr/mailman/listinfo/cocci
[Cocci] [PATCH] Coccinelle: api: Add SmPL script “use_platform_get_irq.cocci”
From: Markus Elfring Date: Wed, 23 Sep 2020 18:30:25 +0200 A wrapper function is available since the commit 7723f4c5ecdb8d832f049f8483beb0d1081cedf6 ("driver core: platform: Add an error message to platform_get_irq*()"). Provide design options for the adjustment of affected source code by the means of the semantic patch language (Coccinelle software). Signed-off-by: Markus Elfring --- .../coccinelle/api/use_platform_get_irq.cocci | 107 ++ 1 file changed, 107 insertions(+) create mode 100644 scripts/coccinelle/api/use_platform_get_irq.cocci diff --git a/scripts/coccinelle/api/use_platform_get_irq.cocci b/scripts/coccinelle/api/use_platform_get_irq.cocci new file mode 100644 index ..0a0b27a3cd1a --- /dev/null +++ b/scripts/coccinelle/api/use_platform_get_irq.cocci @@ -0,0 +1,107 @@ +// SPDX-License-Identifier: GPL-2.0 +/// Simplify a function call combination by using a known wrapper function. +// +// Keywords: wrapper function conversion IRQ resources +// Confidence: High +// Options: --no-includes --include-headers + +virtual context, patch, report, org + +@depends on context@ +expression device, error_code, index, irq, resource, x; +identifier ri; +type t; +@@ +( +*resource = platform_get_resource(device, IORESOURCE_IRQ, index); +*if (resource == NULL) +*{ +* dev_err(&device->dev, ...); +* return error_code; +*} + irq = +* resource->start + ; +| +*t ri = platform_get_resource(device, IORESOURCE_IRQ, index); + ... when != ri + when != device = x +*if (ri == NULL) +*{ +* dev_err(&device->dev, ...); +* return error_code; +*} + irq = +* ri->start + ; +) + +@depends on patch@ +expression device, error_code, index, irq, resource, x; +identifier ri; +type t; +@@ +( +-resource = platform_get_resource(device, IORESOURCE_IRQ, index); +-if (resource == NULL) +-{ +- dev_err(&device->dev, ...); +- return error_code; +-} + irq = +- resource->start ++ platform_get_irq(device, index) + ; ++if (irq < 0) ++ return irq; +| +-t ri = platform_get_resource(device, IORESOURCE_IRQ, index); + ... when != ri + when != device = x +-if (ri == NULL) +-{ +- dev_err(&device->dev, ...); +- return error_code; +-} + irq = +- ri->start ++ platform_get_irq(device, index) + ; ++if (irq < 0) ++ return irq; +) + +@or depends on org || report@ +expression device, error_code, index, irq, resource, x; +identifier ri; +position p; +type t; +@@ +(resource = platform_get_resource(device, IORESOURCE_IRQ, index); + if (resource == NULL) + { +dev_err(&device->dev, ...); +return error_code; + } + irq = resource->start@p; +| + t ri = platform_get_resource(device, IORESOURCE_IRQ, index); + ... when != ri + when != device = x + if (ri == NULL) + { +dev_err(&device->dev, ...); +return error_code; + } + irq = ri->start@p; +) + +@script:python depends on org@ +p << or.p; +@@ +coccilib.org.print_todo(p[0], "WARNING: opportunity for platform_get_irq()") + +@script:python depends on report@ +p << or.p; +@@ +coccilib.report.print_report(p[0], "WARNING: opportunity for platform_get_irq()") -- 2.28.0 ___ Cocci mailing list Cocci@systeme.lip6.fr https://systeme.lip6.fr/mailman/listinfo/cocci
Re: [Cocci] [PATCH] coccinelle: iterators: Add for_each_child.cocci script
> +// Confidence: High Will an addition like the following be helpful? +// Options: --no-includes --include-headers > +@r@ > +local idexpression n; > +expression e1,e2; > +iterator name for_each_node_by_name, for_each_node_by_type, > +for_each_compatible_node, for_each_matching_node, > +for_each_matching_node_and_match, for_each_child_of_node, > +for_each_available_child_of_node, for_each_node_with_property; > +iterator i; > +statement S; > +expression list [n1] es; > +@@ > + > +( > +( > +for_each_node_by_name(n,e1) S > +| > +for_each_node_by_type(n,e1) S > +| > +for_each_compatible_node(n,e1,e2) S > +| > +for_each_matching_node(n,e1) S > +| > +for_each_matching_node_and_match(n,e1,e2) S > +| > +for_each_child_of_node(e1,n) S > +| > +for_each_available_child_of_node(e1,n) S > +| > +for_each_node_with_property(n,e1) S > +) > +& > +i(es,n,...) S > +) How do you think about to use the following SmPL code variant? @r@ local idexpression n; expression e1, e2; expression list [n1] es; iterator name for_each_node_by_name, for_each_node_by_type, for_each_node_with_property, for_each_matching_node, for_each_matching_node_and_match, for_each_compatible_node, for_each_child_of_node, for_each_available_child_of_node; iterator i; statement S; @@ ( (for_each_node_by_name(n,e1) S |for_each_node_by_type(n,e1) S |for_each_matching_node(n,e1) S |for_each_node_with_property(n,e1) S |for_each_compatible_node(n,e1,e2) S |for_each_matching_node_and_match(n,e1,e2) S |for_each_child_of_node(e1,n) S |for_each_available_child_of_node(e1,n) S ) &i(es,n,...) S ) Regards, Markus ___ Cocci mailing list Cocci@systeme.lip6.fr https://systeme.lip6.fr/mailman/listinfo/cocci
[Cocci] [PATCH] coccinelle: api/atomic_as_refcounter: Improve verbosity
When atomic_as_refcounter script is used by 0day CI and the findings are automatically reported, the message that it gives is confusing to people. I.e. it does not actually state what should be done. Add more information to the message output. Reported-by: Dan Carpenter Signed-off-by: Elena Reshetova --- scripts/coccinelle/api/atomic_as_refcounter.cocci | 12 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/scripts/coccinelle/api/atomic_as_refcounter.cocci b/scripts/coccinelle/api/atomic_as_refcounter.cocci index 0f78d94abc35..0ce7618479fd 100644 --- a/scripts/coccinelle/api/atomic_as_refcounter.cocci +++ b/scripts/coccinelle/api/atomic_as_refcounter.cocci @@ -55,7 +55,8 @@ identifier fname6 =~ ".*call_rcu.*"; p1 << r1.p1; p2 << r1.p2; @@ -msg = "atomic_dec_and_test variation before object free at line %s." +msg = "atomic_dec_and_test variation before object free at line %s. \ +Consider using refcount_t instead of atomic_t for the variable." coccilib.report.print_report(p1[0], msg % (p2[0].line)) @r4 exists@ @@ -88,7 +89,8 @@ fname@p2(y, ...); p1 << r4.p1; p2 << r4.p2; @@ -msg = "atomic_dec_and_test variation before object free at line %s." +msg = "atomic_dec_and_test variation before object free at line %s. \ +Consider using refcount_t instead of atomic_t for the variable." coccilib.report.print_report(p1[0], msg % (p2[0].line)) @r2 exists@ @@ -107,7 +109,8 @@ atomic64_add_unless(&(a)->x,-1,1)@p1 @script:python depends on report@ p1 << r2.p1; @@ -msg = "atomic_add_unless" +msg = "Usage of atomic_add_unless encountered. \ +Consider using refcount_t instead of atomic_t for the variable." coccilib.report.print_report(p1[0], msg) @r3 exists@ @@ -126,5 +129,6 @@ x = atomic64_add_return@p1(-1, ...); @script:python depends on report@ p1 << r3.p1; @@ -msg = "x = atomic_add_return(-1, ...)" +msg = "Usage of x = atomic_add_return(-1, ...) encountered. \ +Consider using refcount_t instead of atomic_t for the variable." coccilib.report.print_report(p1[0], msg) -- 2.25.1 ___ Cocci mailing list Cocci@systeme.lip6.fr https://systeme.lip6.fr/mailman/listinfo/cocci
[Cocci] [PATCH 2/2] Documentation: Coccinelle: Modify parallelisation information in docs
This patchset modifies coccicheck to use at most one thread per core by default for optimal performance. Modify documentation in coccinelle.rst to reflect the same. Signed-off-by: Sumera Priyadarsini --- Documentation/dev-tools/coccinelle.rst | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Documentation/dev-tools/coccinelle.rst b/Documentation/dev-tools/coccinelle.rst index 74c5e6aeeff5..a27a4867018c 100644 --- a/Documentation/dev-tools/coccinelle.rst +++ b/Documentation/dev-tools/coccinelle.rst @@ -130,8 +130,8 @@ To enable verbose messages set the V= variable, for example:: Coccinelle parallelization -- -By default, coccicheck tries to run as parallel as possible. To change -the parallelism, set the J= variable. For example, to run across 4 CPUs:: +By default, coccicheck uses at most only one thread per core of the system. +To change the parallelism, set the J= variable. For example, to run across 4 CPUs:: make coccicheck MODE=report J=4 -- 2.25.1 ___ Cocci mailing list Cocci@systeme.lip6.fr https://systeme.lip6.fr/mailman/listinfo/cocci
[Cocci] [PATCH 1/2] scripts: coccicheck: Change default value for parallelism
By default, coccicheck utilizes all available threads to implement parallelisation. However, when all available threads are used, a decrease in performance is noted. The elapsed time is minimum when at most one thread per core is used. For example, on benchmarking the semantic patch kfree.cocci for usb/serial using hyperfine, the outputs obtained for J=5 and J=2 are 1.32 and 1.90 times faster than those for J=10 and J=9 respectively for two separate runs. For the larger drivers/staging directory, minimium elapsed time is obtained for J=3 which is 1.86 times faster than that for J=12. The optimal J value does not exceed 6 in any of the test runs. The benchmarks are run on a machine with 6 cores, with 2 threads per core, i.e, 12 hyperthreads in all. To improve performance, modify coccicheck to use at most only one thread per core by default. Signed-off-by: Sumera Priyadarsini --- Changes in V2: - Change commit message as suggested by Julia Lawall Changes in V3: - Use J/2 as optimal value for machines with more than 8 hyperthreads as well. --- scripts/coccicheck | 5 + 1 file changed, 5 insertions(+) diff --git a/scripts/coccicheck b/scripts/coccicheck index e04d328210ac..a72aa6c037ff 100755 --- a/scripts/coccicheck +++ b/scripts/coccicheck @@ -75,8 +75,13 @@ else OPTIONS="--dir $KBUILD_EXTMOD $COCCIINCLUDE" fi +# Use only one thread per core by default if hyperthreading is enabled +THREADS_PER_CORE=$(lscpu | grep "Thread(s) per core: " | tr -cd [:digit:]) if [ -z "$J" ]; then NPROC=$(getconf _NPROCESSORS_ONLN) + if [ $THREADS_PER_CORE -gt 1 -a $NPROC -gt 2 ] ; then + NPROC=$((NPROC/2)) + fi else NPROC="$J" fi -- 2.25.1 ___ Cocci mailing list Cocci@systeme.lip6.fr https://systeme.lip6.fr/mailman/listinfo/cocci
[Cocci] [PATCH 0/2] Improve Coccinelle Parallelisation
Coccinelle utilises all available threads to implement parallelisation. However, this results in a decrease in performance. This patchset aims to improve performance by modifying cocciccheck to use at most one thread per core by default. Sumera Priyadarsini (2): scripts: coccicheck: Change default value for parallelism Documentation: Coccinelle: Modify parallelisation information in docs Documentation/dev-tools/coccinelle.rst | 4 ++-- scripts/coccicheck | 5 + 2 files changed, 7 insertions(+), 2 deletions(-) -- 2.25.1 ___ Cocci mailing list Cocci@systeme.lip6.fr https://systeme.lip6.fr/mailman/listinfo/cocci
[Cocci] [PATCH] coccinelle: iterators: Add for_each_child.cocci script
While iterating over child nodes with the for_each functions, if control is transferred from the middle of the loop, as in the case of a break or return or goto, there is no decrement in the reference counter thus ultimately resulting in a memory leak. Add this script to detect potential memory leaks caused by the absence of of_node_put() before break, goto, or, return statements which transfer control outside the loop. Signed-off-by: Sumera Priyadarsini --- .../coccinelle/iterators/for_each_child.cocci | 348 ++ 1 file changed, 348 insertions(+) create mode 100644 scripts/coccinelle/iterators/for_each_child.cocci diff --git a/scripts/coccinelle/iterators/for_each_child.cocci b/scripts/coccinelle/iterators/for_each_child.cocci new file mode 100644 index ..0abc12ca2ad3 --- /dev/null +++ b/scripts/coccinelle/iterators/for_each_child.cocci @@ -0,0 +1,348 @@ +// SPDX-License-Identifier: GPL-2.0-only +// Adds missing of_node_put() before return/break/goto statement within a for_each iterator for child nodes. +//# False positives can be due to function calls within the for_each +//# loop that may encapsulate an of_node_put. +/// +// Confidence: High +// Copyright: (C) 2020 Sumera Priyadarsini +// URL: http://coccinelle.lip6.fr + +virtual patch +virtual context +virtual org +virtual report + +@r@ +local idexpression n; +expression e1,e2; +iterator name for_each_node_by_name, for_each_node_by_type, +for_each_compatible_node, for_each_matching_node, +for_each_matching_node_and_match, for_each_child_of_node, +for_each_available_child_of_node, for_each_node_with_property; +iterator i; +statement S; +expression list [n1] es; +@@ + +( +( +for_each_node_by_name(n,e1) S +| +for_each_node_by_type(n,e1) S +| +for_each_compatible_node(n,e1,e2) S +| +for_each_matching_node(n,e1) S +| +for_each_matching_node_and_match(n,e1,e2) S +| +for_each_child_of_node(e1,n) S +| +for_each_available_child_of_node(e1,n) S +| +for_each_node_with_property(n,e1) S +) +& +i(es,n,...) S +) + +@ruleone depends on patch && !context && !org && !report@ + +local idexpression r.n; +iterator r.i,i1; +expression e; +expression list [r.n1] es; +statement S; +@@ + + i(es,n,...) { + ... +( + of_node_put(n); +| + e = n +| + return n; +| + i1(...,n,...) S +| ++ of_node_put(n); +? return ...; +) + ... when any + } + +@ruletwo depends on patch && !context && !org && !report@ + +local idexpression r.n; +iterator r.i,i1,i2; +expression e,e1; +expression list [r.n1] es; +statement S,S2; +@@ + + i(es,n,...) { + ... +( + of_node_put(n); +| + e = n +| + i1(...,n,...) S +| ++ of_node_put(n); +? break; +) + ... when any + } +... when != n +when strict +( + n = e1; +| +?i2(...,n,...) S2 +) + +@rulethree depends on patch && !context && !org && !report exists@ + +local idexpression r.n; +iterator r.i,i1,i2; +expression e,e1; +identifier l; +expression list [r.n1] es; +statement S,S2; +@@ + + i(es,n,...) { + ... +( + of_node_put(n); +| + e = n +| + i1(...,n,...) S +| ++ of_node_put(n); +? goto l; +) + ... when any + } +... when exists +l: ... when != n + when strict +( + n = e1; +| +?i2(...,n,...) S2 +) + +// + +@ruleone_context depends on !patch && (context || org || report) exists@ +statement S; +expression e; +expression list[r.n1] es; +iterator r.i, i1; +local idexpression r.n; +position j0, j1; +@@ + + i@j0(es,n,...) { + ... +( + of_node_put(n); +| + e = n +| + return n; +| + i1(...,n,...) S +| + return @j1 ...; +) + ... when any + } + +@ruleone_disj depends on !patch && (context || org || report)@ +expression list[r.n1] es; +iterator r.i; +local idexpression r.n; +position ruleone_context.j0, ruleone_context.j1; +@@ + +* i@j0(es,n,...) { + ... +*return @j1...; + ... when any + } + +@ruletwo_context depends on !patch && (context || org || report) exists@ +statement S, S2; +expression e, e1; +expression list[r.n1] es; +iterator r.i, i1, i2; +local idexpression r.n; +position j0, j2; +@@ + + i@j0(es,n,...) { + ... +( + of_node_put(n); +| + e = n +| + i1(...,n,...) S +| + break@j2; +) + ... when any + } +... when != n +when strict +( + n = e1; +| +?i2(...,n,...) S2 +) + +@ruletwo_disj depends on !patch && (context || org || report)@ +statement S2; +expression e1; +expression list[r.n1] es; +iterator r.i, i2; +local idexpression r.n; +position ruletwo_context.j0, ruletwo_context.j2; +@@ + +* i@j0(es,n,...) { + ... +*break @j2; + ... when any + } +... when != n +when strict +( + n = e1; +| +?i2(...,n,...) S2 +) + +@rulethree_context depends on !patch && (context || org || report) exists@ +identifier l; +statement S,S2; +expression e, e1; +expression list[r.n1] es; +iterator r.i, i1, i2; +local idexpression r.n; +position j0, j3; +@@ + + i@j0(es,n,...) { + ... +( + of_node_put(n); +| + e = n +| + i1(...,n,...) S +| + goto l@j3; +) + ... when any + } +... when e