[Cocci] Matching format strings

[Cocci] Matching format strings

[Ondřej Kuzník]

Hi all,
I'm looking into what's needed to replace OpenLDAP's debug macro
implementation which at the moment always takes 3 arguments:

Debug(LEVEL, "format", arg1, arg2, arg3);

to something more useful (and get rid of the obvious compiler warnings
we get from doing things like that).

There are two (or three) cases I need to handle where I hope coccinelle
would be able to help:

1. Where less than three arguments are needed, they are always filled
with zeros:

Debug(level, "text with less than three format specifiers", 0, 0, 0);

We want to change that to

Debug( level, "format" );

or equivalent. This is by far the most common one. But obviously where
we have zeros as arguments, but there is a corresponding % format
specifier, we don't want to drop those.

2. When more are needed, usually the code looks like this:

snprintf( buffer, size, "format", ... );
Debug( level, "text: %s\n", buffer, ... );

We'd like to change that to:

Debug( level, "new format", merged arguments );

There are fewer of those but still too many for a human to maintain
while we discuss the merits of this change on openldap-devel with
regards to C89, old compilers and stuff like that :)

2a. There might be a place that reuses the buffer contents, but that's
easy to check for I guess?

Looking around the docs and the archive, there is some python support,
but I have no idea how to use it yet to pick up the format strings, test
whether a Debug line matches and prepare the change.

Is coccinelle capable of expressing a semantic patch like that, and if
yes, do you have any pointers and suggestions about how I should go
about writing it?

Thanks!

-- 
Ond?ej Kuzn?k
Senior Software Engineer
Symas Corporation                       http://www.symas.com
Packaged, certified, and supported LDAP solutions powered by OpenLDAP

[Cocci] Matching format strings

[Julia Lawall]

On Thu, 29 Jun 2017, Ond?ej Kuzn?k wrote:

> Hi all,
> I'm looking into what's needed to replace OpenLDAP's debug macro
> implementation which at the moment always takes 3 arguments:
>
> Debug(LEVEL, "format", arg1, arg2, arg3);
>

> to something more useful (and get rid of the obvious compiler warnings
> we get from doing things like that).
>
> There are two (or three) cases I need to handle where I hope coccinelle
> would be able to help:
>
> 1. Where less than three arguments are needed, they are always filled
> with zeros:
>
> Debug(level, "text with less than three format specifiers", 0, 0, 0);
>
> We want to change that to
>
> Debug( level, "format" );
>
> or equivalent. This is by far the most common one. But obviously where
> we have zeros as arguments, but there is a corresponding % format
> specifier, we don't want to drop those.

See demos/format.cocci.  So you could have somthing like

@@
format d;
expression level,str;
@@

(
Debug(level,"...%@d at ...",0,0,0)
|
- Debug(level,str,0,0,0)
+ Debug(level,str)
)

>
> 2. When more are needed, usually the code looks like this:
>
> snprintf( buffer, size, "format", ... );
> Debug( level, "text: %s\n", buffer, ... );
>
> We'd like to change that to:
>
> Debug( level, "new format", merged arguments );
>
> There are fewer of those but still too many for a human to maintain
> while we discuss the merits of this change on openldap-devel with
> regards to C89, old compilers and stuff like that :)
>
> 2a. There might be a place that reuses the buffer contents, but that's
> easy to check for I guess?

I'm not sure to understand what is wanted here.

You can indeed also use python.  Perhaps a metavariable like the following
would be helpful:

constant char[] x : script:python() { ok_string(x) };

ok_string should be defined at the top of the file in

@initialize:python@
@@

def ok_string ...

julia

> Looking around the docs and the archive, there is some python support,
> but I have no idea how to use it yet to pick up the format strings, test
> whether a Debug line matches and prepare the change.
>
> Is coccinelle capable of expressing a semantic patch like that, and if
> yes, do you have any pointers and suggestions about how I should go
> about writing it?
>
> Thanks!
>
> --
> Ond?ej Kuzn?k
> Senior Software Engineer
> Symas Corporation                       http://www.symas.com
> Packaged, certified, and supported LDAP solutions powered by OpenLDAP
> _______________________________________________
> Cocci mailing list
> Cocci at systeme.lip6.fr
> https://systeme.lip6.fr/mailman/listinfo/cocci
>

[Cocci] Matching format strings

[Ondřej Kuzník]

On Thu, Jun 29, 2017 at 10:31:43AM -0600, Julia Lawall wrote:
> 
> 
> On Thu, 29 Jun 2017, Ond?ej Kuzn?k wrote:
> 
>> Hi all,
>> I'm looking into what's needed to replace OpenLDAP's debug macro
>> implementation which at the moment always takes 3 arguments:
>>
>> Debug(LEVEL, "format", arg1, arg2, arg3);
>>
>> to something more useful (and get rid of the obvious compiler warnings
>> we get from doing things like that).
>>
>> There are two (or three) cases I need to handle where I hope coccinelle
>> would be able to help:
>>
>> 1. Where less than three arguments are needed, they are always filled
>> with zeros:
>>
>> Debug(level, "text with less than three format specifiers", 0, 0, 0);
>>
>> We want to change that to
>>
>> Debug( level, "format" );
>>
>> or equivalent. This is by far the most common one. But obviously where
>> we have zeros as arguments, but there is a corresponding % format
>> specifier, we don't want to drop those.
> 
> See demos/format.cocci.  So you could have somthing like
> 
> @@
> format d;
> expression level,str;
> @@
> 
> (
> Debug(level,"...%@d at ...",0,0,0)
> |
> - Debug(level,str,0,0,0)
> + Debug(level,str)
> )

Sweet! I missed that one, this should save me loads of time :)

Using Debian Stretch at the moment and format.cocci doesn't seem to
generate the correct patch, it outputs

--- format.c
+++ /tmp/cocci-output-16488-ebe9ee-format.c
@@ -3,5 +3,5 @@ int main () {
   foo("blah %d two %2x three %s xxx %d");
   foo("xyz %d %d %0.2f %s three\n");
   foo("xyz %d %0.2f abc");
-  foo("xxx %s");
+  foo("yyy %@d@");
 }

instead, is that a known issue in that version (as in, should I use
latest git)?

>> 2. When more are needed, usually the code looks like this:
>>
>> snprintf( buffer, size, "format", ... );
>> Debug( level, "text: %s\n", buffer, ... );
>>
>> We'd like to change that to:
>>
>> Debug( level, "new format", merged arguments );
>>
>> There are fewer of those but still too many for a human to maintain
>> while we discuss the merits of this change on openldap-devel with
>> regards to C89, old compilers and stuff like that :)
>>
>> 2a. There might be a place that reuses the buffer contents, but that's
>> easy to check for I guess?
> 
> I'm not sure to understand what is wanted here.

One of the more complicated examples is here https://github.com/openldap/openldap/blob/master/servers/slapd/back-meta/search.c#L302-L311

I would need to be able to convert

char	buf[ SLAP_TEXT_BUFLEN ];
snprintf( buf, sizeof( buf ),
        "retrying URI=\"%s\" DN=\"%s\"",
        mt->mt_uri,
        BER_BVISNULL( &msc->msc_bound_ndn ) ?
        "" : msc->msc_bound_ndn.bv_val );

Debug( LDAP_DEBUG_ANY,
        "%s meta_search_dobind_init[%d]: %s.\n",
        op->o_log_prefix, candidate, buf );

into

Debug( LDAP_DEBUG_ANY,
        "%s meta_search_dobind_init[%d]: "
        "retrying URI=\"%s\" DN=\"%s\""
        ".\n",
        op->o_log_prefix, candidate, buf,
        mt->mt_uri,
        BER_BVISNULL( &msc->msc_bound_ndn ) ?
        "" : msc->msc_bound_ndn.bv_val );

or equivalent.

I'll play around with the format string support a bit, but does it make
the above possible out of the box without extra python code?

The one I might want to be able to skip converting is
https://github.com/openldap/openldap/blob/master/servers/slapd/config.c#L801
since the buffer is used repeatedly. But I think I might be able to pull
that off.

> You can indeed also use python.  Perhaps a metavariable like the following
> would be helpful:
> 
> constant char[] x : script:python() { ok_string(x) };
> 
> ok_string should be defined at the top of the file in
> 
> @initialize:python@
> @@
> 
> def ok_string ...
> 
> julia

-- 
Ond?ej Kuzn?k
Senior Software Engineer
Symas Corporation                       http://www.symas.com
Packaged, certified, and supported LDAP solutions powered by OpenLDAP

[Cocci] Matching format strings

[Julia Lawall]

On Thu, 29 Jun 2017, Ond?ej Kuzn?k wrote:

> On Thu, Jun 29, 2017 at 10:31:43AM -0600, Julia Lawall wrote:
> >
> >
> > On Thu, 29 Jun 2017, Ond?ej Kuzn?k wrote:
> >
> >> Hi all,
> >> I'm looking into what's needed to replace OpenLDAP's debug macro
> >> implementation which at the moment always takes 3 arguments:
> >>
> >> Debug(LEVEL, "format", arg1, arg2, arg3);
> >>
> >> to something more useful (and get rid of the obvious compiler warnings
> >> we get from doing things like that).
> >>
> >> There are two (or three) cases I need to handle where I hope coccinelle
> >> would be able to help:
> >>
> >> 1. Where less than three arguments are needed, they are always filled
> >> with zeros:
> >>
> >> Debug(level, "text with less than three format specifiers", 0, 0, 0);
> >>
> >> We want to change that to
> >>
> >> Debug( level, "format" );
> >>
> >> or equivalent. This is by far the most common one. But obviously where
> >> we have zeros as arguments, but there is a corresponding % format
> >> specifier, we don't want to drop those.
> >
> > See demos/format.cocci.  So you could have somthing like
> >
> > @@
> > format d;
> > expression level,str;
> > @@
> >
> > (
> > Debug(level,"...%@d at ...",0,0,0)
> > |
> > - Debug(level,str,0,0,0)
> > + Debug(level,str)
> > )
>
> Sweet! I missed that one, this should save me loads of time :)
>
> Using Debian Stretch at the moment and format.cocci doesn't seem to
> generate the correct patch, it outputs
>
> --- format.c
> +++ /tmp/cocci-output-16488-ebe9ee-format.c
> @@ -3,5 +3,5 @@ int main () {
>    foo("blah %d two %2x three %s xxx %d");
>    foo("xyz %d %d %0.2f %s three\n");
>    foo("xyz %d %0.2f abc");
> -  foo("xxx %s");
> +  foo("yyy %@d@");
>  }
>
> instead, is that a known issue in that version (as in, should I use
> latest git)?

Use the version from github.

I will answer the rest shortly.

julia

>
> >> 2. When more are needed, usually the code looks like this:
> >>
> >> snprintf( buffer, size, "format", ... );
> >> Debug( level, "text: %s\n", buffer, ... );
> >>
> >> We'd like to change that to:
> >>
> >> Debug( level, "new format", merged arguments );
> >>
> >> There are fewer of those but still too many for a human to maintain
> >> while we discuss the merits of this change on openldap-devel with
> >> regards to C89, old compilers and stuff like that :)
> >>
> >> 2a. There might be a place that reuses the buffer contents, but that's
> >> easy to check for I guess?
> >
> > I'm not sure to understand what is wanted here.
>
> One of the more complicated examples is here https://github.com/openldap/openldap/blob/master/servers/slapd/back-meta/search.c#L302-L311
>
> I would need to be able to convert
>
> char	buf[ SLAP_TEXT_BUFLEN ];
> snprintf( buf, sizeof( buf ),
>         "retrying URI=\"%s\" DN=\"%s\"",
>         mt->mt_uri,
>         BER_BVISNULL( &msc->msc_bound_ndn ) ?
>         "" : msc->msc_bound_ndn.bv_val );
>
> Debug( LDAP_DEBUG_ANY,
>         "%s meta_search_dobind_init[%d]: %s.\n",
>         op->o_log_prefix, candidate, buf );
>
> into
>
> Debug( LDAP_DEBUG_ANY,
>         "%s meta_search_dobind_init[%d]: "
>         "retrying URI=\"%s\" DN=\"%s\""
>         ".\n",
>         op->o_log_prefix, candidate, buf,
>         mt->mt_uri,
>         BER_BVISNULL( &msc->msc_bound_ndn ) ?
>         "" : msc->msc_bound_ndn.bv_val );
>
> or equivalent.
>
> I'll play around with the format string support a bit, but does it make
> the above possible out of the box without extra python code?
>
> The one I might want to be able to skip converting is
> https://github.com/openldap/openldap/blob/master/servers/slapd/config.c#L801
> since the buffer is used repeatedly. But I think I might be able to pull
> that off.
>
> > You can indeed also use python.  Perhaps a metavariable like the following
> > would be helpful:
> >
> > constant char[] x : script:python() { ok_string(x) };
> >
> > ok_string should be defined at the top of the file in
> >
> > @initialize:python@
> > @@
> >
> > def ok_string ...
> >
> > julia
>
> --
> Ond?ej Kuzn?k
> Senior Software Engineer
> Symas Corporation                       http://www.symas.com
> Packaged, certified, and supported LDAP solutions powered by OpenLDAP
>

[Cocci] Matching format strings

[Julia Lawall]

> I would need to be able to convert
>
> char	buf[ SLAP_TEXT_BUFLEN ];
> snprintf( buf, sizeof( buf ),
>         "retrying URI=\"%s\" DN=\"%s\"",
>         mt->mt_uri,
>         BER_BVISNULL( &msc->msc_bound_ndn ) ?
>         "" : msc->msc_bound_ndn.bv_val );
>
> Debug( LDAP_DEBUG_ANY,
>         "%s meta_search_dobind_init[%d]: %s.\n",
>         op->o_log_prefix, candidate, buf );
>
> into
>
> Debug( LDAP_DEBUG_ANY,
>         "%s meta_search_dobind_init[%d]: "
>         "retrying URI=\"%s\" DN=\"%s\""
>         ".\n",
>         op->o_log_prefix, candidate, buf,
>         mt->mt_uri,
>         BER_BVISNULL( &msc->msc_bound_ndn ) ?
>         "" : msc->msc_bound_ndn.bv_val );
>
> or equivalent.
>
> I'll play around with the format string support a bit, but does it make
> the above possible out of the box without extra python code?

I don't think that the SmPL pattern language will allow you to construct
concatenated strings.  So you would need to use python here.  Try
something like the following (see demos/pythontococci.cocci):

@r1@
constant char [] c1;
constant char [] c2;
@@

f(c1);
g(c2);

@script:python res@
c1 << r1.c1;
c2 << r1.c2;
newstr;
@@

coccinelle.newstr = ... python code to concatenate c1 and c2 ...

@@
constant char [] r1.c1;
constant char [] r1.c2;
identifier res.newstr;
@@

- f(c1);
- g(c2);
+ h(res);

You may have to play with the python code a bit to make the "identifier"
being produced have string quotes in the right place.  Getting a newline
in the right place would be a further challenge.  Perhaps you could just
make an ordinary string, instead of using string concatenation.

julia

>
> The one I might want to be able to skip converting is
> https://github.com/openldap/openldap/blob/master/servers/slapd/config.c#L801
> since the buffer is used repeatedly. But I think I might be able to pull
> that off.
>
> > You can indeed also use python.  Perhaps a metavariable like the following
> > would be helpful:
> >
> > constant char[] x : script:python() { ok_string(x) };
> >
> > ok_string should be defined at the top of the file in
> >
> > @initialize:python@
> > @@
> >
> > def ok_string ...
> >
> > julia
>
> --
> Ond?ej Kuzn?k
> Senior Software Engineer
> Symas Corporation                       http://www.symas.com
> Packaged, certified, and supported LDAP solutions powered by OpenLDAP
>

[Cocci] Matching format strings

[Ondřej Kuzník]

I've just progressed a bit further on the format generation front, there
are some issues though.

With a sample file file like this (and the semantic patch at the end of
the post):

int main()
{
  char buf[BUFSIZ];
  snprintf( buf, sizeof(buf), "%d %s %d", 1, "a", 3+2 );
  Debug( LDAP_DEBUG_STATS, "%s: received %d responses from %s, how exciting!\n",
      __func__, 3, buf, extra_arg, extra_arg1, extra_arg3 );
}

Defining format1 as an expression makes python see '" % d   % s   % d "'
for some reason, what would be the reason for that? Defining it as a
format string changes that (and while python only sees a single string,
it is fine for now).

The main problem that is holding me up is that coccinelle fails with the
following error when trying to use the patch against the code above:
Fatal error: exception Common.Impossible(145)

Not sure what is happening there, and trying to replace merge in the '+'
line with "%@merged@" (with the change to its type) doesn't change that.

Thanks,
Ondrej

The semantic patch:
---------------- 8< ------- snip --------- 8< ---------------
@initialize:python@
@@

#regex from https://stackoverflow.com/questions/30011379/how-can-i-parse-a-c-format-string-in-python
import re
fmtstring = '''\
(                                  # start of capture group 1
%                                  # literal "%"
(?:                                # first option
(?:[-+0 #]{0,5})                   # optional flags
(?:\d+|\*)?                        # width
(?:\.(?:\d+|\*))?                  # precision
(?:h|l|ll|w|I|I32|I64)?            # size
[cCdiouxXeEfgGaAnpsSZ]             # type
) |                                # OR
%%)                                # literal "%%"
'''

regex = re.compile(fmtstring, re.X)

def parse_format(f):
    return tuple((m.span(), m.group()) for m in
        regex.finditer(f))

@b exists@
expression E, L;
expression list args_before, args, args_after;
identifier buf;
format list format1, format2;
position p1, p2;
@@

snprintf at p1( buf, E, "%@format1@", args );
...
Debug@p2( L, "%@format2@", args_before, buf, args_after );

@script:python b_process@
format1 << b.format1;
format2 << b.format2;
args_before << b.args_before;
merged;
@@

pos = len(args_before.elements)
formats = parse_format(format2)

span, format = formats[pos]
merged = format2[:span[0]] + format1 + format2[span[1]:]
print "Merged '%s' into '%s' ->\n\t'%s'" % (format1, format2, merged)

coccinelle.merged = merged

@b_replace@
position b.p1, b.p2;
expression b_process.merged;

expression E, L;
expression list args_before, args, args_after;
identifier buf;
format list format1, format2;
@@

-snprintf at p1( buf, E, "%@format1@", args );
+Debug( L, merged, args_before, args, args_after );
...
-Debug at p2( L, "%@format2@", args_before, buf, args_after );
---------------- 8< ------- snip --------- 8< ---------------

-- 
Ond?ej Kuzn?k
Senior Software Engineer
Symas Corporation                       http://www.symas.com
Packaged, certified, and supported LDAP solutions powered by OpenLDAP

[Cocci] Matching format strings

[Julia Lawall]

> The main problem that is holding me up is that coccinelle fails with the
> following error when trying to use the patch against the code above:
> Fatal error: exception Common.Impossible(145)

You need

identifier b_process.merged;

instead of

expression b_process.merged;

When you do that, you will see that some more quotes are needed on the
string.

julia

[Cocci] Matching format strings

[Ondřej Kuzník]

On Tue, Jul 04, 2017 at 03:32:25PM +0200, Julia Lawall wrote:
>> The main problem that is holding me up is that coccinelle fails with the
>> following error when trying to use the patch against the code above:
>> Fatal error: exception Common.Impossible(145)
> 
> You need
> 
> identifier b_process.merged;
> 
> instead of
> 
> expression b_process.merged;
> 
> When you do that, you will see that some more quotes are needed on the
> string.

That does work and seems to preserve the things that needed escaping,
so just putting quotes around when merging looks enough, great!

There is another scenario that I probably need to be able to handle, a
string like this does not seem to match anything I throw at it (and in
retrospect, you might have mentioned that earlier):

"asd" "fgh" "jkl"

Is there a way around that somehow or match, then pass into python to
handle that?

Thanks,

-- 
Ond?ej Kuzn?k
Senior Software Engineer
Symas Corporation                       http://www.symas.com
Packaged, certified, and supported LDAP solutions powered by OpenLDAP

[Cocci] Matching format strings

[Julia Lawall]

On Tue, 4 Jul 2017, Ond?ej Kuzn?k wrote:

> On Tue, Jul 04, 2017 at 03:32:25PM +0200, Julia Lawall wrote:
> >> The main problem that is holding me up is that coccinelle fails with the
> >> following error when trying to use the patch against the code above:
> >> Fatal error: exception Common.Impossible(145)
> >
> > You need
> >
> > identifier b_process.merged;
> >
> > instead of
> >
> > expression b_process.merged;
> >
> > When you do that, you will see that some more quotes are needed on the
> > string.
>
> That does work and seems to preserve the things that needed escaping,
> so just putting quotes around when merging looks enough, great!
>
> There is another scenario that I probably need to be able to handle, a
> string like this does not seem to match anything I throw at it (and in
> retrospect, you might have mentioned that earlier):
>
> "asd" "fgh" "jkl"
>
> Is there a way around that somehow or match, then pass into python to
> handle that?

This is in the C code?  I think that it should be matched by an
expression.  But run spatch --parse-c file.c to see if the containing
function has a parse error.

julia

[Cocci] Matching format strings

[Ondřej Kuzník]

On Tue, Jul 04, 2017 at 03:53:21PM +0200, Julia Lawall wrote:
> On Tue, 4 Jul 2017, Ond?ej Kuzn?k wrote:
>> That does work and seems to preserve the things that needed escaping,
>> so just putting quotes around when merging looks enough, great!
>>
>> There is another scenario that I probably need to be able to handle, a
>> string like this does not seem to match anything I throw at it (and in
>> retrospect, you might have mentioned that earlier):
>>
>> "asd" "fgh" "jkl"
>>
>> Is there a way around that somehow or match, then pass into python to
>> handle that?
> 
> This is in the C code?  I think that it should be matched by an
> expression.

Hmm, suddenly the problems with passing expressions to python I stated
earlier disappeared, good. Running this takes a log time on some files
(over 10 minutes for ./servers/slapd/back-ldap/bind.c and processing of
servers/slapd/back-meta/search.c has yet to finish after 30 minutes of
processing).

I suppose going through all the potential combinations of snprintf/Debug
statements is what takes so long, any tips on what usually helps to
speed things up?

Also, more importantly, the patch below this seems to remove this
comment:
https://github.com/openldap/openldap/blob/master/servers/slapd/back-asyncmeta/conn.c#L547

{
...
-T buf;
...
ldap_pvt_thread_mutex_lock(lock);
...
-snprintf at p1( buf, E, format1, args );
+Debug( L, merged, args_before, args, args_after );
...
ldap_pvt_thread_mutex_unlock(lock);
...
-Debug at p2( L, format2, args_before, buf, args_after );
...
}

I would like to preserve the comment, is there something I can do about
that?

Thanks,

-- 
Ond?ej Kuzn?k
Senior Software Engineer
Symas Corporation                       http://www.symas.com
Packaged, certified, and supported LDAP solutions powered by OpenLDAP

[Cocci] Matching format strings

[Julia Lawall]

On Tue, 4 Jul 2017, Ond?ej Kuzn?k wrote:

> On Tue, Jul 04, 2017 at 03:53:21PM +0200, Julia Lawall wrote:
> > On Tue, 4 Jul 2017, Ond?ej Kuzn?k wrote:
> >> That does work and seems to preserve the things that needed escaping,
> >> so just putting quotes around when merging looks enough, great!
> >>
> >> There is another scenario that I probably need to be able to handle, a
> >> string like this does not seem to match anything I throw at it (and in
> >> retrospect, you might have mentioned that earlier):
> >>
> >> "asd" "fgh" "jkl"
> >>
> >> Is there a way around that somehow or match, then pass into python to
> >> handle that?
> >
> > This is in the C code?  I think that it should be matched by an
> > expression.
>
> Hmm, suddenly the problems with passing expressions to python I stated
> earlier disappeared, good. Running this takes a log time on some files
> (over 10 minutes for ./servers/slapd/back-ldap/bind.c and processing of
> servers/slapd/back-meta/search.c has yet to finish after 30 minutes of
> processing).
>
> I suppose going through all the potential combinations of snprintf/Debug
> statements is what takes so long, any tips on what usually helps to
> speed things up?

Some functions in those files have a lot of ifs.  Ifs are very expensive,
because both branches have to be considered.

In the rule below at least, I don't think you need the initial
{ ... and final ... }.  That would only seem to be necessary if you have a
lot of redeclarations of variables.

Also, it could help to be more specific about the type of buf.

> Also, more importantly, the patch below this seems to remove this
> comment:
> https://github.com/openldap/openldap/blob/master/servers/slapd/back-asyncmeta/conn.c#L547
>
> {
> ...
> -T buf;
> ...
> ldap_pvt_thread_mutex_lock(lock);
> ...
> -snprintf at p1( buf, E, format1, args );
> +Debug( L, merged, args_before, args, args_after );
> ...
> ldap_pvt_thread_mutex_unlock(lock);
> ...
> -Debug at p2( L, format2, args_before, buf, args_after );
> ...
> }
>
> I would like to preserve the comment, is there something I can do about
> that?

I thought that comments are only removed when they come before removed
code, while here they come after removed code.  I can check on it.

julia

[Cocci] Matching format strings

[Ondřej Kuzník]

On Tue, Jul 04, 2017 at 05:25:46PM +0200, Julia Lawall wrote:
> On Tue, 4 Jul 2017, Ond?ej Kuzn?k wrote:
>> Hmm, suddenly the problems with passing expressions to python I stated
>> earlier disappeared, good. Running this takes a log time on some files
>> (over 10 minutes for ./servers/slapd/back-ldap/bind.c and processing of
>> servers/slapd/back-meta/search.c has yet to finish after 30 minutes of
>> processing).
>>
>> I suppose going through all the potential combinations of snprintf/Debug
>> statements is what takes so long, any tips on what usually helps to
>> speed things up?
> 
> Some functions in those files have a lot of ifs.  Ifs are very expensive,
> because both branches have to be considered.

Do they have to be considered every time or can coccinelle sometimes
understand that taking neither branch can affect the match? (as in, can
I adjust the patch to help coccinelle somehow?).

> In the rule below at least, I don't think you need the initial
> { ... and final ... }.  That would only seem to be necessary if you have a
> lot of redeclarations of variables.

That is the case at least in servers/slapd/back-meta/search.c, where
buf is often declared just before it is used and I hoped that would
speed things up slightly.

> Also, it could help to be more specific about the type of buf.

I have tried

{
-char buf[S];
-snprintf at p1( buf, E, format1, args );
-Debug at p2( L, format2, args_before, buf, args_after );
+Debug( L, merged, args_before, args, args_after );
... when != buf
}

To see whether I can remove a lot of the simple ones quickly and then
work with a simpler file, but this one seems to take the same amount of
time once I put the "... when != buf" in.

>> Also, more importantly, the patch below this seems to remove this
>> comment:
>> https://github.com/openldap/openldap/blob/master/servers/slapd/back-asyncmeta/conn.c#L547
>>
>> [...]
>>
>> I would like to preserve the comment, is there something I can do about
>> that?
> 
> I thought that comments are only removed when they come before removed
> code, while here they come after removed code.  I can check on it.

Thanks!


-- 
Ond?ej Kuzn?k
Senior Software Engineer
Symas Corporation                       http://www.symas.com
Packaged, certified, and supported LDAP solutions powered by OpenLDAP

[Cocci] Matching format strings

[Julia Lawall]

On Tue, 4 Jul 2017, Ond?ej Kuzn?k wrote:

> On Tue, Jul 04, 2017 at 05:25:46PM +0200, Julia Lawall wrote:
> > On Tue, 4 Jul 2017, Ond?ej Kuzn?k wrote:
> >> Hmm, suddenly the problems with passing expressions to python I stated
> >> earlier disappeared, good. Running this takes a log time on some files
> >> (over 10 minutes for ./servers/slapd/back-ldap/bind.c and processing of
> >> servers/slapd/back-meta/search.c has yet to finish after 30 minutes of
> >> processing).
> >>
> >> I suppose going through all the potential combinations of snprintf/Debug
> >> statements is what takes so long, any tips on what usually helps to
> >> speed things up?
> >
> > Some functions in those files have a lot of ifs.  Ifs are very expensive,
> > because both branches have to be considered.
>
> Do they have to be considered every time or can coccinelle sometimes
> understand that taking neither branch can affect the match? (as in, can
> I adjust the patch to help coccinelle somehow?).
>
> > In the rule below at least, I don't think you need the initial
> > { ... and final ... }.  That would only seem to be necessary if you have a
> > lot of redeclarations of variables.
>
> That is the case at least in servers/slapd/back-meta/search.c, where
> buf is often declared just before it is used and I hoped that would
> speed things up slightly.
>
> > Also, it could help to be more specific about the type of buf.
>
> I have tried
>
> {
> -char buf[S];
> -snprintf at p1( buf, E, format1, args );
> -Debug at p2( L, format2, args_before, buf, args_after );
> +Debug( L, merged, args_before, args, args_after );
> ... when != buf
> }
>
> To see whether I can remove a lot of the simple ones quickly and then
> work with a simpler file, but this one seems to take the same amount of
> time once I put the "... when != buf" in.

OK.  Probably the expensive cases are the ones that happen to match this
pattern...

julia


>
> >> Also, more importantly, the patch below this seems to remove this
> >> comment:
> >> https://github.com/openldap/openldap/blob/master/servers/slapd/back-asyncmeta/conn.c#L547
> >>
> >> [...]
> >>
> >> I would like to preserve the comment, is there something I can do about
> >> that?
> >
> > I thought that comments are only removed when they come before removed
> > code, while here they come after removed code.  I can check on it.
>
> Thanks!
>
>
> --
> Ond?ej Kuzn?k
> Senior Software Engineer
> Symas Corporation                       http://www.symas.com
> Packaged, certified, and supported LDAP solutions powered by OpenLDAP
>

[Cocci] Matching format strings

[Ondřej Kuzník]

On Tue, Jul 04, 2017 at 06:09:10PM +0200, Julia Lawall wrote:
> 
> 
> On Tue, 4 Jul 2017, Ond?ej Kuzn?k wrote:
> 
>> On Tue, Jul 04, 2017 at 05:25:46PM +0200, Julia Lawall wrote:
>>> On Tue, 4 Jul 2017, Ond?ej Kuzn?k wrote:
>>>> Hmm, suddenly the problems with passing expressions to python I stated
>>>> earlier disappeared, good. Running this takes a log time on some files
>>>> (over 10 minutes for ./servers/slapd/back-ldap/bind.c and processing of
>>>> servers/slapd/back-meta/search.c has yet to finish after 30 minutes of
>>>> processing).
>>>>
>>>> I suppose going through all the potential combinations of snprintf/Debug
>>>> statements is what takes so long, any tips on what usually helps to
>>>> speed things up?
>>>
>>> Some functions in those files have a lot of ifs.  Ifs are very expensive,
>>> because both branches have to be considered.
>>
>> Do they have to be considered every time or can coccinelle sometimes
>> understand that taking neither branch can affect the match? (as in, can
>> I adjust the patch to help coccinelle somehow?).
>>
>>> In the rule below at least, I don't think you need the initial
>>> { ... and final ... }.  That would only seem to be necessary if you have a
>>> lot of redeclarations of variables.
>>
>> That is the case at least in servers/slapd/back-meta/search.c, where
>> buf is often declared just before it is used and I hoped that would
>> speed things up slightly.
>>
>> > Also, it could help to be more specific about the type of buf.
>>
>> I have tried
>>
>> {
>> -char buf[S];
>> -snprintf at p1( buf, E, format1, args );
>> -Debug at p2( L, format2, args_before, buf, args_after );
>> +Debug( L, merged, args_before, args, args_after );
>> ... when != buf
>> }
>>
>> To see whether I can remove a lot of the simple ones quickly and then
>> work with a simpler file, but this one seems to take the same amount of
>> time once I put the "... when != buf" in.
> 
> OK.  Probably the expensive cases are the ones that happen to match this
> pattern...

My test file at the moment is https://github.com/openldap/openldap/blob/master/servers/slapd/back-asyncmeta/meta_result.c
which calls snprintf in two places only and both code blocks are very
short. Still, the spatch takes slightly over 1000 seconds to process the
file with the below:

@shortcut@
identifier buf;                                         
expression S, E, L;
expression list args_before, args, args_after;          
expression format1, format2;                            
@@                                                      

{
-char buf[S]; 
-snprintf( buf, E, format1, args );
-Debug( L, format2, args_before, buf, args_after );
+Debug( L, "merged", args_before, args, args_after );   
... when != buf                                         
}

I feel I'm missing something in how coccinelle works to be able to
make its job easier here.

-- 
Ond?ej Kuzn?k
Senior Software Engineer
Symas Corporation                       http://www.symas.com
Packaged, certified, and supported LDAP solutions powered by OpenLDAP

[Cocci] Matching format strings

[Julia Lawall]

On Tue, 4 Jul 2017, Ond?ej Kuzn?k wrote:

> On Tue, Jul 04, 2017 at 06:09:10PM +0200, Julia Lawall wrote:
> >
> >
> > On Tue, 4 Jul 2017, Ond?ej Kuzn?k wrote:
> >
> >> On Tue, Jul 04, 2017 at 05:25:46PM +0200, Julia Lawall wrote:
> >>> On Tue, 4 Jul 2017, Ond?ej Kuzn?k wrote:
> >>>> Hmm, suddenly the problems with passing expressions to python I stated
> >>>> earlier disappeared, good. Running this takes a log time on some files
> >>>> (over 10 minutes for ./servers/slapd/back-ldap/bind.c and processing of
> >>>> servers/slapd/back-meta/search.c has yet to finish after 30 minutes of
> >>>> processing).
> >>>>
> >>>> I suppose going through all the potential combinations of snprintf/Debug
> >>>> statements is what takes so long, any tips on what usually helps to
> >>>> speed things up?
> >>>
> >>> Some functions in those files have a lot of ifs.  Ifs are very expensive,
> >>> because both branches have to be considered.
> >>
> >> Do they have to be considered every time or can coccinelle sometimes
> >> understand that taking neither branch can affect the match? (as in, can
> >> I adjust the patch to help coccinelle somehow?).
> >>
> >>> In the rule below at least, I don't think you need the initial
> >>> { ... and final ... }.  That would only seem to be necessary if you have a
> >>> lot of redeclarations of variables.
> >>
> >> That is the case at least in servers/slapd/back-meta/search.c, where
> >> buf is often declared just before it is used and I hoped that would
> >> speed things up slightly.
> >>
> >> > Also, it could help to be more specific about the type of buf.
> >>
> >> I have tried
> >>
> >> {
> >> -char buf[S];
> >> -snprintf at p1( buf, E, format1, args );
> >> -Debug at p2( L, format2, args_before, buf, args_after );
> >> +Debug( L, merged, args_before, args, args_after );
> >> ... when != buf
> >> }
> >>
> >> To see whether I can remove a lot of the simple ones quickly and then
> >> work with a simpler file, but this one seems to take the same amount of
> >> time once I put the "... when != buf" in.
> >
> > OK.  Probably the expensive cases are the ones that happen to match this
> > pattern...
>
> My test file at the moment is https://github.com/openldap/openldap/blob/master/servers/slapd/back-asyncmeta/meta_result.c
> which calls snprintf in two places only and both code blocks are very
> short. Still, the spatch takes slightly over 1000 seconds to process the
> file with the below:
>
> @shortcut@
> identifier buf;
> expression S, E, L;
> expression list args_before, args, args_after;
> expression format1, format2;
> @@
>
> {
> -char buf[S];
> -snprintf( buf, E, format1, args );
> -Debug( L, format2, args_before, buf, args_after );
> +Debug( L, "merged", args_before, args, args_after );
> ... when != buf
> }
>
> I feel I'm missing something in how coccinelle works to be able to
> make its job easier here.

It is searching though the control flow paths.  In this case, one of the
blocks is:

                        {
		                char    buf[ SLAP_TEXT_BUFLEN ];

                                snprintf( buf, sizeof( buf ),
	                                "%s meta_send_entry(\"%s\"): "
	                                "slap_bv2undef_ad(%s): %s\n",
					op->o_log_prefix, ent.e_name.bv_val,
	                                mapped.bv_val, text );

                                Debug( LDAP_DEBUG_ANY, "%s", buf, 0, 0 );
                                ( void )ber_scanf( &ber, "x" /* [W] */ );
                                op->o_tmpfree( attr, op->o_tmpmemctx );
                                continue;
                        }

I think that it is drifting off into infinity due to the continue.

How about adding a timeout (eg --timeout 120) and then see how much is
left over.

julia

[Cocci] Matching format strings

[Ondřej Kuzník]

On Tue, Jul 04, 2017 at 06:40:56PM +0200, Julia Lawall wrote:
> On Tue, 4 Jul 2017, Ond?ej Kuzn?k wrote:
>> My test file at the moment is https://github.com/openldap/openldap/blob/master/servers/slapd/back-asyncmeta/meta_result.c
>> which calls snprintf in two places only and both code blocks are very
>> short. Still, the spatch takes slightly over 1000 seconds to process the
>> file with the below:
>>
>> @shortcut@
>> identifier buf;
>> expression S, E, L;
>> expression list args_before, args, args_after;
>> expression format1, format2;
>> @@
>>
>> {
>> -char buf[S];
>> -snprintf( buf, E, format1, args );
>> -Debug( L, format2, args_before, buf, args_after );
>> +Debug( L, "merged", args_before, args, args_after );
>> ... when != buf
>> }
>>
>> I feel I'm missing something in how coccinelle works to be able to
>> make its job easier here.
> 
> It is searching though the control flow paths.  In this case, one of the
> blocks is:
> 
>                         {
> 		                char    buf[ SLAP_TEXT_BUFLEN ];
> 
>                                 snprintf( buf, sizeof( buf ),
> 	                                "%s meta_send_entry(\"%s\"): "
> 	                                "slap_bv2undef_ad(%s): %s\n",
> 					op->o_log_prefix, ent.e_name.bv_val,
> 	                                mapped.bv_val, text );
> 
>                                 Debug( LDAP_DEBUG_ANY, "%s", buf, 0, 0 );
>                                 ( void )ber_scanf( &ber, "x" /* [W] */ );
>                                 op->o_tmpfree( attr, op->o_tmpmemctx );
>                                 continue;
>                         }
> 
> I think that it is drifting off into infinity due to the continue.

Possibly, but "buf" is only declared in the local scope and by providing
the scope parenthesis without the ellipsis after it, I was hoping
coccinelle would be able to infer the scope and maybe even deduce that
the continue always means the scope is left and that path can be
ignored? But that's probably not that easy.

> How about adding a timeout (eg --timeout 120) and then see how much is
> left over.

I get:
timeout (we abort)
Fatal error: exception Common.Timeout

and no patch is produced, if I wait the 1028 seconds, I get the expected
patch. I think that supports your hypothesis about trying to process the
continue.

Thanks,

-- 
Ond?ej Kuzn?k
Senior Software Engineer
Symas Corporation                       http://www.symas.com
Packaged, certified, and supported LDAP solutions powered by OpenLDAP

[Cocci] Matching format strings

[Julia Lawall]

> > How about adding a timeout (eg --timeout 120) and then see how much is
> > left over.
>
> I get:
> timeout (we abort)
> Fatal error: exception Common.Timeout
>
> and no patch is produced, if I wait the 1028 seconds, I get the expected
> patch. I think that supports your hypothesis about trying to process the
> continue.

If you run Coccinelle on the whole directory, it should timeout on some
files, and then work on the other ones.  Indeed, if there is a timeout at
one point for a file then you won't get any patch for that file at all.

julia

[Cocci] Matching format strings

[Ondřej Kuzník]

On Tue, Jul 04, 2017 at 06:59:39PM +0200, Julia Lawall wrote:
>>> How about adding a timeout (eg --timeout 120) and then see how much is
>>> left over.
>>
>> I get:
>> timeout (we abort)
>> Fatal error: exception Common.Timeout
>>
>> and no patch is produced, if I wait the 1028 seconds, I get the expected
>> patch. I think that supports your hypothesis about trying to process the
>> continue.
> 
> If you run Coccinelle on the whole directory, it should timeout on some
> files, and then work on the other ones.  Indeed, if there is a timeout at
> one point for a file then you won't get any patch for that file at all.

Ah, OK, I understand the suggestion now. It does speed up the initial
testing of the patch over the codebase.

Looking at some of the code (well, comments rather), coccinelle is
building a CTL formula and a state machine for the C file and tries to
find the states where it matches? Is that correct?

Does it try to compress the state machine based on the predicates
involved? BDDs tend to be rather helpful with CTL model checking. Not
sure how useful that is, depending on the information that needs to be
extracted (and I've not touched model checking in ages).

Sorry if this is completely wrong.

I vaguely remember seeing something like this somewhere, but can't find
it anymore. That might simplify the three-pronged construction I use
into a single patch hunk like the below. Is there a construction like
that available or have I just dreamed that up?

@@
(stuff here also contains format1, format2, args_before and others as
usual)
identifier merged : python merge_formats(format1, format2, args_before);
@@

-snprintf( buf, E, format1, args );
-Debug( L, format2, args_before, buf, args_after );
+Debug( L, merged, args_before, args, args_after );

-- 
Ond?ej Kuzn?k
Senior Software Engineer
Symas Corporation                       http://www.symas.com
Packaged, certified, and supported LDAP solutions powered by OpenLDAP

[Cocci] Matching format strings

[Julia Lawall]

> Looking at some of the code (well, comments rather), coccinelle is
> building a CTL formula and a state machine for the C file and tries to
> find the states where it matches? Is that correct?

Yes, this is correct.

> Does it try to compress the state machine based on the predicates
> involved? BDDs tend to be rather helpful with CTL model checking. Not
> sure how useful that is, depending on the information that needs to be
> extracted (and I've not touched model checking in ages).

No, we don't do that.  Basically, the graph itself is pretty small, but
the problem is all of the possible variable bindings.  Our reasoning was
that BDDs would not be helpful.

> Sorry if this is completely wrong.
>
> I vaguely remember seeing something like this somewhere, but can't find
> it anymore. That might simplify the three-pronged construction I use
> into a single patch hunk like the below. Is there a construction like
> that available or have I just dreamed that up?
>
> @@
> (stuff here also contains format1, format2, args_before and others as
> usual)
> identifier merged : python merge_formats(format1, format2, args_before);
> @@
>
> -snprintf( buf, E, format1, args );
> -Debug( L, format2, args_before, buf, args_after );
> +Debug( L, merged, args_before, args, args_after );

You made it up :)  Currently, the only thing you can do with python on the
fly like this is write predicates, not create variable values.  The exact
syntax is, eg

identifier x : script:python(...) { ... }

The first ... is any inherited metavariables that the script code needs,
but variables defined in the same rule are not allowed.  The second ...
needs to look like C code.  Typically, it would be a function call, and
then you would define the function in the

@initialize:python@
@@

code at the beginning of the script, where you can put whatever python
code you want.

julia

> --
> Ond?ej Kuzn?k
> Senior Software Engineer
> Symas Corporation                       http://www.symas.com
> Packaged, certified, and supported LDAP solutions powered by OpenLDAP
>

[Cocci] Matching format strings

[Ondřej Kuzník]

On Tue, Jul 04, 2017 at 07:53:13PM +0200, Julia Lawall wrote:
>> Looking at some of the code (well, comments rather), coccinelle is
>> building a CTL formula and a state machine for the C file and tries to
>> find the states where it matches? Is that correct?
> 
> Yes, this is correct.
> 
>> Does it try to compress the state machine based on the predicates
>> involved? BDDs tend to be rather helpful with CTL model checking. Not
>> sure how useful that is, depending on the information that needs to be
>> extracted (and I've not touched model checking in ages).
> 
> No, we don't do that.  Basically, the graph itself is pretty small, but
> the problem is all of the possible variable bindings.  Our reasoning was
> that BDDs would not be helpful.

So the variable bindings are not expressible as predicates (which is
what I would have expected to explode the state space)? I guess not
since I have dealt with files take a lot of time to process and spatch
doesn't actually consume that much more space, where CTL model-checking
complexity is linear to the graph size. So I guess that answers my
question.

>> Sorry if this is completely wrong.
>>
>> I vaguely remember seeing something like this somewhere, but can't find
>> it anymore. That might simplify the three-pronged construction I use
>> into a single patch hunk like the below. Is there a construction like
>> that available or have I just dreamed that up?
>>
>> @@
>> (stuff here also contains format1, format2, args_before and others as
>> usual)
>> identifier merged : python merge_formats(format1, format2, args_before);
>> @@
>>
>> -snprintf( buf, E, format1, args );
>> -Debug( L, format2, args_before, buf, args_after );
>> +Debug( L, merged, args_before, args, args_after );
> 
> You made it up :)  Currently, the only thing you can do with python on the
> fly like this is write predicates, not create variable values.  The exact
> syntax is, eg

Shame, maybe a feature request? Thing is that while the following is a
useful idiom to generate new code with python where coccinelle can't
(yet) generate it itself, it does seem unnecessarily complex:

@a@
bindings
@@
a at p1
b at p2
c at p3

@script:python a_process@
read << a.bindings
exported;
@@
code

@a_change@
a.bindings
identifier a_process.exported;
@@
a at p1
-b at p2
+new(stuff, exported)
-c@p3

where with a few restrictions (e.g. we don't set up cyclic dependencies,
what the python code is allowed to do, ...) this would be able to
express the same and make things easier:

@@
bindings
identifier x : script:python(some bindings, even from this rule) { code }
@@
a
-b
+new(stuff, x)
-c

> identifier x : script:python(...) { ... }
> 
> The first ... is any inherited metavariables that the script code needs,
> but variables defined in the same rule are not allowed.  The second ...
> needs to look like C code.  Typically, it would be a function call, and
> then you would define the function in the
> 
> @initialize:python@
> @@
> 
> code at the beginning of the script, where you can put whatever python
> code you want.

All in all, I'm getting close to getting a testable patch, though :)
Thanks for the great support!

-- 
Ond?ej Kuzn?k
Senior Software Engineer
Symas Corporation                       http://www.symas.com
Packaged, certified, and supported LDAP solutions powered by OpenLDAP

[Cocci] Matching format strings

[Julia Lawall]

> @@
> bindings
> identifier x : script:python(some bindings, even from this rule) { code }
> @@
> a
> -b
> +new(stuff, x)
> -c

OK, maybe it could be

identifier x = script:python(some bindings, but not from this rule) { code }

Bindings from the same rule are problematic.  CTL is bottom up, so you
don't know the bindings of the other variables when you are matching a
given one.  I guess a possibility would be to ignore the scripts and
collect all the bindings possible (ie any possible identifier for x above),
and then check them at the end.

julia

[Cocci] Matching format strings

[Julia Lawall]

On Tue, 4 Jul 2017, Julia Lawall wrote:

> > @@
> > bindings
> > identifier x : script:python(some bindings, even from this rule) { code }
> > @@
> > a
> > -b
> > +new(stuff, x)
> > -c
>
> OK, maybe it could be
>
> identifier x = script:python(some bindings, but not from this rule) { code }
>
> Bindings from the same rule are problematic.  CTL is bottom up, so you
> don't know the bindings of the other variables when you are matching a
> given one.  I guess a possibility would be to ignore the scripts and
> collect all the bindings possible (ie any possible identifier for x above),
> and then check them at the end.

I'm sorry already for having suggested this.  It would be a mess, I think.
When there is ..., variables can have different values on different
control-flow paths.  I guess that their quantifiers could be pushed up
around the scripts that depend on them, but I'm not sure that the user
would find the result very understandable.

julia

[Cocci] Matching format strings

[Julia Lawall]

The issue of the deleted comment should be addressed.

julia

[Cocci] Matching format strings

[Ondřej Kuzník]

On Wed, Jul 05, 2017 at 02:20:53PM +0200, Julia Lawall wrote:
> The issue of the deleted comment should be addressed.

Thanks, I can confirm it does the right thing for me.

I have started compiling the result and there seem to be a few issues:

The following rule when applied to servers/slapd/sl_malloc.c will result
in a syntax error since the string on line 408 gets split across several
lines:

@@
format list[2] two;
expression E, A1, A2;
@@

-Debug( E, "%@two@", A1, A2, 0 );
+Debug( E, "%@two@", A1, A2 );

Another issue with the same rule is in servers/slapd/daemon.c, the
first hunk tries to change a multiline macro, but fails to keep the
backslashes in place.

Also, the following patch does not actually pick up anything in
servers/slapd/connection.c, where I would have expected lines 1402
and 1849 to match. What would be the reason?

@@
format list[5] five;
expression E, A1, A2, A3, A4, A5;
@@

-Statslog( E, "%@five@", A1, A2, A3, A4, A5 );
+Debug( E, "%@five@", A1, A2, A3, A4, A5 );

Thanks,

-- 
Ond?ej Kuzn?k
Senior Software Engineer
Symas Corporation                       http://www.symas.com
Packaged, certified, and supported LDAP solutions powered by OpenLDAP

[Cocci] Matching format strings

[Julia Lawall]

On Wed, 5 Jul 2017, Ond?ej Kuzn?k wrote:

> On Wed, Jul 05, 2017 at 02:20:53PM +0200, Julia Lawall wrote:
> > The issue of the deleted comment should be addressed.
>
> Thanks, I can confirm it does the right thing for me.
>
> I have started compiling the result and there seem to be a few issues:
>
> The following rule when applied to servers/slapd/sl_malloc.c will result
> in a syntax error since the string on line 408 gets split across several
> lines:
>
> @@
> format list[2] two;
> expression E, A1, A2;
> @@
>
> -Debug( E, "%@two@", A1, A2, 0 );
> +Debug( E, "%@two@", A1, A2 );

You can avoid the problem by writing the rule as:

@@
format list[2] two;
expression E, A1, A2;
@@

Debug( E, "%@two@", A1, A2
-, 0
 );

Note that Coccinelle will not leave a space before the final ).

julia

>
> Another issue with the same rule is in servers/slapd/daemon.c, the
> first hunk tries to change a multiline macro, but fails to keep the
> backslashes in place.
>
> Also, the following patch does not actually pick up anything in
> servers/slapd/connection.c, where I would have expected lines 1402
> and 1849 to match. What would be the reason?
>
> @@
> format list[5] five;
> expression E, A1, A2, A3, A4, A5;
> @@
>
> -Statslog( E, "%@five@", A1, A2, A3, A4, A5 );
> +Debug( E, "%@five@", A1, A2, A3, A4, A5 );
>
> Thanks,
>
> --
> Ond?ej Kuzn?k
> Senior Software Engineer
> Symas Corporation                       http://www.symas.com
> Packaged, certified, and supported LDAP solutions powered by OpenLDAP
>

[Cocci] Matching format strings

[Julia Lawall]

> Also, the following patch does not actually pick up anything in
> servers/slapd/connection.c, where I would have expected lines 1402
> and 1849 to match. What would be the reason?
>
> @@
> format list[5] five;
> expression E, A1, A2, A3, A4, A5;
> @@
>
> -Statslog( E, "%@five@", A1, A2, A3, A4, A5 );
> +Debug( E, "%@five@", A1, A2, A3, A4, A5 );

Parse error.  For some reason in this file, it decides that NULL is a
typedef.  So most of the file does not parse.  I will look into this.

julia

[Cocci] Matching format strings

[Julia Lawall]

> @@
> format list[5] five;
> expression E, A1, A2, A3, A4, A5;
> @@
>
> -Statslog( E, "%@five@", A1, A2, A3, A4, A5 );
> +Debug( E, "%@five@", A1, A2, A3, A4, A5 );

The problem is the macro LDAP_PF_LOCAL_SENDMSG_ARG that is used in
contexts like:

	c = connection_init( sfd, (Listener *)&dummy_list, "", "",
	        CONN_IS_CLIENT, 0, NULL
                LDAP_PF_LOCAL_SENDMSG_ARG(NULL));

Note the lack of a comma after NULL.  Putting the following in a file
con.h:

#define LDAP_PF_LOCAL_SENDMSG_ARG(x)

and then using

spatch f5.cocci -macro-file-builtins ~/incoming/con.h ~/openldap/servers/slapd/connection.c

gives the expected result.

julia

[Cocci] Matching format strings

[Ondřej Kuzník]

On Wed, Jul 05, 2017 at 10:38:32PM +0200, Julia Lawall wrote:
>> @@
>> format list[5] five;
>> expression E, A1, A2, A3, A4, A5;
>> @@
>>
>> -Statslog( E, "%@five@", A1, A2, A3, A4, A5 );
>> +Debug( E, "%@five@", A1, A2, A3, A4, A5 );
> 
> The problem is the macro LDAP_PF_LOCAL_SENDMSG_ARG that is used in
> contexts like:
> 
> 	c = connection_init( sfd, (Listener *)&dummy_list, "", "",
> 	        CONN_IS_CLIENT, 0, NULL
>                 LDAP_PF_LOCAL_SENDMSG_ARG(NULL));
> 
> Note the lack of a comma after NULL.  Putting the following in a file
> con.h:
> 
> #define LDAP_PF_LOCAL_SENDMSG_ARG(x)
> 
> and then using
> 
> spatch f5.cocci -macro-file-builtins ~/incoming/con.h ~/openldap/servers/slapd/connection.c
> 
> gives the expected result.

Thanks, good thing is make test now seems to pass for the first round of
patches.

Actually, I'm now having a similar issue with ./servers/slapd/daemon.c,
spatch --parse-c will complain about the SLAP_EVENT_DECL macro and I can
see other issues with LDAP_LIST/LDAP_STAILQ/... macros in the output.

This patch is the one that's affected (among others), it will make some
changes, applying them and then running it again will make more until
eventually managing to pick up most or all of the ones that I'd expect:

@@
format list[2] fmt;
expression list[2] args;
expression E;
@@

Debug( E, "%@fmt@", args
-, 0
 );

@@
format list[1] fmt;
expression list[1] args;
expression E;
@@

Debug( E, "%@fmt@", args
-, 0, 0
 );

@@
expression E, S;
@@

Debug( E, S
-, 0, 0, 0
 );

Is there a way to work around that somehow?

Thanks,
Ondrej

-- 
Ond?ej Kuzn?k
Senior Software Engineer
Symas Corporation                       http://www.symas.com
Packaged, certified, and supported LDAP solutions powered by OpenLDAP

[Cocci] Matching format strings

[Julia Lawall]

On Mon, 10 Jul 2017, Ond?ej Kuzn?k wrote:

> On Wed, Jul 05, 2017 at 10:38:32PM +0200, Julia Lawall wrote:
> >> @@
> >> format list[5] five;
> >> expression E, A1, A2, A3, A4, A5;
> >> @@
> >>
> >> -Statslog( E, "%@five@", A1, A2, A3, A4, A5 );
> >> +Debug( E, "%@five@", A1, A2, A3, A4, A5 );
> >
> > The problem is the macro LDAP_PF_LOCAL_SENDMSG_ARG that is used in
> > contexts like:
> >
> > 	c = connection_init( sfd, (Listener *)&dummy_list, "", "",
> > 	        CONN_IS_CLIENT, 0, NULL
> >                 LDAP_PF_LOCAL_SENDMSG_ARG(NULL));
> >
> > Note the lack of a comma after NULL.  Putting the following in a file
> > con.h:
> >
> > #define LDAP_PF_LOCAL_SENDMSG_ARG(x)
> >
> > and then using
> >
> > spatch f5.cocci -macro-file-builtins ~/incoming/con.h ~/openldap/servers/slapd/connection.c
> >
> > gives the expected result.
>
> Thanks, good thing is make test now seems to pass for the first round of
> patches.
>
> Actually, I'm now having a similar issue with ./servers/slapd/daemon.c,
> spatch --parse-c will complain about the SLAP_EVENT_DECL macro and I can
> see other issues with LDAP_LIST/LDAP_STAILQ/... macros in the output.
>
> This patch is the one that's affected (among others), it will make some
> changes, applying them and then running it again will make more until
> eventually managing to pick up most or all of the ones that I'd expect:
>
> @@
> format list[2] fmt;
> expression list[2] args;
> expression E;
> @@
>
> Debug( E, "%@fmt@", args
> -, 0
>  );
>
> @@
> format list[1] fmt;
> expression list[1] args;
> expression E;
> @@
>
> Debug( E, "%@fmt@", args
> -, 0, 0
>  );
>
> @@
> expression E, S;
> @@
>
> Debug( E, S
> -, 0, 0, 0
>  );
>
> Is there a way to work around that somehow?

I'm not sure to understand the relation between the macro problem and the
need to iterate.

julia

>
> Thanks,
> Ondrej
>
> --
> Ond?ej Kuzn?k
> Senior Software Engineer
> Symas Corporation                       http://www.symas.com
> Packaged, certified, and supported LDAP solutions powered by OpenLDAP
>

[Cocci] Matching format strings

[Ondřej Kuzník]

On Mon, Jul 10, 2017 at 06:18:14PM +0200, Julia Lawall wrote:
> On Mon, 10 Jul 2017, Ond?ej Kuzn?k wrote:
>> Thanks, good thing is make test now seems to pass for the first round of
>> patches.
>>
>> Actually, I'm now having a similar issue with ./servers/slapd/daemon.c,
>> spatch --parse-c will complain about the SLAP_EVENT_DECL macro and I can
>> see other issues with LDAP_LIST/LDAP_STAILQ/... macros in the output.
>>
>> This patch is the one that's affected (among others), it will make some
>> changes, applying them and then running it again will make more until
>> eventually managing to pick up most or all of the ones that I'd expect:
>>
>> @@
>> format list[2] fmt;
>> expression list[2] args;
>> expression E;
>> @@
>>
>> Debug( E, "%@fmt@", args
>> -, 0
>>  );
>>
>> @@
>> format list[1] fmt;
>> expression list[1] args;
>> expression E;
>> @@
>>
>> Debug( E, "%@fmt@", args
>> -, 0, 0
>>  );
>>
>> @@
>> expression E, S;
>> @@
>>
>> Debug( E, S
>> -, 0, 0, 0
>>  );
>>
>> Is there a way to work around that somehow?
> 
> I'm not sure to understand the relation between the macro problem and the
> need to iterate.

The above patch will make some changes but not all (even though the
patch is idempotent) but iteration seems to help. It seems that it's
because spatch does not like the file (or rather some of the macros
there).

-- 
Ond?ej Kuzn?k
Senior Software Engineer
Symas Corporation                       http://www.symas.com
Packaged, certified, and supported LDAP solutions powered by OpenLDAP

[Cocci] Matching format strings

[Julia Lawall]

Try running spatch --parse-c on your directory to get a list at the end of
the top 10 things it doesn't like.

julia

On Mon, 10 Jul 2017, Ond?ej Kuzn?k wrote:

> On Mon, Jul 10, 2017 at 06:18:14PM +0200, Julia Lawall wrote:
> > On Mon, 10 Jul 2017, Ond?ej Kuzn?k wrote:
> >> Thanks, good thing is make test now seems to pass for the first round of
> >> patches.
> >>
> >> Actually, I'm now having a similar issue with ./servers/slapd/daemon.c,
> >> spatch --parse-c will complain about the SLAP_EVENT_DECL macro and I can
> >> see other issues with LDAP_LIST/LDAP_STAILQ/... macros in the output.
> >>
> >> This patch is the one that's affected (among others), it will make some
> >> changes, applying them and then running it again will make more until
> >> eventually managing to pick up most or all of the ones that I'd expect:
> >>
> >> @@
> >> format list[2] fmt;
> >> expression list[2] args;
> >> expression E;
> >> @@
> >>
> >> Debug( E, "%@fmt@", args
> >> -, 0
> >>  );
> >>
> >> @@
> >> format list[1] fmt;
> >> expression list[1] args;
> >> expression E;
> >> @@
> >>
> >> Debug( E, "%@fmt@", args
> >> -, 0, 0
> >>  );
> >>
> >> @@
> >> expression E, S;
> >> @@
> >>
> >> Debug( E, S
> >> -, 0, 0, 0
> >>  );
> >>
> >> Is there a way to work around that somehow?
> >
> > I'm not sure to understand the relation between the macro problem and the
> > need to iterate.
>
> The above patch will make some changes but not all (even though the
> patch is idempotent) but iteration seems to help. It seems that it's
> because spatch does not like the file (or rather some of the macros
> there).
>
> --
> Ond?ej Kuzn?k
> Senior Software Engineer
> Symas Corporation                       http://www.symas.com
> Packaged, certified, and supported LDAP solutions powered by OpenLDAP
>

[Cocci] Matching format strings

[Ondřej Kuzník]

On Mon, Jul 10, 2017 at 07:26:36PM +0200, Julia Lawall wrote:
> Try running spatch --parse-c on your directory to get a list at the end of
> the top 10 things it doesn't like.

One of the system dependent macros SLAP_EVENT_DECL is listed in each of
the 10 things. The macro is used in
https://github.com/openldap/openldap/blob/master/servers/slapd/daemon.c#L2343
to declare some variables used in the corresponding event loop.

The macro definition looks like this:

# define SLAP_EVENT_DECL		struct pollfd *revents

or:

# define SLAP_EVENT_DECL	fd_set readfds, writefds; char *rflags

Ignoring this macro is OK I guess, if there is a way to do that with
coccinelle?

Also, moving onto the other patch, I get similar inconsistencies with
./servers/slapd/back-sql/add.c, --parse-c doesn't list any problematic
statements but the output logs a few parse errors that don't make much
sense to me.

Thanks,
Ondrej

> On Mon, 10 Jul 2017, Ond?ej Kuzn?k wrote:
>> On Mon, Jul 10, 2017 at 06:18:14PM +0200, Julia Lawall wrote:
>>> On Mon, 10 Jul 2017, Ond?ej Kuzn?k wrote:
>>>> Thanks, good thing is make test now seems to pass for the first round of
>>>> patches.
>>>>
>>>> Actually, I'm now having a similar issue with ./servers/slapd/daemon.c,
>>>> spatch --parse-c will complain about the SLAP_EVENT_DECL macro and I can
>>>> see other issues with LDAP_LIST/LDAP_STAILQ/... macros in the output.
>>>>
>>>> This patch is the one that's affected (among others), it will make some
>>>> changes, applying them and then running it again will make more until
>>>> eventually managing to pick up most or all of the ones that I'd expect:
>>>>
>>>> @@
>>>> format list[2] fmt;
>>>> expression list[2] args;
>>>> expression E;
>>>> @@
>>>>
>>>> Debug( E, "%@fmt@", args
>>>> -, 0
>>>>  );
>>>>
>>>> @@
>>>> format list[1] fmt;
>>>> expression list[1] args;
>>>> expression E;
>>>> @@
>>>>
>>>> Debug( E, "%@fmt@", args
>>>> -, 0, 0
>>>>  );
>>>>
>>>> @@
>>>> expression E, S;
>>>> @@
>>>>
>>>> Debug( E, S
>>>> -, 0, 0, 0
>>>>  );
>>>>
>>>> Is there a way to work around that somehow?
>>>
>>> I'm not sure to understand the relation between the macro problem and the
>>> need to iterate.
>>
>> The above patch will make some changes but not all (even though the
>> patch is idempotent) but iteration seems to help. It seems that it's
>> because spatch does not like the file (or rather some of the macros
>> there).
>>
>> --
>> Ond?ej Kuzn?k
>> Senior Software Engineer
>> Symas Corporation                       http://www.symas.com
>> Packaged, certified, and supported LDAP solutions powered by OpenLDAP
>>


-- 
Ond?ej Kuzn?k
Senior Software Engineer
Symas Corporation                       http://www.symas.com
Packaged, certified, and supported LDAP solutions powered by OpenLDAP

[Cocci] Matching format strings

[Julia Lawall]

On Tue, 11 Jul 2017, Ond?ej Kuzn?k wrote:

> On Mon, Jul 10, 2017 at 07:26:36PM +0200, Julia Lawall wrote:
> > Try running spatch --parse-c on your directory to get a list at the end of
> > the top 10 things it doesn't like.
>
> One of the system dependent macros SLAP_EVENT_DECL is listed in each of
> the 10 things. The macro is used in
> https://github.com/openldap/openldap/blob/master/servers/slapd/daemon.c#L2343
> to declare some variables used in the corresponding event loop.
>
> The macro definition looks like this:
>
> # define SLAP_EVENT_DECL		struct pollfd *revents
>
> or:
>
> # define SLAP_EVENT_DECL	fd_set readfds, writefds; char *rflags
>
> Ignoring this macro is OK I guess, if there is a way to do that with
> coccinelle?

Just

#define SLAP_EVENT_DECL

in the .h file. But there will be stray ;s then.  So you might want to put
the whole first definition.

>
> Also, moving onto the other patch, I get similar inconsistencies with
> ./servers/slapd/back-sql/add.c, --parse-c doesn't list any problematic
> statements but the output logs a few parse errors that don't make much
> sense to me.

Could you send the complete semantic patch that you are currently
considering?  Then I can take a look.

julia

> Thanks,
> Ondrej
>
> > On Mon, 10 Jul 2017, Ond?ej Kuzn?k wrote:
> >> On Mon, Jul 10, 2017 at 06:18:14PM +0200, Julia Lawall wrote:
> >>> On Mon, 10 Jul 2017, Ond?ej Kuzn?k wrote:
> >>>> Thanks, good thing is make test now seems to pass for the first round of
> >>>> patches.
> >>>>
> >>>> Actually, I'm now having a similar issue with ./servers/slapd/daemon.c,
> >>>> spatch --parse-c will complain about the SLAP_EVENT_DECL macro and I can
> >>>> see other issues with LDAP_LIST/LDAP_STAILQ/... macros in the output.
> >>>>
> >>>> This patch is the one that's affected (among others), it will make some
> >>>> changes, applying them and then running it again will make more until
> >>>> eventually managing to pick up most or all of the ones that I'd expect:
> >>>>
> >>>> @@
> >>>> format list[2] fmt;
> >>>> expression list[2] args;
> >>>> expression E;
> >>>> @@
> >>>>
> >>>> Debug( E, "%@fmt@", args
> >>>> -, 0
> >>>>  );
> >>>>
> >>>> @@
> >>>> format list[1] fmt;
> >>>> expression list[1] args;
> >>>> expression E;
> >>>> @@
> >>>>
> >>>> Debug( E, "%@fmt@", args
> >>>> -, 0, 0
> >>>>  );
> >>>>
> >>>> @@
> >>>> expression E, S;
> >>>> @@
> >>>>
> >>>> Debug( E, S
> >>>> -, 0, 0, 0
> >>>>  );
> >>>>
> >>>> Is there a way to work around that somehow?
> >>>
> >>> I'm not sure to understand the relation between the macro problem and the
> >>> need to iterate.
> >>
> >> The above patch will make some changes but not all (even though the
> >> patch is idempotent) but iteration seems to help. It seems that it's
> >> because spatch does not like the file (or rather some of the macros
> >> there).
> >>
> >> --
> >> Ond?ej Kuzn?k
> >> Senior Software Engineer
> >> Symas Corporation                       http://www.symas.com
> >> Packaged, certified, and supported LDAP solutions powered by OpenLDAP
> >>
>
>
> --
> Ond?ej Kuzn?k
> Senior Software Engineer
> Symas Corporation                       http://www.symas.com
> Packaged, certified, and supported LDAP solutions powered by OpenLDAP
>

[Cocci] Matching format strings

[Ondřej Kuzník]

On Tue, Jul 11, 2017 at 03:10:36PM +0200, Julia Lawall wrote:
> On Tue, 11 Jul 2017, Ond?ej Kuzn?k wrote:
>> On Mon, Jul 10, 2017 at 07:26:36PM +0200, Julia Lawall wrote:
>>> Try running spatch --parse-c on your directory to get a list at the end of
>>> the top 10 things it doesn't like.
>>
>> One of the system dependent macros SLAP_EVENT_DECL is listed in each of
>> the 10 things. The macro is used in
>> https://github.com/openldap/openldap/blob/master/servers/slapd/daemon.c#L2343
>> to declare some variables used in the corresponding event loop.
>>
>> The macro definition looks like this:
>>
>> # define SLAP_EVENT_DECL		struct pollfd *revents
>>
>> or:
>>
>> # define SLAP_EVENT_DECL	fd_set readfds, writefds; char *rflags
>>
>> Ignoring this macro is OK I guess, if there is a way to do that with
>> coccinelle?
> 
> Just
> 
> #define SLAP_EVENT_DECL
> 
> in the .h file. But there will be stray ;s then.  So you might want to put
> the whole first definition.

It does seem to improve things, thanks. The line 2496 still doesn't
apply even when I declare SLAP_EVENT_FNAME to something like "name" in
-macro-file-builtins and I think there were similar ones in other files
later. 

>> Also, moving onto the other patch, I get similar inconsistencies with
>> ./servers/slapd/back-sql/add.c, --parse-c doesn't list any problematic
>> statements but the output logs a few parse errors that don't make much
>> sense to me.
> 
> Could you send the complete semantic patch that you are currently
> considering?  Then I can take a look.

Attaching the patches I'm currently working with:
(-macro-file-builtins macros.h)
1. variadic.cocci
2. shortcut.cocci
3. merge.cocci

The back-sql issue above is when trying to apply 2. (shortcut.cocci).
But the one I pasted in my last email is simple enough to illustrate the
issue (trying to merge snprintf and Debug, then get rid of extra if()s)
even without the python stuff.

Thanks,

-- 
Ond?ej Kuzn?k
Senior Software Engineer
Symas Corporation                       http://www.symas.com
Packaged, certified, and supported LDAP solutions powered by OpenLDAP
-------------- next part --------------
A non-text attachment was scrubbed...
Name: macros.h
Type: text/x-chdr
Size: 106 bytes
Desc: not available
URL: <https://systeme.lip6.fr/pipermail/cocci/attachments/20170711/3806a475/attachment.bin>
-------------- next part --------------
@@
identifier Logs =~ "Log[0-9]";
@@
-Logs
+Log

//identifier StatslogTest =~ "StatslogTest"; 
@@
@@
-StatslogTest
+LogTest

@@
format list[2] fmt;
expression list[2] args;
expression E;
@@

Debug( E, "%@fmt@", args
-, 0
 );

@@
format list[1] fmt;
expression list[1] args;
expression E;
@@

Debug( E, "%@fmt@", args
-, 0, 0
 );

@@
expression E, S;
@@

Debug( E, S
-, 0, 0, 0
 );

@@
format list[5] fmt;
expression list[5] args;
expression E;
@@

-Statslog
+Debug
 ( E, "%@fmt@", args );

@@
format list[4] fmt;
expression list[4] args;
expression E;
@@

-Statslog
+Debug
 ( E, "%@fmt@", args
-, 0
 );

@@
format list[3] fmt;
expression list[3] args;
expression E;
@@

-Statslog
+Debug
 ( E, "%@fmt@", args
-, 0, 0
 );

@@
format list[2] fmt;
expression list[2] args;
expression E;
@@

-Statslog
+Debug
 ( E, "%@fmt@", args
-, 0, 0, 0
 );

@@
format list[1] fmt;
expression list[1] args;
expression E;
@@

-Statslog
+Debug
 ( E, "%@fmt@", args
-, 0, 0, 0, 0
 );

@@
expression E, S;
@@

-Statslog
+Debug
 ( E, S
-, 0, 0, 0, 0, 0
 );

@@
identifier Stats =~ "^Statslog";
@@
(
 StatslogEtime
|
-Stats
+Debug
)
-------------- next part --------------
@initialize:python@
@@

#regex from https://stackoverflow.com/questions/30011379/how-can-i-parse-a-c-format-string-in-python
import re
fmtstring = '''\
(                                  # start of capture group 1
%                                  # literal "%"
(?:                                # first option
(?:[-+0 #]{0,5})                   # optional flags
(?:\d+|\*)?                        # width
(?:\.(?:\d+|\*))?                  # precision
(?:h|l|ll|w|I|I32|I64)?            # size
[cCdiouxXeEfgGaAnpsSZ]             # type
) |                                # OR
%%)                                # literal "%%"
'''

regex = re.compile(fmtstring, re.X)

def parse_format(f):
    return tuple((m.span(), m.group()) for m in
        regex.finditer(f))

// covered by others but processing that can take hours on some files
@shortcut@
type T;
identifier buf;
expression I, E, L;
expression list args_before, args, args_after;
expression format1, format2;
position p1, p2;
@@

(
{
\( T buf; \| T buf = I; \| \)
snprintf at p1( buf, E, format1, args );
Debug at p2( L, format2, args_before, buf, args_after );
}
|
{
\( T buf; \| T buf = I; \| \)
snprintf at p1( buf, E, format1, args );
Debug at p2( L, format2, args_before, buf, args_after );
}
)

@script:python shortcut_process@
format1 << shortcut.format1;
format2 << shortcut.format2;
args_before << shortcut.args_before;
merged;
@@

pos = len(args_before.elements)
formats = parse_format(format2)

span, format = formats[pos]
merged = format2[:span[0]] + format1.strip('"') + format2[span[1]:]

coccinelle.merged = merged

@shortcut_replace@
position shortcut.p1, shortcut.p2;
identifier shortcut_process.merged;

type T;
identifier buf;
expression I, E, L;
expression list args_before, args, args_after;
expression format1, format2;
@@

(
-{
-\( T buf; \| T buf = I; \| \)
-snprintf at p1( buf, E, format1, args );
-Debug at p2( L, format2, args_before, buf, args_after );
+Debug( L, merged, args_before, args, args_after );
-}
|
{
-\( T buf; \| T buf = I; \| \)
-snprintf at p1( buf, E, format1, args );
-Debug at p2( L, format2, args_before, buf, args_after );
+Debug( L, merged, args_before, args, args_after );
}
)

@useless_if@
expression L;
@@

-if ( LogTest( L ) ) {
 Debug( L, ... );
-}
-------------- next part --------------
@initialize:python@
@@

#regex from https://stackoverflow.com/questions/30011379/how-can-i-parse-a-c-format-string-in-python
import re
fmtstring = '''\
(                                  # start of capture group 1
%                                  # literal "%"
(?:                                # first option
(?:[-+0 #]{0,5})                   # optional flags
(?:\d+|\*)?                        # width
(?:\.(?:\d+|\*))?                  # precision
(?:h|l|ll|w|I|I32|I64)?            # size
[cCdiouxXeEfgGaAnpsSZ]             # type
) |                                # OR
%%)                                # literal "%%"
'''

regex = re.compile(fmtstring, re.X)

def parse_format(f):
    return tuple((m.span(), m.group()) for m in
        regex.finditer(f))

@a exists@
expression lock, E, L;
expression list args_before, args, args_after;
identifier buf;
expression format1, format2;
type T;
position p1, p2;
@@

{
...
T buf;
...
ldap_pvt_thread_mutex_lock(lock);
...
snprintf at p1( buf, E, format1, args );
...
ldap_pvt_thread_mutex_unlock(lock);
...
Debug at p2( L, format2, args_before, buf, args_after );
...
}

@script:python a_process@
format1 << a.format1;
format2 << a.format2;
args_before << a.args_before;
merged;
@@

pos = len(args_before.elements)
formats = parse_format(format2)

span, format = formats[pos]
merged = format2[:span[0]] + format1.strip('"') + format2[span[1]:]

coccinelle.merged = merged

@a_replace@
position a.p1, a.p2;
identifier a_process.merged;

expression lock, E, L;
expression list args_before, args, args_after;
identifier buf;
expression format1, format2;
type T;
@@

{
...
-T buf;
...
ldap_pvt_thread_mutex_lock(lock);
...
-snprintf at p1( buf, E, format1, args );
+Debug( L, merged, args_before, args, args_after );
...
ldap_pvt_thread_mutex_unlock(lock);
...
-Debug at p2( L, format2, args_before, buf, args_after );
...
}

@b exists@
expression E, L;
expression list args_before, args, args_after;
identifier buf;
expression format1, format2;
position p1, p2;
@@

snprintf at p1( buf, E, format1, args );
...
Debug at p2( L, format2, args_before, buf, args_after );

@script:python b_process@
format1 << b.format1;
format2 << b.format2;
args_before << b.args_before;
merged;
@@

pos = len(args_before.elements)
formats = parse_format(format2)

span, format = formats[pos]
merged = format2[:span[0]] + format1.strip('"') + format2[span[1]:]

coccinelle.merged = merged

@b_replace@
position b.p1, b.p2;
identifier b_process.merged;

expression E, L;
expression list args_before, args, args_after;
identifier buf;
expression format1, format2;
@@

-snprintf at p1( buf, E, format1, args );
+Debug( L, merged, args_before, args, args_after );
...
-Debug@p2( L, format2, args_before, buf, args_after );

[Cocci] Matching format strings

[Julia Lawall]

On Tue, 11 Jul 2017, Ond?ej Kuzn?k wrote:

> On Tue, Jul 11, 2017 at 03:10:36PM +0200, Julia Lawall wrote:
> > On Tue, 11 Jul 2017, Ond?ej Kuzn?k wrote:
> >> On Mon, Jul 10, 2017 at 07:26:36PM +0200, Julia Lawall wrote:
> >>> Try running spatch --parse-c on your directory to get a list at the end of
> >>> the top 10 things it doesn't like.
> >>
> >> One of the system dependent macros SLAP_EVENT_DECL is listed in each of
> >> the 10 things. The macro is used in
> >> https://github.com/openldap/openldap/blob/master/servers/slapd/daemon.c#L2343
> >> to declare some variables used in the corresponding event loop.
> >>
> >> The macro definition looks like this:
> >>
> >> # define SLAP_EVENT_DECL		struct pollfd *revents
> >>
> >> or:
> >>
> >> # define SLAP_EVENT_DECL	fd_set readfds, writefds; char *rflags
> >>
> >> Ignoring this macro is OK I guess, if there is a way to do that with
> >> coccinelle?
> >
> > Just
> >
> > #define SLAP_EVENT_DECL
> >
> > in the .h file. But there will be stray ;s then.  So you might want to put
> > the whole first definition.
>
> It does seem to improve things, thanks. The line 2496 still doesn't
> apply even when I declare SLAP_EVENT_FNAME to something like "name" in
> -macro-file-builtins and I think there were similar ones in other files
> later.
>
> >> Also, moving onto the other patch, I get similar inconsistencies with
> >> ./servers/slapd/back-sql/add.c, --parse-c doesn't list any problematic
> >> statements but the output logs a few parse errors that don't make much
> >> sense to me.
> >
> > Could you send the complete semantic patch that you are currently
> > considering?  Then I can take a look.
>
> Attaching the patches I'm currently working with:
> (-macro-file-builtins macros.h)
> 1. variadic.cocci
> 2. shortcut.cocci
> 3. merge.cocci
>
> The back-sql issue above is when trying to apply 2. (shortcut.cocci).
> But the one I pasted in my last email is simple enough to illustrate the
> issue (trying to merge snprintf and Debug, then get rid of extra if()s)
> even without the python stuff.

Sorry, but I'm lost.  I tried shortcut.cocci on
./servers/slapd/back-sql/add.c, and afterwards
./servers/slapd/back-sql/add.c has no snprintfs.  My command line is:

spatch -macro-file-builtins macros.h --in-place ./servers/slapd/back-sql/add.c shortcut.cocci

julia

>
> Thanks,
>
> --
> Ond?ej Kuzn?k
> Senior Software Engineer
> Symas Corporation                       http://www.symas.com
> Packaged, certified, and supported LDAP solutions powered by OpenLDAP
>

[Cocci] Matching format strings

[Ondřej Kuzník]

On Tue, Jul 11, 2017 at 09:24:24PM +0200, Julia Lawall wrote:
> On Tue, 11 Jul 2017, Ond?ej Kuzn?k wrote:
>> On Tue, Jul 11, 2017 at 03:10:36PM +0200, Julia Lawall wrote:
>>> Could you send the complete semantic patch that you are currently
>>> considering?  Then I can take a look.
>>
>> Attaching the patches I'm currently working with:
>> (-macro-file-builtins macros.h)
>> 1. variadic.cocci
>> 2. shortcut.cocci
>> 3. merge.cocci
>>
>> The back-sql issue above is when trying to apply 2. (shortcut.cocci).
>> But the one I pasted in my last email is simple enough to illustrate the
>> issue (trying to merge snprintf and Debug, then get rid of extra if()s)
>> even without the python stuff.
> 
> Sorry, but I'm lost.  I tried shortcut.cocci on
> ./servers/slapd/back-sql/add.c, and afterwards
> ./servers/slapd/back-sql/add.c has no snprintfs.  My command line is:
> 
> spatch -macro-file-builtins macros.h --in-place ./servers/slapd/back-sql/add.c shortcut.cocci

Yes, it removes the sprintfs, but I would have expected it to produce
this patch (note the if()s have been removed) as it manages in other
files:

--- servers/slapd/back-sql/add.c
+++ /tmp/cocci-output-22443-aee224-add.c
@@ -858,12 +858,10 @@ backsql_add_attr(
 
 #ifdef LDAP_DEBUG
-               if ( LogTest( LDAP_DEBUG_TRACE ) ) {
-                       snprintf( logbuf, sizeof( logbuf ), "val[%lu], id=" BACKSQL_IDNUMFMT,
-                                       i, new_keyval );
-                       Debug( LDAP_DEBUG_TRACE, "   backsql_add_attr(\"%s\"): "
-                               "executing \"%s\" %s\n", 
-                               op->ora_e->e_name.bv_val,
-                               at_rec->bam_add_proc, logbuf );
-               }
+               Debug(LDAP_DEBUG_TRACE,
+                     "   backsql_add_attr(\"%s\"): " "executing \"%s\" val[%lu], id=" BACKSQL_IDNUMFMT\n",
+                     op->ora_e->e_name.bv_val, at_rec->bam_add_proc,
+                     i, new_keyval);
 #endif
                rc = SQLExecute( sth );
@@ -1387,14 +1385,11 @@ backsql_add( Operation *op, SlapReply *r
        }
 
-       if ( LogTest( LDAP_DEBUG_TRACE ) ) {
-               char buf[ SLAP_TEXT_BUFLEN ];
-
-               snprintf( buf, sizeof(buf),
-                       "executing \"%s\" for dn=\"%s\"  oc_map_id=" BACKSQL_IDNUMFMT " p_id=" BACKSQL_IDFMT " keyval=" BACKSQL_IDNUMFMT,
-                       bi->sql_insentry_stmt, op->ora_e->e_name.bv_val,
-                       oc->bom_id, BACKSQL_IDARG(bsi.bsi_base_id.eid_id),
-                       new_keyval );
-               Debug( LDAP_DEBUG_TRACE, "   backsql_add(): %s\n", buf);
-       }
+       Debug(LDAP_DEBUG_TRACE,
+             "   backsql_add(): executing \"%s\" for dn=\"%s\"  oc_map_id=" BACKSQL_IDNUMFMT " p_id=" BACKSQL_IDFMT " keyval=" BACKSQL_IDNUMFMT\n",
+             bi->sql_insentry_stmt, op->ora_e->e_name.bv_val,
+             oc->bom_id, BACKSQL_IDARG(bsi.bsi_base_id.eid_id),
+             new_keyval);
 
        rc = SQLExecute( sth );



-- 
Ond?ej Kuzn?k
Senior Software Engineer
Symas Corporation                       http://www.symas.com
Packaged, certified, and supported LDAP solutions powered by OpenLDAP

[Cocci] Matching format strings

[Ondřej Kuzník]

On Tue, Jul 11, 2017 at 11:05:54PM +0100, Ond?ej Kuzn?k wrote:
> Yes, it removes the sprintfs, but I would have expected it to produce
> this patch (note the if()s have been removed) as it manages in other
> files:
> 
> --- servers/slapd/back-sql/add.c
> +++ /tmp/cocci-output-22443-aee224-add.c
> @@ -858,12 +858,10 @@ backsql_add_attr(
>  
>  #ifdef LDAP_DEBUG
> -               if ( LogTest( LDAP_DEBUG_TRACE ) ) {
> -                       snprintf( logbuf, sizeof( logbuf ), "val[%lu], id=" BACKSQL_IDNUMFMT,
> -                                       i, new_keyval );
> -                       Debug( LDAP_DEBUG_TRACE, "   backsql_add_attr(\"%s\"): "
> -                               "executing \"%s\" %s\n", 
> -                               op->ora_e->e_name.bv_val,
> -                               at_rec->bam_add_proc, logbuf );
> -               }
> +               Debug(LDAP_DEBUG_TRACE,
> +                     "   backsql_add_attr(\"%s\"): " "executing \"%s\" val[%lu], id=" BACKSQL_IDNUMFMT\n",

Oh, somehow the end of the format string gets mangled, presumably in the
Python code, which doesn't expect this at all, so I guess this would be
the reason the last part of the patch never applies. Sorry.

> +                     op->ora_e->e_name.bv_val, at_rec->bam_add_proc,
> +                     i, new_keyval);
>  #endif
>                 rc = SQLExecute( sth );
> @@ -1387,14 +1385,11 @@ backsql_add( Operation *op, SlapReply *r
>         }
>  
> -       if ( LogTest( LDAP_DEBUG_TRACE ) ) {
> -               char buf[ SLAP_TEXT_BUFLEN ];
> -
> -               snprintf( buf, sizeof(buf),
> -                       "executing \"%s\" for dn=\"%s\"  oc_map_id=" BACKSQL_IDNUMFMT " p_id=" BACKSQL_IDFMT " keyval=" BACKSQL_IDNUMFMT,
> -                       bi->sql_insentry_stmt, op->ora_e->e_name.bv_val,
> -                       oc->bom_id, BACKSQL_IDARG(bsi.bsi_base_id.eid_id),
> -                       new_keyval );
> -               Debug( LDAP_DEBUG_TRACE, "   backsql_add(): %s\n", buf);
> -       }
> +       Debug(LDAP_DEBUG_TRACE,
> +             "   backsql_add(): executing \"%s\" for dn=\"%s\"  oc_map_id=" BACKSQL_IDNUMFMT " p_id=" BACKSQL_IDFMT " keyval=" BACKSQL_IDNUMFMT\n",
> +             bi->sql_insentry_stmt, op->ora_e->e_name.bv_val,
> +             oc->bom_id, BACKSQL_IDARG(bsi.bsi_base_id.eid_id),
> +             new_keyval);
>  
>         rc = SQLExecute( sth );
> 

-- 
Ond?ej Kuzn?k
Senior Software Engineer
Symas Corporation                       http://www.symas.com
Packaged, certified, and supported LDAP solutions powered by OpenLDAP

[Cocci] Matching format strings

[Julia Lawall]

> Oh, somehow the end of the format string gets mangled, presumably in the
> Python code, which doesn't expect this at all, so I guess this would be
> the reason the last part of the patch never applies. Sorry.

Yes, it seems to be the inlining of the snprintf string for %s that
doesn't work when the snprintf string ends with a constant.

julia

[Cocci] Matching format strings

[Julia Lawall]

On Tue, 4 Jul 2017, Ond?ej Kuzn?k wrote:

> On Tue, Jul 04, 2017 at 05:25:46PM +0200, Julia Lawall wrote:
> > On Tue, 4 Jul 2017, Ond?ej Kuzn?k wrote:
> >> Hmm, suddenly the problems with passing expressions to python I stated
> >> earlier disappeared, good. Running this takes a log time on some files
> >> (over 10 minutes for ./servers/slapd/back-ldap/bind.c and processing of
> >> servers/slapd/back-meta/search.c has yet to finish after 30 minutes of
> >> processing).
> >>
> >> I suppose going through all the potential combinations of snprintf/Debug
> >> statements is what takes so long, any tips on what usually helps to
> >> speed things up?
> >
> > Some functions in those files have a lot of ifs.  Ifs are very expensive,
> > because both branches have to be considered.
>
> Do they have to be considered every time or can coccinelle sometimes
> understand that taking neither branch can affect the match? (as in, can
> I adjust the patch to help coccinelle somehow?).
>
> > In the rule below at least, I don't think you need the initial
> > { ... and final ... }.  That would only seem to be necessary if you have a
> > lot of redeclarations of variables.
>
> That is the case at least in servers/slapd/back-meta/search.c, where
> buf is often declared just before it is used and I hoped that would
> speed things up slightly.
>
> > Also, it could help to be more specific about the type of buf.
>
> I have tried
>
> {
> -char buf[S];
> -snprintf at p1( buf, E, format1, args );
> -Debug at p2( L, format2, args_before, buf, args_after );
> +Debug( L, merged, args_before, args, args_after );
> ... when != buf
> }

It should help a little bit, although not as much as you would expect, to
put the following rule first:

{
-char buf[S];
-snprintf at p1( buf, E, format1, args );
-Debug at p2( L, format2, args_before, buf, args_after );
+Debug( L, merged, args_before, args, args_after );
}

julia

>
> To see whether I can remove a lot of the simple ones quickly and then
> work with a simpler file, but this one seems to take the same amount of
> time once I put the "... when != buf" in.
>
> >> Also, more importantly, the patch below this seems to remove this
> >> comment:
> >> https://github.com/openldap/openldap/blob/master/servers/slapd/back-asyncmeta/conn.c#L547
> >>
> >> [...]
> >>
> >> I would like to preserve the comment, is there something I can do about
> >> that?
> >
> > I thought that comments are only removed when they come before removed
> > code, while here they come after removed code.  I can check on it.
>
> Thanks!
>
>
> --
> Ond?ej Kuzn?k
> Senior Software Engineer
> Symas Corporation                       http://www.symas.com
> Packaged, certified, and supported LDAP solutions powered by OpenLDAP
>