[Cocci] Moving exception handling code to the end of a function implementation with SmPL?

[Cocci] Moving exception handling code to the end of a function implementation with SmPL?

[Markus Elfring]

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

Re: [Cocci] Moving exception handling code to the end of a function implementation with SmPL?

[Markus Elfring]

> * 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

Re: [Cocci] Moving exception handling code to the end of a function implementation with SmPL?

[Julia Lawall]

[-- 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

Re: [Cocci] Moving exception handling code to the end of a function implementation with SmPL?

[Markus Elfring]

> 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

Re: [Cocci] Moving exception handling code to the end of a function implementation with SmPL?

[Markus Elfring]

>>> 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

Re: [Cocci] Moving exception handling code to the end of a function implementation with SmPL?

[Markus Elfring]

>> 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

Re: [Cocci] Moving exception handling code to the end of a function implementation with SmPL?

[Julia Lawall]

[-- 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

Re: [Cocci] Moving exception handling code to the end of a function implementation with SmPL?

[Markus Elfring]

[-- 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

Re: [Cocci] Moving exception handling code to the end of a function implementation with SmPL?

[Markus Elfring]

>   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

Re: [Cocci] Moving exception handling code to the end of a function implementation with SmPL?

[Julia Lawall]

[-- 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

Re: [Cocci] Moving exception handling code to the end of a function implementation with SmPL?

[Markus Elfring]

>>>   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

Re: [Cocci] Checking the occurrence of trailing spaces in a semantic patch

[Markus Elfring]

> 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

Re: [Cocci] Checking the occurrence of trailing spaces in a semantic patch

[Julia Lawall]

[-- 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

Re: [Cocci] Checking the occurrence of trailing spaces in a semantic patch

[Markus Elfring]

> 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

Re: [Cocci] Checking the occurrence of trailing spaces in a semantic patch

[Markus Elfring]

> 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

Re: [Cocci] Checking code layout for adding jump targets with SmPL

[Markus Elfring]

> 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

Re: [Cocci] Checking code layout for adding jump targets with SmPL

[Markus Elfring]

> 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