public inbox for cocci@systeme.lip6.fr
 help / color / mirror / Atom feed
* [cocci] behavior change in semantic patches since c36b51ffc889 ("don't drop down on single statement")
@ 2025-03-07  0:43 Jacob Keller
  2025-03-07  0:55 ` Julia Lawall
  2025-03-07 10:47 ` [cocci] behavior change in semantic patches since c36b51ffc889 … Markus Elfring
  0 siblings, 2 replies; 14+ messages in thread
From: Jacob Keller @ 2025-03-07  0:43 UTC (permalink / raw)
  To: cocci, Julia Lawall; +Cc: Przemek Kitszel

Hi Julia,

We use semantic patches for code cleanup regularly, and recently noticed
a change in behavior some some semantic patches that are run as part of
a build process. I've provided the patch and an explanation of my
discoveries thus far. Hopefully help track down whats wrong and whether
we need to update these patches, there is some other workaround we can
do, or there is actually a bug in coccinelle/spatch.

I have the following semantic patch:

> @@
> void * void_ptr;
> type T;
> T * typed_ptr;
> @@
> - typed_ptr = (T *)void_ptr
> + typed_ptr = void_ptr

Its goal is to find and remove unnecessary type casts where a cast is on
a void pointer that to another type.

This is used for our build process as we have some code that is shared
between C and C++. The C++ compiler requires the casts to avoid issues,
but C warns about them.

With spatch 1.2.0, this patch works and finds and removes casts.

However, I updated to the master branch of the coccinelle project and
noticed that it no longer finds all of the casts. In particular, it
looks like casts which are part of type declarations don't always get
reliably caught.

I also noticed a behavior change in another patch:

> @@
> expression hw, ptr;
> @@
> (
> - ice_free(hw, ptr);
> + devm_kfree(ice_hw_to_dev(hw),ptr);
> )

This patch updates some code that used an internal wrapper function to
use the appropriate kernel function directly.

Since switching and testing on the master branch which looks like its
the 1.3.0 release, this now transforms code like this:

if (something)
  ice_free(...)

into

if (something) {
  devm_kfree(...)
}

When the function is inside a block like this, it is transformed to
include braces. This often violates our style guide as braces around
single line if or other scopes are not desired.

These changes break our ability to deploy the new version of coccinelle.

I ran a git bisect to determine when these changes broke, and both
changes appear to occur due to the following change:

c36b51ffc889 ("don't drop down on single statement")

I do not know why this commit causes such a behavior change, and am
reaching out for guidance and hope that a suitable workaround can be found.

I was able to partially work around the issue with the type cast:

I was using spatch --parse-cocci and saw some warnings about the patch
file, and refactored it to:

@disable drop_cast@
void * void_ptr;
type T;
T * typed_ptr;
@@
 typed_ptr =
-(T *)
 void_ptr


This fixed so that the patch does find all the typecasts, but has often
left us with undesirable white spacing, for example of there is code
like this:

struct something *a =
	(struct something *)data


Then before it translates to:

struct something *a = data

but now it translates to

struct something *a =
	data

The ability to automatically condense the lines was very nice, and
losing that is a bit annoying.

From the commit description, I doubt you can simply reverse this change.
I was hoping your expertise with semantic patches could help us identify
modified versions of the patches which will reliably work for us in both
the function transformation and cast removal patches.

Thanks,
Jake

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

* Re: [cocci] behavior change in semantic patches since c36b51ffc889 ("don't drop down on single statement")
  2025-03-07  0:43 [cocci] behavior change in semantic patches since c36b51ffc889 ("don't drop down on single statement") Jacob Keller
@ 2025-03-07  0:55 ` Julia Lawall
  2025-03-07 20:24   ` Jacob Keller
  2025-03-07 10:47 ` [cocci] behavior change in semantic patches since c36b51ffc889 … Markus Elfring
  1 sibling, 1 reply; 14+ messages in thread
From: Julia Lawall @ 2025-03-07  0:55 UTC (permalink / raw)
  To: Jacob Keller; +Cc: cocci, Przemek Kitszel



On Thu, 6 Mar 2025, Jacob Keller wrote:

> Hi Julia,
>
> We use semantic patches for code cleanup regularly, and recently noticed
> a change in behavior some some semantic patches that are run as part of
> a build process. I've provided the patch and an explanation of my
> discoveries thus far. Hopefully help track down whats wrong and whether
> we need to update these patches, there is some other workaround we can
> do, or there is actually a bug in coccinelle/spatch.

Thanks for the report, and espcially for the bisect.  I don't think it
would be good to just revert the commit, since it is solving a real
problem.  I will see how to get your cases working properly.

>
> I have the following semantic patch:
>
> > @@
> > void * void_ptr;
> > type T;
> > T * typed_ptr;
> > @@
> > - typed_ptr = (T *)void_ptr
> > + typed_ptr = void_ptr
>
> Its goal is to find and remove unnecessary type casts where a cast is on
> a void pointer that to another type.
>
> This is used for our build process as we have some code that is shared
> between C and C++. The C++ compiler requires the casts to avoid issues,
> but C warns about them.
>
> With spatch 1.2.0, this patch works and finds and removes casts.
>
> However, I updated to the master branch of the coccinelle project and
> noticed that it no longer finds all of the casts. In particular, it
> looks like casts which are part of type declarations don't always get
> reliably caught.
>
> I also noticed a behavior change in another patch:
>
> > @@
> > expression hw, ptr;
> > @@
> > (
> > - ice_free(hw, ptr);
> > + devm_kfree(ice_hw_to_dev(hw),ptr);
> > )

This doesn't need the starting and ending ()
This is just a comemnt, it won't solve the problem.

julia


> This patch updates some code that used an internal wrapper function to
> use the appropriate kernel function directly.
>
> Since switching and testing on the master branch which looks like its
> the 1.3.0 release, this now transforms code like this:
>
> if (something)
>   ice_free(...)
>
> into
>
> if (something) {
>   devm_kfree(...)
> }
>
> When the function is inside a block like this, it is transformed to
> include braces. This often violates our style guide as braces around
> single line if or other scopes are not desired.
>
> These changes break our ability to deploy the new version of coccinelle.
>
> I ran a git bisect to determine when these changes broke, and both
> changes appear to occur due to the following change:
>
> c36b51ffc889 ("don't drop down on single statement")
>
> I do not know why this commit causes such a behavior change, and am
> reaching out for guidance and hope that a suitable workaround can be found.
>
> I was able to partially work around the issue with the type cast:
>
> I was using spatch --parse-cocci and saw some warnings about the patch
> file, and refactored it to:
>
> @disable drop_cast@
> void * void_ptr;
> type T;
> T * typed_ptr;
> @@
>  typed_ptr =
> -(T *)
>  void_ptr
>
>
> This fixed so that the patch does find all the typecasts, but has often
> left us with undesirable white spacing, for example of there is code
> like this:
>
> struct something *a =
> 	(struct something *)data
>
>
> Then before it translates to:
>
> struct something *a = data
>
> but now it translates to
>
> struct something *a =
> 	data
>
> The ability to automatically condense the lines was very nice, and
> losing that is a bit annoying.
>
> From the commit description, I doubt you can simply reverse this change.
> I was hoping your expertise with semantic patches could help us identify
> modified versions of the patches which will reliably work for us in both
> the function transformation and cast removal patches.
>
> Thanks,
> Jake
>

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

* Re: [cocci] behavior change in semantic patches since c36b51ffc889 …
  2025-03-07  0:43 [cocci] behavior change in semantic patches since c36b51ffc889 ("don't drop down on single statement") Jacob Keller
  2025-03-07  0:55 ` Julia Lawall
@ 2025-03-07 10:47 ` Markus Elfring
  2025-03-07 20:53   ` Jacob Keller
  1 sibling, 1 reply; 14+ messages in thread
From: Markus Elfring @ 2025-03-07 10:47 UTC (permalink / raw)
  To: Jacob Keller, cocci; +Cc: Przemek Kitszel

> I also noticed a behavior change in another patch:
>
>> @@
>> expression hw, ptr;
>> @@
>> (
>> - ice_free(hw, ptr);
>> + devm_kfree(ice_hw_to_dev(hw),ptr);
>> )
>
> This patch updates some code that used an internal wrapper function to
> use the appropriate kernel function directly.

Would you like to experiment with another transformation approach?


@replacement@
expression hw;
@@
-ice_free
+devm_kfree
 (
+ ice_hw_to_dev(
  hw
+ )
  , ...
 );


> When the function is inside a block like this, it is transformed to
> include braces. This often violates our style guide as braces around
> single line if or other scopes are not desired.

I became also curious how the support can be improved for source code
analyses and transformations according to compound statements.


> I ran a git bisect to determine when these changes broke, and both
> changes appear to occur due to the following change:

Would you like to share any commands and related background information?


> I was using spatch --parse-cocci and saw some warnings about the patch
> file, and refactored it to:
>
> @disable drop_cast@
> void * void_ptr;
> type T;
> T * typed_ptr;
> @@
>  typed_ptr =
> -(T *)
>  void_ptr

I find it interesting that you tried also an SmPL script variant out
with a better change precision.


> This fixed so that the patch does find all the typecasts, but has often
> left us with undesirable white spacing, …

Will development interests grow for topics like the following?

* Advanced data processing for whitespace characters
  2016-01-12
  https://github.com/coccinelle/coccinelle/issues/58

* Fix indentation algorithm for Linux coding style
  2016-08-29
  https://github.com/coccinelle/coccinelle/issues/75

* Fix usage of white-space characters at two places for Linux coding style
  2016-08-29
  https://github.com/coccinelle/coccinelle/issues/76

* Preserve selected whitespace characters (for Linux coding style)
  2025-02-28
  https://github.com/coccinelle/coccinelle/issues/392


Regards,
Markus

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

* Re: [cocci] behavior change in semantic patches since c36b51ffc889 ("don't drop down on single statement")
  2025-03-07  0:55 ` Julia Lawall
@ 2025-03-07 20:24   ` Jacob Keller
  0 siblings, 0 replies; 14+ messages in thread
From: Jacob Keller @ 2025-03-07 20:24 UTC (permalink / raw)
  To: Julia Lawall; +Cc: cocci, Przemek Kitszel



On 3/6/2025 4:55 PM, Julia Lawall wrote:
> 
> 
> On Thu, 6 Mar 2025, Jacob Keller wrote:
> 
>> Hi Julia,
>>
>> We use semantic patches for code cleanup regularly, and recently noticed
>> a change in behavior some some semantic patches that are run as part of
>> a build process. I've provided the patch and an explanation of my
>> discoveries thus far. Hopefully help track down whats wrong and whether
>> we need to update these patches, there is some other workaround we can
>> do, or there is actually a bug in coccinelle/spatch.
> 
> Thanks for the report, and espcially for the bisect.  I don't think it
> would be good to just revert the commit, since it is solving a real
> problem.  I will see how to get your cases working properly.
> 

Thanks :)


>>> @@
>>> expression hw, ptr;
>>> @@
>>> (
>>> - ice_free(hw, ptr);
>>> + devm_kfree(ice_hw_to_dev(hw),ptr);
>>> )
> 
> This doesn't need the starting and ending ()
> This is just a comemnt, it won't solve the problem.
> 
> julia
> 

Yep. I am honestly not sure where those came from, we've had this one
around for some time.

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

* Re: [cocci] behavior change in semantic patches since c36b51ffc889 …
  2025-03-07 10:47 ` [cocci] behavior change in semantic patches since c36b51ffc889 … Markus Elfring
@ 2025-03-07 20:53   ` Jacob Keller
  2025-03-07 22:47     ` Jacob Keller
                       ` (2 more replies)
  0 siblings, 3 replies; 14+ messages in thread
From: Jacob Keller @ 2025-03-07 20:53 UTC (permalink / raw)
  To: Markus Elfring, cocci; +Cc: Przemek Kitszel



On 3/7/2025 2:47 AM, Markus Elfring wrote:
>> I also noticed a behavior change in another patch:
>>
>>> @@
>>> expression hw, ptr;
>>> @@
>>> (
>>> - ice_free(hw, ptr);
>>> + devm_kfree(ice_hw_to_dev(hw),ptr);
>>> )
>>
>> This patch updates some code that used an internal wrapper function to
>> use the appropriate kernel function directly.
> 
> Would you like to experiment with another transformation approach?
> 
> 
> @replacement@
> expression hw;
> @@
> -ice_free
> +devm_kfree
>  (
> + ice_hw_to_dev(
>   hw
> + )
>   , ...
>  );
> 

I tried this out and it does seem to fix the white spacing for our
particular cases. @Julia, this is an acceptable workaround and change
for us if you can't or don't want to figure out the spaciness issues in
the bisected commit.

> 
>> When the function is inside a block like this, it is transformed to
>> include braces. This often violates our style guide as braces around
>> single line if or other scopes are not desired.
> 
> I became also curious how the support can be improved for source code
> analyses and transformations according to compound statements.
> 
> 
>> I ran a git bisect to determine when these changes broke, and both
>> changes appear to occur due to the following change:
> 
> Would you like to share any commands and related background information?
> 
I ran a fairly standard git bisect:

$ git bisect start
$ git bisect good 1.2.0
$ git bisect bad master
< this checked out a commit>
$ ./auto gen && ./configure --prefix=$HOME && make clean && make && \
  make install
$ cd ~/project/
$ make BUILD=KERNEL
< I compared the resulting code with the known working case for 1.2.0,
and then can inform git bisect appropriately. >
$ cd ~/cocci
$ git bisect <good/bad>
< this will check out another commit >

From here, I just repeated the process until git bisect told me which
commit had the breaking change.

There were a couple gotchas where the checked out commit didn't compile
but I managed to figure out which changes were needed and manually
pulled those in each time.

In particular, about my project:

We have a repository with code that is originally designed to be shared
among multiple drivers. This code uses constructs like "ice_free", or
typecasts required for C++, but which the Linux kernel does not. We need
the code we ultimately contribute to the kernel to follow the kernel
guidelines.

The semantic patches are applied automatically as part of the build
process, and not something we commit. This process was done in order to
try and align what we distribute in our out-of-tree releases with what
we end up submitting to the kernel community.

This usage is likely different from many projects where the patches are
used to find and commit fixed code. In our case, committing the result
(and thus being able to manually tweak it) are not viable because that
would break other downstream consumers of this code internally.

We've been working towards moving away from this process and essentially
forking the Linux code we have from the rest, but it is difficult as
there are many stakeholders involved.

> 
>> I was using spatch --parse-cocci and saw some warnings about the patch
>> file, and refactored it to:
>>
>> @disable drop_cast@
>> void * void_ptr;
>> type T;
>> T * typed_ptr;
>> @@
>>  typed_ptr =
>> -(T *)
>>  void_ptr
> 
> I find it interesting that you tried also an SmPL script variant out
> with a better change precision.
> 

I tried several other approaches and found they also didn't work in one
way or another. This one had the best results minus the white spacing
issues. In some sense it is 'better' since it preserves existing line
breaks and white space...  but thats the opposite of what we want in our
case.

> 
>> This fixed so that the patch does find all the typecasts, but has often
>> left us with undesirable white spacing, …
> 
> Will development interests grow for topics like the following?
> 
> * Advanced data processing for whitespace characters
>   2016-01-12
>   https://github.com/coccinelle/coccinelle/issues/58
> 
> * Fix indentation algorithm for Linux coding style
>   2016-08-29
>   https://github.com/coccinelle/coccinelle/issues/75
> 
> * Fix usage of white-space characters at two places for Linux coding style
>   2016-08-29
>   https://github.com/coccinelle/coccinelle/issues/76
> 
> * Preserve selected whitespace characters (for Linux coding style)
>   2025-02-28
>   	
> 

I think the white spacing is a hard problem, because each project will
have their own style and rules. For many cases, the expectation is that
a human would manually cleanup the results.

In my case, we run this as part of our build process to clean up code
automatically, and don't commit the results. I suspect this is counter
to the original intention. One option would be to run a style formatting
tool after transforms, to allow a tool dedicated to code formatting vs
trying to make spatch and coccinelle do everything these can do.

I do think improvements in the way you can specify spacing would be
useful but may complicate the already difficult syntax of semantic patches.

> 
> Regards,
> Markus


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

* Re: [cocci] behavior change in semantic patches since c36b51ffc889 …
  2025-03-07 20:53   ` Jacob Keller
@ 2025-03-07 22:47     ` Jacob Keller
  2025-03-08  9:07       ` Markus Elfring
  2025-03-08  8:39     ` Markus Elfring
  2025-03-08  9:45     ` [cocci] Evolution of transformation processes? Markus Elfring
  2 siblings, 1 reply; 14+ messages in thread
From: Jacob Keller @ 2025-03-07 22:47 UTC (permalink / raw)
  To: Markus Elfring, cocci; +Cc: Przemek Kitszel



On 3/7/2025 12:53 PM, Jacob Keller wrote:
> 
> 
> On 3/7/2025 2:47 AM, Markus Elfring wrote:
>>> I also noticed a behavior change in another patch:
>>>
>>>> @@
>>>> expression hw, ptr;
>>>> @@
>>>> (
>>>> - ice_free(hw, ptr);
>>>> + devm_kfree(ice_hw_to_dev(hw),ptr);
>>>> )
>>>
>>> This patch updates some code that used an internal wrapper function to
>>> use the appropriate kernel function directly.
>>
>> Would you like to experiment with another transformation approach?
>>
>>
>> @replacement@
>> expression hw;
>> @@
>> -ice_free
>> +devm_kfree
>>  (
>> + ice_hw_to_dev(
>>   hw
>> + )
>>   , ...
>>  );
>>
> 
For what its worth, I was finally able to get things working pretty much
the same as before with the following semantic patches:

> @@
> expression hw;
> @@
> -ice_free
> +devm_kfree
>  (
> +ice_hw_to_dev(
>  hw
> +)
>  , ...)


and

> @disable drop_cast@
> void * void_ptr;
> type T;
> @@
> -(T *)void_ptr
> +void_ptr

These seem to properly apply with both 1.2.0 and 1.3.0, and mostly
handle the white spacing properly. There were exactly 2 places where I
ended up having to re-arrange the original code slightly having to do
with constructions like:

type ptr =
   (type)value

where the original version would have replaced the entire line and
cleaned things up to fit. These were easy enough to fix up on my end.

As a bonus, this caught several missed typecasts of function arguments,
as well as some assignments that the original hadn't caught for some reason.

I think this change should be acceptable on our end and we don't need to
worry about fixes to spatch. Again, I re-tested both 1.3.0 and 1.2.0 and
get the same behavior now.

Appreciate the suggestions and especially the work you guys do on this tool!

Thanks,
Jake

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

* Re: [cocci] behavior change in semantic patches since c36b51ffc889 …
  2025-03-07 20:53   ` Jacob Keller
  2025-03-07 22:47     ` Jacob Keller
@ 2025-03-08  8:39     ` Markus Elfring
  2025-03-10 18:27       ` Jacob Keller
  2025-03-08  9:45     ` [cocci] Evolution of transformation processes? Markus Elfring
  2 siblings, 1 reply; 14+ messages in thread
From: Markus Elfring @ 2025-03-08  8:39 UTC (permalink / raw)
  To: Jacob Keller, cocci; +Cc: Przemek Kitszel

> I tried this out and it does seem to fix the white spacing for our
> particular cases.

Thanks for such positive feedback.

Will your development interests grow also for further SmPL code variations?


> From here, I just repeated the process until git bisect told me which
> commit had the breaking change.

How many iterations did you need to try out here?


> There were a couple gotchas where the checked out commit didn't compile
> but I managed to figure out which changes were needed and manually
> pulled those in each time.

Do you see chances to improve such a process anyhow?


> I think the white spacing is a hard problem, because each project will
> have their own style and rules. For many cases, the expectation is that
> a human would manually cleanup the results.

Various contributors are struggling with desirable adjustments also
in affected areas.


> In my case, we run this as part of our build process to clean up code
> automatically, and don't commit the results.

Thanks for another bit of background information.


>                                              I suspect this is counter
> to the original intention.

It seems that you got into a position where you may stress the importance
of safe data processing automation.


>                            One option would be to run a style formatting
> tool after transforms, to allow a tool dedicated to code formatting vs
> trying to make spatch and coccinelle do everything these can do.

Are you looking for ways to avoid extra efforts for code reformatting?


> I do think improvements in the way you can specify spacing would be
> useful but may complicate the already difficult syntax of semantic patches.
Adhering to code style requirements can be challenging.

Regards,
Markus

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

* Re: [cocci] behavior change in semantic patches since c36b51ffc889 …
  2025-03-07 22:47     ` Jacob Keller
@ 2025-03-08  9:07       ` Markus Elfring
  2025-03-10 18:34         ` Jacob Keller
  0 siblings, 1 reply; 14+ messages in thread
From: Markus Elfring @ 2025-03-08  9:07 UTC (permalink / raw)
  To: Jacob Keller, cocci; +Cc: Przemek Kitszel

> For what its worth, I was finally able to get things working pretty much
> the same as before with the following semantic patches:

This is a nice feedback.


>> @@
>> expression hw;
>> @@
>> -ice_free
>> +devm_kfree
>>  (
>> +ice_hw_to_dev(
>>  hw
>> +)
>>  , ...)

Did you observe that the omission of a semicolon at the end
triggered desirable effects?



> and
>
>> @disable drop_cast@
>> void * void_ptr;
>> type T;
>> @@
>> -(T *)void_ptr
>> +void_ptr

Would another SmPL code variant be occasionally helpful?

-(T *)
 void_ptr


Are there cases to consider where casts should be preserved?

Regards,
Markus

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

* Re: [cocci] Evolution of transformation processes?
  2025-03-07 20:53   ` Jacob Keller
  2025-03-07 22:47     ` Jacob Keller
  2025-03-08  8:39     ` Markus Elfring
@ 2025-03-08  9:45     ` Markus Elfring
  2025-03-10 18:23       ` Jacob Keller
  2 siblings, 1 reply; 14+ messages in thread
From: Markus Elfring @ 2025-03-08  9:45 UTC (permalink / raw)
  To: Jacob Keller, Przemek Kitszel, cocci

> In particular, about my project:
>
> We have a repository with code that is originally designed to be shared
> among multiple drivers. This code uses constructs like "ice_free", or
> typecasts required for C++, but which the Linux kernel does not. We need
> the code we ultimately contribute to the kernel to follow the kernel
> guidelines.
>
> The semantic patches are applied automatically as part of the build
> process, and not something we commit. This process was done in order to
> try and align what we distribute in our out-of-tree releases with what
> we end up submitting to the kernel community.

Thanks for such background information.


> This usage is likely different from many projects where the patches are
> used to find and commit fixed code. In our case, committing the result
> (and thus being able to manually tweak it) are not viable because that
> would break other downstream consumers of this code internally.

* How much do you still need to recheck results from automated data processing?

* Would you like to achieve reasonably low system failure rates?

* May you represent any parts of organisations which can influence
  the growth of development resources in more significant ways?


Regards,
Markus

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

* Re: [cocci] Evolution of transformation processes?
  2025-03-08  9:45     ` [cocci] Evolution of transformation processes? Markus Elfring
@ 2025-03-10 18:23       ` Jacob Keller
  0 siblings, 0 replies; 14+ messages in thread
From: Jacob Keller @ 2025-03-10 18:23 UTC (permalink / raw)
  To: Markus Elfring, Przemek Kitszel, cocci



On 3/8/2025 1:45 AM, Markus Elfring wrote:
>> In particular, about my project:
>>
>> We have a repository with code that is originally designed to be shared
>> among multiple drivers. This code uses constructs like "ice_free", or
>> typecasts required for C++, but which the Linux kernel does not. We need
>> the code we ultimately contribute to the kernel to follow the kernel
>> guidelines.
>>
>> The semantic patches are applied automatically as part of the build
>> process, and not something we commit. This process was done in order to
>> try and align what we distribute in our out-of-tree releases with what
>> we end up submitting to the kernel community.
> 
> Thanks for such background information.
> 
> 
>> This usage is likely different from many projects where the patches are
>> used to find and commit fixed code. In our case, committing the result
>> (and thus being able to manually tweak it) are not viable because that
>> would break other downstream consumers of this code internally.
> 
> * How much do you still need to recheck results from automated data processing?
> 

We do have some monitoring to check if the output changes when we update
the coccinelle version, but otherwise don't manually check each time we
run the same script.

> * Would you like to achieve reasonably low system failure rates?
> 

In our setup, we only use patches which have very low failure rates.
This is a requirement because they are applied automatically and
expected to be consistent and catch all the changes we care about. In
most cases, the failure rate must be zero or else the resulting code
will not compile.

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

* Re: [cocci] behavior change in semantic patches since c36b51ffc889 …
  2025-03-08  8:39     ` Markus Elfring
@ 2025-03-10 18:27       ` Jacob Keller
  2025-03-11  9:26         ` Przemek Kitszel
  0 siblings, 1 reply; 14+ messages in thread
From: Jacob Keller @ 2025-03-10 18:27 UTC (permalink / raw)
  To: Markus Elfring, cocci; +Cc: Przemek Kitszel



On 3/8/2025 12:39 AM, Markus Elfring wrote:
>> I tried this out and it does seem to fix the white spacing for our
>> particular cases.
> 
> Thanks for such positive feedback.
> 
> Will your development interests grow also for further SmPL code variations?
> 
> 
>> From here, I just repeated the process until git bisect told me which
>> commit had the breaking change.
> 
> How many iterations did you need to try out here?
> 

About a dozen. Bisection is powerful because it scales with the
logarithm of the number of commits.

> 
>> There were a couple gotchas where the checked out commit didn't compile
>> but I managed to figure out which changes were needed and manually
>> pulled those in each time.
> 
> Do you see chances to improve such a process anyhow?
> 

Most projects which use bisect heavily try to ensure that at least
compilation works with every change committed. This ensures that you can
build the project at any arbitrary commit in order to use the bisect
process.

>>                            One option would be to run a style formatting
>> tool after transforms, to allow a tool dedicated to code formatting vs
>> trying to make spatch and coccinelle do everything these can do.
> 
> Are you looking for ways to avoid extra efforts for code reformatting?
> 

In my experience, our existing coding style is not 100% replicated in
any of the formatting tools. It would take a significant effort to get
to the point where we could run a formatting tool without completely
re-formatting the entire code base.

> 
>> I do think improvements in the way you can specify spacing would be
>> useful but may complicate the already difficult syntax of semantic patches.
> Adhering to code style requirements can be challenging.
> 

Yep. I'm ok with some compromises, and we often have accepted those in
the past when adding new semantic patches.

However, this particular case is a change in the behavior of spatch,
which is a bit more difficult for us.

> Regards,
> Markus


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

* Re: [cocci] behavior change in semantic patches since c36b51ffc889 …
  2025-03-08  9:07       ` Markus Elfring
@ 2025-03-10 18:34         ` Jacob Keller
  2025-03-11  9:10           ` Markus Elfring
  0 siblings, 1 reply; 14+ messages in thread
From: Jacob Keller @ 2025-03-10 18:34 UTC (permalink / raw)
  To: Markus Elfring, cocci; +Cc: Przemek Kitszel



On 3/8/2025 1:07 AM, Markus Elfring wrote:
>> For what its worth, I was finally able to get things working pretty much
>> the same as before with the following semantic patches:
> 
> This is a nice feedback.
> 
> 
>>> @@
>>> expression hw;
>>> @@
>>> -ice_free
>>> +devm_kfree
>>>  (
>>> +ice_hw_to_dev(
>>>  hw
>>> +)
>>>  , ...)
> 
> Did you observe that the omission of a semicolon at the end
> triggered desirable effects?
> 

I'm not sure if that impacted things, but it may have.

> 
> 
>> and
>>
>>> @disable drop_cast@
>>> void * void_ptr;
>>> type T;
>>> @@
>>> -(T *)void_ptr
>>> +void_ptr
> 
> Would another SmPL code variant be occasionally helpful?
> 
> -(T *)
>  void_ptr
> 

I tried this version. The big issue here is that spatch will remove the
typecast, but tries to leave any code outside of -/+ alone. This results
in the types remaining on their old line when code is formatted like this:

var = (cast)
   value;

This is sometimes done to avoid line length issues. With the style I
used, we get:

var = value;

But with the style you propose here we get:

var =
   value;

Which is not always desirable.

I found that both styles can be helpful depending on what you're doing.

> 
> Are there cases to consider where casts should be preserved?
> 

Yes, I ran into this exact issue with this aggressive version because
there are some casts which do something like cast a void pointer with
immediate access, without saving it to a variable. This more aggressive
version ended up triggering on these.

We managed to get one that works with a little more context:

@disable drop_cast@
void * void_ptr;
type T;
T * typed_ptr;
@@
 typed_ptr =
-(T *)void_ptr
+void_ptr


This version puts the typed_ptr as just context. This change alone seems
to be sufficient to get coccinelle to work with both versions, while
still preserving the line transformations we desired as I mentioned above.

There were exactly 2 places in the code base where this produced
different results, with the form:

type var =
   (cast)value;

Because the "type var" is in a context line, it doesn't get changed and
squashed with the value.

We just modified these two places to put the cast on the first line so
it would get squashed appropriately.

> Regards,
> Markus


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

* Re: [cocci] behavior change in semantic patches since c36b51ffc889 …
  2025-03-10 18:34         ` Jacob Keller
@ 2025-03-11  9:10           ` Markus Elfring
  0 siblings, 0 replies; 14+ messages in thread
From: Markus Elfring @ 2025-03-11  9:10 UTC (permalink / raw)
  To: Jacob Keller, cocci; +Cc: Przemek Kitszel

…
> I found that both styles can be helpful depending on what you're doing.

This is good to know.

Do the mentioned technical details point any remaining open issues out
also for the software documentation?


Would you get into the mood to influence affected source code
(including OCaml or TeX from Coccinelle) any more?


> There were exactly 2 places in the code base where this produced
> different results, with the form:
Will such places become more interesting for the clarification
of code layout concerns?

Regards,
Markus

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

* Re: [cocci] behavior change in semantic patches since c36b51ffc889 …
  2025-03-10 18:27       ` Jacob Keller
@ 2025-03-11  9:26         ` Przemek Kitszel
  0 siblings, 0 replies; 14+ messages in thread
From: Przemek Kitszel @ 2025-03-11  9:26 UTC (permalink / raw)
  To: Jacob Keller, Markus Elfring; +Cc: cocci

>> Are you looking for ways to avoid extra efforts for code reformatting?
>>
> 
> In my experience, our existing coding style is not 100% replicated in
> any of the formatting tools. It would take a significant effort to get
> to the point where we could run a formatting tool without completely
> re-formatting the entire code base.

The most problematic rule is "don't reformat code you don't touch",
so the formatter needs to limit it's scope.

Not only to the lines touched, but "things touched", see:

-	int var = ...;
+	int val = ...;

	// ...

-	err = long_call_a_bit_wrong_perhaps_too_long_line(var, other);
+	err = long_call_a_bit_wrong_perhaps_too_long_line(val, other);

so, the rename of poor "var" to better (but same length) "val",
you don't want to break long_call_a_bit_wrong_perhaps_too_long_line()
into multiple lines here

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

end of thread, other threads:[~2025-03-18  9:11 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-03-07  0:43 [cocci] behavior change in semantic patches since c36b51ffc889 ("don't drop down on single statement") Jacob Keller
2025-03-07  0:55 ` Julia Lawall
2025-03-07 20:24   ` Jacob Keller
2025-03-07 10:47 ` [cocci] behavior change in semantic patches since c36b51ffc889 … Markus Elfring
2025-03-07 20:53   ` Jacob Keller
2025-03-07 22:47     ` Jacob Keller
2025-03-08  9:07       ` Markus Elfring
2025-03-10 18:34         ` Jacob Keller
2025-03-11  9:10           ` Markus Elfring
2025-03-08  8:39     ` Markus Elfring
2025-03-10 18:27       ` Jacob Keller
2025-03-11  9:26         ` Przemek Kitszel
2025-03-08  9:45     ` [cocci] Evolution of transformation processes? Markus Elfring
2025-03-10 18:23       ` Jacob Keller

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox