Re: [Cocci] struct type renaming in the absence of certain function calls on members
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()
|
rw_wunlock()
)
@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()
|
rw_runlock()
)
@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
()
@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
()
@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(, e);
+ mtx_init(, 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
()
@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
(>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
(>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(>m, e);
+ mtx_init(>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
(>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
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
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
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
Re: [Cocci] struct type renaming in the absence of certain function calls on members
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
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
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()
> |
> rw_wunlock()
> )
>
> @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()
> |
> rw_runlock()
> )
>
> @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
> ()
>
> @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
> ()
>
> @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(, e);
> + mtx_init(, 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
> ()
>
> @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
> (>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
> (>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(>m, e);
> + mtx_init(>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
> (>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
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()
> |
> rw_wunlock()
> )
>
> @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()
> |
> rw_runlock()
> )
>
> @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
> ()
>
> @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
> ()
>
> @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(, e);
> + mtx_init(, 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
> ()
>
> @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
> (>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
> (>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(>m, e);
> + mtx_init(>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
> (>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
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()` and `rwlock_wunlock()`, but there
> are no calls to `rwlock_rlock()` or `rwlock_runlock()`, then
> change `M`'s type from `struct rwlock` to `struct lock` and change
> calls to `rwlock_wlock()` and `rwlock_wunlock()` to
> `lock_lock()` and `lock_unlock()`, 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(>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()` and
> `rwlock_runlock()`. 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()
|
rwlock_wunlock()
)
@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()
|
rwlock_runlock()
)
@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
()
@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
()
@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
(>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
(>m)
___
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci
