[Cocci] modifying initializers with spatch?

[Cocci] modifying initializers with spatch?

[Johannes Berg]

Hi,

I've been playing with C99 initializers a bit, what I'm trying to do is
replace them with explicit memset()/assignments.

This works as expected:

@@
identifier x;
type t;
@@
?t x =
-{0};
+{};


This actually removes the braces, but not only if they're empty, and it
leaves the inner content there without braces, which won't even compile
afterwards:

@@
identifier x;
type t;
@@
?t x
- ={}
?;

e.g.:

-	const u8 q[] = { 1, (const u8) '*', 0 };
+	const u8 q[] 1, (const u8) '*', 0;


I've also not managed to add any memset(&x, 0, sizeof(x)); in the same
patch, especially since it should go after all other declarations.

Do you think this is possible at all?

johannes

[Cocci] modifying initializers with spatch?

[Julia Lawall]

On Thu, 26 Jan 2017, Johannes Berg wrote:

> Hi,
>
> I've been playing with C99 initializers a bit, what I'm trying to do is
> replace them with explicit memset()/assignments.
>
> This works as expected:
>
> @@
> identifier x;
> type t;
> @@
> ?t x =
> -{0};
> +{};
>
>
> This actually removes the braces, but not only if they're empty, and it
> leaves the inner content there without braces, which won't even compile
> afterwards:
>
> @@
> identifier x;
> type t;
> @@
> ?t x
> - ={}
> ?;

I'm not sure what you are trying to do here.  Remove only the empty ones?

>
> e.g.:
>
> -	const u8 q[] = { 1, (const u8) '*', 0 };
> +	const u8 q[] 1, (const u8) '*', 0;
>
>
> I've also not managed to add any memset(&x, 0, sizeof(x)); in the same
> patch, especially since it should go after all other declarations.

Here you would want something like:

f(...) {
  when != S
+ memset(...);
  S1
  ...
}

S and S1 should be statements.  If there could be multiple memsets at the
beginning of a single function, use ++ instead of +.

julia

>
> Do you think this is possible at all?
>
> johannes
> _______________________________________________
> Cocci mailing list
> Cocci at systeme.lip6.fr
> https://systeme.lip6.fr/mailman/listinfo/cocci
>

[Cocci] modifying initializers with spatch?

[Johannes Berg]

> > @@
> > identifier x;
> > type t;
> > @@
> > ?t x
> > - ={}
> > ?;
> 
> I'm not sure what you are trying to do here.??Remove only the empty
> ones?

I actually tried something more like

@@
@@
identifier x;
type t;
@@
 t x
- ={}
 ;
+memset(&x, 0, sizeof(x));

but that didn't really work either :)

> > I've also not managed to add any memset(&x, 0, sizeof(x)); in the
> > same
> > patch, especially since it should go after all other declarations.
> 
> Here you would want something like:
> 
> f(...) {
> ? when != S
> + memset(...);
> ? S1
> ? ...
> }
> 
> S and S1 should be statements.??If there could be multiple memsets at
> the beginning of a single function, use ++ instead of +.

Ok, so I should combine with the previous one before - do I need the
f(...) or are just the braces enough? This could be in a nested block,
after all.

johannes

[Cocci] modifying initializers with spatch?

[Julia Lawall]

On Thu, 26 Jan 2017, Johannes Berg wrote:

>
> > > @@
> > > identifier x;
> > > type t;
> > > @@
> > > ?t x
> > > - ={}
> > > ?;
> >
> > I'm not sure what you are trying to do here.??Remove only the empty
> > ones?
>
> I actually tried something more like
>
> @@
> @@
> identifier x;
> type t;
> @@
>  t x
> - ={}
>  ;
> +memset(&x, 0, sizeof(x));
>
> but that didn't really work either :)
>
> > > I've also not managed to add any memset(&x, 0, sizeof(x)); in the
> > > same
> > > patch, especially since it should go after all other declarations.
> >
> > Here you would want something like:
> >
> > f(...) {
> > ? when != S
> > + memset(...);
> > ? S1
> > ? ...
> > }
> >
> > S and S1 should be statements.??If there could be multiple memsets at
> > the beginning of a single function, use ++ instead of +.
>
> Ok, so I should combine with the previous one before - do I need the
> f(...) or are just the braces enough? This could be in a nested block,
> after all.

Try for the braces.  Maybe something like

{
  ... when != { ... }
  T x
-     = {0}
  ;
  ... when != S
      when any
++ memset(...)
   S1
   ... when any
}

[Cocci] modifying initializers with spatch?

[Johannes Berg]

> Try for the braces.??Maybe something like
> 
> {
> ? ... when != { ... }
> ? T x
> -?????= {0}
> ? ;
> ? ... when != S
> ??????when any
> ++ memset(...)
> ???S1
> ???... when any
> }

Yep, thanks! (I'm on the phone, and didn't get a chance to try yet)

That works for anything that's actually "= {0}" today, but for I can't
even match "= {}" properly, it seems?

johannes

[Cocci] modifying initializers with spatch?

[Julia Lawall]

On Thu, 26 Jan 2017, Johannes Berg wrote:

> > Try for the braces.??Maybe something like
> >
> > {
> > ? ... when != { ... }
> > ? T x
> > -?????= {0}
> > ? ;
> > ? ... when != S
> > ??????when any
> > ++ memset(...)
> > ???S1
> > ???... when any
> > }
>
> Yep, thanks! (I'm on the phone, and didn't get a chance to try yet)
>
> That works for anything that's actually "= {0}" today, but for I can't
> even match "= {}" properly, it seems?

The problem is that Coccinelle lets you put fewer field initializers than
are actually available.  The reasoning is that they are optional for
fields, and the user doesn't really know how many will be present in each
case.  {} is the degenerate case where none are mentioned.  But that
should not apply to your case, 1) because you are removing the braces, and
2) because you aren't doing field initializations anyway.  I'll have to
look into it.

julia

[Cocci] modifying initializers with spatch?

[Johannes Berg]

> The problem is that Coccinelle lets you put fewer field initializers
> than
> are actually available.??The reasoning is that they are optional for
> fields, and the user doesn't really know how many will be present in
> each
> case.??{} is the degenerate case where none are mentioned.??But that
> should not apply to your case, 1) because you are removing the
> braces, and
> 2) because you aren't doing field initializations anyway.??I'll have
> to look into it.

Ok, thanks.

I actually had a separate thought, that I could perhaps do something
like


{
... when != { ... }
T x = {
-  .field = E,
};
... when != S
    when any
++  x.field = E;


To get rid of all the initializers, but (as I'm still on the phone) I
haven't tried it yet :)

Btw, how was the ++ supposed to work? I had to run this more than once
to actually make it catch multiple instances of = {0}. Also, in the
case above, even if it works, the order of initializations might be
inverted if I run the spatch multiple times, so not sure I should do
that.

johannes

[Cocci] modifying initializers with spatch?

[Julia Lawall]

On Thu, 26 Jan 2017, Johannes Berg wrote:

>
> > The problem is that Coccinelle lets you put fewer field initializers
> > than
> > are actually available.??The reasoning is that they are optional for
> > fields, and the user doesn't really know how many will be present in
> > each
> > case.??{} is the degenerate case where none are mentioned.??But that
> > should not apply to your case, 1) because you are removing the
> > braces, and
> > 2) because you aren't doing field initializations anyway.??I'll have
> > to look into it.
>
> Ok, thanks.
>
> I actually had a separate thought, that I could perhaps do something
> like
>
>
> {
> ... when != { ... }
> T x = {
> -  .field = E,
> };
> ... when != S
>     when any
> ++  x.field = E;
>
>
> To get rid of all the initializers, but (as I'm still on the phone) I
> haven't tried it yet :)
>
> Btw, how was the ++ supposed to work? I had to run this more than once
> to actually make it catch multiple instances of = {0}. Also, in the
> case above, even if it works, the order of initializations might be
> inverted if I run the spatch multiple times, so not sure I should do
> that.

Put a when any above the T x = line.

On the other hand, ++ makes no guarantee about the order in which things
are generated.

To ensure the order, put the when any above the T x line, and drop the one
below and run multiple times.  Then each run will work on the last one and
put it first, so they will come out in order.

julia

>
> johannes
>

[Cocci] modifying initializers with spatch?

[Julia Lawall]

Partial progress:  The github version will no longer match {} against
{ 0, 1, 2 }, but it persists in matching {} against { .a = 1, .b = 2 },
and will still remove the outer braces without removing the rest.

julia

[Cocci] modifying initializers with spatch?

[Johannes Berg]

On Fri, 2017-01-27 at 08:09 +0100, Julia Lawall wrote:
> Partial progress:??The github version will no longer match {} against
> { 0, 1, 2 }, but it persists in matching {} against { .a = 1, .b = 2
> },
> and will still remove the outer braces without removing the rest.

Out of curiosity: will it require some form of "..." to match something
inside now?

We have a few things like this:

@@
identifier x;
identifer fn;
@@
my_time x = {
+#ifdef XYZ
 .this_method_needs_xyz = fn,
+#endif
 ...
 };

in backports etc. and I've always wondered why it didn't need (or even
accept, IIRC) ... before the +#ifdef line.

johannes

[Cocci] modifying initializers with spatch?

[Julia Lawall]

On Fri, 27 Jan 2017, Johannes Berg wrote:

> On Fri, 2017-01-27 at 08:09 +0100, Julia Lawall wrote:
> > Partial progress:??The github version will no longer match {} against
> > { 0, 1, 2 }, but it persists in matching {} against { .a = 1, .b = 2
> > },
> > and will still remove the outer braces without removing the rest.
>
> Out of curiosity: will it require some form of "..." to match something
> inside now?

No.

>
> We have a few things like this:
>
> @@
> identifier x;
> identifer fn;
> @@
> my_time x = {
> +#ifdef XYZ
>  .this_method_needs_xyz = fn,
> +#endif
>  ...
>  };
>
> in backports etc. and I've always wondered why it didn't need (or even
> accept, IIRC) ... before the +#ifdef line.

It considers that the rule writer has no knowledge or control of the field
names that are provided or the order in which they appear, so you can
specify any subset of them.  You only need ... when you want to add a new
one at the beginning or at the end of the structure.  The ... lets you
specify beginning or end.

I want to allow you to remove the {} only when all of the field
initializations are also removed.  Some work though is required.

julia

[Cocci] modifying initializers with spatch?

[Johannes Berg]

On Fri, 2017-01-27 at 14:11 +0100, Julia Lawall wrote:

> > Out of curiosity: will it require some form of "..." to match
> > something inside now?
> 
> No.

Ok.

> It considers that the rule writer has no knowledge or control of the
> field names that are provided or the order in which they appear, so
> you can specify any subset of them.??You only need ... when you want
> to add a new one at the beginning or at the end of the
> structure.??The ... lets you specify beginning or end.

Interesting. I thought I needed the ... at the end, but that may have
been a case of "let's see if I need both - doesn't work - remove one -
ok works now" :)

> I want to allow you to remove the {} only when all of the field
> initializations are also removed.??Some work though is required.

That'd make sense.

I'm very grateful for this. I'm dealing with an awful compiler that
emits stupid code for C99 initializers, so I'm thinking of using what
we were discussing here to remove them all at build time. I'm going to
be travelling (again) next week, but will give it a try later.

Thanks a lot!

johannes

[Cocci] modifying initializers with spatch?

[Johannes Berg]

> > To get rid of all the initializers, but (as I'm still on the phone)
> > I haven't tried it yet :)
> > 
> 
> Put a when any above the T x = line.
> 
> On the other hand, ++ makes no guarantee about the order in which
> things are generated.
> 
> To ensure the order, put the when any above the T x line, and drop
> the one below and run multiple times.??Then each run will work on the
> last one and put it first, so they will come out in order.

Finally got a chance to try this.

I can't seem to get the ordering right. I'm trying this:

@@
type T;
expression E;
statement S1, S2;
identifier x;
identifier f;
@@
?{
... when != { ... }
????when any
?T x = {
-??.f = E,
?};
... when != S1
+ x.f = E;
?S2
?...
?}

on a very simple file:

int main()
{
	struct foo bar = {
		.y = 8,
		.x = 7,
	};

	printf("%d\n", bar.x);
	printf("%d\n", bar.y);
}

but it doesn't end well:

$ spatch??--sp-file /tmp/init.spatch /tmp/test.c
init_defs_builtins: /usr/lib/coccinelle/standard.h
HANDLING: /tmp/test.c
?????
previous modification:

? <<< x.f = E;
CONTEXT
According to environment 3:
???rule starting on line 1.E -> 8
???rule starting on line 1.f -> id y
???rule starting on line 1.x -> id bar

current modification:

? <<< x.f = E;
CONTEXT
According to environment 3:
???rule starting on line 1.E -> 7
???rule starting on line 1.f -> id x
???rule starting on line 1.x -> id bar

Fatal error: exception Failure("rule starting on line 1: already tagged
token:\nC code context\nFile \"/tmp/test.c\", line 8, column
1,??charpos = 59\n????around = 'printf', whole content =
\tprintf(\"%d\\n\", bar.x);")


If I use ++ instead of +, it still replaces all of them, so I can't run
multiple times (usefully anyway)

OTOH, I haven't found any place where the order actually matters (in
the code base I'm interested in), and although it seems that it
theoretically could matter, I'm not sure I really need to solve that
problem - although I'm thinking I might want to run this not just once
but before each compile, to keep the code neater.

(Note that I also haven't upgraded to the git version yet, TBD)

johannes

[Cocci] modifying initializers with spatch?

[Julia Lawall]

On Fri, 27 Jan 2017, Johannes Berg wrote:

>
> > > To get rid of all the initializers, but (as I'm still on the phone)
> > > I haven't tried it yet :)
> > >
> >
> > Put a when any above the T x = line.
> >
> > On the other hand, ++ makes no guarantee about the order in which
> > things are generated.
> >
> > To ensure the order, put the when any above the T x line, and drop
> > the one below and run multiple times.??Then each run will work on the
> > last one and put it first, so they will come out in order.
>
> Finally got a chance to try this.
>
> I can't seem to get the ordering right. I'm trying this:
>
> @@
> type T;
> expression E;
> statement S1, S2;
> identifier x;
> identifier f;
> @@
> ?{
> ... when != { ... }
> ????when any
> ?T x = {
> -??.f = E,
> ?};
> ... when != S1
> + x.f = E;
> ?S2
> ?...
> ?}
>
> on a very simple file:
>
> int main()
> {
> 	struct foo bar = {
> 		.y = 8,
> 		.x = 7,
> 	};
>
> 	printf("%d\n", bar.x);
> 	printf("%d\n", bar.y);
> }
>
> but it doesn't end well:
>
> $ spatch??--sp-file /tmp/init.spatch /tmp/test.c
> init_defs_builtins: /usr/lib/coccinelle/standard.h
> HANDLING: /tmp/test.c
> ?????
> previous modification:
>
> ? <<< x.f = E;
> CONTEXT
> According to environment 3:
> ???rule starting on line 1.E -> 8
> ???rule starting on line 1.f -> id y
> ???rule starting on line 1.x -> id bar
>
> current modification:
>
> ? <<< x.f = E;
> CONTEXT
> According to environment 3:
> ???rule starting on line 1.E -> 7
> ???rule starting on line 1.f -> id x
> ???rule starting on line 1.x -> id bar
>
> Fatal error: exception Failure("rule starting on line 1: already tagged
> token:\nC code context\nFile \"/tmp/test.c\", line 8, column
> 1,??charpos = 59\n????around = 'printf', whole content =
> \tprintf(\"%d\\n\", bar.x);")

Yeah, it doesn't work.  There is no way to get the fields out of the
structure one at a time.  So ++ seems like the only option.  The suggested
organization would work for when there is more than one structure
definition in the function, but doesn't help for multiple fields within a
structure.

julia


>
>
> If I use ++ instead of +, it still replaces all of them, so I can't run
> multiple times (usefully anyway)
>
> OTOH, I haven't found any place where the order actually matters (in
> the code base I'm interested in), and although it seems that it
> theoretically could matter, I'm not sure I really need to solve that
> problem - although I'm thinking I might want to run this not just once
> but before each compile, to keep the code neater.
>
> (Note that I also haven't upgraded to the git version yet, TBD)
>
> johannes
>

[Cocci] modifying initializers with spatch?

[Johannes Berg]

On Fri, 2017-01-27 at 22:52 +0100, Julia Lawall wrote:

> > @@
> > type T;
> > expression E;
> > statement S1, S2;
> > identifier x;
> > identifier f;
> > @@
> > ?{
> > ... when != { ... }
> > ????when any
> > ?T x = {
> > -??.f = E,
> > ?};
> > ... when != S1
> > + x.f = E;
> > ?S2
> > ?...
> > ?}

> Yeah, it doesn't work.??There is no way to get the fields out of the
> structure one at a time.??So ++ seems like the only option.??

Right. Like I said, I don't think it matters much for me right now. I
also found that most of these are actually in the part of the code that
I can change and don't really need to touch at build time, although
it'd be nice to be able to write the code in the more canonical C99
style and still do it at build time.

> The suggested
> organization would work for when there is more than one structure
> definition in the function, but doesn't help for multiple fields
> within a
> structure.

Right, it easily works for multiple structures, and it does even seem
to keep the order of variables, even if it doesn't keep the order of
the fields within each variable (but sorts them alphabetically instead,
apparently)

The git version should now actually be sufficient for me, since you
said it will no longer remove braces from "= { 1, 2, 3}", and I'm
moving all the initializers anyway like indicated above, and
translating "={0}" to "={}" as well. I'll need to build & try it, but
if it fails on some obscure cases and removes the braces anyway the
compile will fail so I'm not really worried :)

johannes

[Cocci] modifying initializers with spatch?

[Julia Lawall]

On Fri, 27 Jan 2017, Johannes Berg wrote:

> On Fri, 2017-01-27 at 22:52 +0100, Julia Lawall wrote:
>
> > > @@
> > > type T;
> > > expression E;
> > > statement S1, S2;
> > > identifier x;
> > > identifier f;
> > > @@
> > > ?{
> > > ... when != { ... }
> > > ????when any
> > > ?T x = {
> > > -??.f = E,
> > > ?};
> > > ... when != S1
> > > + x.f = E;
> > > ?S2
> > > ?...
> > > ?}
>
> > Yeah, it doesn't work.??There is no way to get the fields out of the
> > structure one at a time.??So ++ seems like the only option.??
>
> Right. Like I said, I don't think it matters much for me right now. I
> also found that most of these are actually in the part of the code that
> I can change and don't really need to touch at build time, although
> it'd be nice to be able to write the code in the more canonical C99
> style and still do it at build time.
>
> > The suggested
> > organization would work for when there is more than one structure
> > definition in the function, but doesn't help for multiple fields
> > within a
> > structure.
>
> Right, it easily works for multiple structures, and it does even seem
> to keep the order of variables, even if it doesn't keep the order of
> the fields within each variable (but sorts them alphabetically instead,
> apparently)
>
> The git version should now actually be sufficient for me, since you
> said it will no longer remove braces from "= { 1, 2, 3}", and I'm
> moving all the initializers anyway like indicated above, and
> translating "={0}" to "={}" as well. I'll need to build & try it, but
> if it fails on some obscure cases and removes the braces anyway the
> compile will fail so I'm not really worried :)

Ok, sounds good.  Let me know if you encounter further problems.

julia

[Cocci] modifying initializers with spatch?

[Johannes Berg]

On Fri, 2017-01-27 at 23:09 +0100, Julia Lawall wrote:
> 
> Ok, sounds good.??Let me know if you encounter further problems.

You jinxed it ;-)
Now with the git version.

struct foo v = {
	.nested = {
		.x = 7,
		.y = 8,
	},
};

still broke, because of course my initializer replacement rule didn't
actually find it, and then the bug hit and removed the {} anyway even
though they weren't empty :)

Similarly,

char * foo[] = {
	[1] = "name1",
	[2] = "name2",
};

was butchered quite a bit :)

The latter case - though not with strings - actually does seem to be
present in the code I care about, but I can probably just replace it
manually first.

Interestingly, this whole exercise is obviously not even a good idea
for static variables - need to see if I can exclude those somehow,
first naive attempts didn't work :)

johannes

[Cocci] modifying initializers with spatch?

[Johannes Berg]

On Fri, 2017-01-27 at 23:09 +0100, Julia Lawall wrote:
> 
> Ok, sounds good.??Let me know if you encounter further problems.

I've now ended up with the following overall:
https://p.sipsolutions.net/15e6265ea961d9a6.txt

Interestingly, that takes minutes to run on this file:
https://w1.fi/cgit/hostap/tree/wpa_supplicant/dbus/dbus_new_handlers_p2p.c

Not that it matters much to me, but I can't really explain it, all
other files I found finish quickly.

johannes

[Cocci] modifying initializers with spatch?

[Johannes Berg]

On Sat, 2017-01-28 at 00:02 +0100, Johannes Berg wrote:
> On Fri, 2017-01-27 at 23:09 +0100, Julia Lawall wrote:
> > 
> > Ok, sounds good.??Let me know if you encounter further problems.
> 
> I've now ended up with the following overall:
> https://p.sipsolutions.net/15e6265ea961d9a6.txt
> 
> Interestingly, that takes minutes to run on this file:
> https://w1.fi/cgit/hostap/tree/wpa_supplicant/dbus/dbus_new_handlers_
> p2p.c

Well, it's been 20 minutes - I'm giving up :)

johannes

[Cocci] modifying initializers with spatch?

[Julia Lawall]

On Sat, 28 Jan 2017, Johannes Berg wrote:

> On Sat, 2017-01-28 at 00:02 +0100, Johannes Berg wrote:
> > On Fri, 2017-01-27 at 23:09 +0100, Julia Lawall wrote:
> > >
> > > Ok, sounds good.??Let me know if you encounter further problems.
> >
> > I've now ended up with the following overall:
> > https://p.sipsolutions.net/15e6265ea961d9a6.txt
> >
> > Interestingly, that takes minutes to run on this file:
> > https://w1.fi/cgit/hostap/tree/wpa_supplicant/dbus/dbus_new_handlers_
> > p2p.c
>
> Well, it's been 20 minutes - I'm giving up :)

It seems that the control flow was quite complete in one function, and the
when any at the end of the rule was taking a long time.  But actually you
don't need the outer ... and braces at all.  Furthermore, from the initial
declaration to the first following statement, there should be no branches,
so it is safe to put exists on each rule, ie trace through each possible
control-flow path individually.  The revised semantic patch is attached.
It terminates quite quickly.

You can add an option --timeout 120, or some other number of seconds.  It
will tell you if there are any timeouts, so you can see where there may
be problems.

julia
-------------- next part --------------
@nonconst1@
type T;
identifier x;
position p;
@@
const T x at p = { ... };

@exists@
type T;
expression E;
statement S1, S2;
identifier x;
identifier f;
position p != nonconst1.p;
@@

 T x at p = {
-  .f = E,
 };
... when != S1
++ x.f = E;
 S2

@nonconst2@
type T;
identifier x;
position p;
@@
const T x at p = { ... };

@exists@
type T;
statement S1, S2;
identifier x;
position p != nonconst2.p;
@@

 T x at p
- = {0}
 ;
... when != S1
    when != x
++ _memclr(&x, sizeof(x));
 S2

@nonconst3@
type T;
identifier x;
position p;
@@
const T x at p = { ... };

@exists@
type T;
statement S1, S2;
identifier x;
position p != nonconst3.p;
@@

 T x at p
- = {}
 ;
... when != S1
    when != x
++ _memclr(&x, sizeof(x));
 S2

[Cocci] modifying initializers with spatch?

[Johannes Berg]

> It seems that the control flow was quite complete in one function,
> and the when any at the end of the rule was taking a long time.

Oh, interesting, but that makes sense.

> But actually you don't need the outer ... and braces at
> all.??Furthermore, from the initial declaration to the first
> following statement, there should be no branches,
> so it is safe to put exists on each rule, ie trace through each
> possible control-flow path individually.??The revised semantic patch
> is attached. It terminates quite quickly.

Good point, thanks!

> You can add an option --timeout 120, or some other number of
> seconds.??It will tell you if there are any timeouts, so you can see
> where there may be problems.

That's also useful, didn't know about that. :)

johannes