[Cocci] OpenLDAP debug statements

[Cocci] OpenLDAP debug statements

[Ondřej Kuzník]

Hi again,
having achieved some consensus to go ahead with this in the project and
some time to spare again, I have started on refining the patch to get as large
part of the OpenLDAP source code converted as possible.

There seem to be a few hundred occurrences that I can't seem to get
coccinelle to process, a few too many to process manually (by an order
of magnitude).

An example file with the most is servers/slapd/back-sql/add.c (55
instances of the Debug macro that should get some arguments dropped but
don't). Like others, spatch --parse-c finds no issues with the file.

Is there some way to improve the patch to get the false negatives down?

These are the instances I would expect removed (including the 9 that do
get picked up):
------8<-------
@@
expression E, E1, E2, S;
@@

Debug( E, S, E1, E2
*, 0
 );
------8<-------

This is the patch:
------8<-------
@@
identifier Logs =~ "Log[0-9]";
@@
-Logs
+Log

@@
@@
-StatslogTest
+LogTest

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

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

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

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

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

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

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

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

@@
expression E, S;
@@

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

@@
expression E, S;
@@

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

@@
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
)
------8<-------

And this is the macro file I use:
------8<-------
#define LDAP_PF_LOCAL_SENDMSG_ARG(x)

#define LDAP_P(x) x
#define LDAP_F(x) extern x
#define LDAP_V(x) extern x

#define LDAP_GCCATTR(x)                                 
#define LDAP_XSTRING(x) ""                              
#define LDAP_CONCAT(x,y) x                              

#define LDAP_CONST const                                
#define LDAP_BEGIN_DECL                                 
#define LDAP_END_DECL                                   

#define SLAP_EVENT_DECL                                 
#define SLAP_EVENT_FNAME

#define BACKSQL_ARBITRARY_KEY
#define BACKSQL_IDNUMFMT "%llu"
#define BACKSQL_IDFMT "%s"
#define BACKSQL_IDARG(arg) ((arg).bv_val)
------8<-------

Thanks,

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

[Cocci] OpenLDAP debug statements

[Julia Lawall]

On Tue, 29 Aug 2017, Ond?ej Kuzn?k wrote:

> Hi again,
> having achieved some consensus to go ahead with this in the project and
> some time to spare again, I have started on refining the patch to get as large
> part of the OpenLDAP source code converted as possible.
>
> There seem to be a few hundred occurrences that I can't seem to get
> coccinelle to process, a few too many to process manually (by an order
> of magnitude).

I believe that the problem is that he strings are split up into
multiple pieces.  The format list metavariable is not matching in that
case.  I will look into it further.

julia

>
> An example file with the most is servers/slapd/back-sql/add.c (55
> instances of the Debug macro that should get some arguments dropped but
> don't). Like others, spatch --parse-c finds no issues with the file.
>
> Is there some way to improve the patch to get the false negatives down?
>
> These are the instances I would expect removed (including the 9 that do
> get picked up):
> ------8<-------
> @@
> expression E, E1, E2, S;
> @@
>
> Debug( E, S, E1, E2
> *, 0
>  );
> ------8<-------
>
> This is the patch:
> ------8<-------
> @@
> identifier Logs =~ "Log[0-9]";
> @@
> -Logs
> +Log
>
> @@
> @@
> -StatslogTest
> +LogTest
>
> @@
> format list[2] fmt;
> expression list[2] args;
> expression E;
> @@
>
> Debug( E, "%@fmt@", args
> -, 0
>  );
>
> @@
> format list[2] fmt;
> expression list[2] args;
> expression E;
> @@
>
> Debug( E, "%@fmt@", args
> -, NULL
>  );
>
> @@
> format list[1] fmt;
> expression list[1] args;
> expression E;
> @@
>
> Debug( E, "%@fmt@", args
> -, 0, 0
>  );
>
> @@
> format list[1] fmt;
> expression list[1] args;
> expression E;
> @@
>
> Debug( E, "%@fmt@", args
> -, NULL, NULL
>  );
>
> @@
> expression E, S;
> @@
>
> Debug( E, S
> -, 0, 0, 0
>  );
>
> @@
> expression E, S;
> @@
>
> Debug( E, S
> -, NULL, NULL, NULL
>  );
>
> @@
> 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
> )
> ------8<-------
>
> And this is the macro file I use:
> ------8<-------
> #define LDAP_PF_LOCAL_SENDMSG_ARG(x)
>
> #define LDAP_P(x) x
> #define LDAP_F(x) extern x
> #define LDAP_V(x) extern x
>
> #define LDAP_GCCATTR(x)
> #define LDAP_XSTRING(x) ""
> #define LDAP_CONCAT(x,y) x
>
> #define LDAP_CONST const
> #define LDAP_BEGIN_DECL
> #define LDAP_END_DECL
>
> #define SLAP_EVENT_DECL
> #define SLAP_EVENT_FNAME
>
> #define BACKSQL_ARBITRARY_KEY
> #define BACKSQL_IDNUMFMT "%llu"
> #define BACKSQL_IDFMT "%s"
> #define BACKSQL_IDARG(arg) ((arg).bv_val)
> ------8<-------
>
> 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] OpenLDAP debug statements

[Julia Lawall]

On Tue, 29 Aug 2017, Ond?ej Kuzn?k wrote:

> Hi again,
> having achieved some consensus to go ahead with this in the project and
> some time to spare again, I have started on refining the patch to get as large
> part of the OpenLDAP source code converted as possible.
>
> There seem to be a few hundred occurrences that I can't seem to get
> coccinelle to process, a few too many to process manually (by an order
> of magnitude).
>

Allowing format list to match strings in multiple pieces seems like a
somewhat complex undertaking.

Perhaps the following could be acceptable?

@initialize:ocaml@
@@

let fmtn(fmt,n) =
   List.length (Str.split_delim (Str.regexp_string "%") fmt) = 3

@@
char[] fmt : script:ocaml() { fmtn(fmt,2) };
expression list[2] args;
expression E;
@@

Debug( E, fmt, args
-, 0
 );

You would have to duplicate the latter rule for each possible number of
arugments.

julia

[Cocci] OpenLDAP debug statements

[Ondřej Kuzník]

On Tue, Aug 29, 2017 at 11:17:53PM +0200, Julia Lawall wrote:
> On Tue, 29 Aug 2017, Ond?ej Kuzn?k wrote:
>> Hi again,
>> having achieved some consensus to go ahead with this in the project and
>> some time to spare again, I have started on refining the patch to get as large
>> part of the OpenLDAP source code converted as possible.
>>
>> There seem to be a few hundred occurrences that I can't seem to get
>> coccinelle to process, a few too many to process manually (by an order
>> of magnitude).
>>
> 
> Allowing format list to match strings in multiple pieces seems like a
> somewhat complex undertaking.

Hi Julia,
I thought matching multiple part strings had worked before, but maybe
not the format string part of it. Again, I have little understanding of
how the parser works, but maybe it might be able to keep around a second
internal representation of the string as it is concatenated and extract
this information from that, even if it did not allow all manipulations
on it?

Or maybe a solution that could be shared with the one for
stringification/concatenation (# and ##)? But at the moment, I think
coccinelle struggles with these as well, so maybe not.

> Perhaps the following could be acceptable?
> 
> @initialize:ocaml@
> @@
> 
> let fmtn(fmt,n) =
>    List.length (Str.split_delim (Str.regexp_string "%") fmt) = 3
> 
> @@
> char[] fmt : script:ocaml() { fmtn(fmt,2) };
> expression list[2] args;
> expression E;
> @@
> 
> Debug( E, fmt, args
> -, 0
>  );
> 
> You would have to duplicate the latter rule for each possible number of
> arugments.

I'll test it out, the potential problem I see is when I look into the
second step, merging the sprintf(buf, fmt,...);Debug(...,fmt2,...,buf,...)
calls into a single Debug call. I think I needed similar information
there but had no idea how complex the format string was, but will check
that patch again as well.

Thanks,

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

[Cocci] OpenLDAP debug statements

[Julia Lawall]

On Tue, 29 Aug 2017, Ond?ej Kuzn?k wrote:

> On Tue, Aug 29, 2017 at 11:17:53PM +0200, Julia Lawall wrote:
> > On Tue, 29 Aug 2017, Ond?ej Kuzn?k wrote:
> >> Hi again,
> >> having achieved some consensus to go ahead with this in the project and
> >> some time to spare again, I have started on refining the patch to get as large
> >> part of the OpenLDAP source code converted as possible.
> >>
> >> There seem to be a few hundred occurrences that I can't seem to get
> >> coccinelle to process, a few too many to process manually (by an order
> >> of magnitude).
> >>
> >
> > Allowing format list to match strings in multiple pieces seems like a
> > somewhat complex undertaking.
>
> Hi Julia,
> I thought matching multiple part strings had worked before, but maybe
> not the format string part of it.

No, not the format string part.

> Again, I have little understanding of
> how the parser works, but maybe it might be able to keep around a second
> internal representation of the string as it is concatenated and extract
> this information from that, even if it did not allow all manipulations
> on it?

This was my thought also, but looking at the code, it seems complex.

julia

>
> Or maybe a solution that could be shared with the one for
> stringification/concatenation (# and ##)? But at the moment, I think
> coccinelle struggles with these as well, so maybe not.
>
> > Perhaps the following could be acceptable?
> >
> > @initialize:ocaml@
> > @@
> >
> > let fmtn(fmt,n) =
> >    List.length (Str.split_delim (Str.regexp_string "%") fmt) = 3
> >
> > @@
> > char[] fmt : script:ocaml() { fmtn(fmt,2) };
> > expression list[2] args;
> > expression E;
> > @@
> >
> > Debug( E, fmt, args
> > -, 0
> >  );
> >
> > You would have to duplicate the latter rule for each possible number of
> > arugments.
>
> I'll test it out, the potential problem I see is when I look into the
> second step, merging the sprintf(buf, fmt,...);Debug(...,fmt2,...,buf,...)
> calls into a single Debug call. I think I needed similar information
> there but had no idea how complex the format string was, but will check
> that patch again as well.
>
> Thanks,
>
> --
> Ond?ej Kuzn?k
> Senior Software Engineer
> Symas Corporation                       http://www.symas.com
> Packaged, certified, and supported LDAP solutions powered by OpenLDAP
>

[Cocci] OpenLDAP debug statements

[Ondřej Kuzník]

On Wed, Aug 30, 2017 at 07:25:00AM +0200, Julia Lawall wrote:
> On Tue, 29 Aug 2017, Ond?ej Kuzn?k wrote:
>> On Tue, Aug 29, 2017 at 11:17:53PM +0200, Julia Lawall wrote:
>>> Allowing format list to match strings in multiple pieces seems like a
>>> somewhat complex undertaking.
>>
>> Again, I have little understanding of
>> how the parser works, but maybe it might be able to keep around a second
>> internal representation of the string as it is concatenated and extract
>> this information from that, even if it did not allow all manipulations
>> on it?
> 
> This was my thought also, but looking at the code, it seems complex.

OK, I understand. I any case, the suggestion below has very few false
negatives and no false positives that I could find, thanks a lot!

The only false negatives are related to macro expansion, which probably
ties into the same limitation. It's strings like this:

"string" BACKSQL_IDFMT "rest of string"

Where BACKSQL_IDFMT is "%llu" or similar. Given how few of these there
are and that there have been no false positives from that that I could
see, I'm happy.

I will see how the sprintf merging will pan out shortly after I clean
out the false negatives and re-test the patch series up to this point.

Again, many thanks! Without coccinelle, this project would not have been
possible at all.

>>> Perhaps the following could be acceptable?
>>>
>>> @initialize:ocaml@
>>> @@
>>>
>>> let fmtn(fmt,n) =
>>>    List.length (Str.split_delim (Str.regexp_string "%") fmt) = 3
>>>
>>> @@
>>> char[] fmt : script:ocaml() { fmtn(fmt,2) };
>>> expression list[2] args;
>>> expression E;
>>> @@
>>>
>>> Debug( E, fmt, args
>>> -, 0
>>>  );
>>>
>>> You would have to duplicate the latter rule for each possible number of
>>> arugments.
>>
>> I'll test it out, the potential problem I see is when I look into the
>> second step, merging the sprintf(buf, fmt,...);Debug(...,fmt2,...,buf,...)
>> calls into a single Debug call. I think I needed similar information
>> there but had no idea how complex the format string was, but will check
>> that patch again as well.

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

[Cocci] OpenLDAP debug statements

[Julia Lawall]

On Wed, 30 Aug 2017, Ond?ej Kuzn?k wrote:

> On Wed, Aug 30, 2017 at 07:25:00AM +0200, Julia Lawall wrote:
> > On Tue, 29 Aug 2017, Ond?ej Kuzn?k wrote:
> >> On Tue, Aug 29, 2017 at 11:17:53PM +0200, Julia Lawall wrote:
> >>> Allowing format list to match strings in multiple pieces seems like a
> >>> somewhat complex undertaking.
> >>
> >> Again, I have little understanding of
> >> how the parser works, but maybe it might be able to keep around a second
> >> internal representation of the string as it is concatenated and extract
> >> this information from that, even if it did not allow all manipulations
> >> on it?
> >
> > This was my thought also, but looking at the code, it seems complex.
>
> OK, I understand. I any case, the suggestion below has very few false
> negatives and no false positives that I could find, thanks a lot!
>
> The only false negatives are related to macro expansion, which probably
> ties into the same limitation. It's strings like this:
>
> "string" BACKSQL_IDFMT "rest of string"
>
> Where BACKSQL_IDFMT is "%llu" or similar. Given how few of these there
> are and that there have been no false positives from that that I could
> see, I'm happy.

Maybe if you put this macro in with the others, then it will find it.

>
> I will see how the sprintf merging will pan out shortly after I clean
> out the false negatives and re-test the patch series up to this point.
>
> Again, many thanks! Without coccinelle, this project would not have been
> possible at all.

:)

julia

> >>> Perhaps the following could be acceptable?
> >>>
> >>> @initialize:ocaml@
> >>> @@
> >>>
> >>> let fmtn(fmt,n) =
> >>>    List.length (Str.split_delim (Str.regexp_string "%") fmt) = 3
> >>>
> >>> @@
> >>> char[] fmt : script:ocaml() { fmtn(fmt,2) };
> >>> expression list[2] args;
> >>> expression E;
> >>> @@
> >>>
> >>> Debug( E, fmt, args
> >>> -, 0
> >>>  );
> >>>
> >>> You would have to duplicate the latter rule for each possible number of
> >>> arugments.
> >>
> >> I'll test it out, the potential problem I see is when I look into the
> >> second step, merging the sprintf(buf, fmt,...);Debug(...,fmt2,...,buf,...)
> >> calls into a single Debug call. I think I needed similar information
> >> there but had no idea how complex the format string was, but will check
> >> that patch again as well.
>
> --
> Ond?ej Kuzn?k
> Senior Software Engineer
> Symas Corporation                       http://www.symas.com
> Packaged, certified, and supported LDAP solutions powered by OpenLDAP
>

[Cocci] OpenLDAP debug statements

[Ondřej Kuzník]

On Wed, Aug 30, 2017 at 03:05:04PM +0200, Julia Lawall wrote:
> On Wed, 30 Aug 2017, Ond?ej Kuzn?k wrote:
>> OK, I understand. I any case, the suggestion below has very few false
>> negatives and no false positives that I could find, thanks a lot!
>>
>> The only false negatives are related to macro expansion, which probably
>> ties into the same limitation. It's strings like this:
>>
>> "string" BACKSQL_IDFMT "rest of string"
>>
>> Where BACKSQL_IDFMT is "%llu" or similar. Given how few of these there
>> are and that there have been no false positives from that that I could
>> see, I'm happy.
> 
> Maybe if you put this macro in with the others, then it will find it.

I already have, but I guess the Ocaml code doesn't have access to the
expanded version.

Moving onto the next step, I'm having problems with coccinelle rejecting
some patches for reasons I don't understand. For example it throws an
error applying the patch below even though some other files are fine and
the file in question parses fine without having to pass a builtin macro
file.

The patch tries to merge an snprintf and Debug if Debug is the only
consumer of that buffer plus removes the LogTest and buffer declaration
if appropriate.

This is the error I get on the file:

-----8<------
HANDLING: servers/slapd/back-asyncmeta/bind.c
     
previous modification:
MINUS
  >>> Debug(L, merged, args_before, args, args_after);

According to environment 10:
   shortcut.p1 -> poss[(servers/slapd/back-asyncmeta/bind.c,asyncmeta_dobind_init,((1679,0),(1910,1)),(1865,2),(1865,10))]
   shortcut.p2 -> poss[(servers/slapd/back-asyncmeta/bind.c,asyncmeta_dobind_init,((1679,0),(1910,1)),(1867,2),(1867,7))]
   shortcut_process.merged -> id "### %s asyncmeta_search_dobind_init[%d] mc=%p ld=%p rc=%d\n"
   shortcut_replace.args_after -> <<exprlist>>
   shortcut_replace.args_before -> <<exprlist>>
   shortcut_replace.L -> LDAP_DEBUG_ANY
   shortcut_replace.args -> <<exprlist>>
   shortcut.p1 -> poss[(servers/slapd/back-asyncmeta/bind.c,asyncmeta_dobind_init,((1679,0),(1910,1)),(1865,2),(1865,10))]
   shortcut.p2 -> poss[(servers/slapd/back-asyncmeta/bind.c,asyncmeta_dobind_init,((1679,0),(1910,1)),(1867,2),(1867,7))]
   shortcut_process.merged -> id "### %s asyncmeta_search_dobind_init[%d] mc=%p ld=%p rc=%d\n"

current modification:
MINUS
According to environment 9:
   shortcut.p1 -> poss[(servers/slapd/back-asyncmeta/bind.c,asyncmeta_dobind_init,((1679,0),(1910,1)),(1865,2),(1865,10))]
   shortcut.p2 -> poss[(servers/slapd/back-asyncmeta/bind.c,asyncmeta_dobind_init,((1679,0),(1910,1)),(1867,2),(1867,7))]
   shortcut_process.merged -> id "### %s asyncmeta_search_dobind_init[%d] mc=%p ld=%p rc=%d\n"
   shortcut_replace.L -> LDAP_DEBUG_ANY
   shortcut_replace.args_before -> <<exprlist>>
   shortcut_replace.args_after -> <<exprlist>>
   shortcut.p1 -> poss[(servers/slapd/back-asyncmeta/bind.c,asyncmeta_dobind_init,((1679,0),(1910,1)),(1865,2),(1865,10))]
   shortcut.p2 -> poss[(servers/slapd/back-asyncmeta/bind.c,asyncmeta_dobind_init,((1679,0),(1910,1)),(1867,2),(1867,7))]
   shortcut_process.merged -> id "### %s asyncmeta_search_dobind_init[%d] mc=%p ld=%p rc=%d\n"

shortcut_replace: already tagged token:
C code context
File "servers/slapd/back-asyncmeta/bind.c", line 1867, column 2, charpos = 45042
  around = 'Debug',
  whole content =               Debug( LDAP_DEBUG_ANY, "### %s %s\n",
-----8<------

The patch in question:

-----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))

def insert_at_pos(fmt, s, pos):
    formats = parse_format(fmt)
    span, format = formats[pos]
    acc = fmt[:span[0]]
    if s.startswith('"'):
        acc += s[1:]
    else:
        acc += '" '
        acc += s
    if acc.endswith('"'):
        acc = acc[:-1] + fmt[span[1]:]
    else:
        acc += ' "'
        acc += fmt[span[1]:]
    return acc

// covered by others but processing those 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; \| T buf; \)
snprintf at p1( buf, E, format1, args );
Debug at p2( L, format2, args_before, buf, args_after );
}
|
{
\( T buf = {...}; \| T buf = I; \| T buf; \| \)
snprintf at p1( buf, E, format1, args );
Debug at p2( L, format2, args_before, buf, args_after );
}
|
\( T buf = {...}; \| T buf = I; \| T buf; \| \)
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)
coccinelle.merged = insert_at_pos(format2, format1, pos)

@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; \| T buf; \| \)
-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; \| T buf; \| \)
-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; \| T buf; \| \)
-snprintf at p1( buf, E, format1, args );
-Debug@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, ... );
-}
-----8<------

Thanks,

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

[Cocci] OpenLDAP debug statements

[Julia Lawall]

The problem is in the rule shortcut_replace.  You have:

(
-{
-\( T buf = {...}; \| T buf = I; \| T buf; \| \)
-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; \| T buf; \| \)
-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; \| T buf; \| \)
-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 );
)

There are three case (1, 2, and 3).  Normally a disjunction like this
matches from top to bottom, ie if the first case matches the ohers are not
considered.  Here however the patterns don' all start matching at the same
node.  The first two do, ie at an {, but the third does not.  So the third
will match over top of what was matched by the previous ones.  So there
will b multiple matches, and Coccinelle would prefer to do nothing rather
than to do something inconsistent.

As far as I can see, there is no real difference between the cases.  It
could probably be easier to just put the third case, and then have some
other rules to clean up any kinds of {} that occur.

I think that it should also be possible to define a metavariable

initializer i;

And then replace

T buf = {...}; \| T buf = I;

by T buf = i;

julia

[Cocci] OpenLDAP debug statements

[Ondřej Kuzník]

On Thu, Aug 31, 2017 at 04:09:30PM +0200, Julia Lawall wrote:
> The problem is in the rule shortcut_replace.  You have:
> 
> [...]
> 
> There are three case (1, 2, and 3).  Normally a disjunction like this
> matches from top to bottom, ie if the first case matches the ohers are not
> considered.  Here however the patterns don' all start matching at the same
> node.  The first two do, ie at an {, but the third does not.  So the third
> will match over top of what was matched by the previous ones.  So there
> will b multiple matches, and Coccinelle would prefer to do nothing rather
> than to do something inconsistent.
> 
> As far as I can see, there is no real difference between the cases.  It
> could probably be easier to just put the third case, and then have some
> other rules to clean up any kinds of {} that occur.

Yes, that has worked quite well actually. It has caused one problem,
though, splitting it up has made coccinelle strip curly brackets all
over the place, which is a bit problematic in places like this:

if (...) {
    /* comment */
    Debug( ... );
}

turning it into something very hard to read, especially if there is an
else branch:

if (...)
    /* comment */
    Debug( ... );

So far, nothing I've tried has helped limiting this, those include:
- pinning it on "+Debug at p3" which coccinelle rejects.
- using the below, probably because of your observation above
(
 if (...) {
     Debug(...);
 }
|
-{
 Debug(...);
-}
)
- the above but splitting that in two rules and a remembered location

The current patch I'm using is:
-----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))

def insert_at_pos(fmt, s, pos):
    formats = parse_format(fmt)
    span, format = formats[pos]
    acc = fmt[:span[0]]
    if s.startswith('"'):
        acc += s[1:]
    else:
        acc += '" '
        acc += s
    if acc.endswith('"'):
        acc = acc[:-1] + fmt[span[1]:]
    else:
        acc += ' "'
        acc += fmt[span[1]:]
    return acc

// covered by others but processing that can take hours on some files
@shortcut@
identifier buf;
expression E, L;
expression list args_before, args, args_after;
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 shortcut_process@
format1 << shortcut.format1;
format2 << shortcut.format2;
args_before << shortcut.args_before;
merged;
@@

pos = len(args_before.elements)
coccinelle.merged = insert_at_pos(format2, format1, pos)

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

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

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

@@
type T;
initializer I;
@@
{
-\( T buf = I; \| T buf; \)
 Debug( ... );
}

@@
@@
-{
 Debug( ... );
-}

@useless_if@
expression L;
@@

-if ( LogTest( L ) ) {
 Debug( L, ... );
-}
-----8<------

> I think that it should also be possible to define a metavariable
> 
> initializer i;
> 
> And then replace
> 
> T buf = {...}; \| T buf = I;
> 
> by T buf = i;

That does work, thanks.

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

[Cocci] OpenLDAP debug statements

[Julia Lawall]

On Thu, 31 Aug 2017, Ond?ej Kuzn?k wrote:

> On Thu, Aug 31, 2017 at 04:09:30PM +0200, Julia Lawall wrote:
> > The problem is in the rule shortcut_replace.  You have:
> >
> > [...]
> >
> > There are three case (1, 2, and 3).  Normally a disjunction like this
> > matches from top to bottom, ie if the first case matches the ohers are not
> > considered.  Here however the patterns don' all start matching at the same
> > node.  The first two do, ie at an {, but the third does not.  So the third
> > will match over top of what was matched by the previous ones.  So there
> > will b multiple matches, and Coccinelle would prefer to do nothing rather
> > than to do something inconsistent.
> >
> > As far as I can see, there is no real difference between the cases.  It
> > could probably be easier to just put the third case, and then have some
> > other rules to clean up any kinds of {} that occur.
>
> Yes, that has worked quite well actually. It has caused one problem,
> though, splitting it up has made coccinelle strip curly brackets all
> over the place, which is a bit problematic in places like this:
>
> if (...) {
>     /* comment */
>     Debug( ... );
> }
>
> turning it into something very hard to read, especially if there is an
> else branch:
>
> if (...)
>     /* comment */
>     Debug( ... );

So you want to drop the braces if there is no comment and keep the brace
if there is a comment?  You can't match on comments.  The only thing you
could do is check the number of lines between the { and Debug.

@r@
position p1,p2;
@@

{@p1
Debug at p2(...)
}

@script:python@
p1 << r.p1;
p2 << r.p2;
@@

if p2[0].line - p1.line >= 2:
   cocci.include_match(False)

@@
position r.p1,r.p2;
@@

-{@p1
Debug at p2(...)
-}

julia


>
> So far, nothing I've tried has helped limiting this, those include:
> - pinning it on "+Debug at p3" which coccinelle rejects.
> - using the below, probably because of your observation above
> (
>  if (...) {
>      Debug(...);
>  }
> |
> -{
>  Debug(...);
> -}
> )
> - the above but splitting that in two rules and a remembered location
>
> The current patch I'm using is:
> -----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))
>
> def insert_at_pos(fmt, s, pos):
>     formats = parse_format(fmt)
>     span, format = formats[pos]
>     acc = fmt[:span[0]]
>     if s.startswith('"'):
>         acc += s[1:]
>     else:
>         acc += '" '
>         acc += s
>     if acc.endswith('"'):
>         acc = acc[:-1] + fmt[span[1]:]
>     else:
>         acc += ' "'
>         acc += fmt[span[1]:]
>     return acc
>
> // covered by others but processing that can take hours on some files
> @shortcut@
> identifier buf;
> expression E, L;
> expression list args_before, args, args_after;
> 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 shortcut_process@
> format1 << shortcut.format1;
> format2 << shortcut.format2;
> args_before << shortcut.args_before;
> merged;
> @@
>
> pos = len(args_before.elements)
> coccinelle.merged = insert_at_pos(format2, format1, pos)
>
> @shortcut_replace@
> position shortcut.p1, shortcut.p2;
> identifier shortcut_process.merged;
>
> identifier buf;
> expression E, L;
> expression list args_before, args, args_after;
> expression format1, format2;
> @@
>
> -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 );
>
> @@
> type T;
> initializer I;
> @@
> {
> -\( T buf = I; \| T buf; \)
>  Debug( ... );
> }
>
> @@
> @@
> -{
>  Debug( ... );
> -}
>
> @useless_if@
> expression L;
> @@
>
> -if ( LogTest( L ) ) {
>  Debug( L, ... );
> -}
> -----8<------
>
> > I think that it should also be possible to define a metavariable
> >
> > initializer i;
> >
> > And then replace
> >
> > T buf = {...}; \| T buf = I;
> >
> > by T buf = i;
>
> That does work, thanks.
>
> --
> Ond?ej Kuzn?k
> Senior Software Engineer
> Symas Corporation                       http://www.symas.com
> Packaged, certified, and supported LDAP solutions powered by OpenLDAP
>

[Cocci] OpenLDAP debug statements

[Ondřej Kuzník]

On Thu, Aug 31, 2017 at 05:52:14PM +0200, Julia Lawall wrote:
> On Thu, 31 Aug 2017, Ond?ej Kuzn?k wrote:
>> Yes, that has worked quite well actually. It has caused one problem,
>> though, splitting it up has made coccinelle strip curly brackets all
>> over the place, which is a bit problematic in places like this:
>>
>> if (...) {
>>     /* comment */
>>     Debug( ... );
>> }
>>
>> turning it into something very hard to read, especially if there is an
>> else branch:
>>
>> if (...)
>>     /* comment */
>>     Debug( ... );
> 
> So you want to drop the braces if there is no comment and keep the brace
> if there is a comment?  You can't match on comments.  The only thing you
> could do is check the number of lines between the { and Debug.

Something slightly different, I want to remove the braces only if they
do not "belong" to an if statement.

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

[Cocci] OpenLDAP debug statements

[Julia Lawall]

On Thu, 31 Aug 2017, Ond?ej Kuzn?k wrote:

> On Thu, Aug 31, 2017 at 05:52:14PM +0200, Julia Lawall wrote:
> > On Thu, 31 Aug 2017, Ond?ej Kuzn?k wrote:
> >> Yes, that has worked quite well actually. It has caused one problem,
> >> though, splitting it up has made coccinelle strip curly brackets all
> >> over the place, which is a bit problematic in places like this:
> >>
> >> if (...) {
> >>     /* comment */
> >>     Debug( ... );
> >> }
> >>
> >> turning it into something very hard to read, especially if there is an
> >> else branch:
> >>
> >> if (...)
> >>     /* comment */
> >>     Debug( ... );
> >
> > So you want to drop the braces if there is no comment and keep the brace
> > if there is a comment?  You can't match on comments.  The only thing you
> > could do is check the number of lines between the { and Debug.
>
> Something slightly different, I want to remove the braces only if they
> do not "belong" to an if statement.

Does that occur?

In any case, just do:

@r@
position p1;
statement S;
@@

if (...) {@p1  Debug(...); } else S

@@
position p1 != r.p1;
@@

- {@p1
  Debug(...);
- }

julia

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

[Cocci] OpenLDAP debug statements

[Ondřej Kuzník]

On Thu, Aug 31, 2017 at 06:01:07PM +0200, Julia Lawall wrote:
> On Thu, 31 Aug 2017, Ond?ej Kuzn?k wrote:
>> On Thu, Aug 31, 2017 at 05:52:14PM +0200, Julia Lawall wrote:
>>> So you want to drop the braces if there is no comment and keep the brace
>>> if there is a comment?  You can't match on comments.  The only thing you
>>> could do is check the number of lines between the { and Debug.
>>
>> Something slightly different, I want to remove the braces only if they
>> do not "belong" to an if statement.
> 
> Does that occur?
> 
> In any case, just do:
> 
> @r@
> position p1;
> statement S;
> @@
> 
> if (...) {@p1  Debug(...); } else S
> 
> @@
> position p1 != r.p1;
> @@
> 
> -{@p1
>   Debug(...);
> -}

I have tried the following:

@guard@
position p;
@@

if ( ... ) {@p 
 Debug( ... );
}

@@
position p != guard.p;
@@
-{@p
 Debug( ... );
-}

But it still modifies servers/slapd/back-asyncmeta/bind.c:879

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

[Cocci] OpenLDAP debug statements

[Julia Lawall]

On Thu, 31 Aug 2017, Ond?ej Kuzn?k wrote:

> On Thu, Aug 31, 2017 at 06:01:07PM +0200, Julia Lawall wrote:
> > On Thu, 31 Aug 2017, Ond?ej Kuzn?k wrote:
> >> On Thu, Aug 31, 2017 at 05:52:14PM +0200, Julia Lawall wrote:
> >>> So you want to drop the braces if there is no comment and keep the brace
> >>> if there is a comment?  You can't match on comments.  The only thing you
> >>> could do is check the number of lines between the { and Debug.
> >>
> >> Something slightly different, I want to remove the braces only if they
> >> do not "belong" to an if statement.
> >
> > Does that occur?
> >
> > In any case, just do:
> >
> > @r@
> > position p1;
> > statement S;
> > @@
> >
> > if (...) {@p1  Debug(...); } else S
> >
> > @@
> > position p1 != r.p1;
> > @@
> >
> > -{@p1
> >   Debug(...);
> > -}
>
> I have tried the following:
>
> @guard@
> position p;
> @@
>
> if ( ... ) {@p
>  Debug( ... );
> }

You didn't put the else S.

If you add it, the rule will still match cases with no else.

julia

>
> @@
> position p != guard.p;
> @@
> -{@p
>  Debug( ... );
> -}
>
> But it still modifies servers/slapd/back-asyncmeta/bind.c:879
>
> --
> Ond?ej Kuzn?k
> Senior Software Engineer
> Symas Corporation                       http://www.symas.com
> Packaged, certified, and supported LDAP solutions powered by OpenLDAP
>