Re: [PATCH 01/16] coccinelle: misc: secs_to_jiffies: Patch expressions too

Re: [PATCH 01/16] coccinelle: misc: secs_to_jiffies: Patch expressions too

[Markus Elfring]

> Teach the script to suggest conversions for timeout patterns where the
> arguments to msecs_to_jiffies() are expressions as well.

I propose to take another look at implementation details for such a script variant
according to the semantic patch language.


…
> +++ b/scripts/coccinelle/misc/secs_to_jiffies.cocci
> @@ -11,12 +11,22 @@
>
>  virtual patch
…
> -@depends on patch@ constant C; @@
> +@depends on patch@
> +expression E;
> +@@
>
> -- msecs_to_jiffies(C * MSEC_PER_SEC)
> -+ secs_to_jiffies(C)
> +-msecs_to_jiffies
> ++secs_to_jiffies
> + (E
> +- * \( 1000 \| MSEC_PER_SEC \)
> + )

1. I do not see a need to keep an SmPL rule for the handling of constants
   (or literals) after the suggested extension for expressions.

2. I find it nice that you indicate an attempt to make the shown SmPL code
   a bit more succinct.
   Unfortunately, further constraints should be taken better into account
   for the current handling of isomorphisms (and corresponding SmPL disjunctions).
   Thus I would find an SmPL rule (like the following) more appropriate.

@adjustment@
expression e;
@@
-msecs_to_jiffies
+secs_to_jiffies
 (
(
-e * 1000
|
-e * MSEC_PER_SEC
)
+e
 )


3. It seems that you would like to support only a single operation mode so far.
   This system aspect can trigger further software development challenges.


Regards,
Markus

Re: [PATCH 01/16] coccinelle: misc: secs_to_jiffies: Patch expressions too

[Easwar Hariharan]

On 1/28/2025 1:02 PM, Markus Elfring wrote:
>> Teach the script to suggest conversions for timeout patterns where the
>> arguments to msecs_to_jiffies() are expressions as well.
> 
> I propose to take another look at implementation details for such a script variant
> according to the semantic patch language.
> 
> 
> …
>> +++ b/scripts/coccinelle/misc/secs_to_jiffies.cocci
>> @@ -11,12 +11,22 @@
>>
>>  virtual patch
> …
>> -@depends on patch@ constant C; @@
>> +@depends on patch@
>> +expression E;
>> +@@
>>
>> -- msecs_to_jiffies(C * MSEC_PER_SEC)
>> -+ secs_to_jiffies(C)
>> +-msecs_to_jiffies
>> ++secs_to_jiffies
>> + (E
>> +- * \( 1000 \| MSEC_PER_SEC \)
>> + )
> 
> 1. I do not see a need to keep an SmPL rule for the handling of constants
>    (or literals) after the suggested extension for expressions.

Can you explain why? Would the expression rule also address the cases
where it's a constant or literal?

> 2. I find it nice that you indicate an attempt to make the shown SmPL code
>    a bit more succinct.
>    Unfortunately, further constraints should be taken better into account
>    for the current handling of isomorphisms (and corresponding SmPL disjunctions).
>    Thus I would find an SmPL rule (like the following) more appropriate.
> 

Sorry, I couldn't follow your sentence construction or reasoning here. I
don't see how my patch is deficient, or different from your suggestion
below, especially given that it follows your feedback from part 1:
https://lore.kernel.org/all/9088f9a2-c4ab-4098-a255-25120df5c497@web.de/

Can you point out specifically what SmPL isomorphisms or disjunctions
are broken with the patch in its current state?

Thanks,
Easwar

Re: [01/16] coccinelle: misc: secs_to_jiffies: Patch expressions too

[Markus Elfring]

>> …
>>> +++ b/scripts/coccinelle/misc/secs_to_jiffies.cocci
>>> @@ -11,12 +11,22 @@
>>>
>>>  virtual patch
>> …
>>> -@depends on patch@ constant C; @@
>>> +@depends on patch@
>>> +expression E;
>>> +@@
>>>
>>> -- msecs_to_jiffies(C * MSEC_PER_SEC)
>>> -+ secs_to_jiffies(C)
>>> +-msecs_to_jiffies
>>> ++secs_to_jiffies
>>> + (E
>>> +- * \( 1000 \| MSEC_PER_SEC \)
>>> + )
>>
>> 1. I do not see a need to keep an SmPL rule for the handling of constants
>>    (or literals) after the suggested extension for expressions.
>
> Can you explain why? Would the expression rule also address the cases
> where it's a constant or literal?

Probably, yes.


>> 2. I find it nice that you indicate an attempt to make the shown SmPL code
>>    a bit more succinct.
>>    Unfortunately, further constraints should be taken better into account
>>    for the current handling of isomorphisms (and corresponding SmPL disjunctions).
>>    Thus I would find an SmPL rule (like the following) more appropriate.
>>
>
> Sorry, I couldn't follow your sentence construction or reasoning here.
> I don't see how my patch is deficient, or different from your suggestion
> below, especially given that it follows your feedback from part 1:
> https://lore.kernel.org/all/9088f9a2-c4ab-4098-a255-25120df5c497@web.de/

I tend also to present possibilities for succinct SmPL code.
Unfortunately, software dependencies can trigger corresponding target conflicts.


> Can you point out specifically what SmPL isomorphisms or disjunctions
> are broken with the patch in its current state?

Please take another look at related information sources.
Would you like to achieve any benefits from commutativity (for multiplications)?
https://gitlab.inria.fr/coccinelle/coccinelle/-/blob/bd08cad3f802229dc629a13eefef2018c620e905/standard.iso#L241
https://github.com/coccinelle/coccinelle/blob/cca22217d1b4316224e80a18d0b08dd351234497/standard.iso#L241


Regards,
Markus

Re: [01/16] coccinelle: misc: secs_to_jiffies: Patch expressions too

[Markus Elfring]

> I tend also to present possibilities for succinct SmPL code.
> Unfortunately, software dependencies can trigger corresponding target conflicts.

@adjustment@
expression e;
@@
-msecs_to_jiffies
+secs_to_jiffies
 (
(
-e * 1000
|
-e * MSEC_PER_SEC
)
+e
 )


A command (like the following) can indicate how isomorphisms are applied
for the transformation of some data into SmPL disjunctions.
https://gitlab.inria.fr/coccinelle/coccinelle/-/blob/bd08cad3f802229dc629a13eefef2018c620e905/standard.iso#L252-257

Markus_Elfring@Sonne:…/Projekte/Coccinelle/Probe> spatch --parse-cocci suggestion3_for_Easwar_Hariharan-20250128.cocci
…
@adjustment@
expression e;
@@


(
-msecs_to_jiffies
  >>> secs_to_jiffies
(-e -* -1000
  <<< e
)
|
-msecs_to_jiffies
  >>> secs_to_jiffies
(-1000 -* -e
  <<< e
)
|
-msecs_to_jiffies
  >>> secs_to_jiffies
(-e -* -MSEC_PER_SEC
  <<< e
)
|
-msecs_to_jiffies
  >>> secs_to_jiffies
(-MSEC_PER_SEC -* -e
  <<< e
)
)


Grep query
msecs_to_jiffies



I find parts of such a data representation improvable.
I would usually expect here that parentheses for the selection of call parameters
will not appear in the first text column
(so that confusion will be avoided for the usage of delimiters according to SmPL disjunctions).



The isomorphism specifications represent also a software development status.
It seems that they do not contain direct support for SmPL disjunctions so far
(as an explicit entity).



The identifier “HZ” is used by the referenced macro.
https://elixir.bootlin.com/linux/v6.13/source/include/linux/jiffies.h#L530-L540
https://lore.kernel.org/linux-kernel/173831299312.31546.8797889985487965830.tip-bot2@tip-bot2/

Is there a need to take further (preprocessor symbol) variations better into account?



How do you think about the handling of multiplication factors within bigger expressions
(and not only at the beginning or end of a term)?



Would you be looking for further restrictions on expression combinations?

Regards,
Markus

Re: [PATCH 01/16] coccinelle: misc: secs_to_jiffies: Patch expressions too

[Markus Elfring]

> Teach the script to suggest conversions for timeout patterns where the
> arguments to msecs_to_jiffies() are expressions as well.

Does anything hinder to benefit any more from a source code analysis approach
(like the following by the extended means of the semantic patch language)?


// SPDX-License-Identifier: GPL-2.0
/// Simplify statements by using a known wrapper macro.
/// Replace selected msecs_to_jiffies() calls by secs_to_jiffies().
//
// Keywords: wrapper macro conversion secs seconds jiffies
// Confidence: High
// Options: --no-includes --include-headers

virtual context, patch, report, org

@depends on context@
expression e;
@@
*msecs_to_jiffies
 (
(e * 1000
|e * MSEC_PER_SEC
)
 )

@depends on patch@
expression e;
@@
-msecs_to_jiffies
+secs_to_jiffies
 (
(
-e * 1000
|
-e * MSEC_PER_SEC
)
+e
 )

@x depends on org || report@
expression e;
position p;
@@
 msecs_to_jiffies@p
 (
(e * 1000
|e * MSEC_PER_SEC
)
 )

@script:python depends on org@
p << x.p;
@@
coccilib.org.print_todo(p[0], "WARNING: opportunity for secs_to_jiffies()")

@script:python depends on report@
p << x.p;
@@
coccilib.report.print_report(p[0], "WARNING: opportunity for secs_to_jiffies()")


Regards,
Markus

Re: [PATCH 01/16] coccinelle: misc: secs_to_jiffies: Patch expressions too

[Easwar Hariharan]

On 1/30/2025 3:01 AM, Markus Elfring wrote:
>> Teach the script to suggest conversions for timeout patterns where the
>> arguments to msecs_to_jiffies() are expressions as well.
> 
> Does anything hinder to benefit any more from a source code analysis approach
> (like the following by the extended means of the semantic patch language)?
> 

Thank you, this is much more useful feedback, specifically due to the
suggested patch below. I did intend to learn about the other modes and
progressively upgrade secs_to_jiffies.cocci with them in the future once
the existing instances were resolved, to help with future code
submissions. The patch below will be super helpful in that.

As it stands, I'll fix up the current rules in v2 following your
suggestion to keep the multiplication in each line to allow Coccinelle
to use the commutativity properties and find more instances.

I'll refrain from implementing the report mode until current instances
have been fixed because of the issue we have already seen[1] with CI
builds being broken. I would not want to break a strict CI build that is
looking for coccicheck REPORT to return 0 results.

[1]:
https://lore.kernel.org/all/20250129-secs_to_jiffles-v1-1-35a5e16b9f03@chromium.org/

<snip>

Thanks,
Easwar (he/him)

Re: [01/16] coccinelle: misc: secs_to_jiffies: Patch expressions too

[Markus Elfring]

> As it stands, I'll fix up the current rules in v2 following your
> suggestion to keep the multiplication in each line to allow Coccinelle
> to use the commutativity properties and find more instances.

Corresponding software development challenges can eventually be clarified further.


> I'll refrain from implementing the report mode until current instances
> have been fixed because of the issue we have already seen[1] with CI
> builds being broken. I would not want to break a strict CI build that is
> looking for coccicheck REPORT to return 0 results.

You got into the mood to test support for an information in the software documentation.
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/dev-tools/coccinelle.rst?h=v6.13#n92
“…
Note that not all semantic patches implement all modes.
…”

Regards,
Markus