Re: [Cocci] how to make substitutions at the end of the function, vs. the end of each block ?

2021-07-24 Thread Andrew  Yourtchenko


> On 23 Jul 2021, at 21:35, Julia Lawall  wrote:
> 
> 
>> 
>> I missed the fact that the "return ERR" were actually not replaced on
>> this example at all,
> 
> This is because there are no occurrences of return ERR; in your code.
> Probably you expected ERR to be a metavariable, but it's not.  

I had it declared at one point declared as “expression ERR;”, so total PEBCAK 
here, sorry for that! 


> Actually,
> fully capitalizing your metavariables is not a good idea.  Coccinelle
> expects that fully capitalized things are constants, as in #define ERR -1,
> and so when they appear at random places, it doesn't comment about that.
> If it had been in lowercase and used as the argument of a return (or as a
> function argument, right-hand side of an assignment, etc), Coccinelle
> would have printed a warning wondering if you expected it to be a
> metavariable.

Oh, thanks a lot for the tip! 

> 
> If ERR is declared as an expression metavariable, then lots of returns are
> replaced.

Excellent, thanks a lot for the education ! :)

—a

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


Re: [Cocci] how to make substitutions at the end of the function, vs. the end of each block ?

2021-07-23 Thread Julia Lawall
Here is another attempt:

@initialize:ocaml@
@@

let check p =
  let p = List.hd p in
  p.line_end = p.current_element_line_end

@ detect_func @
identifier CLI_FN, AVM, AIN, ACMD;
fresh identifier LAIN = "line_" ## AIN;
expression ERR, exp;
statement S1;
typedef clib_error_t, vlib_main_t, unformat_input_t, vlib_cli_command_t;
@@

static clib_error_t *CLI_FN (vlib_main_t * AVM, unformat_input_t *
AIN, vlib_cli_command_t * ACMD)
{
+ clib_error_t *e = 0;
+  unformat_input_t *LAIN;
...
+ if (!unformat_user (AIN, unformat_line_input, LAIN)) {
+return 0;
+ }
+
-  while (unformat_check_input (AIN) != UNFORMAT_END_OF_INPUT)
+ while (unformat_check_input (LAIN) != UNFORMAT_END_OF_INPUT)
S1
<...
- return ERR;
+ e = ERR;
+ goto done;
...>
}

// the following rule should rematch the function matched in the previous rule
@@
identifier detect_func.CLI_FN, detect_func.AVM, detect_func.AIN, 
detect_func.LAIN, detect_func.ACMD;
position p : script:ocaml() { check p }; // check that the matched position is 
at the end of the function
@@

static clib_error_t *CLI_FN (vlib_main_t * AVM, unformat_input_t *
AIN, vlib_cli_command_t * ACMD)
{
... when exists
+done:
+ unformat_free(LAIN);
+ return e;
}@p
___
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci


Re: [Cocci] how to make substitutions at the end of the function, vs. the end of each block ?

2021-07-23 Thread Julia Lawall
> yourtch@ayourtch-lnx:~/cocci$ diff -c patch-old.cocci patch-new.cocci
> *** patch-old.cocci   2021-07-22 22:41:19.516957878 +0200
> --- patch-new.cocci   2021-07-22 22:41:52.625184341 +0200
> ***
> *** 3,8 
> --- 3,9 
>   fresh identifier LAIN = "line_" ## AIN;
>
>   statement S1;
> + expression exp;
>
>   typedef clib_error_t, vlib_main_t, unformat_input_t, vlib_cli_command_t;
>   @@
> ***
> *** 20,30 
>   -  while (unformat_check_input (AIN) != UNFORMAT_END_OF_INPUT)
>   + while (unformat_check_input (LAIN) != UNFORMAT_END_OF_INPUT)
>   S1
> ! <...
>   - return ERR;
>   + e = ERR;
>   + goto done;
> ! ...>
>   +done:
>   + unformat_free(LAIN);
>   + return e;
> --- 21,32 
>   -  while (unformat_check_input (AIN) != UNFORMAT_END_OF_INPUT)
>   + while (unformat_check_input (LAIN) != UNFORMAT_END_OF_INPUT)
>   S1
> ! <... when != true exp
> !  when exists
>   - return ERR;
>   + e = ERR;
>   + goto done;
> ! ...>
>   +done:
>   + unformat_free(LAIN);
>   + return e;
> ayourtch@ayourtch-lnx:~/cocci$
>
> And the result was the same...

My trick doesn't work because the return of interest is under a switch,
where there is no test expression that has the value true or false.  I
will see if something else can be done.

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


Re: [Cocci] how to make substitutions at the end of the function, vs. the end of each block ?

2021-07-23 Thread Julia Lawall
> I missed the fact that the "return ERR" were actually not replaced on
> this example at all,

This is because there are no occurrences of return ERR; in your code.
Probably you expected ERR to be a metavariable, but it's not.  Actually,
fully capitalizing your metavariables is not a good idea.  Coccinelle
expects that fully capitalized things are constants, as in #define ERR -1,
and so when they appear at random places, it doesn't comment about that.
If it had been in lowercase and used as the argument of a return (or as a
function argument, right-hand side of an assignment, etc), Coccinelle
would have printed a warning wondering if you expected it to be a
metavariable.

If ERR is declared as an expression metavariable, then lots of returns are
replaced.

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


Re: [Cocci] how to make substitutions at the end of the function, vs. the end of each block ?

2021-07-22 Thread Andrew  Yourtchenko
Hi Julia,

thanks a lot for such a quick reply! :)

I tried this:

yourtch@ayourtch-lnx:~/cocci$ diff -c patch-old.cocci patch-new.cocci
*** patch-old.cocci 2021-07-22 22:41:19.516957878 +0200
--- patch-new.cocci 2021-07-22 22:41:52.625184341 +0200
***
*** 3,8 
--- 3,9 
  fresh identifier LAIN = "line_" ## AIN;

  statement S1;
+ expression exp;

  typedef clib_error_t, vlib_main_t, unformat_input_t, vlib_cli_command_t;
  @@
***
*** 20,30 
  -  while (unformat_check_input (AIN) != UNFORMAT_END_OF_INPUT)
  + while (unformat_check_input (LAIN) != UNFORMAT_END_OF_INPUT)
  S1
! <...
  - return ERR;
  + e = ERR;
  + goto done;
! ...>
  +done:
  + unformat_free(LAIN);
  + return e;
--- 21,32 
  -  while (unformat_check_input (AIN) != UNFORMAT_END_OF_INPUT)
  + while (unformat_check_input (LAIN) != UNFORMAT_END_OF_INPUT)
  S1
! <... when != true exp
!  when exists
  - return ERR;
  + e = ERR;
  + goto done;
! ...>
  +done:
  + unformat_free(LAIN);
  + return e;
ayourtch@ayourtch-lnx:~/cocci$

And the result was the same... but I am now seeing another bug in my
original patch - *none* of the return statements within the switch()
statement are replaced as well... and somewhat start to get the idea
of why the idea of using the --allow-inconsistent-paths may not have
been a good one :)

 spatch --sp-file patch-new.cocci file.c
init_defs_builtins: /usr/bin/../lib/coccinelle/standard.h
HANDLING: file.c
detect_func: node 84: }1[1] in syn_filter_enable_disable_command_fn
reachable by inconsistent control-flow paths

I am not sure I grok the }1[1] notation well, but my hunch is that it
gets lost somewhere around that switch() statement, and I just told it
"continue anyway", so it is hard to complain the result may be
unexpected :-)

Debugging it a bit more, though I think i found at least the issue of
the "inconsistent paths" - there is a "return" inside a case
statement, followed by break. I copypasted the example file from a
longer production code and didn't check. That break is correctly
detected as dead code, but seems to make the analysis engine give up ?
There are actually two of those "return; break" sequences. After I
remove the "break", I can perform the patching without the
"inconsistent control flow path" error, but the result is exactly the
same as in the beginning...

I missed the fact that the "return ERR" were actually not replaced on
this example at all, so naturally trying the "hacky" approach didn't
work either... I think I am still not fully getting the relationship
between the semantic patch syntax and the code paths, i will give
another read to the docs and see if they eventually sink in... :-)

The basic examples in the doc seem to be pretty straightforward, but
the move to a more complicated cases like this seems to introduce a
mental bump, I think I just need to persist on it a bit more :-)

--a





On 7/22/21, Julia Lawall  wrote:
>
>
> On Thu, 22 Jul 2021, Andrew   Yourtchenko wrote:
>
>> Hi all,
>>
>> I work on the VPP project (http://fd.io/ - open source software
>> dataplane), and  tried to use coccinelle to make a relatively
>> non-trivial change
>> as in the mail https://lists.fd.io/g/vpp-dev/message/17532 - it seemed
>> to be a very good candidate - boring enough to be painful to do by
>> hand, complex enough to make sed inadequate for it.
>
> Thanks for trying Coccinelle :)
>
>>
>> I came up with this semantic patch:
>>
>>
>> @ detect_func @
>> identifier CLI_FN, AVM, AIN, ACMD;
>> fresh identifier LAIN = "line_" ## AIN;
>>
>> statement S1;
>>
>> typedef clib_error_t, vlib_main_t, unformat_input_t, vlib_cli_command_t;
>> @@
>>
>> static clib_error_t *CLI_FN (vlib_main_t * AVM, unformat_input_t *
>> AIN, vlib_cli_command_t * ACMD)
>> {
>> + clib_error_t *e = 0;
>> +  unformat_input_t *LAIN;
>> ...
>> + if (!unformat_user (AIN, unformat_line_input, LAIN)) {
>> +return 0;
>> + }
>> +
>> -  while (unformat_check_input (AIN) != UNFORMAT_END_OF_INPUT)
>> + while (unformat_check_input (LAIN) != UNFORMAT_END_OF_INPUT)
>> S1
>> <...
>> - return ERR;
>> + e = ERR;
>> + goto done;
>> ...>
>> +done:
>> + unformat_free(LAIN);
>> + return e;
>> }
>
> The problem has to do with the fact that Coccinelle is actually oriented
> around control-flow graphs. So it doesn't know which end of a control-flow
> path is actually the end of the function.
>
> You can try adjusting the line <... above as follows:
>
> <... when != true exp
>  when exists
>
> exp should be declared as an expression metavariable.  The when != true
> thing means that the path cannot cross a true branch across a test of an
> expression that matches exp (ie any expression).  The when exists means
> that the paths through this region of code are considered individually.
>
> I'm not certain that this will work in every case.  It will be necessary
> to check the results carefully.
>
> Another possible hack is to first replace every return under and if,
> while, etc by something 

Re: [Cocci] how to make substitutions at the end of the function, vs. the end of each block ?

2021-07-22 Thread Julia Lawall


On Thu, 22 Jul 2021, Andrew   Yourtchenko wrote:

> Hi all,
>
> I work on the VPP project (http://fd.io/ - open source software
> dataplane), and  tried to use coccinelle to make a relatively
> non-trivial change
> as in the mail https://lists.fd.io/g/vpp-dev/message/17532 - it seemed
> to be a very good candidate - boring enough to be painful to do by
> hand, complex enough to make sed inadequate for it.

Thanks for trying Coccinelle :)

>
> I came up with this semantic patch:
>
>
> @ detect_func @
> identifier CLI_FN, AVM, AIN, ACMD;
> fresh identifier LAIN = "line_" ## AIN;
>
> statement S1;
>
> typedef clib_error_t, vlib_main_t, unformat_input_t, vlib_cli_command_t;
> @@
>
> static clib_error_t *CLI_FN (vlib_main_t * AVM, unformat_input_t *
> AIN, vlib_cli_command_t * ACMD)
> {
> + clib_error_t *e = 0;
> +  unformat_input_t *LAIN;
> ...
> + if (!unformat_user (AIN, unformat_line_input, LAIN)) {
> +return 0;
> + }
> +
> -  while (unformat_check_input (AIN) != UNFORMAT_END_OF_INPUT)
> + while (unformat_check_input (LAIN) != UNFORMAT_END_OF_INPUT)
> S1
> <...
> - return ERR;
> + e = ERR;
> + goto done;
> ...>
> +done:
> + unformat_free(LAIN);
> + return e;
> }

The problem has to do with the fact that Coccinelle is actually oriented
around control-flow graphs. So it doesn't know which end of a control-flow
path is actually the end of the function.

You can try adjusting the line <... above as follows:

<... when != true exp
 when exists

exp should be declared as an expression metavariable.  The when != true
thing means that the path cannot cross a true branch across a test of an
expression that matches exp (ie any expression).  The when exists means
that the paths through this region of code are considered individually.

I'm not certain that this will work in every case.  It will be necessary
to check the results carefully.

Another possible hack is to first replace every return under and if,
while, etc by something else, and then rewrite all of the returns in a
third rule afterwards.  This is pretty ugly, but may be more reliable.

julia

>
> I attempt to run it on this test file:
>
> ubuntu@vpp-dev:~$ cat ~/test.c
> static clib_error_t *
> syn_filter_enable_disable_command_fn (vlib_main_t * vm,
>   unformat_input_t * input,
>   vlib_cli_command_t * cmd)
> {
>   vnet_main_t *vnm = vnet_get_main ();
>   u32 sw_if_index = ~0;
>   int enable_disable = 1;
>   int rv;
>
>   while (unformat_check_input (input) != UNFORMAT_END_OF_INPUT)
> {
>   if (unformat (input, "disable"))
> enable_disable = 0;
>   else if (unformat (input, "%U", unformat_vnet_sw_interface,
>  vnm, _if_index))
> ;
>   else
> break;
> }
>
>   if (sw_if_index == ~0)
> return clib_error_return (0, "Please specify an interface...");
>
>   rv = syn_filter_enable_disable (sw_if_index, enable_disable);
>
>   switch (rv)
> {
> case 0:
>   break;
>
> case VNET_API_ERROR_INVALID_SW_IF_INDEX:
>   return clib_error_return
> (0, "Invalid interface, only works on physical ports");
>   break;
>
> case VNET_API_ERROR_UNIMPLEMENTED:
>   return clib_error_return (0,
> "Device driver doesn't support redirection");
>   break;
>
> case VNET_API_ERROR_INVALID_VALUE:
>   return clib_error_return (0, "feature arc not found");
>
> case VNET_API_ERROR_INVALID_VALUE_2:
>   return clib_error_return (0, "feature node not found");
>
> default:
>   return clib_error_return (0, "syn_filter_enable_disable returned %d",
> rv);
> }
>   return 0;
> }
> ubuntu@vpp-dev:~$
>
>
> However, when I run it, the "done: " label, etc. gets inserted twice:
>
> ubuntu@vpp-dev:~$ spatch --sp-file /tmp/rules.sp
> --allow-inconsistent-paths  ~/test.c
> init_defs_builtins: /usr/bin/../lib/coccinelle/standard.h
> HANDLING: /home/ubuntu/test.c
> diff =
> --- /home/ubuntu/test.c
> +++ /tmp/cocci-output-56896-8f35c5-test.c
> @@ -3,12 +3,18 @@ syn_filter_enable_disable_command_fn (vl
>   unformat_input_t * input,
>   vlib_cli_command_t * cmd)
>  {
> +  clib_error_t *e = 0;
> +  unformat_input_t *line_input;
>vnet_main_t *vnm = vnet_get_main ();
>u32 sw_if_index = ~0;
>int enable_disable = 1;
>int rv;
>
> -  while (unformat_check_input (input) != UNFORMAT_END_OF_INPUT)
> +  if (!unformat_user(input, unformat_line_input, line_input)) {
> +  return 0;
> +  }
> +
> +while (unformat_check_input(line_input) != UNFORMAT_END_OF_INPUT)
>  {
>if (unformat (input, "disable"))
> enable_disable = 0;
> @@ -48,6 +54,12 @@ syn_filter_enable_disable_command_fn (vl
>  default:
>return clib_error_return (0, "syn_filter_enable_disable returned %d",
> rv);
> -}
> +done: