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 <[email protected]> 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 >> [email protected] >> https://systeme.lip6.fr/mailman/listinfo/cocci >> _______________________________________________ Cocci mailing list [email protected] https://systeme.lip6.fr/mailman/listinfo/cocci
