[Cocci] Searching for duplicate statements in if branches (with SmPL)?

[Cocci] Searching for duplicate statements in if branches (with SmPL)?

[Markus Elfring]

Hello,

I have constructed the following small SmPL script.


@duplicated_code@
identifier work;
statement s1, s2;
type T;
@@
 T work(...)
 {
 ... when any
*if (...)
*{
    ... when any
*   s1
*   s2
*}
 ... when any
*if (...)
*{
    ... when any
*   s1
*   s2
*}
 ... when any
 }


Test result:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/net/ethernet/ti/netcp_ethss.c?id=75eac7b5f68b0a0671e795ac636457ee27cc11d8#n3562
https://elixir.bootlin.com/linux/v5.1-rc4/source/drivers/net/ethernet/ti/netcp_ethss.c#L3562

@@ -1829,11 +1829,7 @@ static void gbe_reset_mod_stats(struct g
 	int i;

 	for (i = 0; i < gbe_dev->num_et_stats; i++) {
-		if (gbe_dev->et_stats[i].type == stats_mod) {
 			p_stats_entry = base + gbe_dev->et_stats[i].offset;
-			gbe_dev->hw_stats[i] = 0;
-			gbe_dev->hw_stats_prev[i] = readl(p_stats_entry);
-		}
 	}
 }

@@ -2808,10 +2804,6 @@ static int gbe_set_rx_mode(void *intf_pr
 	cpsw_ale_control_set(ale, 0, ALE_AGEOUT, 1);
 	do {
 		cpu_relax();
-		if (cpsw_ale_control_get(ale, 0, ALE_AGEOUT)) {
-			ret = 0;
-			break;
-		}

 	} while (time_after(timeout, jiffies));

…
@@ -3657,31 +3636,14 @@ static int gbe_probe(struct netcp_device

 	ret = netcp_txpipe_init(&gbe_dev->tx_pipe, netcp_device,
 				gbe_dev->dma_chan_name, gbe_dev->tx_queue_id);
-	if (ret) {
-		of_node_put(interfaces);
-		return ret;
-	}

 	ret = netcp_txpipe_open(&gbe_dev->tx_pipe);
-	if (ret) {
-		of_node_put(interfaces);
-		return ret;
-	}

 	/* Create network interfaces */
 	INIT_LIST_HEAD(&gbe_dev->gbe_intf_head);
 	for_each_child_of_node(interfaces, interface) {
 		ret = of_property_read_u32(interface, "slave-port", &slave_num);
-		if (ret) {
-			dev_err(dev, "missing slave-port parameter, skipping interface configuration for %pOFn\n",
-				interface);
-			continue;
-		}
 		gbe_dev->num_slaves++;
-		if (gbe_dev->num_slaves >= gbe_dev->max_num_slaves) {
-			of_node_put(interface);
-			break;
-		}
 	}
 	of_node_put(interfaces);



Now I wonder again why the first two diff hunks are presented by this
source code search approach. It seems that the hits for the functions
“gbe_reset_mod_stats” and “gbe_set_rx_mode” are false positives
(because only one if statement is marked there).

The last diff hunk points an implementation detail out which is discussed
for the update suggestion “ethernet: ti: eliminate a bit of duplicate code
in gbe_probe()”.
https://lkml.org/lkml/2019/4/9/979
https://lore.kernel.org/patchwork/patch/1059888/
https://lore.kernel.org/lkml/1554864935-8299-1-git-send-email-wen.yang99@zte.com.cn/

But it shows also source code places where the SmPL specification is
too generic so far.


Another SmPL script variant presents also a result for additional considerations.


@duplicated_code@
identifier work;
statement s1, s2, es;
type T;
@@
 T work(...)
 {
 ... when any
*if (...)
*{
    ... when any
*   s1
*   s2
*}
 else
 es
 ... when any
*if (...)
*{
    ... when any
*   s1
*   s2
*}
 else
 es
 ... when any
 }


@@ -3205,14 +3205,8 @@ static void init_secondary_ports(struct
 				       slave->phy_node,
 				       gbe_adjust_link_sec_slaves,
 				       0, phy_mode);
-		if (!slave->phy) {
 			dev_err(dev, "phy not found for slave %d\n",
 				slave->slave_num);
-		} else {
-			dev_dbg(dev, "phy found: id is: 0x%s\n",
-				phydev_name(slave->phy));
-			phy_start(slave->phy);
-		}
 	}
 }


I am curious if better solutions can be found for such software.

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

Re: [Cocci] Searching for duplicate statements in if branches (with SmPL)?

[Markus Elfring]

> Your ifs may be in a loop.

Such source code structures are actually shown for the first two diff hunks.
Will any further adjustments be needed to take this aspect better into account
for the presented analysis approach?

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

Re: [Cocci] Searching for duplicate statements in if branches (with SmPL)?

[Markus Elfring]

> If you want to be sure that you aren't in a loop,

I have got other expectations for the software behaviour.


> you have to ue position variables to be sure that the two matched fragments are different.

I wonder how loops could matter for the presented analysis approach.

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

Re: [Cocci] Searching for duplicate statements in if branches (with SmPL)?

[Markus Elfring]

> If you want to be sure that you aren't in a loop,

I do not want to filter on this implementation detail for the shown analysis approach.


> you have to ue position variables to be sure that the two matched fragments are different.

I find this information questionable.

* How should source code search specifications from different places match here?

* Where is the knowledge still incomplete for the desired software behaviour?

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

Re: [Cocci] Searching for duplicate statements in if branches (with SmPL)?

[Markus Elfring]

>>> If you want to be sure that you aren't in a loop,
>>
>> I do not want to filter on this implementation detail for the shown analysis approach.
>
> I don't know what you mean by "I do not want to".

I suggest to improve this clarification by additional case distinctions.


> You mean that you want results where both matched ifs are actually the same,

I suggest also to adjust this interpretation.

* You are right that such a special use case is shown by the update suggestion
  “ethernet: ti: eliminate a bit of duplicate code in gbe_probe()”.
  https://lkml.org/lkml/2019/4/9/979
  https://lore.kernel.org/patchwork/patch/1059888/
  https://lore.kernel.org/lkml/1554864935-8299-1-git-send-email-wen.yang99@zte.com.cn/

* I would like to search for statements which were repeated (duplicated)
  at the end of different if branches (after varying condition checks).


> because they are in a loop,

Duplicate statements can occur also there.


> or do you mean that you are too lazy to express this necessary information
> in your rule.

We have got a different understanding for the desired requirements.


> In the latter case, I can't help you.

I assume that this issue would need another communication approach.


> If you don't want to do what is needed, then find some other tool.

I hope that the software situation can be clarified and improved further.

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

Re: [Cocci] Checking influence of loops?

[Markus Elfring]

> Your ifs may be in a loop.

Does your technology treat this control flow in special ways?

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

Re: [Cocci] Checking influence of loops?

[Markus Elfring]

>>> Your ifs may be in a loop.
>>
>> Does your technology treat this control flow in special ways?
>
> No.

Would the quoted information be irrelevant then for the discussed use case?

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

Re: [Cocci] Checking influence of loops?

[Markus Elfring]

>>>>> Your ifs may be in a loop.
>>>>
>>>> Does your technology treat this control flow in special ways?
>>>
>>> No.
>>
>> Would the quoted information be irrelevant then for the discussed use case?
>
> No idea what the above means.

It seems that there is another communication difficulty involved.

Your feedback indicated concerns for the handling of loops
in the shown source code search results, didn't it?


> There are control flow paths that cause ... to connect things.

I guess that these “connections” need further clarifications.


> The process of matching doesn't know if the connection is due to a loop,
> and if, a goto, or anything else.

Should two search specifications (with SmPL ellipses) between them
be kept separate for the desired diff output?

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

Re: [Cocci] Checking influence of loops?

[Julia Lawall]

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



On Sat, 13 Apr 2019, Markus Elfring wrote:

> >>>>> Your ifs may be in a loop.
> >>>>
> >>>> Does your technology treat this control flow in special ways?
> >>>
> >>> No.
> >>
> >> Would the quoted information be irrelevant then for the discussed use case?
> >
> > No idea what the above means.
>
> It seems that there is another communication difficulty involved.
>
> Your feedback indicated concerns for the handling of loops
> in the shown source code search results, didn't it?
>
>
> > There are control flow paths that cause ... to connect things.
>
> I guess that these “connections” need further clarifications.
>
>
> > The process of matching doesn't know if the connection is due to a loop,
> > and if, a goto, or anything else.
>
> Should two search specifications (with SmPL ellipses) between them
> be kept separate for the desired diff output?

I don't understand any of your answers precisely.

If you have a pattern like

AAA
...
BBB

The AAA and BBB might match the same code if there is a possible flow of
execution from the match of AAA to itself.

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 influence of loops?

[Markus Elfring]

>> Should two search specifications (with SmPL ellipses) between them
>> be kept separate for the desired diff output?
>
> I don't understand any of your answers precisely.

I hope that we can come closer again to a better understanding
of the reported software situation.


> If you have a pattern like
>
> AAA
> ...
> BBB
>
> The AAA and BBB might match the same code

I agree to this information in principle.
But I imagine that this source code should be found
at different positions.


> if there is a possible flow of execution from the match
> of AAA to itself.

I am curious on the circumstances when such details
would be really relevant.

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

Re: [Cocci] Checking influence of loops?

[Julia Lawall]

On Sat, 13 Apr 2019, Markus Elfring wrote:

> >> Should two search specifications (with SmPL ellipses) between them
> >> be kept separate for the desired diff output?
> >
> > I don't understand any of your answers precisely.
>
> I hope that we can come closer again to a better understanding
> of the reported software situation.
>
>
> > If you have a pattern like
> >
> > AAA
> > ...
> > BBB
> >
> > The AAA and BBB might match the same code
>
> I agree to this information in principle.
> But I imagine that this source code should be found
> at different positions.

No there is no such constraint.

>
>
> > if there is a possible flow of execution from the match
> > of AAA to itself.
>
> I am curious on the circumstances when such details
> would be really relevant.

Typically when there is a loop or goto.  But there is no special handling
of either construct.

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

Re: [Cocci] Checking influence of loops?

[Markus Elfring]

>>> If you have a pattern like
>>>
>>> AAA
>>> ...
>>> BBB
>>>
>>> The AAA and BBB might match the same code
>>
>> I agree to this information in principle.
>> But I imagine that this source code should be found
>> at different positions.
>
> No there is no such constraint.

My software expectations might be different from your view here.


>>> if there is a possible flow of execution from the match
>>> of AAA to itself.
>>
>> I am curious on the circumstances when such details
>> would be really relevant.
>
> Typically when there is a loop or goto.

I find such a case distinction interesting.


> But there is no special handling of either construct.

Does this information contain a contradiction?

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

Re: [Cocci] Checking influence of loops?

[Markus Elfring]

>>> Typically when there is a loop or goto.
>>
>> I find such a case distinction interesting.
>>
>>
>>> But there is no special handling of either construct.
>>
>> Does this information contain a contradiction?
>
> No.  Draw a control flow graph of a loop and see what should happen for
> such a pattern.

I am still curious if it can be achieved that two statements
(with a SmPL ellipsis between them) will be found only at different positions.

How should be excluded that two generic search specifications
will be merged?

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

Re: [Cocci] Clarification for SmPL ellipsis functionality

[Markus Elfring]

>> I am still curious if it can be achieved that two statements
>> (with a SmPL ellipsis between them) will be found only at different positions.
>>
>> How should be excluded that two generic search specifications
>> will be merged?
>
> Use position variables and use a second rule that forces the positions to
> be different.

Thanks for this suggestion.


> I already explained this.

It might take another while to take such an “explanation”
better into account.

Does this feedback indicate any background information
where another addition would be helpful also in the manual
for the semantic patch language?


> You surely don't like it,

Your impression can be appropriate here.


> but nothing is going to change in this direction,

Under which circumstances will corresponding adjustments get
more chances?


> so the discussion (at least on my side) ends here.

Will the software documentation be completed anyhow?

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

Re: [Cocci] Clarification for SmPL functionality around position variables

[Markus Elfring]

> Use position variables and use a second rule that forces the positions to
> be different.  I already explained this.

I suggest to take another look at the provided information.

I have extended the analysis approach like the following.


@find@
identifier work;
position p1, p2;
statement s1, s2;
type T;
@@
 T work(...)
 {
 ... when any
 if@p1 (...)
 {
    ... when any
    s1
    s2
 }
 ... when any
 if@p2 (...)
 {
    ... when any
    s1
    s2
 }
 ... when any
 }

@duplicated_code@
identifier work;
position pos != find.p2, find.p1, find.p2;
statement s1, s2;
type T;
@@
 T work(...)
 {
 ... when any
*if@p1@pos (...)
*{
    ... when any
*   s1
*   s2
*}
 ... when any
*if@p2 (...)
*{
    ... when any
*   s1
*   s2
*}
 ... when any
 }


> You surely don't like it, but nothing is going to change in this direction,
> so the discussion (at least on my side) ends here.

Now I stumble on another error message.


elfring@Sonne:~/Projekte/Coccinelle/janitor> spatch --parse-cocci show_same_statements7.cocci
…
Variable find.p2 in duplicated_code cannot be used as both a position and a constraint



By the way:
Will it matter that the determined source code positions are not only different
but also that one place should be before the other?

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

Re: [Cocci] Clarification for SmPL functionality around position variables

[Julia Lawall]

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



On Sun, 14 Apr 2019, Markus Elfring wrote:

> > Use position variables and use a second rule that forces the positions to
> > be different.  I already explained this.
>
> I suggest to take another look at the provided information.
>
> I have extended the analysis approach like the following.
>
>
> @find@
> identifier work;
> position p1, p2;
> statement s1, s2;
> type T;
> @@
>  T work(...)
>  {
>  ... when any
>  if@p1 (...)
>  {
>     ... when any
>     s1
>     s2
>  }
>  ... when any
>  if@p2 (...)
>  {
>     ... when any
>     s1
>     s2
>  }
>  ... when any
>  }
>
> @duplicated_code@
> identifier work;
> position pos != find.p2, find.p1, find.p2;
> statement s1, s2;
> type T;
> @@
>  T work(...)
>  {
>  ... when any
> *if@p1@pos (...)
> *{
>     ... when any
> *   s1
> *   s2
> *}
>  ... when any
> *if@p2 (...)
> *{
>     ... when any
> *   s1
> *   s2
> *}
>  ... when any
>  }
>
>
> > You surely don't like it, but nothing is going to change in this direction,
> > so the discussion (at least on my side) ends here.
>
> Now I stumble on another error message.
>
>
> elfring@Sonne:~/Projekte/Coccinelle/janitor> spatch --parse-cocci show_same_statements7.cocci
> …
> Variable find.p2 in duplicated_code cannot be used as both a position and a constraint

I think that you have to declare find.p1 and find.p2 on a subsequent line.

julia

>
>
>
> By the way:
> Will it matter that the determined source code positions are not only different
> but also that one place should be before the other?
>
> 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] Clarification for SmPL functionality around position variables

[Markus Elfring]

>> elfring@Sonne:~/Projekte/Coccinelle/janitor> spatch --parse-cocci show_same_statements7.cocci
>> …
>> Variable find.p2 in duplicated_code cannot be used as both a position and a constraint
>
> I think that you have to declare find.p1 and find.p2 on a subsequent line.

I have observed the same error message then.


I have adjusted the SmPL code also in the following way.

@duplicated_code@
identifier work;
position find.p1, find.p2;
position pos != find.p2;
statement s1, s2;
type T;
@@
…


elfring@Sonne:~/Projekte/Coccinelle/janitor> spatch --parse-cocci show_same_statements7x.cocci
…
meta: parse error:
  File "show_same_statements7x.cocci", line 29, column 21, charpos = 315
  around = 'p2',
  whole content = position pos != find.p2;


How will the software situation evolve further?

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

Re: [Cocci] Clarification for SmPL functionality around position variables

[Markus Elfring]

>>>> Variable find.p2 in duplicated_code cannot be used as both a position and a constraint
>>>
>>> I think that you have to declare find.p1 and find.p2 on a subsequent line.
>>
>> I have observed the same error message then.
>
> Go back and read what I wrote.

I suggest to take another look also at my feedback.


> You put the metavariables on a previous line,

Yes, too.


> and I told you to put them on a subsequent line.

I tried three different orderings out for these position variables
in the meantime.

Will the implementation be fixed for all affected software components anyhow?

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

Re: [Cocci] Clarification for SmPL functionality around position variables

[Markus Elfring]

>> I tried three different orderings out for these position variables
>> in the meantime.
>>
>> Will the implementation be fixed for all affected software components anyhow?
>
> No.

I am still curious then under which circumstances this software will be corrected.

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