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

["Andrew 👽 Yourtchenko" <ayourtch@gmail.com>]

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 <julia.lawall@inria.fr> 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 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, &sw_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:
>> +      unformat_free(line_input);
>> +      return e;
>> +  }
>>    return 0;
>> +done:
>> +  unformat_free(line_input);
>> +  return e;
>>  }
>> ubuntu@vpp-dev:~$
>>
>>
>> I get a feeling I am missing something fundamental - but RTFM did not
>> help much... What am I doing wrong / missing ?
>> Could anyone please nudge me in the correct direction ?
>>
>> Thanks a lot!
>>
>> --a
>> _______________________________________________
>> Cocci mailing list
>> Cocci@systeme.lip6.fr
>> https://systeme.lip6.fr/mailman/listinfo/cocci
>>
_______________________________________________
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci