[Cocci] Substitution of function call to structure parameter

[Cocci] Substitution of function call to structure parameter

[Maxime Ripard]

[-- Attachment #1.1: Type: text/plain, Size: 1913 bytes --]

Hi,

I'm trying to do an API rework of DRM, and that rewrite involves
patching a number of drivers to use a structure field instead of a
funtion call.

There's a bunch of cases that need to be covered, and I can't get all
of them to work.

So far, my current script is, with the current shortcomings as comment
below each rule.

@@
identifier fn;
identifier dev, cmd;
@@

fn (struct drm_device *dev,
    ...,
    struct drm_mode_fb_cmd2 *cmd,
    ...)
{
+ const struct drm_format_info *info = drm_get_format_info(dev, cmd);
...
- drm_format_num_planes(cmd->pixel_format)
+ info->num_planes
...
}

// This on works on most cases, ie:
// https://elixir.bootlin.com/linux/v5.0/source/drivers/gpu/drm/armada/armada_fb.c#L87
// However, for some reason unknown to me, it doesn't match:
// https://elixir.bootlin.com/linux/v5.0/source/drivers/gpu/drm/tegra/fb.c#L129

@@
identifier fb;
@@
...
struct drm_framebuffer *fb;
...
- drm_format_num_planes(fb->format->format)
+ fb->format->num_planes

// This one seems to work properly

@@
expression arg;
identifier fb;
@@
...
struct drm_framebuffer *fb;
...
- drm_format_num_planes(arg)
+ fb->format->num_planes

// This one seems to work in some cases, such as
// https://elixir.bootlin.com/linux/v5.0/source/drivers/gpu/drm/vc4/vc4_plane.c#L490
// But it also matches in cases where fb hasn't been properly assigned before, such as:
// https://elixir.bootlin.com/linux/v5.0/source/drivers/gpu/drm/tegra/fb.c#L142

@@
identifier info;
@@
...
struct drm_format_info *info;
...
- drm_format_num_planes(info->format)
+ info->num_planes

// This one seems to work too

I'm pretty new to coccinelle, so I'm not quite sure how to fix these
errors properly, even though that looks pretty simple.

Thanks!
Maxime

-- 
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[-- Attachment #2: Type: text/plain, Size: 136 bytes --]

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

Re: [Cocci] Substitution of function call to structure parameter

[Julia Lawall]

On Tue, 12 Mar 2019, Maxime Ripard wrote:

> Hi,
>
> I'm trying to do an API rework of DRM, and that rewrite involves
> patching a number of drivers to use a structure field instead of a
> funtion call.
>
> There's a bunch of cases that need to be covered, and I can't get all
> of them to work.
>
> So far, my current script is, with the current shortcomings as comment
> below each rule.
>
> @@
> identifier fn;
> identifier dev, cmd;
> @@
>
> fn (struct drm_device *dev,
>     ...,
>     struct drm_mode_fb_cmd2 *cmd,
>     ...)
> {
> + const struct drm_format_info *info = drm_get_format_info(dev, cmd);
> ...
> - drm_format_num_planes(cmd->pixel_format)
> + info->num_planes
> ...
> }
>
> // This on works on most cases, ie:
> // https://elixir.bootlin.com/linux/v5.0/source/drivers/gpu/drm/armada/armada_fb.c#L87
> // However, for some reason unknown to me, it doesn't match:
> // https://elixir.bootlin.com/linux/v5.0/source/drivers/gpu/drm/tegra/fb.c#L129

... means by default that what comes before or after the ... should not
appear in the ...  fb.c has the call in a loop, so it happens over and
over (... is following paths in the control-flow graph, not the AST.

Replace the first ... by <+... and the second one by ...+>.  That will
replace all calls, ensuring that there is at least once to motivate the
need for adding the declaration of info.


>
> @@
> identifier fb;
> @@
> ...
> struct drm_framebuffer *fb;
> ...
> - drm_format_num_planes(fb->format->format)
> + fb->format->num_planes
>
> // This one seems to work properly

How about:

@@
struct drm_framebuffer *fb;
@@

- drm_format_num_planes(fb->format->format)
+ fb->format->num_planes

That is, you don't need to insist on there being a local variable
declaration, you just want an expression of the right type.  This will
also be much more efficient.

> @@
> expression arg;
> identifier fb;
> @@
> ...
> struct drm_framebuffer *fb;
> ...
> - drm_format_num_planes(arg)
> + fb->format->num_planes
>
> // This one seems to work in some cases, such as
> // https://elixir.bootlin.com/linux/v5.0/source/drivers/gpu/drm/vc4/vc4_plane.c#L490
> // But it also matches in cases where fb hasn't been properly assigned before, such as:
> // https://elixir.bootlin.com/linux/v5.0/source/drivers/gpu/drm/tegra/fb.c#L142

OK, it looks like what you want is:

@@
struct drm_framebuffer *fb;
expression e.
@@

fb = e;
<...
- drm_format_num_planes(arg)
+ fb->format->num_planes
...>

That is, you find an assignment of fb, and then anywhere after that you
have a call, you can replace it.  This is <... ...> rather than <+...
...+> so that it can match several 0 or more occurrences.

>
> @@
> identifier info;
> @@
> ...
> struct drm_format_info *info;
> ...
> - drm_format_num_planes(info->format)
> + info->num_planes
>
> // This one seems to work too

Still it would be better to update it as suggested above.

julia

> I'm pretty new to coccinelle, so I'm not quite sure how to fix these
> errors properly, even though that looks pretty simple.
>
> Thanks!
> Maxime
>
> --
> Maxime Ripard, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com
>
_______________________________________________
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci

Re: [Cocci] Substitution of function call to structure parameter

[Maxime Ripard]

[-- Attachment #1.1: Type: text/plain, Size: 2805 bytes --]

Hi Julia,

Thanks a lot for your answer,

On Tue, Mar 12, 2019 at 10:03:57AM +0100, Julia Lawall wrote:
> > @@
> > expression arg;
> > identifier fb;
> > @@
> > ...
> > struct drm_framebuffer *fb;
> > ...
> > - drm_format_num_planes(arg)
> > + fb->format->num_planes
> >
> > // This one seems to work in some cases, such as
> > // https://elixir.bootlin.com/linux/v5.0/source/drivers/gpu/drm/vc4/vc4_plane.c#L490
> > // But it also matches in cases where fb hasn't been properly assigned before, such as:
> > // https://elixir.bootlin.com/linux/v5.0/source/drivers/gpu/drm/tegra/fb.c#L142
> 
> OK, it looks like what you want is:
> 
> @@
> struct drm_framebuffer *fb;
> expression e.
> @@
> 
> fb = e;
> <...
> - drm_format_num_planes(arg)
> + fb->format->num_planes
> ...>
> 
> That is, you find an assignment of fb, and then anywhere after that you
> have a call, you can replace it.  This is <... ...> rather than <+...
> ...+> so that it can match several 0 or more occurrences.

It looks however that there's a difference between a variable
declaration and assignment, and only an assignment.

The snippet above doesn't match
https://elixir.bootlin.com/linux/v5.0/source/drivers/gpu/drm/vc4/vc4_plane.c#L490

Whereas using

@@
identifier fb;
expression arg;
expression e;
@@

  struct drm_framebuffer *fb = e;
  <...
- drm_format_num_planes(arg)
+ fb->format->num_planes
  ...>

Work for example. Is there a way to match both an assignment and a
declaration + assignment?

The following script covers all cases now:

@@
identifier fn;
identifier dev, cmd;
@@

fn (struct drm_device *dev,
    ...,
    struct drm_mode_fb_cmd2 *cmd,
    ...)
{
+ const struct drm_format_info *info = drm_get_format_info(dev, cmd);
  <+...
- drm_format_num_planes(cmd->pixel_format)
+ info->num_planes
  ...+>
}    

@@
struct drm_framebuffer *fb;
@@

- drm_format_num_planes(fb->format->format)
+ fb->format->num_planes

@@
identifier fb;
expression arg;
expression e;
@@

  struct drm_framebuffer *fb = e;
  <...
- drm_format_num_planes(arg)
+ fb->format->num_planes
  ...>

@@
struct drm_format_info *info;
@@

- drm_format_num_planes(info->format)
+ info->num_planes

@@
identifier val;
expression arg;
@@

{
+ const struct drm_format_info *info;
  <+...
- val = drm_format_num_planes(arg);
+ info = drm_format_info(arg);
+ val = info->num_planes;
  ...+>
}

@ rfunc @
identifier f = drm_format_num_planes;
identifier fourcc;
typedef uint32_t;
@@

- int f(uint32_t fourcc)
- {
- ...
- }

@@
identifier rfunc.f;
declarer name EXPORT_SYMBOL;
@@

- EXPORT_SYMBOL(f);

Thanks again!
Maxime

-- 
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[-- Attachment #2: Type: text/plain, Size: 136 bytes --]

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

Re: [Cocci] Substitution of function call to structure parameter

[Julia Lawall]

On Tue, 12 Mar 2019, Maxime Ripard wrote:

> Hi Julia,
>
> Thanks a lot for your answer,
>
> On Tue, Mar 12, 2019 at 10:03:57AM +0100, Julia Lawall wrote:
> > > @@
> > > expression arg;
> > > identifier fb;
> > > @@
> > > ...
> > > struct drm_framebuffer *fb;
> > > ...
> > > - drm_format_num_planes(arg)
> > > + fb->format->num_planes
> > >
> > > // This one seems to work in some cases, such as
> > > // https://elixir.bootlin.com/linux/v5.0/source/drivers/gpu/drm/vc4/vc4_plane.c#L490
> > > // But it also matches in cases where fb hasn't been properly assigned before, such as:
> > > // https://elixir.bootlin.com/linux/v5.0/source/drivers/gpu/drm/tegra/fb.c#L142
> >
> > OK, it looks like what you want is:
> >
> > @@
> > struct drm_framebuffer *fb;
> > expression e.
> > @@
> >
> > fb = e;
> > <...
> > - drm_format_num_planes(arg)
> > + fb->format->num_planes
> > ...>
> >
> > That is, you find an assignment of fb, and then anywhere after that you
> > have a call, you can replace it.  This is <... ...> rather than <+...
> > ...+> so that it can match several 0 or more occurrences.
>
> It looks however that there's a difference between a variable
> declaration and assignment, and only an assignment.
>
> The snippet above doesn't match
> https://elixir.bootlin.com/linux/v5.0/source/drivers/gpu/drm/vc4/vc4_plane.c#L490
>
> Whereas using
>
> @@
> identifier fb;
> expression arg;
> expression e;
> @@
>
>   struct drm_framebuffer *fb = e;
>   <...
> - drm_format_num_planes(arg)
> + fb->format->num_planes
>   ...>
>
> Work for example. Is there a way to match both an assignment and a
> declaration + assignment?

Try dropping the ; on the assignment fb = e

>
> The following script covers all cases now:

Great!

julia

> @@
> identifier fn;
> identifier dev, cmd;
> @@
>
> fn (struct drm_device *dev,
>     ...,
>     struct drm_mode_fb_cmd2 *cmd,
>     ...)
> {
> + const struct drm_format_info *info = drm_get_format_info(dev, cmd);
>   <+...
> - drm_format_num_planes(cmd->pixel_format)
> + info->num_planes
>   ...+>
> }
>
> @@
> struct drm_framebuffer *fb;
> @@
>
> - drm_format_num_planes(fb->format->format)
> + fb->format->num_planes
>
> @@
> identifier fb;
> expression arg;
> expression e;
> @@
>
>   struct drm_framebuffer *fb = e;
>   <...
> - drm_format_num_planes(arg)
> + fb->format->num_planes
>   ...>
>
> @@
> struct drm_format_info *info;
> @@
>
> - drm_format_num_planes(info->format)
> + info->num_planes
>
> @@
> identifier val;
> expression arg;
> @@
>
> {
> + const struct drm_format_info *info;
>   <+...
> - val = drm_format_num_planes(arg);
> + info = drm_format_info(arg);
> + val = info->num_planes;
>   ...+>
> }
>
> @ rfunc @
> identifier f = drm_format_num_planes;
> identifier fourcc;
> typedef uint32_t;
> @@
>
> - int f(uint32_t fourcc)
> - {
> - ...
> - }
>
> @@
> identifier rfunc.f;
> declarer name EXPORT_SYMBOL;
> @@
>
> - EXPORT_SYMBOL(f);
>
> Thanks again!
> Maxime
>
> --
> Maxime Ripard, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com
>
_______________________________________________
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci

Re: [Cocci] Substitution of function call to structure parameter

[Maxime Ripard]

[-- Attachment #1.1: Type: text/plain, Size: 2234 bytes --]



On Tue, Mar 12, 2019 at 12:20:49PM +0100, Julia Lawall wrote:
> 
> 
> On Tue, 12 Mar 2019, Maxime Ripard wrote:
> 
> > Hi Julia,
> >
> > Thanks a lot for your answer,
> >
> > On Tue, Mar 12, 2019 at 10:03:57AM +0100, Julia Lawall wrote:
> > > > @@
> > > > expression arg;
> > > > identifier fb;
> > > > @@
> > > > ...
> > > > struct drm_framebuffer *fb;
> > > > ...
> > > > - drm_format_num_planes(arg)
> > > > + fb->format->num_planes
> > > >
> > > > // This one seems to work in some cases, such as
> > > > // https://elixir.bootlin.com/linux/v5.0/source/drivers/gpu/drm/vc4/vc4_plane.c#L490
> > > > // But it also matches in cases where fb hasn't been properly assigned before, such as:
> > > > // https://elixir.bootlin.com/linux/v5.0/source/drivers/gpu/drm/tegra/fb.c#L142
> > >
> > > OK, it looks like what you want is:
> > >
> > > @@
> > > struct drm_framebuffer *fb;
> > > expression e.
> > > @@
> > >
> > > fb = e;
> > > <...
> > > - drm_format_num_planes(arg)
> > > + fb->format->num_planes
> > > ...>
> > >
> > > That is, you find an assignment of fb, and then anywhere after that you
> > > have a call, you can replace it.  This is <... ...> rather than <+...
> > > ...+> so that it can match several 0 or more occurrences.
> >
> > It looks however that there's a difference between a variable
> > declaration and assignment, and only an assignment.
> >
> > The snippet above doesn't match
> > https://elixir.bootlin.com/linux/v5.0/source/drivers/gpu/drm/vc4/vc4_plane.c#L490
> >
> > Whereas using
> >
> > @@
> > identifier fb;
> > expression arg;
> > expression e;
> > @@
> >
> >   struct drm_framebuffer *fb = e;
> >   <...
> > - drm_format_num_planes(arg)
> > + fb->format->num_planes
> >   ...>
> >
> > Work for example. Is there a way to match both an assignment and a
> > declaration + assignment?
> 
> Try dropping the ; on the assignment fb = e

It worked, thanks!

What is the meaning of the semi-column in that case? The obvious would
be that it matches an end of line, but given the behaviour shown
above, I guess there's more to it?

Maxime

-- 
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[-- Attachment #2: Type: text/plain, Size: 136 bytes --]

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

Re: [Cocci] Substitution of function call to structure parameter

[Maxime Ripard]

[-- Attachment #1.1: Type: text/plain, Size: 1469 bytes --]

On Tue, Mar 12, 2019 at 10:03:57AM +0100, Julia Lawall wrote:
> >
> > @@
> > identifier fb;
> > @@
> > ...
> > struct drm_framebuffer *fb;
> > ...
> > - drm_format_num_planes(fb->format->format)
> > + fb->format->num_planes
> >
> > // This one seems to work properly
> 
> How about:
> 
> @@
> struct drm_framebuffer *fb;
> @@
> 
> - drm_format_num_planes(fb->format->format)
> + fb->format->num_planes
> 
> That is, you don't need to insist on there being a local variable
> declaration, you just want an expression of the right type.  This will
> also be much more efficient.

I have an extra question on that one. Are function parameters also
considered expressions with that syntax?

Let's say for example, I want to replace the call to drm_format_info
by drm_get_format_info in that function:
https://elixir.bootlin.com/linux/v5.0/source/drivers/gpu/drm/omapdrm/omap_fb.c#L340

That snippet doesn't work:
@@
struct drm_device *dev;
struct drm_mode_fb_cmd2 *cmd;
@@

- drm_format_info(cmd->pixel_format)
+ drm_get_format_info(dev, cmd)

with spatch producing the following warnings and errors:

warning: rule starting on line 1: metavariable dev not used in the - or context code
error: rule starting on line 1: dev appears only in + code

Does that mean I have to explicitly have the function prototype as
context?

Maxime

-- 
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[-- Attachment #2: Type: text/plain, Size: 136 bytes --]

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

Re: [Cocci] Substitution of function call to structure parameter

[Julia Lawall]

> > Try dropping the ; on the assignment fb = e
>
> It worked, thanks!
>
> What is the meaning of the semi-column in that case? The obvious would
> be that it matches an end of line, but given the behaviour shown
> above, I guess there's more to it?

In SmPL you specify complete terms.  For example, you can't just write

if (...)

You have to put at least a then branch.

Likewise, if you have e1 = e2;, the only complete term it matches is a
complete top-level assignment.  It can't match the last n tokens of a
declaration.  On the other hand e1 = e2 is an expression.  It can be part
of other things.

If you think a bit about my explanation, you will quickly see that it is
not quite right, because int x = 12; doesn't have a subterm of the form x
= 12 according to the grammar of C.  So there is a small internal hack to
get around that issue.

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

Re: [Cocci] Substitution of function call to structure parameter

[Julia Lawall]

On Tue, 12 Mar 2019, Maxime Ripard wrote:

> On Tue, Mar 12, 2019 at 10:03:57AM +0100, Julia Lawall wrote:
> > >
> > > @@
> > > identifier fb;
> > > @@
> > > ...
> > > struct drm_framebuffer *fb;
> > > ...
> > > - drm_format_num_planes(fb->format->format)
> > > + fb->format->num_planes
> > >
> > > // This one seems to work properly
> >
> > How about:
> >
> > @@
> > struct drm_framebuffer *fb;
> > @@
> >
> > - drm_format_num_planes(fb->format->format)
> > + fb->format->num_planes
> >
> > That is, you don't need to insist on there being a local variable
> > declaration, you just want an expression of the right type.  This will
> > also be much more efficient.
>
> I have an extra question on that one. Are function parameters also
> considered expressions with that syntax?
>
> Let's say for example, I want to replace the call to drm_format_info
> by drm_get_format_info in that function:
> https://elixir.bootlin.com/linux/v5.0/source/drivers/gpu/drm/omapdrm/omap_fb.c#L340
>
> That snippet doesn't work:
> @@
> struct drm_device *dev;
> struct drm_mode_fb_cmd2 *cmd;
> @@
>
> - drm_format_info(cmd->pixel_format)
> + drm_get_format_info(dev, cmd)
>
> with spatch producing the following warnings and errors:
>
> warning: rule starting on line 1: metavariable dev not used in the - or context code
> error: rule starting on line 1: dev appears only in + code
>
> Does that mean I have to explicitly have the function prototype as
> context?

It has nothing to do with function parameters.  You have plus code that
refers to a metavariable (dev) that doesn't appear in the - code.  So
Coccinelle has no idea what you want it to be.

There is no shortcut to match both a local variable and a function
parameter.  You need to have a specific rule matching the form of the
function:

f(...,struct drm_device *dev,...) {
<+...
- drm_format_info(cmd->pixel_format)
+ drm_get_format_info(dev, cmd)
...+>
}

You could use either <... ...> or <+... ...+>.  <+... ...+> indicates that
a call to drm_format_info is required, which will cause Coccinele to
mostly skip processing of functions that don't contain it.

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