cocci.inria.fr archive mirror
 help / color / mirror / Atom feed
* [Cocci] [PATCH] coccinelle: semantic patch for missing of_node_put
@ 2019-05-07  8:12 Wen Yang
  2019-05-07 15:27 ` Markus Elfring
  0 siblings, 1 reply; 17+ messages in thread
From: Wen Yang @ 2019-05-07  8:12 UTC (permalink / raw)
  To: linux-kernel; +Cc: wang.yi59, Michal Marek, Nicolas Palix, cocci, 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

^ permalink raw reply related	[flat|nested] 17+ messages in thread

* Re: [Cocci] [PATCH] coccinelle: semantic patch for missing of_node_put
  2019-05-07  8:12 [Cocci] [PATCH] coccinelle: semantic patch for missing of_node_put Wen Yang
@ 2019-05-07 15:27 ` Markus Elfring
  2019-05-09  1:47   ` wen.yang99
  0 siblings, 1 reply; 17+ messages in thread
From: Markus Elfring @ 2019-05-07 15:27 UTC (permalink / raw)
  To: Wen Yang, cocci; +Cc: Yi Wang, Michal Marek, Nicolas Palix, linux-kernel

> 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

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [Cocci] [PATCH] coccinelle: semantic patch for missing of_node_put
  2019-05-07 15:27 ` Markus Elfring
@ 2019-05-09  1:47   ` wen.yang99
  2019-05-09  8:10     ` [Cocci] Coccinelle: " Markus Elfring
  0 siblings, 1 reply; 17+ messages in thread
From: wen.yang99 @ 2019-05-09  1:47 UTC (permalink / raw)
  To: Markus.Elfring; +Cc: wang.yi59, michal.lkml, nicolas.palix, linux-kernel, cocci


[-- 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

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [Cocci] Coccinelle: semantic patch for missing of_node_put
  2019-05-09  1:47   ` wen.yang99
@ 2019-05-09  8:10     ` Markus Elfring
  0 siblings, 0 replies; 17+ messages in thread
From: Markus Elfring @ 2019-05-09  8:10 UTC (permalink / raw)
  To: Wen Yang, cocci; +Cc: Yi Wang, Michal Marek, Nicolas Palix, linux-kernel

> 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

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [Cocci] Coccinelle: semantic patch for missing of_node_put
  2019-06-04  5:50   ` wen.yang99
  2019-06-04  6:36     ` Markus Elfring
@ 2019-06-04 11:28     ` Markus Elfring
  1 sibling, 0 replies; 17+ messages in thread
From: Markus Elfring @ 2019-06-04 11:28 UTC (permalink / raw)
  To: Wen Yang; +Cc: Michal Marek, linux-doc, Nicolas Palix, linux-kernel, cocci

> 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

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [Cocci] Coccinelle: semantic patch for missing of_node_put
  2019-06-04  8:55       ` wen.yang99
@ 2019-06-04  9:08         ` Markus Elfring
  0 siblings, 0 replies; 17+ messages in thread
From: Markus Elfring @ 2019-06-04  9:08 UTC (permalink / raw)
  To: Wen Yang, linux-doc; +Cc: Michal Marek, Nicolas Palix, linux-kernel, cocci

>> 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

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [Cocci] Coccinelle: semantic patch for missing of_node_put
  2019-06-04  6:36     ` Markus Elfring
@ 2019-06-04  8:55       ` wen.yang99
  2019-06-04  9:08         ` Markus Elfring
  0 siblings, 1 reply; 17+ messages in thread
From: wen.yang99 @ 2019-06-04  8:55 UTC (permalink / raw)
  To: Markus.Elfring; +Cc: michal.lkml, nicolas.palix, linux-doc, linux-kernel, cocci


[-- 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

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [Cocci] Coccinelle: semantic patch for missing of_node_put
  2019-06-04  5:50   ` wen.yang99
@ 2019-06-04  6:36     ` Markus Elfring
  2019-06-04  8:55       ` wen.yang99
  2019-06-04 11:28     ` Markus Elfring
  1 sibling, 1 reply; 17+ messages in thread
From: Markus Elfring @ 2019-06-04  6:36 UTC (permalink / raw)
  To: Wen Yang, Julia Lawall, linux-doc
  Cc: Michal Marek, Nicolas Palix, linux-kernel, cocci

> 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

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [Cocci] Coccinelle: semantic patch for missing of_node_put
  2019-06-04  5:08 ` Markus Elfring
@ 2019-06-04  5:50   ` wen.yang99
  2019-06-04  6:36     ` Markus Elfring
  2019-06-04 11:28     ` Markus Elfring
  0 siblings, 2 replies; 17+ messages in thread
From: wen.yang99 @ 2019-06-04  5:50 UTC (permalink / raw)
  To: Markus.Elfring; +Cc: michal.lkml, nicolas.palix, linux-doc, linux-kernel, cocci


[-- 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

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [Cocci] Coccinelle: semantic patch for missing of_node_put
       [not found] <201905171432571474636@zte.com.cn>
                   ` (2 preceding siblings ...)
  2019-05-18 14:43 ` Markus Elfring
@ 2019-06-04  5:08 ` Markus Elfring
  2019-06-04  5:50   ` wen.yang99
  3 siblings, 1 reply; 17+ messages in thread
From: Markus Elfring @ 2019-06-04  5:08 UTC (permalink / raw)
  To: Wen Yang, Julia Lawall, linux-doc
  Cc: Michal Marek, Nicolas Palix, linux-kernel, cocci

> 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

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [Cocci] Coccinelle: semantic patch for missing of_node_put
  2019-05-20 19:53         ` Julia Lawall
@ 2019-05-20 20:11           ` Markus Elfring
  0 siblings, 0 replies; 17+ messages in thread
From: Markus Elfring @ 2019-05-20 20:11 UTC (permalink / raw)
  To: Julia Lawall
  Cc: Sasha Levin, Michal Marek, Nicolas Palix, LKML, Coccinelle,
	Pavel Machek, Wen Yang

> 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

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [Cocci] Coccinelle: semantic patch for missing of_node_put
  2019-05-20 17:20       ` Sasha Levin
@ 2019-05-20 19:53         ` Julia Lawall
  2019-05-20 20:11           ` Markus Elfring
  0 siblings, 1 reply; 17+ messages in thread
From: Julia Lawall @ 2019-05-20 19:53 UTC (permalink / raw)
  To: Sasha Levin
  Cc: michal.lkml, nicolas.palix, linux-kernel, cocci, Markus.Elfring,
	Pavel Machek, wen.yang99



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

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [Cocci] Coccinelle: semantic patch for missing of_node_put
  2019-05-20  9:52     ` Julia Lawall
@ 2019-05-20 17:20       ` Sasha Levin
  2019-05-20 19:53         ` Julia Lawall
  0 siblings, 1 reply; 17+ messages in thread
From: Sasha Levin @ 2019-05-20 17:20 UTC (permalink / raw)
  To: Julia Lawall
  Cc: michal.lkml, nicolas.palix, linux-kernel, cocci, Markus.Elfring,
	Pavel Machek, wen.yang99

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

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [Cocci] Coccinelle: semantic patch for missing of_node_put
       [not found]   ` <20190520093303.GA9320@atrey.karlin.mff.cuni.cz>
@ 2019-05-20  9:52     ` Julia Lawall
  2019-05-20 17:20       ` Sasha Levin
  0 siblings, 1 reply; 17+ messages in thread
From: Julia Lawall @ 2019-05-20  9:52 UTC (permalink / raw)
  To: Pavel Machek
  Cc: sashal, michal.lkml, nicolas.palix, linux-kernel, cocci,
	Markus.Elfring, wen.yang99



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

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [Cocci] Coccinelle: semantic patch for missing of_node_put
       [not found] <201905171432571474636@zte.com.cn>
  2019-05-17  8:10 ` Markus Elfring
       [not found] ` <alpine.DEB.2.20.1905170912590.4014@hadrien>
@ 2019-05-18 14:43 ` Markus Elfring
  2019-06-04  5:08 ` Markus Elfring
  3 siblings, 0 replies; 17+ messages in thread
From: Markus Elfring @ 2019-05-18 14:43 UTC (permalink / raw)
  To: Wen Yang, Coccinelle, linux-doc; +Cc: Michal Marek, Nicolas Palix, linux-kernel

> $ 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

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [Cocci] Coccinelle: semantic patch for missing of_node_put
       [not found] ` <alpine.DEB.2.20.1905170912590.4014@hadrien>
@ 2019-05-17  8:22   ` Markus Elfring
       [not found]   ` <20190520093303.GA9320@atrey.karlin.mff.cuni.cz>
  1 sibling, 0 replies; 17+ messages in thread
From: Markus Elfring @ 2019-05-17  8:22 UTC (permalink / raw)
  To: Julia Lawall; +Cc: Michal Marek, Nicolas Palix, linux-kernel, Wen Yang, cocci

> 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

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [Cocci] Coccinelle: semantic patch for missing of_node_put
       [not found] <201905171432571474636@zte.com.cn>
@ 2019-05-17  8:10 ` Markus Elfring
       [not found] ` <alpine.DEB.2.20.1905170912590.4014@hadrien>
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 17+ messages in thread
From: Markus Elfring @ 2019-05-17  8:10 UTC (permalink / raw)
  To: Wen Yang, cocci; +Cc: Michal Marek, Nicolas Palix, linux-kernel

> 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

^ permalink raw reply	[flat|nested] 17+ messages in thread

end of thread, other threads:[~2019-06-04 11:30 UTC | newest]

Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-05-07  8:12 [Cocci] [PATCH] coccinelle: semantic patch for missing of_node_put Wen Yang
2019-05-07 15:27 ` Markus Elfring
2019-05-09  1:47   ` wen.yang99
2019-05-09  8:10     ` [Cocci] Coccinelle: " Markus Elfring
     [not found] <201905171432571474636@zte.com.cn>
2019-05-17  8:10 ` Markus Elfring
     [not found] ` <alpine.DEB.2.20.1905170912590.4014@hadrien>
2019-05-17  8:22   ` Markus Elfring
     [not found]   ` <20190520093303.GA9320@atrey.karlin.mff.cuni.cz>
2019-05-20  9:52     ` Julia Lawall
2019-05-20 17:20       ` Sasha Levin
2019-05-20 19:53         ` Julia Lawall
2019-05-20 20:11           ` Markus Elfring
2019-05-18 14:43 ` Markus Elfring
2019-06-04  5:08 ` Markus Elfring
2019-06-04  5:50   ` wen.yang99
2019-06-04  6:36     ` Markus Elfring
2019-06-04  8:55       ` wen.yang99
2019-06-04  9:08         ` Markus Elfring
2019-06-04 11:28     ` Markus Elfring

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).