Re: Coccinelle: semantic patch for missing of_node_put

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

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

Agreed.  Just tell Sasha.

julia

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

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

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

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

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

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

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

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

Re: Coccinelle: Searching for “when done” in function comments

[Markus Elfring]

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

I have constructed another SmPL script variant.

It can point out that the text “ when done” occurs in comment lines before
48 function implementations which are provided by 20 source files of
the software “Linux next-20190605”.
Will this source code analysis result trigger further development efforts
around detection of unique wordings in these descriptions?

Regards,
Markus

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

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

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

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