Re: [Outreachy kernel] [PATCH 1/2] Staging: lustre: ptlrpc: Make functions static

[Arnd Bergmann <arnd@arndb.de>]

On Thursday 01 October 2015 16:12:53 Julia Lawall wrote:
> On Thu, 1 Oct 2015, Arnd Bergmann wrote:
> > On Thursday 01 October 2015 15:49:21 Julia Lawall wrote:
> > > On Thu, 1 Oct 2015, Arnd Bergmann wrote:
> > > > On Thursday 01 October 2015 15:20:39 Julia Lawall wrote:
> > > > > On Thu, 1 Oct 2015, Shraddha Barke wrote:
> > > > > > > Note that there are hundreds more symbols in lustre that should be made
> > > > > > > static.
> > > > > > > Most of them are harder to find because they have a declaration in a header
> > > > > > > file
> > > > > > > despite being only used in one place.
> > > > >
> > > > > Why does it make it hard to find?  One can make a spearate check in .c
> > > > > files.
> > > >
> > > > We currently get a sparse warning for each global identifier definition
> > > > that is not preceded by a declaration, but since most of our tools
> > > > (gcc, sparse, coccinelle, ...) work on a single file at a time, they
> > > > do not know whether a global symbol that is defined in a C file and
> > > > (incorrectly) declared in a header is used in another C file or not.
> > >
> > > OK, but from Coccinlle you can just call grep, and check the extensions on
> > > the returned file names, so it is not a big problem.
> >
> > I'm not following here. How exactly would you find symbols like this with coccinelle?
> >
> > subsystem.h:
> >
> > extern int subsystem_state;
> >
> > subsystem.c:
> >
> > #include "subsystem.h"
> > long subsystem_state;
> >
> > static void function(void)
> > {
> > 	...
> > 	subsystem_state++;
> > 	...
> > }
> >
> > with no users of this variable outside of this file.
> 
> The following uses ocaml, because I'm not very good at python...
> 
> @r disable optional_storage@
> identifier f;
> position p;
> @@
> 
> f@p(...) { ... }
> 
> @script:ocaml@
> p << r.p;
> f << r.f;
> @@
> 
> let l = Common,cmd_to_list
>           (Printf.sprintf "cd /my/linux/path; git grep %s" f) in
> let other_files =
>   List.map (function x -> List.hd(Str.split (Str.regexp ":") x)) l in
> let file = (List.hd p).file in
> match List.filter (function x -> not(x = file)) other_files with
>   [] -> Printf.printf "function %s is only used in file %s\n" f file
> | _ -> ()
> 
> That is, find the definition of a function with the pattern matching rule,
> then obtain the name and position of the function definition, then in the
> ocaml/python rule, grep for the name of the function, and extract the
> names of the files from grep.  Drop all the names that are the same as
> that of the file containing the definition, and print an error message if
> the result is an empty list.
> 
> The disable optional_storage at the beginning of the first rule prevents
> the function definition from matching a static function, since static
> isn't mentioned in the pattern.
> 
> This takes care of the case of function definitions.  Something similar
> could probably be done for other kinds fo variables.  That could be a bit
> more awkward, though because variable declarations both inside and outside
> function definitions look alike.

Ok, I see. Yes, this looks nice, though there might still be a small number
of false positives (e.g. identifier used in inline function in the header
that declares it, inline function used elsewhere) and false negatives
(function name mentioned in a .c file comment elsewhere but not actually
called).

I don't actually understand half the code you write, but conceptually
it makes sense and is similar to what I did manually. If you plan to
actually make use of this method, there are a couple of improvements
that I can think of:

- "git grep -w" to match only whole words
- "git grep -l" to list the file names only, rather than manually parsing
  the default grep output
- filter out declarations from header files as you mentioned (I don't
  see this in your code).

	Arnd