Coccinelle archive on lore.kernel.org
 help / color / Atom feed
* Re: [Cocci] phy: tegra: xusb: Complete exception handling in probe functions
       [not found] <fe6868a4-01f5-2832-9081-7643be0ab4a1@web.de>
@ 2019-10-29  9:40 ` Markus Elfring
  2019-10-30 10:44   ` [Cocci] drivers/phy/tegra: Completion for exception handling in probe functions with SmPL? Markus Elfring
  0 siblings, 1 reply; 8+ messages in thread
From: Markus Elfring @ 2019-10-29  9:40 UTC (permalink / raw)
  To: linux-tegra, JC Kuo, Jonathan Hunter, Kishon Vijay Abraham I,
	Thierry Reding
  Cc: kernel-janitors, Kangjie Lu, LKML, Navid Emamdoost,
	Stephen McCamant, Coccinelle

> I have got the impression then that the exception handling is incomplete so far
> at these source code places.

How do you think about to try the following small script for the semantic patch
language out on the directory “drivers/phy/tegra”?

@display@
expression object;
identifier exit;
@@
 ... when any
 object = kzalloc(...)
 ...
 if (...)
 {
*kfree(object);
 goto exit;
 }
 ...
 if (...)
*   goto unregister;


Will any search pattern variations become more interesting for corresponding
automatic software transformations?

Regards,
Markus
_______________________________________________
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [Cocci] drivers/phy/tegra: Completion for exception handling in probe functions with SmPL?
  2019-10-29  9:40 ` [Cocci] phy: tegra: xusb: Complete exception handling in probe functions Markus Elfring
@ 2019-10-30 10:44   ` Markus Elfring
  2019-10-30 17:30     ` [Cocci] xusb-tegra186: Renaming known jump labels " Markus Elfring
                       ` (2 more replies)
  0 siblings, 3 replies; 8+ messages in thread
From: Markus Elfring @ 2019-10-30 10:44 UTC (permalink / raw)
  To: linux-tegra, Coccinelle
  Cc: Jonathan Hunter, YueHaibing, JC Kuo, Kangjie Lu, kernel-janitors,
	LKML, Kishon Vijay Abraham I, Navid Emamdoost, Stephen McCamant,
	Thierry Reding, Dan Carpenter

> Will any search pattern variations become more interesting for corresponding
> automatic software transformations?

I hoped to achieve something together with the semantic patch language
by the following transformation approach.

@adjustment@
expression object;
identifier exit;
@@
 object = kzalloc(...)
 ...
 if (...)
-{  kfree(object);
    goto
-        exit
+        release_memory
    ;
-}
 ... when any
 device_unregister(...);
-exit
+release_memory
 :
+kfree(object);
 return ERR_PTR(...);


Do you find such a change suggestion reasonable (in principle)?

But I stumble on another unexpected test result.

elfring@Sonne:~/Projekte/Linux/next-patched> spatch drivers/phy/tegra/xusb-tegra186.c ~/Projekte/Coccinelle/janitor/complete_exception_handling_in_probe_functions1.cocci
init_defs_builtins: /usr/local/bin/../lib/coccinelle/standard.h
HANDLING: drivers/phy/tegra/xusb-tegra186.c


How would you like to clarify why diff hunks were not generated
for functions like “tegra186_usb2_pad_probe” and “tegra186_usb3_pad_probe”
in such an use case?
https://elixir.bootlin.com/linux/v5.4-rc2/source/drivers/phy/tegra/xusb-tegra186.c#L445
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/phy/tegra/xusb-tegra186.c?id=bbf711682cd570697086e88388a2c718da918894#n445

Regards,
Markus
_______________________________________________
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [Cocci] xusb-tegra186: Renaming known jump labels with SmPL?
  2019-10-30 10:44   ` [Cocci] drivers/phy/tegra: Completion for exception handling in probe functions with SmPL? Markus Elfring
@ 2019-10-30 17:30     ` " Markus Elfring
  2019-10-30 18:15     ` [Cocci] xusb-tegra186: Adding a function call behind a label " Markus Elfring
  2019-10-30 20:50     ` [Cocci] drivers/phy/tegra: Completion for exception handling in probe functions " Markus Elfring
  2 siblings, 0 replies; 8+ messages in thread
From: Markus Elfring @ 2019-10-30 17:30 UTC (permalink / raw)
  To: Coccinelle

> But I stumble on another unexpected test result.

I have tried a related tiny transformation approach out.

@renaming@
@@
-out
+exit
 :
 return ERR_PTR(...);


I got a diff hunk like the following then.

elfring@Sonne:~/Projekte/Linux/next-patched> spatch drivers/phy/tegra/xusb-tegra186.c ~/Projekte/Coccinelle/janitor/rename_out_label1.cocci
…
warning: line 3: should out be a metavariable?
…
@@ -483,7 +483,7 @@ tegra186_usb2_pad_probe(struct tegra_xus

 unregister:
 	device_unregister(&pad->dev);
-out:
+exit :
 	return ERR_PTR(err);
 }
…


Should the addition of an extra space character usually be avoided
for such a jump label?

Regards,
Markus
_______________________________________________
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [Cocci] xusb-tegra186: Adding a function call behind a label with SmPL?
  2019-10-30 10:44   ` [Cocci] drivers/phy/tegra: Completion for exception handling in probe functions with SmPL? Markus Elfring
  2019-10-30 17:30     ` [Cocci] xusb-tegra186: Renaming known jump labels " Markus Elfring
@ 2019-10-30 18:15     ` " Markus Elfring
       [not found]       ` <2db8691d768571f6c275f3d89401df23@lip6.fr>
  2019-10-30 20:50     ` [Cocci] drivers/phy/tegra: Completion for exception handling in probe functions " Markus Elfring
  2 siblings, 1 reply; 8+ messages in thread
From: Markus Elfring @ 2019-10-30 18:15 UTC (permalink / raw)
  To: Coccinelle

> But I stumble on another unexpected test result.

I would like point out that the following simple (?) transformation approach
does not generate the expected diff hunk at the moment.

@addition@
expression object;
@@
 object = kzalloc(...)
 ... when any
 device_unregister(...);
 out:
+kfree(object);
 return ERR_PTR(...);


elfring@Sonne:~/Projekte/Linux/next-patched> spatch drivers/phy/tegra/xusb-tegra186.c ~/Projekte/Coccinelle/janitor/add_kfree_call1.cocci


Will further collateral evolution happen then also for the Coccinelle software?

Regards,
Markus
_______________________________________________
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [Cocci] xusb-tegra186: Adding a function call behind a label with SmPL?
       [not found]       ` <2db8691d768571f6c275f3d89401df23@lip6.fr>
@ 2019-10-30 20:00         ` Markus Elfring
  2019-10-30 20:44           ` Julia Lawall
  2019-10-31  8:12         ` Markus Elfring
  1 sibling, 1 reply; 8+ messages in thread
From: Markus Elfring @ 2019-10-30 20:00 UTC (permalink / raw)
  To: Julia Lawall; +Cc: Coccinelle

> There is no reason why a patch should be generated in this case.
> As you should know well, A ... B only matches in a transformation case
> if every path from A leads to code matching B.  That is not the case in your example.

The exception handling code should usually be executed at the end of
function implementations after an error situation was detected.

Do you try to refer to specific information from the software documentation
like the following?

https://github.com/coccinelle/coccinelle/blob/ed1eb8e06f800739d3992158d36945c0c4c6f0c7/docs/manual/cocci_syntax.tex#L179
“…
A depends on clause can further indicate whether the clause should be satisfied
for all the branches (forall) or only for one (exists). exists is the default.
…”

The following simple transformation approach seems to work in the way
which I expected somehow initially.

@addition exists@
expression object;
@@
 object = kzalloc(...)
 ... when any
 device_unregister(...);
 out:
+kfree(object);
 return ERR_PTR(...);


Does this change specification indicate then a disagreement about
a default SmPL rule property?

Regards,
Markus
_______________________________________________
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [Cocci] xusb-tegra186: Adding a function call behind a label with SmPL?
  2019-10-30 20:00         ` Markus Elfring
@ 2019-10-30 20:44           ` Julia Lawall
  0 siblings, 0 replies; 8+ messages in thread
From: Julia Lawall @ 2019-10-30 20:44 UTC (permalink / raw)
  To: Markus Elfring; +Cc: Coccinelle

[-- Attachment #1: Type: text/plain, Size: 1311 bytes --]



On Wed, 30 Oct 2019, Markus Elfring wrote:

> > There is no reason why a patch should be generated in this case.
> > As you should know well, A ... B only matches in a transformation case
> > if every path from A leads to code matching B.  That is not the case in your example.
>
> The exception handling code should usually be executed at the end of
> function implementations after an error situation was detected.
>
> Do you try to refer to specific information from the software documentation
> like the following?
>
> https://github.com/coccinelle/coccinelle/blob/ed1eb8e06f800739d3992158d36945c0c4c6f0c7/docs/manual/cocci_syntax.tex#L179
> “…
> A depends on clause can further indicate whether the clause should be satisfied
> for all the branches (forall) or only for one (exists). exists is the default.
> …”

There is no depends on in your code.  The cited text is not relevant.

julia

>
> The following simple transformation approach seems to work in the way
> which I expected somehow initially.
>
> @addition exists@
> expression object;
> @@
>  object = kzalloc(...)
>  ... when any
>  device_unregister(...);
>  out:
> +kfree(object);
>  return ERR_PTR(...);
>
>
> Does this change specification indicate then a disagreement about
> a default SmPL rule property?
>
> Regards,
> Markus
>

[-- Attachment #2: Type: text/plain, Size: 136 bytes --]

_______________________________________________
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [Cocci] drivers/phy/tegra: Completion for exception handling in probe functions with SmPL?
  2019-10-30 10:44   ` [Cocci] drivers/phy/tegra: Completion for exception handling in probe functions with SmPL? Markus Elfring
  2019-10-30 17:30     ` [Cocci] xusb-tegra186: Renaming known jump labels " Markus Elfring
  2019-10-30 18:15     ` [Cocci] xusb-tegra186: Adding a function call behind a label " Markus Elfring
@ 2019-10-30 20:50     ` " Markus Elfring
  2 siblings, 0 replies; 8+ messages in thread
From: Markus Elfring @ 2019-10-30 20:50 UTC (permalink / raw)
  To: linux-tegra, Coccinelle
  Cc: Jonathan Hunter, YueHaibing, JC Kuo, Kangjie Lu, kernel-janitors,
	LKML, Kishon Vijay Abraham I, Navid Emamdoost, Stephen McCamant,
	Thierry Reding, Dan Carpenter

> But I stumble on another unexpected test result.

I got more promising results by the following transformation approach.

@adjustment exists@
expression object;
identifier exit;
@@
 object = kzalloc(...)
 ...
 if (...)
-{  kfree(object);
    goto
-        exit
+        release_memory
    ;
-}
 ... when any
 device_unregister(...);
-exit
+release_memory
 :
+kfree(object);
 return ERR_PTR(...);


The scope property of such a SmPL rule occasionally needs also more
software development attention.
https://github.com/coccinelle/coccinelle/blob/ed1eb8e06f800739d3992158d36945c0c4c6f0c7/docs/manual/cocci_syntax.tex#L860

I observe that the pretty-printing for the generated source code will need
further improvements (according to the Linux coding style).

Example:

elfring@Sonne:~/Projekte/Linux/next-patched> spatch drivers/phy/tegra/xusb-tegra186.c ~/Projekte/Coccinelle/janitor/complete_exception_handling_in_probe_functions6.cocci
…
@@ -461,10 +461,8 @@ tegra186_usb2_pad_probe(struct tegra_xus
 	pad->soc = soc;

 	err = tegra_xusb_pad_init(pad, padctl, np);
-	if (err < 0) {
-		kfree(usb2);
-		goto out;
-	}
+	if (err < 0)
+		goto release_memory;

 	priv->usb2_trk_clk = devm_clk_get(&pad->dev, "trk");
 	if (IS_ERR(priv->usb2_trk_clk)) {
@@ -483,7 +481,8 @@ tegra186_usb2_pad_probe(struct tegra_xus

 unregister:
 	device_unregister(&pad->dev);
-out:
+release_memory :
+kfree(usb2);
 	return ERR_PTR(err);
 }
…


How reasonable do you find the presented update suggestion?

Regards,
Markus
_______________________________________________
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [Cocci] xusb-tegra186: Adding a function call behind a label with SmPL?
       [not found]       ` <2db8691d768571f6c275f3d89401df23@lip6.fr>
  2019-10-30 20:00         ` Markus Elfring
@ 2019-10-31  8:12         ` Markus Elfring
  1 sibling, 0 replies; 8+ messages in thread
From: Markus Elfring @ 2019-10-31  8:12 UTC (permalink / raw)
  To: Julia Lawall; +Cc: Coccinelle

>> @addition@
>> expression object;
>> @@
>>  object = kzalloc(...)
>>  ... when any
>>  device_unregister(...);
>>  out:
>> +kfree(object);
>>  return ERR_PTR(...);
>
> There is no reason why a patch should be generated in this case.

We come along different software development views also for this use case.
I tend still to interpret some of my source code search patterns in other directions.


> As you should know well, A ... B only matches in a transformation case
> if every path from A leads to code matching B.

Now I imagine that an explanation of such information can become more interesting
also based on the applied technologies as a knowledge background.
https://github.com/coccinelle/coccinelle/blob/ed1eb8e06f800739d3992158d36945c0c4c6f0c7/docs/manual/cocci_syntax.tex#L860

https://en.wikipedia.org/wiki/Computation_tree_logic#Relations_with_other_logics


> That is not the case in your example.

The shown SmPL script can probably indicate that a return statement should occasionally
be executed if a jump to it (according to the label “out” for example) was performed before
(in one if branch).

The following simple transformation approach seems to work also in the way
which I expected somehow initially.

@addition@
expression object;
@@
 object = kzalloc(...)
 ... when exists
 device_unregister(...);
 out:
+kfree(object);
 return ERR_PTR(...);


Can any extensions for the software documentation help to make the application
of the semantic patch language easier and safer finally?

Regards,
Markus
_______________________________________________
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, back to index

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
     [not found] <fe6868a4-01f5-2832-9081-7643be0ab4a1@web.de>
2019-10-29  9:40 ` [Cocci] phy: tegra: xusb: Complete exception handling in probe functions Markus Elfring
2019-10-30 10:44   ` [Cocci] drivers/phy/tegra: Completion for exception handling in probe functions with SmPL? Markus Elfring
2019-10-30 17:30     ` [Cocci] xusb-tegra186: Renaming known jump labels " Markus Elfring
2019-10-30 18:15     ` [Cocci] xusb-tegra186: Adding a function call behind a label " Markus Elfring
     [not found]       ` <2db8691d768571f6c275f3d89401df23@lip6.fr>
2019-10-30 20:00         ` Markus Elfring
2019-10-30 20:44           ` Julia Lawall
2019-10-31  8:12         ` Markus Elfring
2019-10-30 20:50     ` [Cocci] drivers/phy/tegra: Completion for exception handling in probe functions " Markus Elfring

Coccinelle archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/cocci/0 cocci/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 cocci cocci/ https://lore.kernel.org/cocci \
		cocci@systeme.lip6.fr
	public-inbox-index cocci

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/fr.lip6.systeme.cocci


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git