cocci: multiple versions of function with different arguments

cocci: multiple versions of function with different arguments

[Stefan Assmann]

Hi Julia,

I'm trying to generate a patch to define a function with different
arguments depending on the kernel version. I'm having some difficulties,
maybe you can tell me what's wrong with the following approach.

@ rule1 @
struct net_device_ops ops;
identifier vlan_rx_add_vid_func;
@@
ops.ndo_vlan_rx_add_vid = vlan_rx_add_vid_func;

@ rule2 @
identifier rule1.vlan_rx_add_vid_func;
expression a,b,c;
@@
+#if LINUX_VERSION_CODE >= KERNEL_VERSION(3,10,0)
        vlan_rx_add_vid_func(a, b, c)
+#else
+       vlan_rx_add_vid_func(a, c)
+#endif


This results in
spatch --sp-file test3.cocci drivers/net/ethernet/intel/igb/igb_main.c
init_defs_builtins: /usr/local/share/coccinelle/standard.h
282 302
Fatal error: exception Failure("plus: parse error:
 = File "test3.cocci", line 14, column 2,  charpos = 282
    around = 'vlan_rx_add_vid_func', whole content = +  vlan_rx_add_vid_func(a, c)
")


If I remove "vlan_rx_add_vid_func(a, c)" it works, but that defeats the
whole idea.

Thanks!

  Stefan

Re: cocci: multiple versions of function with different arguments

[Johannes Berg]

On Wed, 2014-04-23 at 12:27 +0200, Stefan Assmann wrote:

> I'm trying to generate a patch to define a function with different
> arguments depending on the kernel version. I'm having some difficulties,
> maybe you can tell me what's wrong with the following approach.

I can't answer that, but given the premise

> +#if LINUX_VERSION_CODE >= KERNEL_VERSION(3,10,0)
>         vlan_rx_add_vid_func(a, b, c)
> +#else
> +       vlan_rx_add_vid_func(a, c)
> +#endif

and the fact that later kernel versions add an argument, wouldn't it be
much easier to provide a macro in the appropriate header file:

#define vlan_rx_add_vid_func(a,b,c) vlan_rx_add_vid_func(a,c)

johannes

Re: cocci: multiple versions of function with different arguments

[Stefan Assmann]

On 23.04.2014 12:41, Johannes Berg wrote:
> On Wed, 2014-04-23 at 12:27 +0200, Stefan Assmann wrote:
> 
>> I'm trying to generate a patch to define a function with different
>> arguments depending on the kernel version. I'm having some difficulties,
>> maybe you can tell me what's wrong with the following approach.
> 
> I can't answer that, but given the premise
> 
>> +#if LINUX_VERSION_CODE >= KERNEL_VERSION(3,10,0)
>>         vlan_rx_add_vid_func(a, b, c)
>> +#else
>> +       vlan_rx_add_vid_func(a, c)
>> +#endif
> 
> and the fact that later kernel versions add an argument, wouldn't it be
> much easier to provide a macro in the appropriate header file:
> 
> #define vlan_rx_add_vid_func(a,b,c) vlan_rx_add_vid_func(a,c)
> 
> johannes
> 

That's one way to handle it but I'm sure there will be other cases where
we might use coccinelle similarly, where a define might be tricky.
Luis asked me to do some cleanup on igb, so I'm exploring ways to make
it easier for other network drivers as well.

If for some reason cocci should have a problem with this the fallback
to a define is a good idea.

Thanks Johannes!

  Stefan

Re: cocci: multiple versions of function with different arguments

[Johannes Berg]

On Wed, 2014-04-23 at 13:13 +0200, Stefan Assmann wrote:

> > #define vlan_rx_add_vid_func(a,b,c) vlan_rx_add_vid_func(a,c)

> That's one way to handle it but I'm sure there will be other cases where
> we might use coccinelle similarly, where a define might be tricky.
> Luis asked me to do some cleanup on igb, so I'm exploring ways to make
> it easier for other network drivers as well.
> 
> If for some reason cocci should have a problem with this the fallback
> to a define is a good idea.

I don't disagree that we might have to solve this cocci problem, but I
would much prefer the define over the cocci patch as it leaves the
output code closer to the input code, which can be important for some
use cases (e.g. ours - development based on a backport tree.)

So even if we can figure out how to make such a thing work with cocci,
I'd argue that we should pick the define (in this particular case.)

johannes

Re: cocci: multiple versions of function with different arguments

[Julia Lawall]

On Wed, 23 Apr 2014, Stefan Assmann wrote:

> On 23.04.2014 12:41, Johannes Berg wrote:
> > On Wed, 2014-04-23 at 12:27 +0200, Stefan Assmann wrote:
> >
> >> I'm trying to generate a patch to define a function with different
> >> arguments depending on the kernel version. I'm having some difficulties,
> >> maybe you can tell me what's wrong with the following approach.
> >
> > I can't answer that, but given the premise
> >
> >> +#if LINUX_VERSION_CODE >= KERNEL_VERSION(3,10,0)
> >>         vlan_rx_add_vid_func(a, b, c)
> >> +#else
> >> +       vlan_rx_add_vid_func(a, c)
> >> +#endif
> >
> > and the fact that later kernel versions add an argument, wouldn't it be
> > much easier to provide a macro in the appropriate header file:
> >
> > #define vlan_rx_add_vid_func(a,b,c) vlan_rx_add_vid_func(a,c)
> >
> > johannes
> >
>
> That's one way to handle it but I'm sure there will be other cases where
> we might use coccinelle similarly, where a define might be tricky.
> Luis asked me to do some cleanup on igb, so I'm exploring ways to make
> it easier for other network drivers as well.
>
> If for some reason cocci should have a problem with this the fallback
> to a define is a good idea.
>
> Thanks Johannes!

I think that the problem is that the SmPL parser doesn't really understand
about the ifdef.  So it sees one expression right after another one, which
it is not happy about.  It would be fine if there were semicolons after
the function call.  If you can move up to the statement level in each
possible case, that might be a solution, but it might be too awkward in
practice.  If the macro solution is feasible, that could be better.  Fewer
changes to the source code as well.

julia

Re: cocci: multiple versions of function with different arguments

[Johannes Berg]

On Wed, 2014-04-23 at 14:07 +0200, Julia Lawall wrote:

> I think that the problem is that the SmPL parser doesn't really understand
> about the ifdef.  So it sees one expression right after another one, which
> it is not happy about.  It would be fine if there were semicolons after
> the function call.  If you can move up to the statement level in each
> possible case, that might be a solution, but it might be too awkward in
> practice.  If the macro solution is feasible, that could be better.  Fewer
> changes to the source code as well.

Yeah I was confused about all this as well - turns out that he's not
trying to do function calls but rather function declarations:

#if ...
static int foo(void *a, int b, char c)
#else
static int foo(void *a, char c)
#endif
{
 ...
}

or such.

I'm not sure how that would compile (it would require "b" being unused),
but still.

johannes

Re: cocci: multiple versions of function with different arguments

[Julia Lawall]

On Wed, 23 Apr 2014, Johannes Berg wrote:

> On Wed, 2014-04-23 at 14:07 +0200, Julia Lawall wrote:
>
> > I think that the problem is that the SmPL parser doesn't really understand
> > about the ifdef.  So it sees one expression right after another one, which
> > it is not happy about.  It would be fine if there were semicolons after
> > the function call.  If you can move up to the statement level in each
> > possible case, that might be a solution, but it might be too awkward in
> > practice.  If the macro solution is feasible, that could be better.  Fewer
> > changes to the source code as well.
>
> Yeah I was confused about all this as well - turns out that he's not
> trying to do function calls but rather function declarations:
>
> #if ...
> static int foo(void *a, int b, char c)
> #else
> static int foo(void *a, char c)
> #endif
> {
>  ...
> }
>
> or such.
>
> I'm not sure how that would compile (it would require "b" being unused),
> but still.

I can think  about it, but it is pretty far from the vision of Coccinelle,
which is that the combination of context code and + code should be a valid
syntactic unit, and on the side that we don't think much about ifdefs.

But I will think about it, because I can see that it is not a solvable
problem with the current technology.

thanks,
julia

Re: cocci: multiple versions of function with different arguments

[Johannes Berg]

On Wed, 2014-04-23 at 14:14 +0200, Julia Lawall wrote:

> I can think  about it, but it is pretty far from the vision of Coccinelle,
> which is that the combination of context code and + code should be a valid
> syntactic unit, and on the side that we don't think much about ifdefs.
> 
> But I will think about it, because I can see that it is not a solvable
> problem with the current technology.

Luis kinda solved it by introducing an intermediate function that would
call the original, and then #ifdef'ing the assignment of the function
instead.

(This is only a problem when there's a function assignment to start
with, otherwise there's likely no need to modify the function
declaration - this only comes up with function pointer APIs)

johannes

Re: cocci: multiple versions of function with different arguments

[Stefan Assmann]

On 23.04.2014 14:18, Johannes Berg wrote:
> On Wed, 2014-04-23 at 14:14 +0200, Julia Lawall wrote:
> 
>> I can think  about it, but it is pretty far from the vision of Coccinelle,
>> which is that the combination of context code and + code should be a valid
>> syntactic unit, and on the side that we don't think much about ifdefs.
>>
>> But I will think about it, because I can see that it is not a solvable
>> problem with the current technology.
> 
> Luis kinda solved it by introducing an intermediate function that would
> call the original, and then #ifdef'ing the assignment of the function
> instead.
> 
> (This is only a problem when there's a function assignment to start
> with, otherwise there's likely no need to modify the function
> declaration - this only comes up with function pointer APIs)
> 
> johannes
> 

Thanks Johannes and Julia. I think the missing semicolon is what caused
the issue. I'll do some tests and report back.

  Stefan

Re: cocci: multiple versions of function with different arguments

[Luis R. Rodriguez]

On Wed, Apr 23, 2014 at 4:16 AM, Johannes Berg
<johannes@sipsolutions.net> wrote:
>> If for some reason cocci should have a problem with this the fallback
>> to a define is a good idea.
>
> I don't disagree that we might have to solve this cocci problem, but I
> would much prefer the define over the cocci patch as it leaves the
> output code closer to the input code, which can be important for some
> use cases (e.g. ours - development based on a backport tree.)

Coccinelle has a slight overhead cost in comparison to using our
headers through defines, static inlines, or a new exported symbol on
the backports module (called compat now). The code generation cost is
is O(0) for a backport done through the headers / backports module,
using Coccinelle we have a slight overhead before compile time, during
code generation time. We want to minimize this as much as possible.
Because of this I would also prefer a define for the example use case
unless there's another reason that a define or a new symbol cannot be
used. We should use Coccinelle to try to solve backports for solutions
we didn't have strategies for, that is we should strive to backport
first through the backports module / header files, then as a second
step consider Coccinelle, and only if Coccinelle cannot solve our
backports we leave nasty legacy patches lying around.

  Luis

Re: cocci: multiple versions of function with different arguments

[Stefan Assmann]

On 23.04.2014 20:27, Luis R. Rodriguez wrote:
> On Wed, Apr 23, 2014 at 4:16 AM, Johannes Berg
> <johannes@sipsolutions.net> wrote:
>>> If for some reason cocci should have a problem with this the fallback
>>> to a define is a good idea.
>>
>> I don't disagree that we might have to solve this cocci problem, but I
>> would much prefer the define over the cocci patch as it leaves the
>> output code closer to the input code, which can be important for some
>> use cases (e.g. ours - development based on a backport tree.)
> 
> Coccinelle has a slight overhead cost in comparison to using our
> headers through defines, static inlines, or a new exported symbol on
> the backports module (called compat now). The code generation cost is
> is O(0) for a backport done through the headers / backports module,
> using Coccinelle we have a slight overhead before compile time, during
> code generation time. We want to minimize this as much as possible.
> Because of this I would also prefer a define for the example use case
> unless there's another reason that a define or a new symbol cannot be
> used. We should use Coccinelle to try to solve backports for solutions
> we didn't have strategies for, that is we should strive to backport
> first through the backports module / header files, then as a second
> step consider Coccinelle, and only if Coccinelle cannot solve our
> backports we leave nasty legacy patches lying around.

If we use use the define I think we still have to patch the individual
driver because we're trying to assign a function pointer with 3
arguments when there are only 2 expected. I could be wrong though.


What I'm trying to do here is deal with, for example, new (or changed)
function pointers in struct net_device_ops. So instead of patching
every driver manually we could have a cocci patch that changes/ifdefs
all drivers at once for specific kernel versions.

Here's an example.
80d5c3689b886308247da295a228a54df49a44f6 introduces the following change
to struct net_device_ops.
        int                     (*ndo_vlan_rx_add_vid)(struct net_device *dev,
-                                                      unsigned short vid);
+                                                      __be16 proto, u16 vid);

Ultimately what I'd like to see is automagically
- change/ifdef function calls in all drivers
- change/ifdef function declarations/prototypes in all drivers
- adapt code? That's a maybe, my cocci skill is still pretty low


Yes there will be overhead, it's not for free. But there's potential to
ease adding of new drivers and reduce overall maintenance overhead.
Also if I look at the recent changes, we might run cocci in parallel
which should reduce the runtime overhead further.

Here's another example.
5f8444a3fa617076f8da51a3e8ecce01a5d7f738 adds function pointer
ndo_set_vf_spoofchk to struct net_device_ops in kernel 3.2. In previous
kernel we might be able to get away by just commenting out everything
related to ndo_set_vf_spoofchk in the drivers with a cocci patch.

  Stefan

Re: cocci: multiple versions of function with different arguments

[Luis R. Rodriguez]

On Thu, Apr 24, 2014 at 4:42 AM, Stefan Assmann <sassmann@kpanic.de> wrote:
> Here's an example.
> 80d5c3689b886308247da295a228a54df49a44f6 introduces the following change
> to struct net_device_ops.
>         int                     (*ndo_vlan_rx_add_vid)(struct net_device *dev,
> -                                                      unsigned short vid);
> +                                                      __be16 proto, u16 vid);


Yeah that seems reasonable to use Coccinelle for!

  Luis