[Cocci] Reducing source code around return statements with SmPL?

[Cocci] Reducing source code around return statements with SmPL?

[Markus Elfring]

Hello,

The following small script for the semantic patch language
can point various source code places out for further considerations.

@replacement@
expression x;
identifier rc;
@@
-rc = x;
 return
-       rc
+       x
 ;

@deletion@
identifier replacement.rc;
type t;
@@
-t rc;
 ... when != rc


Unfortunately, this source code transformation approach seems to be
too generic. Places will be suggested for a change which should
be kept untouched.

The replacement of assignments should be restricted to local variables.
I guess that the metavariable type “local idexpression” should work
for this purpose.
But I got the impression that its use triggers design challenges
for the removal of a variable by an inherited metavariable in the
second SmPL rule.
Which adjustments should be taken better into account for this use case?

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

Re: [Cocci] Reducing source code around return statements with SmPL?

[Julia Lawall]

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



On Mon, 23 Sep 2019, Markus Elfring wrote:

> Hello,
>
> The following small script for the semantic patch language
> can point various source code places out for further considerations.
>
> @replacement@
> expression x;
> identifier rc;

Add: local idexpression lrc;

> @@
> -rc = x;

Replace by - lrc@rc = x;

julia

>  return
> -       rc
> +       x
>  ;
>
> @deletion@
> identifier replacement.rc;
> type t;
> @@
> -t rc;
>  ... when != rc
>
>
> Unfortunately, this source code transformation approach seems to be
> too generic. Places will be suggested for a change which should
> be kept untouched.
>
> The replacement of assignments should be restricted to local variables.
> I guess that the metavariable type “local idexpression” should work
> for this purpose.
> But I got the impression that its use triggers design challenges
> for the removal of a variable by an inherited metavariable in the
> second SmPL rule.
> Which adjustments should be taken better into account for this use case?
>
> 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] Reducing source code around return statements with SmPL?

[Markus Elfring]

>> The following small script for the semantic patch language
>> can point various source code places out for further considerations.
>>
>> @replacement@
>> expression x;
>> identifier rc;
>
> Add: local idexpression lrc;
>
>> @@
>> -rc = x;
>
> Replace by - lrc@rc = x;
>
> julia
>
>>  return
>> -       rc
>> +       x
>>  ;

Thanks for your advice.


>> Which adjustments should be taken better into account for this use case?

The shown metavariable combination is interesting, isn't it?
2064 Linux source files are pointed out by this transformation approach.

* Does this number need any additional thoughts?

* Will it become helpful to integrate such a SmPL script
  into a corresponding Linux directory?

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

Re: [Cocci] Reducing source code around return statements with SmPL?

[Markus Elfring]

>> @@
>> -rc = x;
>
> Replace by - lrc@rc = x;

I have constructed the following test script for the semantic patch language.

@check1@
expression x;
identifier rc;
local idexpression lrc;
position p;
@@
 lrc@rc = x@p;
 return lrc;

@check2@
identifier check1.rc;
position p;
type t;
@@
 t rc@p;
 ... when != rc

@script:python to_do1@
p << check1.p;
@@
coccilib.org.print_todo(p[0],
                        "WARNING: An expression was assigned to a local variable before it will be returned by the subsequent statement.")

@script:python to_do2@
p << check2.p;
v << check2.rc;
@@
coccilib.org.print_todo(p[0],
                        "INFO: May the local variable \""
                        + v
                        + "\" be deleted?")


I wonder about a corresponding test result.

elfring@Sonne:~/Projekte/Coccinelle/janitor> spatch --parse-cocci move_code_to_return1.cocci
…
scriptmeta: semantic error: bad rule check2 or bad variable rc
  File "move_code_to_return1.cocci", line 26, column 14, charpos = 474
  around = ';',
  whole content = v << check2.rc;


I imagine that it would be easier to understand that the content from
the inherited metavariable can be directly used.
The SmPL specification “v << check1.rc;” gets accepted by Coccinelle software
and seems to work then in the way I would expect it also for the other script variant.
Will any more adjustments become relevant for such a source code search approach?

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

Re: [Cocci] Reducing source code around return statements with SmPL?

[Julia Lawall]

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



On Tue, 24 Sep 2019, Markus Elfring wrote:

> >> @@
> >> -rc = x;
> >
> > Replace by - lrc@rc = x;
>
> I have constructed the following test script for the semantic patch language.
>
> @check1@
> expression x;
> identifier rc;
> local idexpression lrc;
> position p;
> @@
>  lrc@rc = x@p;
>  return lrc;
>
> @check2@
> identifier check1.rc;
> position p;
> type t;
> @@
>  t rc@p;
>  ... when != rc
>
> @script:python to_do1@
> p << check1.p;
> @@
> coccilib.org.print_todo(p[0],
>                         "WARNING: An expression was assigned to a local variable before it will be returned by the subsequent statement.")
>
> @script:python to_do2@
> p << check2.p;
> v << check2.rc;
> @@
> coccilib.org.print_todo(p[0],
>                         "INFO: May the local variable \""
>                         + v
>                         + "\" be deleted?")
>
>
> I wonder about a corresponding test result.
>
> elfring@Sonne:~/Projekte/Coccinelle/janitor> spatch --parse-cocci move_code_to_return1.cocci
> …
> scriptmeta: semantic error: bad rule check2 or bad variable rc
>   File "move_code_to_return1.cocci", line 26, column 14, charpos = 474
>   around = ';',
>   whole content = v << check2.rc;
>
>
> I imagine that it would be easier to understand that the content from
> the inherited metavariable can be directly used.
> The SmPL specification “v << check1.rc;” gets accepted by Coccinelle software
> and seems to work then in the way I would expect it also for the other script variant.
> Will any more adjustments become relevant for such a source code search approach?

rc comes from check1 so it should be inherited from check1.

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] Reducing source code around return statements with SmPL?

[Markus Elfring]

> rc comes from check1 so it should be inherited from check1.

* Did you get any other impression from the specification “identifier check1.rc;”
  in the SmPL rule “check2” for this test case?

* Can a SmPL specification like “v << check2.rc;” look also reasonable here
  (despite of the error message which is reported so far)?

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

Re: [Cocci] Reducing source code around return statements with SmPL?

[Markus Elfring]

>> * Can a SmPL specification like “v << check2.rc;” look also reasonable here
>>   (despite of the error message which is reported so far)?
>
> Many things may look reasonable.

This is generally fine.


> I tell you how Coccinelle is designed,

Thanks for such feedback.


> you can ignore the information if you like.

I am trying once more to achieve another bit of collateral evolution
in this software area.
Will it become helpful to use content also from inherited metavariables directly
(instead of referring only to the base item)?

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

Re: [Cocci] Reducing source code around return statements with SmPL?

[Markus Elfring]

>> Will it become helpful to use content also from inherited metavariables directly
>> (instead of referring only to the base item)?
>
> I think it would be confusing.

I hope that corresponding software development concerns can be clarified.


> Things should have only one name.
I hope that you are also used to object inheritance (and the application of
virtual function calls).
Can the direct access be more convenient (and even safer) for the
involved object hierarchy?

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

Re: [Cocci] Reducing source code around return statements with SmPL?

[Markus Elfring]

> rc comes from check1 so it should be inherited from check1.

I stumble on understanding difficulties for the currently supported
software functionality.

@check1@
expression x;
identifier rc;
local idexpression lrc;
position p;
@@
 lrc@rc = x@p;
 return lrc;

@check2@
identifier check1.rc;
position p;
type t;
@@
 t rc@p;
 ... when != rc

@script:python to_do1@
p << check1.p;
@@
coccilib.org.print_todo(p[0],
                        "WARNING: An expression was assigned to a local variable before it will be returned by the subsequent statement.")

@script:python to_do2@
p << check2.p;
v << check1.rc;
@@
coccilib.org.print_todo(p[0],
                        "INFO: May the local variable \""
                        + v
                        + "\" be deleted?")


elfring@Sonne:~/Projekte/Linux/next-patched> spatch ~/Projekte/Coccinelle/janitor/move_code_to_return2.cocci arch/arm64/kernel/ptrace.c

Three source code places are pointed out by the SmPL rule “to_do1”
as it would be expected.
Unfortunately, I miss the message “INFO: May the local variable "err" be deleted?”
for the function “ptrace_hbp_set_addr” according to the SmPL rule “to_do2”.
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/kernel/ptrace.c?id=08f103b9a9502974109fab47ea35ca8542c4e57a#n451

Which adjustments will become relevant here?

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

Re: [Cocci] Reducing source code around return statements with SmPL?

[Julia Lawall]

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



On Tue, 24 Sep 2019, Markus Elfring wrote:

> > rc comes from check1 so it should be inherited from check1.
>
> I stumble on understanding difficulties for the currently supported
> software functionality.
>
> @check1@
> expression x;
> identifier rc;
> local idexpression lrc;
> position p;
> @@
>  lrc@rc = x@p;
>  return lrc;
>
> @check2@
> identifier check1.rc;
> position p;
> type t;
> @@
>  t rc@p;
>  ... when != rc
>
> @script:python to_do1@
> p << check1.p;
> @@
> coccilib.org.print_todo(p[0],
>                         "WARNING: An expression was assigned to a local variable before it will be returned by the subsequent statement.")
>
> @script:python to_do2@
> p << check2.p;
> v << check1.rc;
> @@
> coccilib.org.print_todo(p[0],
>                         "INFO: May the local variable \""
>                         + v
>                         + "\" be deleted?")
>
>
> elfring@Sonne:~/Projekte/Linux/next-patched> spatch ~/Projekte/Coccinelle/janitor/move_code_to_return2.cocci arch/arm64/kernel/ptrace.c
>
> Three source code places are pointed out by the SmPL rule “to_do1”
> as it would be expected.
> Unfortunately, I miss the message “INFO: May the local variable "err" be deleted?”
> for the function “ptrace_hbp_set_addr” according to the SmPL rule “to_do2”.
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/kernel/ptrace.c?id=08f103b9a9502974109fab47ea35ca8542c4e57a#n451
>
> Which adjustments will become relevant here?

In that function err is used elsewhere, so the second message should not
be printed.

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] Reducing source code around return statements with SmPL?

[Markus Elfring]

>> elfring@Sonne:~/Projekte/Linux/next-patched> spatch ~/Projekte/Coccinelle/janitor/move_code_to_return2.cocci arch/arm64/kernel/ptrace.c
>>
>> Three source code places are pointed out by the SmPL rule “to_do1”
>> as it would be expected.
>> Unfortunately, I miss the message “INFO: May the local variable "err" be deleted?”
>> for the function “ptrace_hbp_set_addr” according to the SmPL rule “to_do2”.
>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/kernel/ptrace.c?id=08f103b9a9502974109fab47ea35ca8542c4e57a#n451
>>
>> Which adjustments will become relevant here?
>
> In that function err is used elsewhere, so the second message should not
> be printed.

I disagree to this view (after the removal of previous uses).

The following SmPL script variant can point also questionable variable declarations out.

@check1@
expression x;
identifier rc;
local idexpression lrc;
position p;
@@
 lrc@rc = x@p;
 return lrc;

@check2@
identifier check1.rc;
position check1.p;
type t;
@@
 t rc@p;
 ... when != rc

@script:python to_do1@
p << check1.p;
@@
coccilib.org.print_todo(p[0],
                        "WARNING: An expression was assigned to a local variable before it will be returned by the subsequent statement.")

@script:python to_do2@
p << check1.p;
v << check1.rc;
@@
coccilib.org.print_todo(p[0],
                        "INFO: May the local variable \""
                        + v
                        + "\" be deleted?")


But how should be achieved that the notification for declarations
which can be omitted will be presented only once for each affected local variable?

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

Re: [Cocci] Reducing source code around return statements with SmPL?

[Julia Lawall]

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



On Tue, 24 Sep 2019, Markus Elfring wrote:

> >> elfring@Sonne:~/Projekte/Linux/next-patched> spatch ~/Projekte/Coccinelle/janitor/move_code_to_return2.cocci arch/arm64/kernel/ptrace.c
> >>
> >> Three source code places are pointed out by the SmPL rule “to_do1”
> >> as it would be expected.
> >> Unfortunately, I miss the message “INFO: May the local variable "err" be deleted?”
> >> for the function “ptrace_hbp_set_addr” according to the SmPL rule “to_do2”.
> >> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/kernel/ptrace.c?id=08f103b9a9502974109fab47ea35ca8542c4e57a#n451
> >>
> >> Which adjustments will become relevant here?
> >
> > In that function err is used elsewhere, so the second message should not
> > be printed.
>
> I disagree to this view (after the removal of previous uses).

What is causing the previous uses to be removed?

julia

>
> The following SmPL script variant can point also questionable variable declarations out.
>
> @check1@
> expression x;
> identifier rc;
> local idexpression lrc;
> position p;
> @@
>  lrc@rc = x@p;
>  return lrc;
>
> @check2@
> identifier check1.rc;
> position check1.p;
> type t;
> @@
>  t rc@p;
>  ... when != rc
>
> @script:python to_do1@
> p << check1.p;
> @@
> coccilib.org.print_todo(p[0],
>                         "WARNING: An expression was assigned to a local variable before it will be returned by the subsequent statement.")
>
> @script:python to_do2@
> p << check1.p;
> v << check1.rc;
> @@
> coccilib.org.print_todo(p[0],
>                         "INFO: May the local variable \""
>                         + v
>                         + "\" be deleted?")
>
>
> But how should be achieved that the notification for declarations
> which can be omitted will be presented only once for each affected local variable?
>
> Regards,
> Markus
>

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

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

Re: [Cocci] Reducing source code around return statements with SmPL?

[Markus Elfring]

> What is causing the previous uses to be removed?

Thanks for your reminder.

Now I see that I need to repeat the deletion specification from
other SmPL script variants.

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

Re: [Cocci] Reducing source code around return statements with SmPL?

[Markus Elfring]

> What is causing the previous uses to be removed?

Now I am looking for related software development possibilities for
such a source code adjustment.
Such changes trigger the output of diff hunks by default.
Can the display be temporarily switched off for specific data processing steps?

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

Re: [Cocci] Reducing source code around return statements with SmPL?

[Julia Lawall]

On Wed, 25 Sep 2019, Markus Elfring wrote:

> > What is causing the previous uses to be removed?
>
> Now I am looking for related software development possibilities for
> such a source code adjustment.
> Such changes trigger the output of diff hunks by default.
> Can the display be temporarily switched off for specific data processing steps?

--no-show-diff.  But it is not temporary.  There will be no diffs at all.

julia

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

Re: [Cocci] Reducing source code around return statements with SmPL?

[Markus Elfring]

>> Can the display be temporarily switched off for specific data processing steps?
>
> --no-show-diff.  But it is not temporary.  There will be no diffs at all.

* I would like to choose this setting for the execution of selected SmPL rules
  (instead of referring to a known command line parameter).

* Do I need to take an extra child process into account?

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

Re: [Cocci] Reducing source code around return statements with SmPL?

[Markus Elfring]

>> * I would like to choose this setting for the execution of selected SmPL rules
>>   (instead of referring to a known command line parameter).
>
> This option is not offered.

How are the chances to extend the Coccinelle software for this purpose?

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

Re: [Cocci] Reducing source code around return statements with SmPL?

[Markus Elfring]

>>>> * I would like to choose this setting for the execution of selected SmPL rules
>>>>   (instead of referring to a known command line parameter).
>>>
>>> This option is not offered.
>>
>> How are the chances to extend the Coccinelle software for this purpose?
>
> None.

It's a pity.


> I think you could use ( & ) to do what you want without this functionality.

I wonder how the usage of SmPL conjunctions can help more for the discussed use case.

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

Re: [Cocci] Reducing source code around return statements with SmPL?

[Markus Elfring]

>>> I think you could use ( & ) to do what you want without this functionality.
>>
>> I wonder how the usage of SmPL conjunctions can help more for the discussed use case.
>
> So try it and find out.

I have got doubts about the applicability in this case.


> No need to waste my time reading about the fact that you wonder about it.

Would any other contributors help to improve the situation a bit more?

SmPL conjunctions do not trigger the deletion of questionable source code.
One shown SmPL rule depends on the check (by a SmPL when constraint)
that a local variable is not used any more after source code places were adjusted.

How is the status for the support of SmPL disjunctions in a mentioned test combination?

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

Re: [Cocci] Reducing source code around return statements with SmPL?

[Markus Elfring]

>> @@
>> -rc = x;
>
> Replace by - lrc@rc = x;

I have constructed further test scripts for the semantic patch language.

@display1@
expression x;
identifier rc;
local idexpression lrc;
@@
(
 if (...)
*{
*lrc@rc = x;
*return lrc;
*}
|
*lrc@rc = x;
*return lrc;
)

@display2@
identifier display1.rc;
type t;
@@
*t rc;
 ... when != rc


The following test result is generated so far for an example.
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/kernel/ptrace.c?id=08f103b9a9502974109fab47ea35ca8542c4e57a

elfring@Sonne:~/Projekte/Linux/next-patched> spatch ~/Projekte/Coccinelle/janitor/show_code_for_moving_to_return1.cocci arch/arm64/kernel/ptrace.c
…
     (ONCE) already tagged but only removed, so safe
…
@@ -290,7 +290,6 @@ static struct perf_event *ptrace_hbp_cre
 {
 	struct perf_event *bp;
 	struct perf_event_attr attr;
-	int err, type;

 	switch (note_type) {
 	case NT_ARM_HW_BREAK:
@@ -329,7 +328,6 @@ static int ptrace_hbp_fill_attr_ctrl(uns
 				     struct arch_hw_breakpoint_ctrl ctrl,
 				     struct perf_event_attr *attr)
 {
-	int err, len, type, offset, disabled = !ctrl.enabled;

 	attr->disabled = disabled;
 	if (disabled)
@@ -434,10 +432,6 @@ static int ptrace_hbp_set_ctrl(unsigned
 	struct arch_hw_breakpoint_ctrl ctrl;

 	bp = ptrace_hbp_get_initialised_bp(note_type, tsk, idx);
-	if (IS_ERR(bp)) {
-		err = PTR_ERR(bp);
-		return err;
-	}

 	attr = bp->attr;
 	decode_ctrl_reg(uctrl, &ctrl);
@@ -458,15 +452,9 @@ static int ptrace_hbp_set_addr(unsigned
 	struct perf_event_attr attr;

 	bp = ptrace_hbp_get_initialised_bp(note_type, tsk, idx);
-	if (IS_ERR(bp)) {
-		err = PTR_ERR(bp);
-		return err;
-	}

 	attr = bp->attr;
 	attr.bp_addr = addr;
-	err = modify_user_hw_breakpoint(bp, &attr);
-	return err;
 }

 #define PTRACE_HBP_ADDR_SZ	sizeof(u64)



I suggest to compare this output with the following source code transformation approach.

@replacement@
expression x;
identifier rc;
local idexpression lrc;
@@
-lrc@rc = x;
 return
-       rc
+       x
 ;

@deletion@
identifier replacement.rc;
type t;
@@
-t rc;
 ... when != rc


elfring@Sonne:~/Projekte/Linux/next-patched> spatch ~/Projekte/Coccinelle/janitor/simplify_return2.cocci arch/arm64/kernel/ptrace.c
…
@@ -435,8 +435,7 @@ static int ptrace_hbp_set_ctrl(unsigned

 	bp = ptrace_hbp_get_initialised_bp(note_type, tsk, idx);
 	if (IS_ERR(bp)) {
-		err = PTR_ERR(bp);
-		return err;
+		return PTR_ERR(bp);
 	}

 	attr = bp->attr;
@@ -453,20 +452,17 @@ static int ptrace_hbp_set_addr(unsigned
 			       unsigned long idx,
 			       u64 addr)
 {
-	int err;
 	struct perf_event *bp;
 	struct perf_event_attr attr;

 	bp = ptrace_hbp_get_initialised_bp(note_type, tsk, idx);
 	if (IS_ERR(bp)) {
-		err = PTR_ERR(bp);
-		return err;
+		return PTR_ERR(bp);
 	}

 	attr = bp->attr;
 	attr.bp_addr = addr;
-	err = modify_user_hw_breakpoint(bp, &attr);
-	return err;
+	return modify_user_hw_breakpoint(bp, &attr);
 }

 #define PTRACE_HBP_ADDR_SZ	sizeof(u64)


* Why are the functions “ptrace_hbp_create” and “ptrace_hbp_fill_attr_ctrl”
  presented here?

* Why is the first variable declaration from the function “ptrace_hbp_set_addr”
  not marked by the SmPL asterisk functionality in this test example?

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

Re: [Cocci] Reducing source code around return statements with SmPL?

[Julia Lawall]

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



On Tue, 24 Sep 2019, Markus Elfring wrote:

> >> @@
> >> -rc = x;
> >
> > Replace by - lrc@rc = x;
>
> I have constructed further test scripts for the semantic patch language.
>
> @display1@
> expression x;
> identifier rc;
> local idexpression lrc;
> @@
> (
>  if (...)
> *{
> *lrc@rc = x;
> *return lrc;
> *}
> |
> *lrc@rc = x;
> *return lrc;
> )
>
> @display2@
> identifier display1.rc;
> type t;
> @@
> *t rc;
>  ... when != rc
>
>
> The following test result is generated so far for an example.
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/kernel/ptrace.c?id=08f103b9a9502974109fab47ea35ca8542c4e57a
>
> elfring@Sonne:~/Projekte/Linux/next-patched> spatch ~/Projekte/Coccinelle/janitor/show_code_for_moving_to_return1.cocci arch/arm64/kernel/ptrace.c
> …
>      (ONCE) already tagged but only removed, so safe
> …
> @@ -290,7 +290,6 @@ static struct perf_event *ptrace_hbp_cre
>  {
>  	struct perf_event *bp;
>  	struct perf_event_attr attr;
> -	int err, type;
>
>  	switch (note_type) {
>  	case NT_ARM_HW_BREAK:
> @@ -329,7 +328,6 @@ static int ptrace_hbp_fill_attr_ctrl(uns
>  				     struct arch_hw_breakpoint_ctrl ctrl,
>  				     struct perf_event_attr *attr)
>  {
> -	int err, len, type, offset, disabled = !ctrl.enabled;
>
>  	attr->disabled = disabled;
>  	if (disabled)
> @@ -434,10 +432,6 @@ static int ptrace_hbp_set_ctrl(unsigned
>  	struct arch_hw_breakpoint_ctrl ctrl;
>
>  	bp = ptrace_hbp_get_initialised_bp(note_type, tsk, idx);
> -	if (IS_ERR(bp)) {
> -		err = PTR_ERR(bp);
> -		return err;
> -	}
>
>  	attr = bp->attr;
>  	decode_ctrl_reg(uctrl, &ctrl);
> @@ -458,15 +452,9 @@ static int ptrace_hbp_set_addr(unsigned
>  	struct perf_event_attr attr;
>
>  	bp = ptrace_hbp_get_initialised_bp(note_type, tsk, idx);
> -	if (IS_ERR(bp)) {
> -		err = PTR_ERR(bp);
> -		return err;
> -	}
>
>  	attr = bp->attr;
>  	attr.bp_addr = addr;
> -	err = modify_user_hw_breakpoint(bp, &attr);
> -	return err;
>  }
>
>  #define PTRACE_HBP_ADDR_SZ	sizeof(u64)
>
>
>
> I suggest to compare this output with the following source code transformation approach.
>
> @replacement@
> expression x;
> identifier rc;
> local idexpression lrc;
> @@
> -lrc@rc = x;
>  return
> -       rc
> +       x
>  ;
>
> @deletion@
> identifier replacement.rc;
> type t;
> @@
> -t rc;
>  ... when != rc
>
>
> elfring@Sonne:~/Projekte/Linux/next-patched> spatch ~/Projekte/Coccinelle/janitor/simplify_return2.cocci arch/arm64/kernel/ptrace.c
> …
> @@ -435,8 +435,7 @@ static int ptrace_hbp_set_ctrl(unsigned
>
>  	bp = ptrace_hbp_get_initialised_bp(note_type, tsk, idx);
>  	if (IS_ERR(bp)) {
> -		err = PTR_ERR(bp);
> -		return err;
> +		return PTR_ERR(bp);
>  	}
>
>  	attr = bp->attr;
> @@ -453,20 +452,17 @@ static int ptrace_hbp_set_addr(unsigned
>  			       unsigned long idx,
>  			       u64 addr)
>  {
> -	int err;
>  	struct perf_event *bp;
>  	struct perf_event_attr attr;
>
>  	bp = ptrace_hbp_get_initialised_bp(note_type, tsk, idx);
>  	if (IS_ERR(bp)) {
> -		err = PTR_ERR(bp);
> -		return err;
> +		return PTR_ERR(bp);
>  	}
>
>  	attr = bp->attr;
>  	attr.bp_addr = addr;
> -	err = modify_user_hw_breakpoint(bp, &attr);
> -	return err;
> +	return modify_user_hw_breakpoint(bp, &attr);
>  }
>
>  #define PTRACE_HBP_ADDR_SZ	sizeof(u64)
>
>
> * Why are the functions “ptrace_hbp_create” and “ptrace_hbp_fill_attr_ctrl”
>   presented here?

What does "here" mean.  You give two semantic patches with two sets of
output.  Which is here?

> * Why is the first variable declaration from the function “ptrace_hbp_set_addr”
>   not marked by the SmPL asterisk functionality in this test example?

I don't know what you ae referring to so I can't answer precisely, but you
should know that the only connection between your first two rules with a *
is the name of an identifier.  There is no guarantee that the two rules
match code in the same function.

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] Reducing source code around return statements with SmPL?

[Markus Elfring]

>> I suggest to compare this output with the following source code transformation approach.
…
>> * Why are the functions “ptrace_hbp_create” and “ptrace_hbp_fill_attr_ctrl”
>>   presented here?
>
> What does "here" mean.  You give two semantic patches with two sets of
> output.  Which is here?

Please check the relevance of the mentioned two function names once more.


>> * Why is the first variable declaration from the function “ptrace_hbp_set_addr”
>>   not marked by the SmPL asterisk functionality in this test example?
>
> I don't know what you ae referring to so I can't answer precisely,

Do you find corresponding links clearer?
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/kernel/ptrace.c?id=08f103b9a9502974109fab47ea35ca8542c4e57a#n451
https://elixir.bootlin.com/linux/v5.3.1/source/arch/arm64/kernel/ptrace.c#L451


> but you should know that the only connection between your first two rules with a *
> is the name of an identifier.

I would expect that the selected identifier should refer to the same scope
of the enclosing function implementation.


> There is no guarantee that the two rules match code in the same function.

Will any additional metavariables be needed to achieve this?

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

Re: [Cocci] Reducing source code around return statements with SmPL?

[Julia Lawall]

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



On Tue, 24 Sep 2019, Markus Elfring wrote:

> >> I suggest to compare this output with the following source code transformation approach.
> …
> >> * Why are the functions “ptrace_hbp_create” and “ptrace_hbp_fill_attr_ctrl”
> >>   presented here?
> >
> > What does "here" mean.  You give two semantic patches with two sets of
> > output.  Which is here?
>
> Please check the relevance of the mentioned two function names once more.
>
>
> >> * Why is the first variable declaration from the function “ptrace_hbp_set_addr”
> >>   not marked by the SmPL asterisk functionality in this test example?
> >
> > I don't know what you ae referring to so I can't answer precisely,
>
> Do you find corresponding links clearer?
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/kernel/ptrace.c?id=08f103b9a9502974109fab47ea35ca8542c4e57a#n451
> https://elixir.bootlin.com/linux/v5.3.1/source/arch/arm64/kernel/ptrace.c#L451
>
>
> > but you should know that the only connection between your first two rules with a *
> > is the name of an identifier.
>
> I would expect that the selected identifier should refer to the same scope
> of the enclosing function implementation.

It doesn't.  Identifier metavariables have no scope, and scope is only
taken into account within a rule, not beterrn rules.

>
>
> > There is no guarantee that the two rules match code in the same function.
>
> Will any additional metavariables be needed to achieve this?

Maybe you can match the whole function definition.

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] Reducing source code around return statements with SmPL?

[Markus Elfring]

>> I would expect that the selected identifier should refer to the same scope
>> of the enclosing function implementation.
>
> It doesn't.  Identifier metavariables have no scope, and scope is only
> taken into account within a rule, not beterrn rules.

Will this information become relevant also for further improvements
of the software documentation?

Which object references can refer to items in an unique way?

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

Re: [Cocci] Reducing source code around return statements with SmPL?

[Julia Lawall]

On Tue, 24 Sep 2019, Markus Elfring wrote:

> >> I would expect that the selected identifier should refer to the same scope
> >> of the enclosing function implementation.
> >
> > It doesn't.  Identifier metavariables have no scope, and scope is only
> > taken into account within a rule, not beterrn rules.
>
> Will this information become relevant also for further improvements
> of the software documentation?
>
> Which object references can refer to items in an unique way?

Only positions are unique.

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

Re: [Cocci] Reducing source code around return statements with SmPL?

[Markus Elfring]

> Only positions are unique.

Can additional metavariable variants make sense if they would be implicitly combined
with context information which can be safely used across SmPL rules?

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

Re: [Cocci] Reducing source code around return statements with SmPL?

[Markus Elfring]

> Only positions are unique.

I suggest to take information around an error message like
“meta: semantic error: position cannot be inherited over modifications: …”
also better into account.
https://github.com/coccinelle/coccinelle/blob/f284bf3663b362476f14c0c90de7ff7ce3faf9b7/parsing_cocci/parse_aux.ml#L323

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

Re: [Cocci] Reducing source code around return statements with SmPL?

[Markus Elfring]

> Only positions are unique.

I suggest to take information around an error message like
“meta: semantic error: position cannot be inherited over modifications: …”
also better into account.
https://github.com/coccinelle/coccinelle/blob/f284bf3663b362476f14c0c90de7ff7ce3faf9b7/parsing_cocci/parse_aux.ml#L323

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

Re: [Cocci] Reducing source code around return statements with SmPL?

[Markus Elfring]

>> @@
>> -rc = x;
>
> Replace by - lrc@rc = x;

I have constructed another test script for the semantic patch language.

@replacement@
expression x;
identifier rc;
local idexpression lrc;
@@
(
 if (...)
-{
-lrc@rc = x;
 return
-       rc
+       x
 ;
-}
|
-lrc@rc = x;
 return
-       rc
+       x
 ;
)

@deletion@
identifier replacement.rc;
type t;
@@
-t rc;
 ... when != rc


Test result:
elfring@Sonne:~/Projekte/Linux/next-patched> spatch ~/Projekte/Coccinelle/janitor/move_code_to_return3.cocci arch/arm64/kernel/ptrace.c
…
previous modification:
MINUS
  >>> x


According to environment 2:
   replacement.x -> PTR_ERR(bp)

   replacement.rc -> id err


current modification:
MINUS
  >>> x


According to environment 2:
   replacement.x -> PTR_ERR(bp)

   replacement.rc -> id err


exn while in timeout_function
replacement: already tagged token:
C code context
File "arch/arm64/kernel/ptrace.c", line 439, column 9, charpos = 10084
  around = 'err',
  whole content = 		return err;



Will any software improvements be needed around the application
of SmPL disjunctions for such a source code transformation approach?

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

[Cocci] [PATCH] Coccinelle: Add a SmPL script for the reconsideration of specific combinations of assignment and return statements

[Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Wed, 25 Sep 2019 11:55:24 +0200

Values from expressions were occasionally assigned to local variables
before they will be returned by the subsequent statement.
Such expressions can be directly specified in the return statement instead.

Adjust affected source code by the means of the semantic patch language
(Coccinelle software).

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 .../coccinelle/misc/move_code_to_return.cocci | 67 +++++++++++++++++++
 1 file changed, 67 insertions(+)
 create mode 100644 scripts/coccinelle/misc/move_code_to_return.cocci

diff --git a/scripts/coccinelle/misc/move_code_to_return.cocci b/scripts/coccinelle/misc/move_code_to_return.cocci
new file mode 100644
index 000000000000..78cdf84f9aaa
--- /dev/null
+++ b/scripts/coccinelle/misc/move_code_to_return.cocci
@@ -0,0 +1,67 @@
+// SPDX-License-Identifier: GPL-2.0
+/// Return expressions directly instead of assigning them to
+/// local variables immediately before affected statements.
+//
+// Keywords: return statements variable assignments coding style
+// Confidence: Medium
+
+virtual patch
+
+@replacement1 depends on patch@
+expression x;
+identifier f, rc;
+local idexpression lrc;
+type rt;
+@@
+ rt f(...)
+ {
+ ... when any
+ if (...)
+-{
+-lrc@rc = x;
+ return
+-       rc
++       x
+ ;
+-}
+ ... when any
+ }
+
+@replacement2 depends on patch@
+expression x;
+identifier f, rc;
+local idexpression lrc;
+type rt;
+@@
+ rt f(...)
+ {
+ ... when any
+-lrc@rc = x;
+ return
+-       rc
++       x
+ ;
+ ... when any
+ }
+
+@deletion2 depends on patch@
+identifier replacement2.f, replacement2.rc;
+type replacement2.rt, t;
+@@
+ rt f(...)
+ {
+ ... when any
+-t rc;
+ ... when != rc
+ }
+
+@deletion1 depends on patch@
+identifier replacement1.f, replacement1.rc;
+type replacement1.rt, t;
+@@
+ rt f(...)
+ {
+ ... when any
+-t rc;
+ ... when != rc
+ }
--
2.23.0

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

[Cocci] [PATCH v2] Coccinelle: Add a SmPL script for the reconsideration of specific combinations of assignment and return statements

[Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Tue, 1 Oct 2019 15:50:10 +0200

Values from expressions were occasionally assigned to local variables
before they will be returned by the subsequent statement.
Such expressions can be directly specified in the return statement instead.

Adjust affected source code by the means of the semantic patch language
(Coccinelle software).

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---

v2:
* Application of the SmPL construct “<+... … ...+>”
* Addition of a hint for the supported coccicheck operation modes


 .../coccinelle/misc/move_code_to_return.cocci | 73 +++++++++++++++++++
 1 file changed, 73 insertions(+)
 create mode 100644 scripts/coccinelle/misc/move_code_to_return.cocci

diff --git a/scripts/coccinelle/misc/move_code_to_return.cocci b/scripts/coccinelle/misc/move_code_to_return.cocci
new file mode 100644
index 000000000000..22ce7c9d0fd2
--- /dev/null
+++ b/scripts/coccinelle/misc/move_code_to_return.cocci
@@ -0,0 +1,73 @@
+// SPDX-License-Identifier: GPL-2.0
+/// Return expressions directly instead of assigning them to
+/// local variables immediately before affected statements.
+//
+// Keywords: return statements variable assignments coding style
+// Confidence: Medium
+
+virtual patch
+virtual report
+
+@replacement1 depends on patch@
+expression x;
+identifier f, rc;
+local idexpression lrc;
+type rt;
+@@
+ rt f(...)
+ {
+ <+...
+ if (...)
+-{
+-lrc@rc = x;
+ return
+-       rc
++       x
+ ;
+-}
+ ...+>
+ }
+
+@replacement2 depends on patch@
+expression x;
+identifier f, rc;
+local idexpression lrc;
+type rt;
+@@
+ rt f(...)
+ {
+ <+...
+-lrc@rc = x;
+ return
+-       rc
++       x
+ ;
+ ...+>
+ }
+
+@deletion2 depends on patch@
+identifier replacement2.f, replacement2.rc;
+type replacement2.rt, t;
+@@
+ rt f(...)
+ {
+ ... when any
+-t rc;
+ ... when != rc
+ }
+
+@deletion1 depends on patch@
+identifier replacement1.f, replacement1.rc;
+type replacement1.rt, t;
+@@
+ rt f(...)
+ {
+ ... when any
+-t rc;
+ ... when != rc
+ }
+
+@script:python info depends on report@
+@@
+import sys
+sys.stderr.write("INFO: Unfortunately, specific software limitations have got the consequence that only the operation mode “patch” can be supported by this SmPL script so far as expected.\n")
--
2.23.0

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