Re: [cocci] behavior change in semantic patches since c36b51ffc889 …

[Jacob Keller <jacob.e.keller@intel.com>]

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