[Cocci] struct type renaming in the absence of certain function calls on members

[Cocci] struct type renaming in the absence of certain function calls on members

[Jason A. Donenfeld]

Hi,

I'm trying to write a spatch rule that has some inversion logic for a
given struct member, but I'm struggling to express this in SmPL. I'm
also a bit of a coccinelle novice, however. Specifically, I'm trying
to express:

For all structs, for each member `M` of type `struct rwlock`, if there
are calls to `rwlock_wlock(&M)` and `rwlock_wunlock(&M)`, but there
are no calls to `rwlock_rlock(&M)` or `rwlock_runlock(&M)`, then
change `M`'s type from `struct rwlock` to `struct lock` and change
calls to `rwlock_wlock(&M)` and `rwlock_wunlock(&M)` to
`lock_lock(&M)` and `lock_unlock(&M)`, respectively.

In other words, "if I'm not using the reader part of an rwlock, make
it into a normal boring lock."

I realize that tracking whether, in a call to f(&something->M),
something is actually of the type in which rwlock was replaced is kind
of hard (I think?). But I'd be willing to compromise on just assuming
that something->M is always correct, because M always has
unique-enough names. Or maybe that compromise isn't needed due to some
sort of amazing coccinelle type inference, but anyway, I'm willing to
compromise.

The trickiest part seems to be, however, only doing this in the case
where there aren't calls to `rwlock_rlock(&M)` and
`rwlock_runlock(&M)`. I tried using a virtual rule and a depends
clause, but those dependencies don't seem to work over a given
instance of an identifier.

I could probably hack my way toward that with a ridiculous sed
expression, or do it procedurally after parsing the AST. But it seems
like this would be a good case for where Coccinelle makes things
easier, so I thought I'd commit to getting it done with spatch. Any
pointers would be appreciated.

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

Re: [Cocci] struct type renaming in the absence of certain function calls on members

[Julia Lawall]

On Sat, 5 Jun 2021, Jason A. Donenfeld wrote:

> Hi,
>
> I'm trying to write a spatch rule that has some inversion logic for a
> given struct member, but I'm struggling to express this in SmPL. I'm
> also a bit of a coccinelle novice, however. Specifically, I'm trying
> to express:
>
> For all structs, for each member `M` of type `struct rwlock`, if there
> are calls to `rwlock_wlock(&M)` and `rwlock_wunlock(&M)`, but there
> are no calls to `rwlock_rlock(&M)` or `rwlock_runlock(&M)`, then
> change `M`'s type from `struct rwlock` to `struct lock` and change
> calls to `rwlock_wlock(&M)` and `rwlock_wunlock(&M)` to
> `lock_lock(&M)` and `lock_unlock(&M)`, respectively.
>
> In other words, "if I'm not using the reader part of an rwlock, make
> it into a normal boring lock."
>
> I realize that tracking whether, in a call to f(&something->M),
> something is actually of the type in which rwlock was replaced is kind
> of hard (I think?). But I'd be willing to compromise on just assuming
> that something->M is always correct, because M always has
> unique-enough names. Or maybe that compromise isn't needed due to some
> sort of amazing coccinelle type inference, but anyway, I'm willing to
> compromise.
>
> The trickiest part seems to be, however, only doing this in the case
> where there aren't calls to `rwlock_rlock(&M)` and
> `rwlock_runlock(&M)`. I tried using a virtual rule and a depends
> clause, but those dependencies don't seem to work over a given
> instance of an identifier.
>
> I could probably hack my way toward that with a ridiculous sed
> expression, or do it procedurally after parsing the AST. But it seems
> like this would be a good case for where Coccinelle makes things
> easier, so I thought I'd commit to getting it done with spatch. Any
> pointers would be appreciated.

A possible semantic patch is below.  Note that the first line with the
#spatch gives some implicit command line options.  You may want to change
the number of cores.  Note that the semantic patch will directly modify
your code, so don't run it on anything you dont' want to destroy...

There are two parts.  The first part finds relevant structures and locking
functions, and stores which operations are used on which structures in
some hash tables.  That part is run over the entire code base.

After the completion of the first part, the second part looks for
structure definitions and calls that can be changed.

Hopefully the code based doesn't have multiple definitions of the same
structure with different properties.  If that is a concern, it could be
possible to collect the names of the relevant structure definitions in the
first phase as well, and complain about or ignore structure names that are
defined twice.

julia

#spatch -j 4 --recursive-includes --include-headers-for-types --include-headers --in-place

virtual after_start

@initialize:ocaml@
@@

let has_write_table = Hashtbl.create 101
let has_read_table = Hashtbl.create 101

let ok i m =
  let entry = (i,m) in
  Hashtbl.mem has_write_table entry && not(Hashtbl.mem has_read_table entry)

@hasw depends on !after_start@
identifier i,m;
struct i x;
@@

(
rwlock_wlock(&x.m)
|
rwlock_wunlock(&x.m)
)

@script:ocaml@
i << hasw.i;
m << hasw.m;
@@
Hashtbl.replace has_write_table (i,m) ()

@hasr depends on !after_start@
identifier i,m;
struct i x;
@@

(
rwlock_rlock(&x.m)
|
rwlock_runlock(&x.m)
)

@script:ocaml@
i << hasr.i;
m << hasr.m;
@@
Hashtbl.replace has_read_table (i,m) ()

@finalize:ocaml depends on !after_start@
wt << merge.has_write_table;
rt << merge.has_read_table;
@@

let redo ts dst =
  List.iter (Hashtbl.iter (fun k _ -> Hashtbl.add dst k ())) ts in
redo wt has_write_table;
redo rt has_read_table;

let it = new iteration() in
it#add_virtual_rule After_start;
it#register()

(* ----------------------------------------------------------- *)

@ty2 depends on after_start@
identifier i;
identifier m : script:ocaml(i) { ok i m };
@@

struct i {
  ...
- struct rwlock m;
+ struct lock m;
  ...
}

@depends on after_start disable fld_to_ptr@
identifier m;
identifier i : script:ocaml(m) { ok i m };
struct i x;
@@

- rwlock_wlock
+ lock_lock
   (&x.m)

@depends on after_start disable fld_to_ptr@
identifier m;
identifier i : script:ocaml(m) { ok i m };
struct i x;
@@

- rwlock_wunlock
+ lock_unlock
   (&x.m)

@depends on after_start disable fld_to_ptr, ptr_to_array@
identifier m;
identifier i : script:ocaml(m) { ok i m };
struct i *x;
@@

- rwlock_wlock
+ lock_lock
   (&x->m)

@depends on after_start disable fld_to_ptr, ptr_to_array@
identifier m;
identifier i : script:ocaml(m) { ok i m };
struct i *x;
@@

- rwlock_wunlock
+ lock_unlock
   (&x->m)
_______________________________________________
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci

Re: [Cocci] struct type renaming in the absence of certain function calls on members

[Jason A. Donenfeld]

Hi Julia,

Holy mackerel, that's amazing. Thank you so much! Very fancy embedding
the ocaml in there, and just using a hash table. Neat idea.

I'm running it (well, a modified version, pasted below) on a codebase
and finding that it fails to replace the "struct rwlock x" with "struct
mtx x" in most cases, except one, which is strange. I wonder if it's the
parser choking on macros it doesn't understand in the code? Or on
something else?

Code is here, if you're curious:

$ git clone https://git.zx2c4.com/wireguard-freebsd
$ cd wireguard-freebsd/src
$ spatch.opt --sp-file doit.cocci -j 4 --recursive-includes --include-headers-for-types --include-headers --in-place .

I'll keep playing with it to see what's happening...

Jason

== doit.cocci ==

virtual after_start

@initialize:ocaml@
@@

let has_write_table = Hashtbl.create 101
let has_read_table = Hashtbl.create 101

let ok i m =
  let entry = (i,m) in
  Hashtbl.mem has_write_table entry && not(Hashtbl.mem has_read_table entry)

@hasw depends on !after_start@
identifier i,m;
struct i x;
@@

(
rw_wlock(&x.m)
|
rw_wunlock(&x.m)
)

@script:ocaml@
i << hasw.i;
m << hasw.m;
@@
Hashtbl.replace has_write_table (i,m) ()

@hasr depends on !after_start@
identifier i,m;
struct i x;
@@

(
rw_rlock(&x.m)
|
rw_runlock(&x.m)
)

@script:ocaml@
i << hasr.i;
m << hasr.m;
@@
Hashtbl.replace has_read_table (i,m) ()

@finalize:ocaml depends on !after_start@
wt << merge.has_write_table;
rt << merge.has_read_table;
@@

let redo ts dst =
  List.iter (Hashtbl.iter (fun k _ -> Hashtbl.add dst k ())) ts in
redo wt has_write_table;
redo rt has_read_table;

let it = new iteration() in
it#add_virtual_rule After_start;
it#register()

(* ----------------------------------------------------------- *)

@ty2 depends on after_start@
identifier i;
identifier m : script:ocaml(i) { ok i m };
@@

struct i {
  ...
- struct rwlock m;
+ struct mtx m;
  ...
}

@depends on after_start disable fld_to_ptr@
identifier m;
identifier i : script:ocaml(m) { ok i m };
struct i x;
@@

- rw_wlock
+ mtx_lock
   (&x.m)

@depends on after_start disable fld_to_ptr@
identifier m;
identifier i : script:ocaml(m) { ok i m };
struct i x;
@@

- rw_wunlock
+ mtx_unlock
   (&x.m)

@depends on after_start disable fld_to_ptr@
identifier m;
expression e;
identifier i : script:ocaml(m) { ok i m };
struct i x;
@@

- rw_init(&x.m, e);
+ mtx_init(&x.m, e, NULL, MTX_DEF);

@depends on after_start disable fld_to_ptr@
identifier m;
identifier i : script:ocaml(m) { ok i m };
struct i x;
@@

- rw_destroy
+ mtx_destroy
   (&x.m)

@depends on after_start disable fld_to_ptr, ptr_to_array@
identifier m;
identifier i : script:ocaml(m) { ok i m };
struct i *x;
@@

- rw_wlock
+ mtx_lock
   (&x->m)

@depends on after_start disable fld_to_ptr, ptr_to_array@
identifier m;
identifier i : script:ocaml(m) { ok i m };
struct i *x;
@@

- rw_wunlock
+ mtx_unlock
   (&x->m)

@depends on after_start disable fld_to_ptr, ptr_to_array@
identifier m;
expression e;
identifier i : script:ocaml(m) { ok i m };
struct i *x;
@@

- rw_init(&x->m, e);
+ mtx_init(&x->m, e, NULL, MTX_DEF);

@depends on after_start disable fld_to_ptr, ptr_to_array@
identifier m;
identifier i : script:ocaml(m) { ok i m };
struct i *x;
@@

- rw_destroy
+ mtx_destroy
   (&x->m)
_______________________________________________
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci

Re: [Cocci] struct type renaming in the absence of certain function calls on members

[Julia Lawall]

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



On Sat, 5 Jun 2021, Jason A. Donenfeld wrote:

> Hi Julia,
>
> Holy mackerel, that's amazing. Thank you so much! Very fancy embedding
> the ocaml in there, and just using a hash table. Neat idea.
>
> I'm running it (well, a modified version, pasted below) on a codebase
> and finding that it fails to replace the "struct rwlock x" with "struct
> mtx x" in most cases, except one, which is strange. I wonder if it's the
> parser choking on macros it doesn't understand in the code? Or on
> something else?
>
> Code is here, if you're curious:
>
> $ git clone https://git.zx2c4.com/wireguard-freebsd
> $ cd wireguard-freebsd/src
> $ spatch.opt --sp-file doit.cocci -j 4 --recursive-includes --include-headers-for-types --include-headers --in-place .
>
> I'll keep playing with it to see what's happening...

Thanks for the link to the code.  Maybe you need a -I option to tell it
where the header files are?  To check for parsing problems, you can say
spatch --parse-c wireguard-freebsd.  You may need to remove any limit on
the stacksize.

julia

>
> Jason
>
> == doit.cocci ==
>
> virtual after_start
>
> @initialize:ocaml@
> @@
>
> let has_write_table = Hashtbl.create 101
> let has_read_table = Hashtbl.create 101
>
> let ok i m =
>   let entry = (i,m) in
>   Hashtbl.mem has_write_table entry && not(Hashtbl.mem has_read_table entry)
>
> @hasw depends on !after_start@
> identifier i,m;
> struct i x;
> @@
>
> (
> rw_wlock(&x.m)
> |
> rw_wunlock(&x.m)
> )
>
> @script:ocaml@
> i << hasw.i;
> m << hasw.m;
> @@
> Hashtbl.replace has_write_table (i,m) ()
>
> @hasr depends on !after_start@
> identifier i,m;
> struct i x;
> @@
>
> (
> rw_rlock(&x.m)
> |
> rw_runlock(&x.m)
> )
>
> @script:ocaml@
> i << hasr.i;
> m << hasr.m;
> @@
> Hashtbl.replace has_read_table (i,m) ()
>
> @finalize:ocaml depends on !after_start@
> wt << merge.has_write_table;
> rt << merge.has_read_table;
> @@
>
> let redo ts dst =
>   List.iter (Hashtbl.iter (fun k _ -> Hashtbl.add dst k ())) ts in
> redo wt has_write_table;
> redo rt has_read_table;
>
> let it = new iteration() in
> it#add_virtual_rule After_start;
> it#register()
>
> (* ----------------------------------------------------------- *)
>
> @ty2 depends on after_start@
> identifier i;
> identifier m : script:ocaml(i) { ok i m };
> @@
>
> struct i {
>   ...
> - struct rwlock m;
> + struct mtx m;
>   ...
> }
>
> @depends on after_start disable fld_to_ptr@
> identifier m;
> identifier i : script:ocaml(m) { ok i m };
> struct i x;
> @@
>
> - rw_wlock
> + mtx_lock
>    (&x.m)
>
> @depends on after_start disable fld_to_ptr@
> identifier m;
> identifier i : script:ocaml(m) { ok i m };
> struct i x;
> @@
>
> - rw_wunlock
> + mtx_unlock
>    (&x.m)
>
> @depends on after_start disable fld_to_ptr@
> identifier m;
> expression e;
> identifier i : script:ocaml(m) { ok i m };
> struct i x;
> @@
>
> - rw_init(&x.m, e);
> + mtx_init(&x.m, e, NULL, MTX_DEF);
>
> @depends on after_start disable fld_to_ptr@
> identifier m;
> identifier i : script:ocaml(m) { ok i m };
> struct i x;
> @@
>
> - rw_destroy
> + mtx_destroy
>    (&x.m)
>
> @depends on after_start disable fld_to_ptr, ptr_to_array@
> identifier m;
> identifier i : script:ocaml(m) { ok i m };
> struct i *x;
> @@
>
> - rw_wlock
> + mtx_lock
>    (&x->m)
>
> @depends on after_start disable fld_to_ptr, ptr_to_array@
> identifier m;
> identifier i : script:ocaml(m) { ok i m };
> struct i *x;
> @@
>
> - rw_wunlock
> + mtx_unlock
>    (&x->m)
>
> @depends on after_start disable fld_to_ptr, ptr_to_array@
> identifier m;
> expression e;
> identifier i : script:ocaml(m) { ok i m };
> struct i *x;
> @@
>
> - rw_init(&x->m, e);
> + mtx_init(&x->m, e, NULL, MTX_DEF);
>
> @depends on after_start disable fld_to_ptr, ptr_to_array@
> identifier m;
> identifier i : script:ocaml(m) { ok i m };
> struct i *x;
> @@
>
> - rw_destroy
> + mtx_destroy
>    (&x->m)
>

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

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

Re: [Cocci] struct type renaming in the absence of certain function calls on members

[Julia Lawall]

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



On Sat, 5 Jun 2021, Jason A. Donenfeld wrote:

> Hi Julia,
>
> Holy mackerel, that's amazing. Thank you so much! Very fancy embedding
> the ocaml in there, and just using a hash table. Neat idea.
>
> I'm running it (well, a modified version, pasted below) on a codebase
> and finding that it fails to replace the "struct rwlock x" with "struct
> mtx x" in most cases, except one, which is strange. I wonder if it's the
> parser choking on macros it doesn't understand in the code? Or on
> something else?

It seems to be choking on the macros...

julia

>
> Code is here, if you're curious:
>
> $ git clone https://git.zx2c4.com/wireguard-freebsd
> $ cd wireguard-freebsd/src
> $ spatch.opt --sp-file doit.cocci -j 4 --recursive-includes --include-headers-for-types --include-headers --in-place .
>
> I'll keep playing with it to see what's happening...
>
> Jason
>
> == doit.cocci ==
>
> virtual after_start
>
> @initialize:ocaml@
> @@
>
> let has_write_table = Hashtbl.create 101
> let has_read_table = Hashtbl.create 101
>
> let ok i m =
>   let entry = (i,m) in
>   Hashtbl.mem has_write_table entry && not(Hashtbl.mem has_read_table entry)
>
> @hasw depends on !after_start@
> identifier i,m;
> struct i x;
> @@
>
> (
> rw_wlock(&x.m)
> |
> rw_wunlock(&x.m)
> )
>
> @script:ocaml@
> i << hasw.i;
> m << hasw.m;
> @@
> Hashtbl.replace has_write_table (i,m) ()
>
> @hasr depends on !after_start@
> identifier i,m;
> struct i x;
> @@
>
> (
> rw_rlock(&x.m)
> |
> rw_runlock(&x.m)
> )
>
> @script:ocaml@
> i << hasr.i;
> m << hasr.m;
> @@
> Hashtbl.replace has_read_table (i,m) ()
>
> @finalize:ocaml depends on !after_start@
> wt << merge.has_write_table;
> rt << merge.has_read_table;
> @@
>
> let redo ts dst =
>   List.iter (Hashtbl.iter (fun k _ -> Hashtbl.add dst k ())) ts in
> redo wt has_write_table;
> redo rt has_read_table;
>
> let it = new iteration() in
> it#add_virtual_rule After_start;
> it#register()
>
> (* ----------------------------------------------------------- *)
>
> @ty2 depends on after_start@
> identifier i;
> identifier m : script:ocaml(i) { ok i m };
> @@
>
> struct i {
>   ...
> - struct rwlock m;
> + struct mtx m;
>   ...
> }
>
> @depends on after_start disable fld_to_ptr@
> identifier m;
> identifier i : script:ocaml(m) { ok i m };
> struct i x;
> @@
>
> - rw_wlock
> + mtx_lock
>    (&x.m)
>
> @depends on after_start disable fld_to_ptr@
> identifier m;
> identifier i : script:ocaml(m) { ok i m };
> struct i x;
> @@
>
> - rw_wunlock
> + mtx_unlock
>    (&x.m)
>
> @depends on after_start disable fld_to_ptr@
> identifier m;
> expression e;
> identifier i : script:ocaml(m) { ok i m };
> struct i x;
> @@
>
> - rw_init(&x.m, e);
> + mtx_init(&x.m, e, NULL, MTX_DEF);
>
> @depends on after_start disable fld_to_ptr@
> identifier m;
> identifier i : script:ocaml(m) { ok i m };
> struct i x;
> @@
>
> - rw_destroy
> + mtx_destroy
>    (&x.m)
>
> @depends on after_start disable fld_to_ptr, ptr_to_array@
> identifier m;
> identifier i : script:ocaml(m) { ok i m };
> struct i *x;
> @@
>
> - rw_wlock
> + mtx_lock
>    (&x->m)
>
> @depends on after_start disable fld_to_ptr, ptr_to_array@
> identifier m;
> identifier i : script:ocaml(m) { ok i m };
> struct i *x;
> @@
>
> - rw_wunlock
> + mtx_unlock
>    (&x->m)
>
> @depends on after_start disable fld_to_ptr, ptr_to_array@
> identifier m;
> expression e;
> identifier i : script:ocaml(m) { ok i m };
> struct i *x;
> @@
>
> - rw_init(&x->m, e);
> + mtx_init(&x->m, e, NULL, MTX_DEF);
>
> @depends on after_start disable fld_to_ptr, ptr_to_array@
> identifier m;
> identifier i : script:ocaml(m) { ok i m };
> struct i *x;
> @@
>
> - rw_destroy
> + mtx_destroy
>    (&x->m)
>

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

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

Re: [Cocci] struct type renaming in the absence of certain function calls on members

[Julia Lawall]

Try making a file def.h:

#define CK_LIST_HEAD(x,y) int
#define CK_LIST_ENTRY(x) int

and then add to your command line

--macro-file def.h

There seem to be a few other such macros that are needed.

Unrelatedly, you can also add --very-quiet to the command line, to have a
less verbose output.

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

Re: [Cocci] struct type renaming in the absence of certain function calls on members

[Jason A. Donenfeld]

Thanks! That seems to be it indeed. Interestingly, including the
actual path to the real headers with -I didn't fix the issue. I tried
adding other options passed as -D to the compiler in a macro file, in
case that was influencing the parsing, to no avail. So in the end I
indeed went with just defining those to int. Seems to work well! Last
minor cosmetic issue is that it turns `struct a
[space][space][space][space][space][space][space] x` into `struct b
[space] x`, which is easy enough to fix up by hand. (The BSDs do weird
and terrible things with whitespace sometimes.)

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

Re: [Cocci] struct type renaming in the absence of certain function calls on members

[Julia Lawall]

On Sat, 5 Jun 2021, Jason A. Donenfeld wrote:

> Thanks! That seems to be it indeed. Interestingly, including the
> actual path to the real headers with -I didn't fix the issue.

Most of the time, Coccinelle doesn't use macro definitions.  The -I could
have been useful for figuring out some types, but probably wasn't needed
in this case.

> I tried
> adding other options passed as -D to the compiler in a macro file, in
> case that was influencing the parsing, to no avail. So in the end I
> indeed went with just defining those to int. Seems to work well! Last
> minor cosmetic issue is that it turns `struct a
> [space][space][space][space][space][space][space] x` into `struct b
> [space] x`, which is easy enough to fix up by hand.

That's beyond the degree of cleverness we try t oprovide.  but in this
particular case, it would probably not have been needed if the rule for
changing the type name had only changed the name, and not the whole member
declaration.

julia


> (The BSDs do weird
> and terrible things with whitespace sometimes.)
>
> Jason
>
_______________________________________________
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci

Re: [Cocci] struct type renaming in the absence of certain function calls on members

[Jason A. Donenfeld]

That makes sense. Thank you so much for your help. Committed here (and
credited you as co-author):
https://git.zx2c4.com/wireguard-freebsd/commit/?id=b13579613ffcbe56c28df79972a74025007a85b7

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

Re: [Cocci] struct type renaming in the absence of certain function calls on members

[Julia Lawall]

On Sat, 5 Jun 2021, Jason A. Donenfeld wrote:

> That makes sense. Thank you so much for your help. Committed here (and
> credited you as co-author):
> https://git.zx2c4.com/wireguard-freebsd/commit/?id=b13579613ffcbe56c28df79972a74025007a85b7

Cool.  Thanks :)

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