Hello, I became interested in another source code transformation again. I would like to move a bit of common code to the end of a function implementation with the help of the following script for the semantic patch language. @replacement@ expression info, result; identifier target, work; type t != void; @@ t work(...) { <+... if (...) ( -{ -result = info; goto - target + e_nodev ; -} | { ... -result = info; goto - target + e_nodev ; } ) ...+> target: ... return result; +e_nodev: +result = info; +goto target; } The implementation of the function “megasas_mgmt_ioctl_fw” looks like an update candidate. https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/scsi/megaraid/megaraid_sas_base.c?id=4ae004a9bca8bef118c2b4e76ee31c7df4514f18#n7742 https://elixir.bootlin.com/linux/v5.2-rc5/source/drivers/scsi/megaraid/megaraid_sas_base.c#L7742 * I extracted it into a test source file. Unfortunately, I stumble on the error message “replacement: already tagged token: C code context” then. * If I delete a bit more source code for this example, the shown transformation approach can work as expected. * The complete source file seems to be very challenging for testing the run time characteristics. Regards, Markus _______________________________________________ Cocci mailing list Cocci@systeme.lip6.fr https://systeme.lip6.fr/mailman/listinfo/cocci
> * The complete source file seems to be very challenging for testing > the run time characteristics. How are the chances to clarify the different test results for this source code transformation approach with the software combination “Coccinelle 1.0.7-00211-geaa13d59-dirty (OCaml 4.07.1)”? elfring@Sonne:~/Projekte/Linux/next-patched> spatch --profile ~/Projekte/Coccinelle/janitor/move_error_code_assignment_to_function_end1.cocci drivers/scsi/megaraid/megaraid_sas_base.c … timeout (we abort) … profiling result … *full_engine : 200.279202 sec 1 count *bigloop : 199.492610 sec 1 count *Rule replacement : 199.492608 sec 1 count *process_a_ctl_a_env_a_toplevel : 198.390209 sec 1 count *mysat : 198.390177 sec 1 count *ctl : 198.389955 sec 1 count process_a_ctl_a_env_a_toplevel : 1.102299 sec 171 count … Regards, Markus _______________________________________________ Cocci mailing list Cocci@systeme.lip6.fr https://systeme.lip6.fr/mailman/listinfo/cocci
[-- Attachment #1: Type: text/plain, Size: 1301 bytes --] On Thu, 27 Jun 2019, Markus Elfring wrote: > > * The complete source file seems to be very challenging for testing > > the run time characteristics. > > How are the chances to clarify the different test results for this source code > transformation approach with the software combination “Coccinelle 1.0.7-00211-geaa13d59-dirty > (OCaml 4.07.1)”? > > elfring@Sonne:~/Projekte/Linux/next-patched> spatch --profile ~/Projekte/Coccinelle/janitor/move_error_code_assignment_to_function_end1.cocci drivers/scsi/megaraid/megaraid_sas_base.c > … > timeout (we abort) > … > profiling result > … > *full_engine : 200.279202 sec 1 count > *bigloop : 199.492610 sec 1 count > *Rule replacement : 199.492608 sec 1 count > *process_a_ctl_a_env_a_toplevel : 198.390209 sec 1 count > *mysat : 198.390177 sec 1 count > *ctl : 198.389955 sec 1 count > process_a_ctl_a_env_a_toplevel : 1.102299 sec 171 count > … Maybe there are too many metavariable bindings. You can try with the option --debug. Or if that doesn't help with the option --verbose-ctl-engine. julia [-- Attachment #2: Type: text/plain, Size: 136 bytes --] _______________________________________________ Cocci mailing list Cocci@systeme.lip6.fr https://systeme.lip6.fr/mailman/listinfo/cocci
> Maybe there are too many metavariable bindings. Under which circumstances would such a situation really occur? How do you think about my SmPL script example for the shown use case? > You can try with the option --debug. > Or if that doesn't help with the option --verbose-ctl-engine. How can these parameters help to clarify undesirable run time characteristics? Is it eventually easier to explain the following information? elfring@Sonne:~/Projekte/Coccinelle/Probe> spatch --debug megaraid_sas-excerpt1.c ~/Projekte/Coccinelle/janitor/move_error_code_assignment_to_function_end1.cocci … replacement: already tagged token: C code context … Regards, Markus _______________________________________________ Cocci mailing list Cocci@systeme.lip6.fr https://systeme.lip6.fr/mailman/listinfo/cocci
>>> You can try with the option --debug. >>> Or if that doesn't help with the option --verbose-ctl-engine. >> >> How can these parameters help to clarify undesirable run time characteristics? > > How about actually looking at the results yourself? I did that. - I do not find these extra data so helpful at the moment. The influence of presented background information on software execution speed is still unclear. >> replacement: already tagged token: > > You try to add two things one one token, which is not allowed. Are there any other software development challenges to consider for the insertion of a bit of source code at a function implementation end? … out_kfree_ioc: kfree(ioc); return error; } … Regards, Markus _______________________________________________ Cocci mailing list Cocci@systeme.lip6.fr https://systeme.lip6.fr/mailman/listinfo/cocci
>> replacement: already tagged token: > > You try to add two things one one token, which is not allowed. How do you think about to clarify why a source file adjustment like the following can let my SmPL script succeed in a test configuration? elfring@Sonne:~/Projekte/Coccinelle/Probe> diff -u megaraid_sas-excerpt1.c megaraid_sas-excerpt2.c … @@ -32,24 +32,7 @@ goto out_kfree_ioc; } - if (instance->unload == 1) { - error = -ENODEV; - goto out_kfree_ioc; - } - - if (down_interruptible(&instance->ioctl_sem)) { - error = -ERESTARTSYS; - goto out_kfree_ioc; - } - - if (megasas_wait_for_adapter_operational(instance)) { - error = -ENODEV; - goto out_up; - } - - error = megasas_mgmt_fw_ioctl(instance, user_ioc, ioc); -out_up: - up(&instance->ioctl_sem); +// Deleted part out_kfree_ioc: kfree(ioc); Where did the Coccinelle software get the impression that anything would be added too often at the end of such a function implementation? Regards, Markus _______________________________________________ Cocci mailing list Cocci@systeme.lip6.fr https://systeme.lip6.fr/mailman/listinfo/cocci
[-- Attachment #1: Type: text/plain, Size: 1165 bytes --] On Thu, 27 Jun 2019, Markus Elfring wrote: > >> replacement: already tagged token: > > > > You try to add two things one one token, which is not allowed. > > How do you think about to clarify why a source file adjustment > like the following can let my SmPL script succeed in a test configuration? > > elfring@Sonne:~/Projekte/Coccinelle/Probe> diff -u megaraid_sas-excerpt1.c megaraid_sas-excerpt2.c > … > @@ -32,24 +32,7 @@ > goto out_kfree_ioc; > } > > - if (instance->unload == 1) { > - error = -ENODEV; > - goto out_kfree_ioc; > - } > - > - if (down_interruptible(&instance->ioctl_sem)) { > - error = -ERESTARTSYS; > - goto out_kfree_ioc; > - } > - > - if (megasas_wait_for_adapter_operational(instance)) { > - error = -ENODEV; > - goto out_up; > - } > - > - error = megasas_mgmt_fw_ioctl(instance, user_ioc, ioc); > -out_up: > - up(&instance->ioctl_sem); > +// Deleted part > > out_kfree_ioc: > kfree(ioc); > > > Where did the Coccinelle software get the impression that anything > would be added too often at the end of such a function implementation? Without the semantic patch and the C source code, I can't answer the question. julia [-- Attachment #2: Type: text/plain, Size: 136 bytes --] _______________________________________________ Cocci mailing list Cocci@systeme.lip6.fr https://systeme.lip6.fr/mailman/listinfo/cocci
[-- Attachment #1: Type: text/plain, Size: 380 bytes --] >> Where did the Coccinelle software get the impression that anything >> would be added too often at the end of such a function implementation? > > Without the semantic patch and the C source code, You have access to data from both files already. > I can't answer the question. Is it more convenient to check test results for attachments again? Regards, Markus [-- Attachment #2: move_error_code_assignment_to_function_end1.cocci --] [-- Type: text/plain, Size: 323 bytes --] @replacement@ expression info, result; identifier target, work; type t != void; @@ t work(...) { <+... if (...) ( -{ -result = info; goto - target + e_nodev ; -} | { ... -result = info; goto - target + e_nodev ; } ) ...+> target: ... return result; +e_nodev: +result = info; +goto target; } [-- Warning: decoded text below may be mangled, UTF-8 assumed --] [-- Attachment #3: megaraid_sas-excerpt1.c --] [-- Type: text/x-csrc; name="megaraid_sas-excerpt1.c", Size: 1277 bytes --] // SPDX-License-Identifier: GPL-2.0-or-later // Deleted part static int megasas_mgmt_ioctl_fw(struct file *file, unsigned long arg) { struct megasas_iocpacket __user *user_ioc = (struct megasas_iocpacket __user *)arg; struct megasas_iocpacket *ioc; struct megasas_instance *instance; int error; ioc = memdup_user(user_ioc, sizeof(*ioc)); if (IS_ERR(ioc)) return PTR_ERR(ioc); instance = megasas_lookup_instance(ioc->host_no); if (!instance) { error = -ENODEV; goto out_kfree_ioc; } /* Block ioctls in VF mode */ if (instance->requestorId && !allow_vf_ioctls) { error = -ENODEV; goto out_kfree_ioc; } if (atomic_read(&instance->adprecovery) == MEGASAS_HW_CRITICAL_ERROR) { dev_err(&instance->pdev->dev, "Controller in crit error\n"); error = -ENODEV; goto out_kfree_ioc; } if (instance->unload == 1) { error = -ENODEV; goto out_kfree_ioc; } if (down_interruptible(&instance->ioctl_sem)) { error = -ERESTARTSYS; goto out_kfree_ioc; } if (megasas_wait_for_adapter_operational(instance)) { error = -ENODEV; goto out_up; } error = megasas_mgmt_fw_ioctl(instance, user_ioc, ioc); out_up: up(&instance->ioctl_sem); out_kfree_ioc: kfree(ioc); return error; } [-- Attachment #4: Type: text/plain, Size: 136 bytes --] _______________________________________________ Cocci mailing list Cocci@systeme.lip6.fr https://systeme.lip6.fr/mailman/listinfo/cocci
> Unfortunately, I stumble on the error message “replacement: already tagged token: > C code context” then. It might be that the transformation approach was too generic for the implementation of the function “megasas_mgmt_ioctl_fw”. https://elixir.bootlin.com/linux/v5.4-rc5/source/drivers/scsi/megaraid/megaraid_sas_base.c#L8258 https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/scsi/megaraid/megaraid_sas_base.c?id=e472c64aa4fa6150c6076fd36d101d667d71c30a#n8258 The following SmPL script variant can generate an usable test result. @replacement@ expression result; identifier work; type t != void; @@ t work(...) { <+... if (...) ( -{ -result = -ENODEV; goto - out_kfree_ioc + e_nodev ; -} | { ... -result = -ENODEV; goto - out_kfree_ioc + e_nodev ; } ) ...+> out_kfree_ioc: ... when exists return result; +e_nodev: +result = -ENODEV; +goto out_kfree_ioc; } Regards, Markus _______________________________________________ Cocci mailing list Cocci@systeme.lip6.fr https://systeme.lip6.fr/mailman/listinfo/cocci
[-- Attachment #1: Type: text/plain, Size: 1447 bytes --] On Thu, 31 Oct 2019, Markus Elfring wrote: > > Unfortunately, I stumble on the error message “replacement: already tagged token: > > C code context” then. This is what I would expect. You could use one rule with an exists to put a position variable in the place where you want to put a kfree, and then use another rule to put a kfree at that position. julia > > It might be that the transformation approach was too generic for > the implementation of the function “megasas_mgmt_ioctl_fw”. > https://elixir.bootlin.com/linux/v5.4-rc5/source/drivers/scsi/megaraid/megaraid_sas_base.c#L8258 > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/scsi/megaraid/megaraid_sas_base.c?id=e472c64aa4fa6150c6076fd36d101d667d71c30a#n8258 > > The following SmPL script variant can generate an usable test result. > > @replacement@ > expression result; > identifier work; > type t != void; > @@ > t work(...) > { > <+... > if (...) > ( > -{ > -result = -ENODEV; > goto > - out_kfree_ioc > + e_nodev > ; > -} > | > { > ... > -result = -ENODEV; > goto > - out_kfree_ioc > + e_nodev > ; > } > ) > ...+> > out_kfree_ioc: > ... when exists > return result; > +e_nodev: > +result = -ENODEV; > +goto out_kfree_ioc; > } > > > Regards, > Markus > _______________________________________________ > Cocci mailing list > Cocci@systeme.lip6.fr > https://systeme.lip6.fr/mailman/listinfo/cocci > [-- Attachment #2: Type: text/plain, Size: 136 bytes --] _______________________________________________ Cocci mailing list Cocci@systeme.lip6.fr https://systeme.lip6.fr/mailman/listinfo/cocci
>>> Unfortunately, I stumble on the error message “replacement: already tagged token: >>> C code context” then. > > This is what I would expect. I came along different application imaginations. > You could use one rule with an exists to put a position variable in the place > where you want to put a kfree, and then use another rule to put a kfree > at that position. I have got the impression from this information that you think in other directions than the use case I presented here once more (where the mentioned function call is not added). Can this kind of feedback eventually belong to an other recent topic? I would appreciate if transformation conflicts can be reduced also by the generic identification of error codes and jump targets for the combination of a bit of exception handling code. Regards, Markus _______________________________________________ Cocci mailing list Cocci@systeme.lip6.fr https://systeme.lip6.fr/mailman/listinfo/cocci
> The following SmPL script variant can generate an usable test result. Yesterday I noticed during the preparation of a corresponding commit that unwanted space characters were added at three places in the generated patch. elfring@Sonne:~/Projekte/Linux/next-patched> spatch --in-place drivers/scsi/megaraid/megaraid_sas_base.c ~/Projekte/Coccinelle/janitor/move_error_code_assignment_to_function_end2.cocci git GUI: … @@ -8270,31 +8270,24 @@ static int megasas_mgmt_ioctl_fw(struct file *file, unsigned long arg) … + if (!instance) … + if (instance->requestorId && !allow_vf_ioctls) … + if (instance->unload == 1) … How would you like to improve the pretty-printing for the Coccinelle software? Regards, Markus _______________________________________________ Cocci mailing list Cocci@systeme.lip6.fr https://systeme.lip6.fr/mailman/listinfo/cocci
[-- Attachment #1: Type: text/plain, Size: 937 bytes --] On Fri, 1 Nov 2019, Markus Elfring wrote: > > The following SmPL script variant can generate an usable test result. > > Yesterday I noticed during the preparation of a corresponding commit > that unwanted space characters were added at three places in the generated patch. > > elfring@Sonne:~/Projekte/Linux/next-patched> spatch --in-place drivers/scsi/megaraid/megaraid_sas_base.c ~/Projekte/Coccinelle/janitor/move_error_code_assignment_to_function_end2.cocci > > git GUI: > … > @@ -8270,31 +8270,24 @@ static int megasas_mgmt_ioctl_fw(struct file *file, unsigned long arg) > … > + if (!instance) > … > + if (instance->requestorId && !allow_vf_ioctls) > … > + if (instance->unload == 1) > … And the unwanted space characters are where? > > How would you like to improve the pretty-printing for the Coccinelle software? I don't know. How would you like to improve the pretty-printing for the Coccinelle software? julia [-- Attachment #2: Type: text/plain, Size: 136 bytes --] _______________________________________________ Cocci mailing list Cocci@systeme.lip6.fr https://systeme.lip6.fr/mailman/listinfo/cocci
> And the unwanted space characters are where? At the end of the shown three lines. Would you like to try the following commands out on the updated source file? * git diff --check … drivers/scsi/megaraid/megaraid_sas_base.c:8275: trailing whitespace. … * scripts/checkpatch.pl --types TRAILING_WHITESPACE -f … … >> How would you like to improve the pretty-printing for the Coccinelle software? > > I don't know. I hope that this view will change. > How would you like to improve the pretty-printing for the Coccinelle software? I would like to clarify which software components influence the addition of unwanted space characters (at line ends). Regards, Markus _______________________________________________ Cocci mailing list Cocci@systeme.lip6.fr https://systeme.lip6.fr/mailman/listinfo/cocci
> How would you like to improve the pretty-printing for the Coccinelle software? I can reproduce this glitch by the combination of test files like the following, can't you? @adjustment@ expression result; @@ if (...) -{ -result = -ENODEV; goto - out_kfree_ioc + e_nodev ; -} static int megasas_mgmt_ioctl_fw(struct file *file, unsigned long arg) { struct megasas_iocpacket __user *user_ioc = (struct megasas_iocpacket __user *)arg; struct megasas_iocpacket *ioc; struct megasas_instance *instance; int error; ioc = memdup_user(user_ioc, sizeof(*ioc)); if (IS_ERR(ioc)) return PTR_ERR(ioc); instance = megasas_lookup_instance(ioc->host_no); if (!instance) { error = -ENODEV; goto out_kfree_ioc; } // Deleted part out_kfree_ioc: kfree(ioc); return error; } I guess that the trailing space character is just a questionable leftover from the desired deletion of curly brackets according to the affected compound statement in such an use case. Regards, Markus _______________________________________________ Cocci mailing list Cocci@systeme.lip6.fr https://systeme.lip6.fr/mailman/listinfo/cocci
> How would you like to improve the pretty-printing for the Coccinelle software? I guess that another small test approach will trigger further development considerations. @replacement@ @@ if (...) -{ -} +info: +puts("surprise"); elfring@Sonne:~/Projekte/Coccinelle/Probe> spatch empty_compound_statement_for_if1.c replace_empty_compound_statement_for_if4.cocci … @@ -1,5 +1,6 @@ void x(void) { -if (1) { -} +if (1) info: + +puts("surprise"); } Do you find such a test result also questionable? Regards, Markus _______________________________________________ Cocci mailing list Cocci@systeme.lip6.fr https://systeme.lip6.fr/mailman/listinfo/cocci
> I guess that another small test approach will trigger further development considerations. @replacement@ @@ if (...) -{ +info: +puts("surprise"); -} … elfring@Sonne:~/Projekte/Coccinelle/Probe> spatch empty_compound_statement_for_if1.c replace_empty_compound_statement_for_if5.cocci … @@ -1,5 +1,5 @@ void x(void) { -if (1) { -} +puts("surprise"); +if (1) info: } Can such a test result be confusing? Regards, Markus _______________________________________________ Cocci mailing list Cocci@systeme.lip6.fr https://systeme.lip6.fr/mailman/listinfo/cocci