[Cocci] [PATCH] coccinelle: semantic patch for missing of_node_put

[Cocci] [PATCH] coccinelle: semantic patch for missing of_node_put

[Wen Yang]

The call to of_parse_phandle()/of_find_node_by_name() ... returns a node
pointer with refcount incremented thus it must be explicitly decremented
after the last usage.

This SmPL is also looking for places where there is an of_node_put on
some path but not on others.

Suggested-by: Julia Lawall <Julia.Lawall@lip6.fr>
Signed-off-by: Wen Yang <wen.yang99@zte.com.cn>
Cc: Julia Lawall <Julia.Lawall@lip6.fr>
Cc: Gilles Muller <Gilles.Muller@lip6.fr>
Cc: Nicolas Palix <nicolas.palix@imag.fr>
Cc: Michal Marek <michal.lkml@markovi.net>
Cc: Masahiro Yamada <yamada.masahiro@socionext.com>
Cc: cocci@systeme.lip6.fr
---
 scripts/coccinelle/free/of_node_put.cocci | 133 ++++++++++++++++++++++++++++++
 1 file changed, 133 insertions(+)
 create mode 100644 scripts/coccinelle/free/of_node_put.cocci

diff --git a/scripts/coccinelle/free/of_node_put.cocci b/scripts/coccinelle/free/of_node_put.cocci
new file mode 100644
index 0000000..304293c
--- /dev/null
+++ b/scripts/coccinelle/free/of_node_put.cocci
@@ -0,0 +1,133 @@
+// SPDX-License-Identifier: GPL-2.0
+/// Find missing of_node_put
+///
+// Confidence: Moderate
+// Copyright: (C) 2018-2019 Wen Yang, ZTE.
+// Comments:
+// Options: --no-includes --include-headers
+
+virtual report
+virtual org
+
+@initialize:python@
+@@
+
+msg_prefix = "ERROR: missing of_node_put; acquired a node pointer with refcount incremented on line "
+msg_suffix = ", but without a corresponding object release within this function."
+
+seen = set()
+
+def add_if_not_present (p1, p2):
+    if (p1, p2) not in seen:
+        seen.add((p1, p2))
+        return True
+    return False
+
+@r1 exists@
+local idexpression struct device_node *x;
+expression e, e1;
+position p1, p2;
+identifier f;
+statement S;
+type T;
+@@
+
+(
+x = f@p1(...);
+... when != e = (T)x
+    when any
+    when != true x == NULL
+    when != of_node_put(x)
+    when != of_get_next_parent(x)
+    when != of_find_matching_node(x, ...)
+    when != if (x) { ... of_node_put(x) ... }
+    when != if (x) { ... return x; }
+    when != v4l2_async_notifier_add_fwnode_subdev(<...x...>)
+    when != e1 = of_fwnode_handle(x)
+(
+if (x) { ... when forall
+         of_node_put(x) ... }
+|
+return x;
+|
+return of_fwnode_handle(x);
+|
+return@p2 ...;
+)
+&
+x = f(...)
+...
+if (<+...x...+>) S
+...
+of_node_put(x);
+)
+
+@script:python depends on report && r1@
+p1 << r1.p1;
+p2 << r1.p2;
+@@
+
+if(add_if_not_present(p1[0].line, p2[0].line)):
+  coccilib.report.print_report(p2[0], msg_prefix + p1[0].line + msg_suffix)
+
+@script:python depends on org && r1@
+p1 << r1.p1;
+p2 << r1.p2;
+@@
+
+cocci.print_main("acquired a node pointer with refcount incremented", p1)
+cocci.print_secs("needed of_node_put", p2)
+
+@r2 exists@
+local idexpression struct device_node *x;
+expression e, e1;
+position p1, p2;
+statement S;
+type T;
+@@
+
+x = @p1\(of_find_compatible_node\|of_find_node_by_name\|of_parse_phandle\|
+    of_find_node_by_type\|of_find_node_by_name\|of_find_all_nodes\|
+    of_get_cpu_node\|of_get_parent\|of_get_next_parent\|
+    of_get_next_child\|of_get_next_available_child\|of_get_next_cpu_node\|
+    of_get_compatible_child\|of_get_child_by_name\|of_find_node_opts_by_path\|
+    of_find_node_with_property\|of_find_matching_node_and_match\|of_find_node_by_phandle\|
+    of_parse_phandle\)(...);
+...
+if (x == NULL || ...) S
+... when != e = (T)x
+    when any
+    when != true x == NULL
+    when != of_node_put(x)
+    when != of_get_next_parent(x)
+    when != of_find_matching_node(x, ...)
+    when != if (x) { ... of_node_put(x) ... }
+    when != if (x) { ... return x; }
+    when != v4l2_async_notifier_add_fwnode_subdev(<...x...>)
+    when != e1 = of_fwnode_handle(x)
+(
+if (x) { ... when forall
+         of_node_put(x) ... }
+|
+return x;
+|
+return of_fwnode_handle(x);
+|
+return@p2 ...;
+)
+
+@script:python depends on report && r2@
+p1 << r2.p1;
+p2 << r2.p2;
+@@
+
+if(add_if_not_present(p1[0].line, p2[0].line)):
+  coccilib.report.print_report(p2[0], msg_prefix + p1[0].line + msg_suffix)
+
+@script:python depends on org && r2@
+p1 << r2.p1;
+p2 << r2.p2;
+@@
+
+cocci.print_main("acquired a node pointer with refcount incremented", p1)
+cocci.print_secs("needed of_node_put", p2)
-- 
2.9.5

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

[PATCH] coccinelle: semantic patch for missing of_node_put

[Wen Yang]

The call to of_parse_phandle()/of_find_node_by_name() ... returns a node
pointer with refcount incremented thus it must be explicitly decremented
after the last usage.

This SmPL is also looking for places where there is an of_node_put on
some path but not on others.

Suggested-by: Julia Lawall <Julia.Lawall@lip6.fr>
Signed-off-by: Wen Yang <wen.yang99@zte.com.cn>
Cc: Julia Lawall <Julia.Lawall@lip6.fr>
Cc: Gilles Muller <Gilles.Muller@lip6.fr>
Cc: Nicolas Palix <nicolas.palix@imag.fr>
Cc: Michal Marek <michal.lkml@markovi.net>
Cc: Masahiro Yamada <yamada.masahiro@socionext.com>
Cc: cocci@systeme.lip6.fr
---
 scripts/coccinelle/free/of_node_put.cocci | 133 ++++++++++++++++++++++++++++++
 1 file changed, 133 insertions(+)
 create mode 100644 scripts/coccinelle/free/of_node_put.cocci

diff --git a/scripts/coccinelle/free/of_node_put.cocci b/scripts/coccinelle/free/of_node_put.cocci
new file mode 100644
index 0000000..304293c
--- /dev/null
+++ b/scripts/coccinelle/free/of_node_put.cocci
@@ -0,0 +1,133 @@
+// SPDX-License-Identifier: GPL-2.0
+/// Find missing of_node_put
+///
+// Confidence: Moderate
+// Copyright: (C) 2018-2019 Wen Yang, ZTE.
+// Comments:
+// Options: --no-includes --include-headers
+
+virtual report
+virtual org
+
+@initialize:python@
+@@
+
+msg_prefix = "ERROR: missing of_node_put; acquired a node pointer with refcount incremented on line "
+msg_suffix = ", but without a corresponding object release within this function."
+
+seen = set()
+
+def add_if_not_present (p1, p2):
+    if (p1, p2) not in seen:
+        seen.add((p1, p2))
+        return True
+    return False
+
+@r1 exists@
+local idexpression struct device_node *x;
+expression e, e1;
+position p1, p2;
+identifier f;
+statement S;
+type T;
+@@
+
+(
+x = f@p1(...);
+... when != e = (T)x
+    when any
+    when != true x == NULL
+    when != of_node_put(x)
+    when != of_get_next_parent(x)
+    when != of_find_matching_node(x, ...)
+    when != if (x) { ... of_node_put(x) ... }
+    when != if (x) { ... return x; }
+    when != v4l2_async_notifier_add_fwnode_subdev(<...x...>)
+    when != e1 = of_fwnode_handle(x)
+(
+if (x) { ... when forall
+         of_node_put(x) ... }
+|
+return x;
+|
+return of_fwnode_handle(x);
+|
+return@p2 ...;
+)
+&
+x = f(...)
+...
+if (<+...x...+>) S
+...
+of_node_put(x);
+)
+
+@script:python depends on report && r1@
+p1 << r1.p1;
+p2 << r1.p2;
+@@
+
+if(add_if_not_present(p1[0].line, p2[0].line)):
+  coccilib.report.print_report(p2[0], msg_prefix + p1[0].line + msg_suffix)
+
+@script:python depends on org && r1@
+p1 << r1.p1;
+p2 << r1.p2;
+@@
+
+cocci.print_main("acquired a node pointer with refcount incremented", p1)
+cocci.print_secs("needed of_node_put", p2)
+
+@r2 exists@
+local idexpression struct device_node *x;
+expression e, e1;
+position p1, p2;
+statement S;
+type T;
+@@
+
+x = @p1\(of_find_compatible_node\|of_find_node_by_name\|of_parse_phandle\|
+    of_find_node_by_type\|of_find_node_by_name\|of_find_all_nodes\|
+    of_get_cpu_node\|of_get_parent\|of_get_next_parent\|
+    of_get_next_child\|of_get_next_available_child\|of_get_next_cpu_node\|
+    of_get_compatible_child\|of_get_child_by_name\|of_find_node_opts_by_path\|
+    of_find_node_with_property\|of_find_matching_node_and_match\|of_find_node_by_phandle\|
+    of_parse_phandle\)(...);
+...
+if (x == NULL || ...) S
+... when != e = (T)x
+    when any
+    when != true x == NULL
+    when != of_node_put(x)
+    when != of_get_next_parent(x)
+    when != of_find_matching_node(x, ...)
+    when != if (x) { ... of_node_put(x) ... }
+    when != if (x) { ... return x; }
+    when != v4l2_async_notifier_add_fwnode_subdev(<...x...>)
+    when != e1 = of_fwnode_handle(x)
+(
+if (x) { ... when forall
+         of_node_put(x) ... }
+|
+return x;
+|
+return of_fwnode_handle(x);
+|
+return@p2 ...;
+)
+
+@script:python depends on report && r2@
+p1 << r2.p1;
+p2 << r2.p2;
+@@
+
+if(add_if_not_present(p1[0].line, p2[0].line)):
+  coccilib.report.print_report(p2[0], msg_prefix + p1[0].line + msg_suffix)
+
+@script:python depends on org && r2@
+p1 << r2.p1;
+p2 << r2.p2;
+@@
+
+cocci.print_main("acquired a node pointer with refcount incremented", p1)
+cocci.print_secs("needed of_node_put", p2)
-- 
2.9.5

Re: [Cocci] [PATCH] coccinelle: semantic patch for missing of_node_put

[Markus Elfring]

> The call to of_parse_phandle()/of_find_node_by_name() ... returns a node
> pointer with refcount incremented thus it must be explicitly decremented
> after the last usage.
>
> This SmPL is also looking for places where there is an of_node_put on
> some path but not on others.

I suggest to improve this commit description.

* Possible wording:
  There are functions which increment a reference counter for a device node.
  These functions belong to a programming interface for the management
  of information from device trees.
  The counter must be decremented after the last usage of a device node.

  This SmPL script looks also for places where a of_node_put() call is on
  some paths but not on others.

* Will the word “patch” be replaced by “code search” in the commit subject
  because the operation modes “report” and “org” are supported here?


> +@initialize:python@
> +@@

Such a SmPL rule would apply to every possible operation mode.
I have noticed then that the two Python variables from here will be needed
only in two SmPL rules which depend on the mode “report”.

* Thus I would prefer to adjust the dependency specification accordingly.

* Please replace these variables by a separate function like
  the following.
  def display1(p1 ,p2):
     if add_if_not_present(p1[0].line, p2[0].line):
        coccilib.report.print_report(p2[0],
                                     "prefix"
                                     + p1[0].line
                                     + "suffix")


* Please move another bit of duplicate code to a separate function like
  the following.
  def display2(p1 ,p2):
     cocci.print_main("Choose info 1", p1)
     cocci.print_secs("Choose info 2", p2)


> +x = @p1\(of_find_compatible_node\|of_find_node_by_name\|of_parse_phandle\|

If you would like to insist to use such a SmPL disjunction, I would prefer
an other code formatting here.
How do you think about to put each function name on a separate line?

Can such a name list be ever automatically determined from an other
information source?
(Are there circumstances to consider under which the application of
a detailed regular expression would become interesting for a SmPL constraint?)

Will it be influenced by any sort criteria?


> +    when != of_node_put(x)
…
> +    when != if (x) { ... of_node_put(x) ... }

I find the second when constraint specification unnecessary because
the previous one should be sufficient to exclude such a function call.


Can the specification “when != \( of_node_put \| of_get_next_parent \) (x)”
be useful?


> +return x;
> +|
> +return of_fwnode_handle(x);

Can it be nicer to merge this bit of code into another SmPL disjunction?

+return \( x \| of_fwnode_handle(x) \);


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

Re: [PATCH] coccinelle: semantic patch for missing of_node_put

[Markus Elfring]

> The call to of_parse_phandle()/of_find_node_by_name() ... returns a node
> pointer with refcount incremented thus it must be explicitly decremented
> after the last usage.
>
> This SmPL is also looking for places where there is an of_node_put on
> some path but not on others.

I suggest to improve this commit description.

* Possible wording:
  There are functions which increment a reference counter for a device node.
  These functions belong to a programming interface for the management
  of information from device trees.
  The counter must be decremented after the last usage of a device node.

  This SmPL script looks also for places where a of_node_put() call is on
  some paths but not on others.

* Will the word “patch” be replaced by “code search” in the commit subject
  because the operation modes “report” and “org” are supported here?


> +@initialize:python@
> +@@

Such a SmPL rule would apply to every possible operation mode.
I have noticed then that the two Python variables from here will be needed
only in two SmPL rules which depend on the mode “report”.

* Thus I would prefer to adjust the dependency specification accordingly.

* Please replace these variables by a separate function like
  the following.
  def display1(p1 ,p2):
     if add_if_not_present(p1[0].line, p2[0].line):
        coccilib.report.print_report(p2[0],
                                     "prefix"
                                     + p1[0].line
                                     + "suffix")


* Please move another bit of duplicate code to a separate function like
  the following.
  def display2(p1 ,p2):
     cocci.print_main("Choose info 1", p1)
     cocci.print_secs("Choose info 2", p2)


> +x = @p1\(of_find_compatible_node\|of_find_node_by_name\|of_parse_phandle\|

If you would like to insist to use such a SmPL disjunction, I would prefer
an other code formatting here.
How do you think about to put each function name on a separate line?

Can such a name list be ever automatically determined from an other
information source?
(Are there circumstances to consider under which the application of
a detailed regular expression would become interesting for a SmPL constraint?)

Will it be influenced by any sort criteria?


> +    when != of_node_put(x)
…
> +    when != if (x) { ... of_node_put(x) ... }

I find the second when constraint specification unnecessary because
the previous one should be sufficient to exclude such a function call.


Can the specification “when != \( of_node_put \| of_get_next_parent \) (x)”
be useful?


> +return x;
> +|
> +return of_fwnode_handle(x);

Can it be nicer to merge this bit of code into another SmPL disjunction?

+return \( x \| of_fwnode_handle(x) \);


Regards,
Markus

Re: [Cocci] [PATCH] coccinelle: semantic patch for missing of_node_put

[wen.yang99]

[-- Attachment #1.1: Type: text/plain, Size: 4235 bytes --]

Hi Markus,
Thanks for the review.  

> > The call to of_parse_phandle()/of_find_node_by_name() ... returns a node
> > pointer with refcount incremented thus it must be explicitly decremented
> > after the last usage.
> >
> > This SmPL is also looking for places where there is an of_node_put on
> > some path but not on others.
> 
> I suggest to improve this commit description.
> 
> * Possible wording:
> There are functions which increment a reference counter for a device node.
> These functions belong to a programming interface for the management
> of information from device trees.
> The counter must be decremented after the last usage of a device node.
> 
> This SmPL script looks also for places where a of_node_put() call is on
> some paths but not on others.
> 
> * Will the word “patch” be replaced by “code search” in the commit subject
> because the operation modes “report” and “org” are supported here?
> 
> 
> > +@initialize:python@
> > +@@
> 
> Such a SmPL rule would apply to every possible operation mode.
> I have noticed then that the two Python variables from here will be needed
> only in two SmPL rules which depend on the mode “report”.
> 
> * Thus I would prefer to adjust the dependency specification accordingly.
> 
> * Please replace these variables by a separate function like
> the following.
> def display1(p1 ,p2):
> if add_if_not_present(p1[0].line, p2[0].line):
> coccilib.report.print_report(p2[0],
> "prefix"
> + p1[0].line
> + "suffix")
> 
> 
> * Please move another bit of duplicate code to a separate function like
> the following.
> def display2(p1 ,p2):
> cocci.print_main("Choose info 1", p1)
> cocci.print_secs("Choose info 2", p2)
> 
Thanks.
I will update the patch according to your suggestions above.

> > +x = @p1\(of_find_compatible_node\|of_find_node_by_name\|of_parse_phandle\|
> 
> If you would like to insist to use such a SmPL disjunction, I would prefer
> an other code formatting here.
> How do you think about to put each function name on a separate line?
> 
> Can such a name list be ever automatically determined from an other
> information source?
> (Are there circumstances to consider under which the application of
> a detailed regular expression would become interesting for a SmPL constraint?)
> 
> Will it be influenced by any sort criteria?
> 
Thanks. 
It's interesting to get the function list automatically.
I'll try to parse the drivers/of/base.c file based on comments like this
"* Returns a node pointer with refcount incremented, use
* of_node_put() on it when done."
to automatically get the name of the function that needs to be checked.

> > +    when != of_node_put(x)
> …
> > +    when != if (x) { ... of_node_put(x) ... }
> 
> I find the second when constraint specification unnecessary because
> the previous one should be sufficient to exclude such a function call.
> 
Thanks.
I added the "when != if (x) { ... of_node_put(x) ... }" statement to avoid
 false positives similar to the following:
./arch/powerpc/platforms/powermac/setup.c:513:1-7: ERROR: missing of_node_put; 
acquired a node pointer with refcount incremented on line 503, 
but without a corresponding object release within this function.

484 static int __init pmac_declare_of_platform_devices(void)
485 {
486         struct device_node *np;
...
503         np = of_find_node_by_type(NULL, "fcu");
504         if (np == NULL) {
505                 /* Some machines have strangely broken device-tree */
506                 np = of_find_node_by_path("/u3@0,f8000000/i2c@f8001000/fan@15e");
507         }
508         if (np) {
509                 of_platform_device_create(np, "temperature", NULL);
510                 of_node_put(np);
511         }
512 
513         return 0;
514 }

We will continue to analyze the code of coccinelle to confirm whether
this false positive is a bug in coccinelle.
But this statement is currently needed here.

--
Regards,
Wen

> 
> Can the specification “when != \( of_node_put \| of_get_next_parent \) (x)”
> be useful?
> 
> 
> > +return x;
> > +|
> > +return of_fwnode_handle(x);
> 
> Can it be nicer to merge this bit of code into another SmPL disjunction?
> 
> +return \( x \| of_fwnode_handle(x) \);
> 
> 
> Regards,
> Markus

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

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

Re: [Cocci] Coccinelle: semantic patch for missing of_node_put

[Markus Elfring]

> It's interesting to get the function list automatically.

I occasionally imported code data into list variables
or even database tables.


> I'll try to parse the drivers/of/base.c file based on comments like this
> "* Returns a node pointer with refcount incremented, use
> * of_node_put() on it when done."
> to automatically get the name of the function that needs to be checked.

Will feature requests like the following become more interesting?

* Advanced data processing for source code comments
  https://github.com/coccinelle/coccinelle/issues/57

* Add a metavariable for the handling of source code
  https://github.com/coccinelle/coccinelle/issues/140


> We will continue to analyze the code of coccinelle

How will the understanding evolve for the OCaml source code
of this software?


> to confirm whether this false positive is a bug in coccinelle.

I am also curious on how the corresponding clarification will be continued.

By the way:
Yesterday I stumbled on another questionable software behaviour
while trying to apply an update suggestion from our development discussion
on the topic “[v6] coccinelle: semantic code search for missing put_device()”.
https://lore.kernel.org/cocci/201902191014156680299@zte.com.cn/
https://systeme.lip6.fr/pipermail/cocci/2019-February/005620.html


> But this statement is currently needed here.

Will the need be reconsidered?


I got another development concern here:
You propose to use a SmPL conjunction in the rule “r1”.
How does it fit to the previous exclusion specification “when != of_node_put(x)”?

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

Re: Coccinelle: semantic patch for missing of_node_put

[Markus Elfring]

> It's interesting to get the function list automatically.

I occasionally imported code data into list variables
or even database tables.


> I'll try to parse the drivers/of/base.c file based on comments like this
> "* Returns a node pointer with refcount incremented, use
> * of_node_put() on it when done."
> to automatically get the name of the function that needs to be checked.

Will feature requests like the following become more interesting?

* Advanced data processing for source code comments
  https://github.com/coccinelle/coccinelle/issues/57

* Add a metavariable for the handling of source code
  https://github.com/coccinelle/coccinelle/issues/140


> We will continue to analyze the code of coccinelle

How will the understanding evolve for the OCaml source code
of this software?


> to confirm whether this false positive is a bug in coccinelle.

I am also curious on how the corresponding clarification will be continued.

By the way:
Yesterday I stumbled on another questionable software behaviour
while trying to apply an update suggestion from our development discussion
on the topic “[v6] coccinelle: semantic code search for missing put_device()”.
https://lore.kernel.org/cocci/201902191014156680299@zte.com.cn/
https://systeme.lip6.fr/pipermail/cocci/2019-February/005620.html


> But this statement is currently needed here.

Will the need be reconsidered?


I got another development concern here:
You propose to use a SmPL conjunction in the rule “r1”.
How does it fit to the previous exclusion specification “when != of_node_put(x)”?

Regards,
Markus

Re: Coccinelle: semantic patch for missing of_node_put

[Julia Lawall]

A semantic patch has no access to comments.  The only thing I can see to
do is to use python to interact with some external tools.  For example,
you could write some code to collect the comments in a file and the lines
on which they occur, and then get the comment that most closely precedes
the start of the function.

julia

Re: [Cocci] Coccinelle: semantic patch for missing of_node_put

[Markus Elfring]

> A semantic patch has no access to comments.

Thanks for your acknowledgement of the current situation.


> The only thing I can see to do is to use python to interact with some external tools.

I see more software development possibilities.

* Advanced data processing for source code comments
  https://github.com/coccinelle/coccinelle/issues/57

* Add a metavariable for the handling of source code
  https://github.com/coccinelle/coccinelle/issues/140


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

Re: Coccinelle: semantic patch for missing of_node_put

[Pavel Machek]

Hi!

> A semantic patch has no access to comments.  The only thing I can see to
> do is to use python to interact with some external tools.  For example,
> you could write some code to collect the comments in a file and the lines
> on which they occur, and then get the comment that most closely precedes
> the start of the function.

How dangerous is missing of_node_put? AFAICT it will only result into
very small, one-time memory leak, right?

Could we make sure these patches are _not_ going to stable? Leaking
few bytes once per boot is not really a serious bug.
									Pavel

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [Cocci] Coccinelle: semantic patch for missing of_node_put

[Julia Lawall]

On Mon, 20 May 2019, Pavel Machek wrote:

> Hi!
>
> > A semantic patch has no access to comments.  The only thing I can see to
> > do is to use python to interact with some external tools.  For example,
> > you could write some code to collect the comments in a file and the lines
> > on which they occur, and then get the comment that most closely precedes
> > the start of the function.
>
> How dangerous is missing of_node_put? AFAICT it will only result into
> very small, one-time memory leak, right?
>
> Could we make sure these patches are _not_ going to stable? Leaking
> few bytes once per boot is not really a serious bug.

Sasha,

Probably patches that add only of_node_put should not be auto selected for
stable.

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

Re: [Cocci] Coccinelle: semantic patch for missing of_node_put

[Sasha Levin]

On Mon, May 20, 2019 at 11:52:37AM +0200, Julia Lawall wrote:
>
>
>On Mon, 20 May 2019, Pavel Machek wrote:
>
>> Hi!
>>
>> > A semantic patch has no access to comments.  The only thing I can see to
>> > do is to use python to interact with some external tools.  For example,
>> > you could write some code to collect the comments in a file and the lines
>> > on which they occur, and then get the comment that most closely precedes
>> > the start of the function.
>>
>> How dangerous is missing of_node_put? AFAICT it will only result into
>> very small, one-time memory leak, right?
>>
>> Could we make sure these patches are _not_ going to stable? Leaking
>> few bytes once per boot is not really a serious bug.
>
>Sasha,
>
>Probably patches that add only of_node_put should not be auto selected for
>stable.

I can filter them out, but those are fixes, right? Why are we concerned
about them making it into -stable?

--
Thanks,
Sasha
_______________________________________________
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci

Re: [Cocci] Coccinelle: semantic patch for missing of_node_put

[Julia Lawall]

On Mon, 20 May 2019, Sasha Levin wrote:

> On Mon, May 20, 2019 at 11:52:37AM +0200, Julia Lawall wrote:
> >
> >
> > On Mon, 20 May 2019, Pavel Machek wrote:
> >
> > > Hi!
> > >
> > > > A semantic patch has no access to comments.  The only thing I can see to
> > > > do is to use python to interact with some external tools.  For example,
> > > > you could write some code to collect the comments in a file and the
> > > lines
> > > > on which they occur, and then get the comment that most closely precedes
> > > > the start of the function.
> > >
> > > How dangerous is missing of_node_put? AFAICT it will only result into
> > > very small, one-time memory leak, right?
> > >
> > > Could we make sure these patches are _not_ going to stable? Leaking
> > > few bytes once per boot is not really a serious bug.
> >
> > Sasha,
> >
> > Probably patches that add only of_node_put should not be auto selected for
> > stable.
>
> I can filter them out, but those are fixes, right? Why are we concerned
> about them making it into -stable?

One of them may have introduced a crash.  If there is a bad reference
count manipulation elsewhere, then fixing one could cause a later
incorrect one to make a double free.

On the other hand, I don't know if the one that seemed to cause a crash
really caused a crash.  It was detected by syzkaller, and it is also
possible that git bisect ended up at the wrong place.

In any case, forgetting an of_node_put will normally just waste a little
memory, so probably stable kernels don't care.

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

Re: [Cocci] Coccinelle: semantic patch for missing of_node_put

[Markus Elfring]

> On the other hand, I don't know if the one that seemed to cause a crash
> really caused a crash.  It was detected by syzkaller, and it is also
> possible that git bisect ended up at the wrong place.

Do you refer to any known bug report here?


> In any case, forgetting an of_node_put will normally just waste a little
> memory, so probably stable kernels don't care.

Will it be nice to achieve better exception handling also for these
software versions over time?

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

Re: [Cocci] Coccinelle: semantic patch for missing of_node_put

[Markus Elfring]

> 1, A simple method.
> We did some experiments, and we could get the list of functions that need to be considered directly through this script:
>
> $ spatch --tokens-c drivers/of/base.c  2>&1  | grep "Tag3 " | grep "of_node_put() on it when done." | awk -F " - " '{print $1}' | grep  -o "of_[[:print:]]*"

Thanks for your command demonstration.

* How are the chances to get these tags better documented?
  https://github.com/coccinelle/coccinelle/blob/66a1118e04a6aaf1acdae89623313c8e05158a8d/docs/manual/spatch_options.tex#L165

* Would you like to combine the texts from the first two greps
  in a single search pattern?

* I imagine that sort criteria can become relevant for
  the determined function name list.

* Will a software build script be needed for this purpose?


> 2, A general method.
> We also try to get the list of functions to consider by writing a SmPL,
> but this method is not feasible at present, because it is not easy to parse the comment
> header information of these functions.

I am curious if corresponding software development challenges
will be picked up more.


> @r1@
> identifier fn;
> comment x;

This item is not mentioned as a key word in the manual for
the semantic patch language so far while the word is used
at seven places in this document.


> @@
>
> struct device_node * fn (...)
> {
> ...
> }

You can not get the desired information if a metavariable like “x”
is not actually used in the SmPL search code.

How do you think about to take corresponding source code positions
better into account?


> 3, It's probably interesting to get valuable informations from the comments of a function.

Other development tools provide better support for this data processing area.


> We will continue to learn the source code of coccinelle and try to find a way to achieve it.

How will the situation evolve here?


> Please kindly give me some help.

Do you find the following clarification request interesting?

Fix two calls for the program “ocamldoc”
https://github.com/coccinelle/coccinelle/issues/111


> We will continue to optimize this SmPL and send a V2 version next week.

I got another development concern in the meantime.
It seems that you would like to use iteration functionality (add_if_not_present).
https://github.com/coccinelle/coccinelle/blob/99e081e9b89d49301b7bd2c5e5aac88c66eaaa6a/docs/manual/cocci_syntax.tex#L1826

How will it matter here?

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

Re: [Cocci] Coccinelle: semantic patch for missing of_node_put

[Markus Elfring]

> $ spatch --tokens-c drivers/of/base.c  2>&1  | grep "Tag3 " | grep "of_node_put() on it when done." | awk -F " - " '{print $1}' | grep  -o "of_[[:print:]]*"

This command example points some details out for further software development considerations.

1. I find it questionable that relevant data are provided by the output channel
   “stderr” so far.
   https://github.com/coccinelle/coccinelle/blob/66a1118e04a6aaf1acdae89623313c8e05158a8d/docs/manual/spatch_options.tex#L165

2. The OCaml code “"Tag" ^ string_of_int t ^” occurs in three source files.
   * It is commented out in one file.
     https://github.com/coccinelle/coccinelle/blob/761cf6a1fbbf3173896ff61f0ea7e4a83a5b2a57/commons/common.ml#L305

   * These places refer to the source file “dumper.ml 1.2” by Richard W. M. Jones.
     Thus it seems that this code is relevant at the moment.
     https://github.com/coccinelle/coccinelle/blob/175de16bc7e535b6a89a62b81a673b0d0cd7075c/commons/ocamlextra/dumper.ml#L1

3. How will the software documentation evolve here?

4. Safe data processing can be performed only if the involved structures
   will remain clear for a while.
   Is the situation partly unclear?

   Should the information after which function calls the function “of_node_put”
   should be called be determined from any other documentation format?

5. A programming language like “awk” has got the potential to extract useful data
   (also without calling the tool “grep” additionally).

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

Re: [Cocci] Coccinelle: semantic patch for missing of_node_put

[Markus Elfring]

> 2, A general method.
> We also try to get the list of functions to consider by writing a SmPL,
> but this method is not feasible at present, because it is not easy to parse the comment
> header information of these functions.

The situation was improved once more also for the Coccinelle software.
How do you think about to develop any more variants based on information
from a script (like the following) for the semantic patch language?

@initialize:python@
@@
import re, sys
filter = re.compile(" when done")

@find@
comments c;
identifier x;
type t;
@@
 t@c x(...)
 { ... }

@script:python selection@
input << find.c;
@@
if filter.search(input[0].before, 2):
   sys.stderr.write(input[0].before + "\n=====\n")
else:
   cocci.include_match(False)

@display@
identifier find.x;
type find.t;
@@
*t x(...)
 { ... }


Does such a source code analysis approach indicate any details
which should be improved for the affected software documentation?

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

Re: [Cocci] Coccinelle: semantic patch for missing of_node_put

[wen.yang99]

[-- Attachment #1.1: Type: text/plain, Size: 2253 bytes --]

> > 2, A general method.
> > We also try to get the list of functions to consider by writing a SmPL,
> > but this method is not feasible at present, because it is not easy to parse the comment
> > header information of these functions.
> 
> The situation was improved once more also for the Coccinelle software.
> How do you think about to develop any more variants based on information
> from a script (like the following) for the semantic patch language?
> 
> @initialize:python@
> @@
> import re, sys
> filter = re.compile(" when done")
> 
> @find@
> comments c;
> identifier x;
> type t;
> @@
> t@c x(...)
> { ... }
> 
> @script:python selection@
> input << find.c;
> @@
> if filter.search(input[0].before, 2):
> sys.stderr.write(input[0].before + "\n=====\n")
> else:
> cocci.include_match(False)
> 
> @display@
> identifier find.x;
> type find.t;
> @@
> *t x(...)
> { ... }
> 
> 
> Does such a source code analysis approach indicate any details
> which should be improved for the affected software documentation?
Thank you for your example.
We currently use the following Ocaml script to automatically
collect functions that need to be considered.

@initialize:ocaml@
@@

let relevant_str = "use of_node_put() on it when done"

let contains s1 s2 =
    let re = Str.regexp_string s2
    in
        try ignore (Str.search_forward re s1 0); true
        with Not_found -> false

let relevant_functions = ref []

let add_function f c = 
    if not (List.mem f !relevant_functions)
    then 
      begin
        let s = String.concat " "
          (
            (List.map String.lowercase_ascii
	     (List.filter
	       (function x ->
	         Str.string_match
	         (Str.regexp "[a-zA-Z_\\(\\)][-a-zA-Z0-9_\\(\\)]*$")
	       x 0) (Str.split (Str.regexp "[ .;\t\n]+") c)))) in
             Printf.printf "comments: %s\n" s;
             if contains s relevant_str
             then 
               Printf.printf "Found relevant function: %s\n" f;
               relevant_functions := f :: !relevant_functions;
      end

@r@
identifier fn;
comments c;
type T = struct device_node *;
@@

T@c fn(...) {
...
}

@script:ocaml@
f << r.fn;
c << r.c;
@@

let (cb,cm,ca) = List.hd c in
let c = String.concat " " cb in
add_function f c

--
Regards,
Wen

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

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

Re: [Cocci] Coccinelle: semantic patch for missing of_node_put

[Markus Elfring]

> We currently use the following Ocaml script to automatically
> collect functions that need to be considered.
>
> @initialize:ocaml@
> @@
>
> let relevant_str = "use of_node_put() on it when done"

I suggest to reconsider this search pattern.

The mentioned words are distributed over text lines in the discussed
software documentation.
Thus I imagine that an other documentation format would be safer
and more helpful for the determination of a corresponding API
system property.

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

Re: [Cocci] Coccinelle: semantic patch for missing of_node_put

[wen.yang99]

[-- Attachment #1.1: Type: text/plain, Size: 733 bytes --]

> > We currently use the following Ocaml script to automatically
> > collect functions that need to be considered.
> >
> > @initialize:ocaml@
> > @@
> >
> > let relevant_str = "use of_node_put() on it when done"
> 
> I suggest to reconsider this search pattern.
> 
> The mentioned words are distributed over text lines in the discussed
> software documentation.
> Thus I imagine that an other documentation format would be safer
> and more helpful for the determination of a corresponding API
> system property.

Our script will remove '* ','\ n','\t' and so on from the comments in the function header
and then merge them into one line, so we can exactly match the target string
'use of_node_put() on it when done '

--
Regards,
Wen

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

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

Re: [Cocci] Coccinelle: semantic patch for missing of_node_put

[Markus Elfring]

>> Thus I imagine that an other documentation format would be safer
>> and more helpful for the determination of a corresponding API
>> system property.
>
> Our script will remove '* ','\ n','\t' and so on from the comments in the function header
> and then merge them into one line,

* Would you like to keep this adjustment approach (for a while)?

* Will other data structures become nicer for the discussed data extraction?


> so we can exactly match the target string 'use of_node_put() on it when done '

Thanks for this clarification.

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

Re: [Cocci] Coccinelle: semantic patch for missing of_node_put

[Markus Elfring]

> let add_function f c =
>     if not (List.mem f !relevant_functions)
>     then
>       begin
>         let s = String.concat " "
>           (
>             (List.map String.lowercase_ascii
> 	     (List.filter
> 	       (function x ->
> 	         Str.string_match
> 	         (Str.regexp "[a-zA-Z_\\(\\)][-a-zA-Z0-9_\\(\\)]*$")
> 	       x 0) (Str.split (Str.regexp "[ .;\t\n]+") c)))) in

I would interpret one of these function calls in the way
that text splitting is performed here also for space characters
after a concatenation was performed.


>              Printf.printf "comments: %s\n" s;
>              if contains s relevant_str
>              then
>                Printf.printf "Found relevant function: %s\n" f;
>                relevant_functions := f :: !relevant_functions;
>       end
>
> @r@
> identifier fn;
> comments c;
> type T = struct device_node *;
> @@
>
> T@c fn(...) {
> ...
> }
>
> @script:ocaml@
> f << r.fn;
> c << r.c;
> @@
>
> let (cb,cm,ca) = List.hd c in
> let c = String.concat " " cb in
> add_function f c

Can an other data processing variant be more reasonable?

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