Re: [Cocci] [PATCH v2] coccinelle: iterators: Add for_each_child.cocci script

Re: [Cocci] [PATCH v2] coccinelle: iterators: Add for_each_child.cocci script

[Julia Lawall]

[-- Attachment #1: Type: text/plain, Size: 399 bytes --]



On Tue, 13 Oct 2020, Markus Elfring wrote:

> …
> > +(
> > +for_each_node_by_name(n,e1) S
> > +|
> …
> > +|
> > +for_each_node_with_property(n,e1) S
> > +)
> …
>
> I propose to reconsider the coding style for this SmPL disjunction once more.
>
> Date: Thu, 24 Sep 2020 14:45:25 +0200
> https://lore.kernel.org/cocci/5a74b3c5-c0e9-1426-c477-72bb86bcf5ed@web.de/

It's quite fine as is.

julia

[-- Attachment #2: Type: text/plain, Size: 136 bytes --]

_______________________________________________
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci

Re: [Cocci] [PATCH v2] coccinelle: iterators: Add for_each_child.cocci script

[Julia Lawall]

[-- Attachment #1: Type: text/plain, Size: 607 bytes --]



On Tue, 13 Oct 2020, Markus Elfring wrote:

> > > > +(
> > > > +for_each_node_by_name(n,e1) S
> > > > +|
> > > …
> > > > +|
> > > > +for_each_node_with_property(n,e1) S
> > > > +)
> > > …
> > >
> > > I propose to reconsider the coding style for this SmPL disjunction once more.
> > >
> > > Date: Thu, 24 Sep 2020 14:45:25 +0200
> > > https://lore.kernel.org/cocci/5a74b3c5-c0e9-1426-c477-72bb86bcf5ed@web.de/
> >
> > It's quite fine as is.
>
> Would you care a bit more for the clarification of the ordering for the shown macro names?

Why does the ordering matter, since they are all distinct?

julia

[-- Attachment #2: Type: text/plain, Size: 136 bytes --]

_______________________________________________
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci

Re: [Cocci] [PATCH v2] coccinelle: iterators: Add for_each_child.cocci script

[Julia Lawall]

On Tue, 13 Oct 2020, Markus Elfring wrote:

> > > Would you care a bit more for the clarification of the ordering for the shown macro names?
> >
> > Why does the ordering matter, since they are all distinct?
>
> * It might look promising to reorder macro calls according to name criteria
>   and passed parameters.
>
> * But I imagine that the functionality of disjunctions by the semantic patch language
>   can trigger further development considerations more in another direction.
>   https://github.com/coccinelle/coccinelle/blob/730dbb034559b3e549ec0b2973cd0400a3fa072f/docs/manual/cocci_syntax.tex#L1033
>
>   Later source code search patterns will only be checked in such SmPL disjunctions
>   if previous parts did not match.
>   Thus often used code variants should probably be specified at the beginning
>   while special selections should be moved to the end.
>   The sorting of macro calls according to an estimated or actual usage frequency
>   can influence the evaluation characteristics of affected SmPL code,
>   can't it?

No.  As I already pointed out, the different macros are disjoint.  The
order doens't matter.  Only one of the patterns will match any given loop.
If there are nested loops, the pattern will match multiple times.

julia
_______________________________________________
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci

Re: [Cocci] [PATCH v2] coccinelle: iterators: Add for_each_child.cocci script

[Julia Lawall]

On Tue, 13 Oct 2020, Markus Elfring wrote:

> > >   The sorting of macro calls according to an estimated or actual usage frequency
> > >   can influence the evaluation characteristics of affected SmPL code,
> > >   can't it?
> >
> > No.  As I already pointed out, the different macros are disjoint.
>
> This information can be reasonable.
>
>
> > The order doens't matter.
>
> I wonder about such a feedback.
>
> How does it fit to the functionality of SmPL disjunctions?
> Is short-circuit evaluation applied for them?

The rule is applied independently to each node in the control-flow graph.
An example of a node is a loop header.  Any given loop header can match
only one of the provided patterns.  So the order doesn't matter.

julia

>
>
> > Only one of the patterns will match any given loop.
>
> This is generally fine.
> Will their occurrences vary in significant ways in the analysed source code?
>
> Regards,
> Markus
>
_______________________________________________
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci

Re: [Cocci] [PATCH v2] coccinelle: iterators: Add for_each_child.cocci script

[Julia Lawall]

[-- Attachment #1: Type: text/plain, Size: 474 bytes --]



On Tue, 13 Oct 2020, Markus Elfring wrote:

> > The rule is applied independently to each node in the control-flow graph.
>
> Please explain a specific technical detail once more.
> Is short-circuit evaluation applied for a disjunction (according to macro calls)
> in the discussed SmPL rule “r”?

The short circuit evaluation is applied at the level of CFG nodes.  For
each CFG node, it tries the sequence of patters one by one until it finds
one that matches.

julia

[-- Attachment #2: Type: text/plain, Size: 136 bytes --]

_______________________________________________
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci

Re: [Cocci] [PATCH v2] coccinelle: iterators: Add for_each_child.cocci script

[Julia Lawall]

[-- Attachment #1: Type: text/plain, Size: 954 bytes --]



On Wed, 14 Oct 2020, Markus Elfring wrote:

> > > Is short-circuit evaluation applied for a disjunction (according to macro calls)
> > > in the discussed SmPL rule “r”?
> >
> > The short circuit evaluation is applied at the level of CFG nodes.
> > For each CFG node, it tries the sequence of patters one by one until it finds
> > one that matches.
>
> Do you acknowledge then that disjunctions of the semantic patch language
> work in the way so far that subsequent condition parts are not checked any more
> after a match was determined for a specific source code search pattern?

Yes, at the CFG node level.

>
> Is this functionality equivalent to the operation “ordered choice”?
>
> Is another communication difficulty resolved now according to your hint
> that macro calls would be disjoint (and distinct)?
>
>
> Would you expect that the occurrences of the mentioned identifiers are different
> in the analysed source files?

Sure.

julia

[-- Attachment #2: Type: text/plain, Size: 136 bytes --]

_______________________________________________
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci

Re: [Cocci] [PATCH V2] coccinelle: iterators: Add for_each_child.cocci script

[Julia Lawall]

I have one more change to suggest.  This one only affects the patch case,
as the other cases just point to a problem but don't ompletely specify
what to do about it.

> +@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
> + }

There is one occurrence of the following code:

        for_each_available_child_of_node(search, child) {
                name = of_get_property(child, "regulator-compatible", NULL);
                if (!name)
                        name = child->name;

                if (!strcmp(desc->of_match, name)) {
                        of_node_put(search);
                        return of_node_get(child);
                }
        }

In this case, the for_each_available_child_of_node has incremented the
reference count of child by 1, and then the return increments it again.
It would be ok to put an of_not_put before the return, which is done by
the above rule, but the code would be even simpler if it would just leave
the reference count as is.  So before the current return case, you can put
another return case that does the following:

-  return of_node_get(n);
+  return n;

julia
_______________________________________________
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci

[Cocci] [PATCH V2] coccinelle: iterators: Add for_each_child.cocci script

[Sumera Priyadarsini]

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 <sylphrenadin@gmail.com>

----
Changes in V2:
- Add options --include-headers and --no-includes
- Add 'when forall` to rules for break and goto
---
 .../coccinelle/iterators/for_each_child.cocci | 355 ++++++++++++++++++
 1 file changed, 355 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 000000000000..57caf9b8d7a5
--- /dev/null
+++ b/scripts/coccinelle/iterators/for_each_child.cocci
@@ -0,0 +1,355 @@
+// 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
+// Options: --no-includes --include-headers
+
+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
+    when forall
+(
+ 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
+       when forall
+(
+ 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
+    when forall
+(
+ 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
+    when forall
+(
+  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 exists
+l:
+... when != n
+    when strict
+    when forall
+(
+ n = e1;
+|
+?i2(...,n,...) S2
+)
+
+@rulethree_disj depends on !patch && (context || org || report) exists@
+identifier l;
+statement S2;
+expression e1;
+expression list[r.n1] es;
+iterator r.i, i2;
+local idexpression r.n;
+position rulethree_context.j0, rulethree_context.j3;
+@@
+
+*  i@j0(es,n,...) {
+   ...
+*goto l@j3;
+   ... when any
+ }
+... when exists
+ l:
+ ... when != n
+     when strict
+     when forall
+(
+ n = e1;
+|
+?i2(...,n,...) S2
+)
+
+// ----------------------------------------------------------------------------
+
+@script:python ruleone_org depends on org@
+i << r.i;
+j0 << ruleone_context.j0;
+j1 << ruleone_context. j1;
+@@
+
+msg = "WARNING: Function \"%s\" should have of_node_put() before return " % (i)
+coccilib.org.print_safe_todo(j0[0], msg)
+coccilib.org.print_link(j1[0], "")
+
+@script:python ruletwo_org depends on org@
+i << r.i;
+j0 << ruletwo_context.j0;
+j2 << ruletwo_context.j2;
+@@
+
+msg = "WARNING: Function \"%s\" should have of_node_put() before break " % (i)
+coccilib.org.print_safe_todo(j0[0], msg)
+coccilib.org.print_link(j2[0], "")
+
+@script:python rulethree_org depends on org@
+i << r.i;
+j0 << rulethree_context.j0;
+j3 << rulethree_context.j3;
+@@
+
+msg = "WARNING: Function \"%s\" should have of_node_put() before goto " % (i)
+coccilib.org.print_safe_todo(j0[0], msg)
+coccilib.org.print_link(j3[0], "")
+
+// ----------------------------------------------------------------------------
+
+@script:python ruleone_report depends on report@
+i << r.i;
+j0 << ruleone_context.j0;
+j1 << ruleone_context.j1;
+@@
+
+msg = "WARNING: Function \"%s\" should have of_node_put() before return around line %s." % (i, j1[0].line)
+coccilib.report.print_report(j0[0], msg)
+
+@script:python ruletwo_report depends on report@
+i << r.i;
+j0 << ruletwo_context.j0;
+j2 << ruletwo_context.j2;
+@@
+
+msg = "WARNING: Function \"%s\" should have of_node_put() before break around line %s." % (i,j2[0].line)
+coccilib.report.print_report(j0[0], msg)
+
+@script:python rulethree_report depends on report@
+i << r.i;
+j0 << rulethree_context.j0;
+j3 << rulethree_context.j3;
+@@
+
+msg = "WARNING: Function \"%s\" should have of_node_put() before goto around lines %s." % (i,j3[0].line)
+coccilib.report.print_report(j0[0], msg)
-- 
2.25.1

_______________________________________________
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci