Re: [Cocci] how to make substitutions at the end of the function, vs. the end of each block ?
> 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 ?
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 ?
> 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 ?
> 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 ?
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 else,
Re: [Cocci] how to make substitutions at the end of the function, vs. the end of each block ?
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); > -} > +don