[Cocci] semantic patch feasibility

[Cocci] semantic patch feasibility

[Javier Martinez Canillas]

Hello,

I'm a Linux kernel developer doing a big refactoring on the GPIO subsytem.

I was wondering if the change I'm doing manually could be automated by
using cocinelle but I've no experience neither writing semantic
patches nor the SmPL language grammar.

So I wanted to ask people familiar with cocinelle whether is feasible
or not to model my change as a semantic patch so I can start reading
cocinelle papers and looking at examples or if is too complex for
cocinelle then I'll continue grepping and manually change as I've been
doing so far.

I'm sending as an attachment a very incomplete patch so you can get an
idea of the type of refactoring that I'm ding.

Thanks a lot for your help and best regards,
Javier
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-gpio-add-struct-gpio_chip_ops-to-abstract-GPIO-contr.patch
Type: text/x-patch
Size: 55538 bytes
Desc: not available
URL: <https://systeme.lip6.fr/pipermail/cocci/attachments/20140425/6da5b328/attachment-0001.bin>

[Cocci] semantic patch feasibility

[SF Markus Elfring]

> I was wondering if the change I'm doing manually could be automated by
> using cocinelle but I've no experience neither writing semantic
> patches nor the SmPL language grammar.

I would like to point out a general consideration.

Semantic patches can be developed for some use cases. I find that
corresponding efforts are only useful if such a generalised patch can be
applied to a source code base several times.
I doubt that you want to try it out for a single source code adjustment.
It might be that a need will appear to perform a transformation once
more a bit later, doesn't it?

Regards,
Markus

[Cocci] semantic patch feasibility

[Julia Lawall]

On Fri, 25 Apr 2014, Javier Martinez Canillas wrote:

> Hello,
>
> I'm a Linux kernel developer doing a big refactoring on the GPIO subsytem.
>
> I was wondering if the change I'm doing manually could be automated by
> using cocinelle but I've no experience neither writing semantic
> patches nor the SmPL language grammar.

It looks quite possible.  In looking at the code, though, I wasn't sure
what is your strategy for where to place the new structure definition, in
the case where there was no structure before.  Would it be OK to put it
next to the probe function?

julia

[Cocci] semantic patch feasibility

[Julia Lawall]

On Fri, 25 Apr 2014, SF Markus Elfring wrote:

> > I was wondering if the change I'm doing manually could be automated by
> > using cocinelle but I've no experience neither writing semantic
> > patches nor the SmPL language grammar.
>
> I would like to point out a general consideration.
>
> Semantic patches can be developed for some use cases. I find that
> corresponding efforts are only useful if such a generalised patch can be
> applied to a source code base several times.
> I doubt that you want to try it out for a single source code adjustment.
> It might be that a need will appear to perform a transformation once
> more a bit later, doesn't it?

I believe that he has hundreds of use cases.  The attached patch
illustrated a number of them.

julia

[Cocci] semantic patch feasibility

[SF Markus Elfring]

> The attached patch illustrated a number of them.

How do you see the chances for generalisation of this concrete example?

Regards,
Markus

[Cocci] semantic patch feasibility

[Wolfram Sang]

> So I wanted to ask people familiar with cocinelle whether is feasible
> or not to model my change as a semantic patch so I can start reading
> cocinelle papers and looking at examples or if is too complex for
> cocinelle then I'll continue grepping and manually change as I've been
> doing so far.

I would do it with Coccinelle. First learning curve may be steep, but
once mastered, a very flexible tool, if adjustments are needed (e.g.
because proposed API changes).

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <https://systeme.lip6.fr/pipermail/cocci/attachments/20140425/dffd5fae/attachment.asc>

[Cocci] semantic patch feasibility

[Julia Lawall]

On Fri, 25 Apr 2014, SF Markus Elfring wrote:

> > The attached patch illustrated a number of them.
>
> How do you see the chances for generalisation of this concrete example?

I think it should be quite straightforward.  One either takes the
assignments out of the function and puts them into a structure
initializer, or if there is a structure initializer already, one cuts it
up into what is wanted for each of the separate functions.

Maybe creating the name of the new structure is an issue?  I didn't look
carefully enough to see if there was a pattern.

julia

[Cocci] semantic patch feasibility

[Javier Martinez Canillas]

Hello Markus,

On Fri, Apr 25, 2014 at 4:30 PM, SF Markus Elfring
<elfring@users.sourceforge.net> wrote:
>> I was wondering if the change I'm doing manually could be automated by
>> using cocinelle but I've no experience neither writing semantic
>> patches nor the SmPL language grammar.
>
> I would like to point out a general consideration.
>
> Semantic patches can be developed for some use cases. I find that
> corresponding efforts are only useful if such a generalised patch can be
> applied to a source code base several times.
> I doubt that you want to try it out for a single source code adjustment.
> It might be that a need will appear to perform a transformation once
> more a bit later, doesn't it?
>

Thanks a lot for your feedback.

I understand the the primary use for cocinelle could be to have
generalized patches that can detect common semantic errors to have a
flexible and extensible static analysis.

In this case I'm interested in a different use case though. I'm doing
a kernel wide change on a core API used by a lot of drivers (~250) so
is a lot of work to do this manually and I can miss a driver when
grepping the code. And even if I manage to do all the manual changes
(I've already changed a bunch of them), I don't have cross-compiler to
at least build test the drivers for every single architecture to find
out if I made a silly programming mistake.

So I think that it is much safer to express the change as a semantic
patch even if the change is going to be made only once, in order to
avoid introducing any build regressions.

Best regards,
Javier

[Cocci] semantic patch feasibility

[Javier Martinez Canillas]

Hello Julia,

On Fri, Apr 25, 2014 at 6:23 PM, Julia Lawall <julia.lawall@lip6.fr> wrote:
> On Fri, 25 Apr 2014, Javier Martinez Canillas wrote:
>
>> Hello,
>>
>> I'm a Linux kernel developer doing a big refactoring on the GPIO subsytem.
>>
>> I was wondering if the change I'm doing manually could be automated by
>> using cocinelle but I've no experience neither writing semantic
>> patches nor the SmPL language grammar.
>
> It looks quite possible.  In looking at the code, though, I wasn't sure
> what is your strategy for where to place the new structure definition, in
> the case where there was no structure before.  Would it be OK to put it
> next to the probe function?
>
> julia

Thanks a lot for taking the time to look at this!

I've been adding the new structure definition right after all the
functions have been defined since that is a common pattern found in
other kernel subsystems.

But since this is a cleanup and if is going to be easier to define the
semantic patch by placing the structure before the probe function then
that works for me too and later drivers maintainers can send
incremental patches to change wherever they find more suitable.

Best regards,
Javier

[Cocci] semantic patch feasibility

[Javier Martinez Canillas]

On Fri, Apr 25, 2014 at 6:24 PM, Julia Lawall <julia.lawall@lip6.fr> wrote:
>
>
> On Fri, 25 Apr 2014, SF Markus Elfring wrote:
>
>> > I was wondering if the change I'm doing manually could be automated by
>> > using cocinelle but I've no experience neither writing semantic
>> > patches nor the SmPL language grammar.
>>
>> I would like to point out a general consideration.
>>
>> Semantic patches can be developed for some use cases. I find that
>> corresponding efforts are only useful if such a generalised patch can be
>> applied to a source code base several times.
>> I doubt that you want to try it out for a single source code adjustment.
>> It might be that a need will appear to perform a transformation once
>> more a bit later, doesn't it?
>
> I believe that he has hundreds of use cases.  The attached patch
> illustrated a number of them.
>
> julia

Exactly, there are much more drivers than the ones I already modified
in the attached patched. It was just to give you an example of the
level of complexity (or not) of the change I'm doing.

Best regards,
Javier

[Cocci] semantic patch feasibility

[Javier Martinez Canillas]

Hello Julia,

On Fri, Apr 25, 2014 at 8:03 PM, Julia Lawall <julia.lawall@lip6.fr> wrote:
>
>
> On Fri, 25 Apr 2014, SF Markus Elfring wrote:
>
>> > The attached patch illustrated a number of them.
>>
>> How do you see the chances for generalisation of this concrete example?
>
> I think it should be quite straightforward.  One either takes the
> assignments out of the function and puts them into a structure
> initializer, or if there is a structure initializer already, one cuts it
> up into what is wanted for each of the separate functions.
>
> Maybe creating the name of the new structure is an issue?  I didn't look
> carefully enough to see if there was a pattern.
>
> julia

I'm glad to know that should be straightforward. To name the new
structure I've been trying to deduce the naming scheme that the
original driver author used to name the GPIO operations and following
that convention.

But if for the sake of simplicity we use a standard name that will be
used on all drivers, that's ok too. After all the structures are
static and later drivers maintainers can rename the structure if they
want a more suitable name.

Thanks a lot to Markus and you for the feedback. Now that I know that
is possible, I'll study the SmPL grammar, look at the examples and ask
silly questions here if I find issues :-)

Best regards,
Javier

[Cocci] semantic patch feasibility

[SF Markus Elfring]

> I'm doing a kernel wide change on a core API used by a lot of drivers (~250)
> so is a lot of work to do this manually and I can miss a driver when
> grepping the code.

How do you identify your update candidates?
Have you got any specific filter pattern in mind already?

Your task will become more "interesting" if you are going to specify the
precise query on affected source files, won't it?

Regards,
Markus

[Cocci] semantic patch feasibility

[Julia Lawall]

On Fri, 25 Apr 2014, Javier Martinez Canillas wrote:

> Hello Julia,
> 
> On Fri, Apr 25, 2014 at 6:23 PM, Julia Lawall <julia.lawall@lip6.fr> wrote:
> > On Fri, 25 Apr 2014, Javier Martinez Canillas wrote:
> >
> >> Hello,
> >>
> >> I'm a Linux kernel developer doing a big refactoring on the GPIO subsytem.
> >>
> >> I was wondering if the change I'm doing manually could be automated by
> >> using cocinelle but I've no experience neither writing semantic
> >> patches nor the SmPL language grammar.
> >
> > It looks quite possible.  In looking at the code, though, I wasn't sure
> > what is your strategy for where to place the new structure definition, in
> > the case where there was no structure before.  Would it be OK to put it
> > next to the probe function?
> >
> > julia
> 
> Thanks a lot for taking the time to look at this!
> 
> I've been adding the new structure definition right after all the
> functions have been defined since that is a common pattern found in
> other kernel subsystems.
> 
> But since this is a cleanup and if is going to be easier to define the
> semantic patch by placing the structure before the probe function then
> that works for me too and later drivers maintainers can send
> incremental patches to change wherever they find more suitable.

Well, you could also put it after the probe function.

You could also find the ending position of all of the functions and put it 
after the last one, but that could be a bit complicated.

julia

[Cocci] semantic patch feasibility

[Julia Lawall]

On Fri, 25 Apr 2014, Javier Martinez Canillas wrote:

> Hello Julia,
> 
> On Fri, Apr 25, 2014 at 8:03 PM, Julia Lawall <julia.lawall@lip6.fr> wrote:
> >
> >
> > On Fri, 25 Apr 2014, SF Markus Elfring wrote:
> >
> >> > The attached patch illustrated a number of them.
> >>
> >> How do you see the chances for generalisation of this concrete example?
> >
> > I think it should be quite straightforward.  One either takes the
> > assignments out of the function and puts them into a structure
> > initializer, or if there is a structure initializer already, one cuts it
> > up into what is wanted for each of the separate functions.
> >
> > Maybe creating the name of the new structure is an issue?  I didn't look
> > carefully enough to see if there was a pattern.
> >
> > julia
> 
> I'm glad to know that should be straightforward. To name the new
> structure I've been trying to deduce the naming scheme that the
> original driver author used to name the GPIO operations and following
> that convention.
> 
> But if for the sake of simplicity we use a standard name that will be
> used on all drivers, that's ok too. After all the structures are
> static and later drivers maintainers can rename the structure if they
> want a more suitable name.

I thought you could look for the get or set field, see what it is assigned 
to, and make a new name from that one.  For maximum flexibility, you can 
make new names in ocaml or python code.  You can see how to do this in the 
file demos/pythontococci.cocci.

julia

[Cocci] semantic patch feasibility

[Julia Lawall]

On Fri, 25 Apr 2014, SF Markus Elfring wrote:

> > I'm doing a kernel wide change on a core API used by a lot of drivers (~250)
> > so is a lot of work to do this manually and I can miss a driver when
> > grepping the code.
> 
> How do you identify your update candidates?
> Have you got any specific filter pattern in mind already?
> 
> Your task will become more "interesting" if you are going to specify the
> precise query on affected source files, won't it?

I think that it is quite straightforward. He makes a rule that looks for 
the initializations of the structure fields, and then moves those 
initializations to the new structure.

julia

[Cocci] semantic patch feasibility

[Julia Lawall]

On Fri, 25 Apr 2014, Javier Martinez Canillas wrote:

> Hello Markus,
> 
> On Fri, Apr 25, 2014 at 4:30 PM, SF Markus Elfring
> <elfring@users.sourceforge.net> wrote:
> >> I was wondering if the change I'm doing manually could be automated by
> >> using cocinelle but I've no experience neither writing semantic
> >> patches nor the SmPL language grammar.
> >
> > I would like to point out a general consideration.
> >
> > Semantic patches can be developed for some use cases. I find that
> > corresponding efforts are only useful if such a generalised patch can be
> > applied to a source code base several times.
> > I doubt that you want to try it out for a single source code adjustment.
> > It might be that a need will appear to perform a transformation once
> > more a bit later, doesn't it?
> >
> 
> Thanks a lot for your feedback.
> 
> I understand the the primary use for cocinelle could be to have
> generalized patches that can detect common semantic errors to have a
> flexible and extensible static analysis.

Making this kind of frequent change is a perfectly reasonable, indeed 
intended, use of Coccinelle.

julia


> In this case I'm interested in a different use case though. I'm doing
> a kernel wide change on a core API used by a lot of drivers (~250) so
> is a lot of work to do this manually and I can miss a driver when
> grepping the code. And even if I manage to do all the manual changes
> (I've already changed a bunch of them), I don't have cross-compiler to
> at least build test the drivers for every single architecture to find
> out if I made a silly programming mistake.
> 
> So I think that it is much safer to express the change as a semantic
> patch even if the change is going to be made only once, in order to
> avoid introducing any build regressions.
> 
> Best regards,
> Javier
> _______________________________________________
> Cocci mailing list
> Cocci at systeme.lip6.fr
> https://systeme.lip6.fr/mailman/listinfo/cocci
> 

[Cocci] semantic patch feasibility

[Javier Martinez Canillas]

Hello Julia,

On Fri, Apr 25, 2014 at 11:25 PM, Julia Lawall <julia.lawall@lip6.fr> wrote:
>
>
> On Fri, 25 Apr 2014, Javier Martinez Canillas wrote:
>
>> Hello Julia,
>>
>> On Fri, Apr 25, 2014 at 6:23 PM, Julia Lawall <julia.lawall@lip6.fr> wrote:
>> > On Fri, 25 Apr 2014, Javier Martinez Canillas wrote:
>> >
>> >> Hello,
>> >>
>> >> I'm a Linux kernel developer doing a big refactoring on the GPIO subsytem.
>> >>
>> >> I was wondering if the change I'm doing manually could be automated by
>> >> using cocinelle but I've no experience neither writing semantic
>> >> patches nor the SmPL language grammar.
>> >
>> > It looks quite possible.  In looking at the code, though, I wasn't sure
>> > what is your strategy for where to place the new structure definition, in
>> > the case where there was no structure before.  Would it be OK to put it
>> > next to the probe function?
>> >
>> > julia
>>
>> Thanks a lot for taking the time to look at this!
>>
>> I've been adding the new structure definition right after all the
>> functions have been defined since that is a common pattern found in
>> other kernel subsystems.
>>
>> But since this is a cleanup and if is going to be easier to define the
>> semantic patch by placing the structure before the probe function then
>> that works for me too and later drivers maintainers can send
>> incremental patches to change wherever they find more suitable.
>
> Well, you could also put it after the probe function.
>
> You could also find the ending position of all of the functions and put it
> after the last one, but that could be a bit complicated.
>

Besides moving the function pointers assigned to the structure
gpio_chip fields to the new struct gpio_chip_ops, a pointer to the new
struct gpio_chip_ops has to be assigned to the struct gpio_chip .ops
field.

So where to put the new structure is constrained by where struct
gpio_chip fields are set. If is easier we can define to be just before
the function where the assignments were made but I don't know if it
can be generalized.

But I don't want to waste your time since I still didn't have time to
get familiar with cocinelle/spatch syntax to get into the details.
I'll figure out and let you know if I have any issues.

Thanks a lot and best regards,
Javier

[Cocci] semantic patch feasibility

[Javier Martinez Canillas]

Hello Julia,

On Fri, Apr 25, 2014 at 11:28 PM, Julia Lawall <julia.lawall@lip6.fr> wrote:
>
>
> On Fri, 25 Apr 2014, Javier Martinez Canillas wrote:
>
>> Hello Julia,
>>
>> On Fri, Apr 25, 2014 at 8:03 PM, Julia Lawall <julia.lawall@lip6.fr> wrote:
>> >
>> >
>> > On Fri, 25 Apr 2014, SF Markus Elfring wrote:
>> >
>> >> > The attached patch illustrated a number of them.
>> >>
>> >> How do you see the chances for generalisation of this concrete example?
>> >
>> > I think it should be quite straightforward.  One either takes the
>> > assignments out of the function and puts them into a structure
>> > initializer, or if there is a structure initializer already, one cuts it
>> > up into what is wanted for each of the separate functions.
>> >
>> > Maybe creating the name of the new structure is an issue?  I didn't look
>> > carefully enough to see if there was a pattern.
>> >
>> > julia
>>
>> I'm glad to know that should be straightforward. To name the new
>> structure I've been trying to deduce the naming scheme that the
>> original driver author used to name the GPIO operations and following
>> that convention.
>>
>> But if for the sake of simplicity we use a standard name that will be
>> used on all drivers, that's ok too. After all the structures are
>> static and later drivers maintainers can rename the structure if they
>> want a more suitable name.
>
> I thought you could look for the get or set field, see what it is assigned
> to, and make a new name from that one.  For maximum flexibility, you can
> make new names in ocaml or python code.  You can see how to do this in the
> file demos/pythontococci.cocci.
>
> julia

Yes, in most cases there is a prefix before
get,set,direction_input,direction_output, etc and I just use that to
name the structure. But it can't be generalized because not all the
drivers defined all the functions handlers, most of them are optional.
Also some drivers do not use a prefix before the GPIO operations but
in that case I can use a generic name.

Thanks a lot for the pointer about pythontococci.cocci and for your
help in general!

Best regards,
Javier

[Cocci] semantic patch feasibility

[Julia Lawall]

> Besides moving the function pointers assigned to the structure
> gpio_chip fields to the new struct gpio_chip_ops, a pointer to the new
> struct gpio_chip_ops has to be assigned to the struct gpio_chip .ops
> field.
> 
> So where to put the new structure is constrained by where struct
> gpio_chip fields are set. If is easier we can define to be just before
> the function where the assignments were made but I don't know if it
> can be generalized.
> 
> But I don't want to waste your time since I still didn't have time to
> get familiar with cocinelle/spatch syntax to get into the details.
> I'll figure out and let you know if I have any issues.

Good point!

julia

[Cocci] semantic patch feasibility

[SF Markus Elfring]

> Besides moving the function pointers assigned to the structure
> gpio_chip fields to the new struct gpio_chip_ops, a pointer to the new
> struct gpio_chip_ops has to be assigned to the struct gpio_chip .ops field.

Will any macros become relevant for such a source code reorganisation?


> So where to put the new structure is constrained by where struct
> gpio_chip fields are set. If is easier we can define to be just before
> the function where the assignments were made but I don't know if it
> can be generalized.

I am curious how a generalisation should be described here.


> But I don't want to waste your time since I still didn't have time to
> get familiar with cocinelle/spatch syntax to get into the details.

I find that the understanding of this syntax is a secondary issue. Is it more
important to clarify the basic change process before?

Do you need to consider any variations for the initialisation of the affected
data structure?
Is the specification precise enough so that such an algorithm can be added to a
kind of pattern collection?
http://refactoring.com/catalog/extractInterface.html
http://c2.com/cgi/wiki?ExtractInterface

Regards,
Markus

[Cocci] semantic patch feasibility

[Julia Lawall]

On Fri, 25 Apr 2014, Javier Martinez Canillas wrote:

> Hello Julia,
> 
> On Fri, Apr 25, 2014 at 6:23 PM, Julia Lawall <julia.lawall@lip6.fr> wrote:
> > On Fri, 25 Apr 2014, Javier Martinez Canillas wrote:
> >
> >> Hello,
> >>
> >> I'm a Linux kernel developer doing a big refactoring on the GPIO subsytem.
> >>
> >> I was wondering if the change I'm doing manually could be automated by
> >> using cocinelle but I've no experience neither writing semantic
> >> patches nor the SmPL language grammar.
> >
> > It looks quite possible.  In looking at the code, though, I wasn't sure
> > what is your strategy for where to place the new structure definition, in
> > the case where there was no structure before.  Would it be OK to put it
> > next to the probe function?
> >
> > julia
> 
> Thanks a lot for taking the time to look at this!
> 
> I've been adding the new structure definition right after all the
> functions have been defined since that is a common pattern found in
> other kernel subsystems.
> 
> But since this is a cleanup and if is going to be easier to define the
> semantic patch by placing the structure before the probe function then
> that works for me too and later drivers maintainers can send
> incremental patches to change wherever they find more suitable.

I would suggest to start with the case where there is already a global 
structure declaration.  Then you kow where to put the new structure and 
can probably find a reasonable algorithm for its name.

julia

[Cocci] semantic patch feasibility

[Javier Martinez Canillas]

Hello Markus

On Sat, Apr 26, 2014 at 10:30 AM, SF Markus Elfring
<elfring@users.sourceforge.net> wrote:
>> Besides moving the function pointers assigned to the structure
>> gpio_chip fields to the new struct gpio_chip_ops, a pointer to the new
>> struct gpio_chip_ops has to be assigned to the struct gpio_chip .ops field.
>
> Will any macros become relevant for such a source code reorganisation?
>

Not really, I was not thinking in adding any macros although there are
drivers using macros to initialize the data structure if that is what
you mean, I'll add more detail about this point below.

>
>> So where to put the new structure is constrained by where struct
>> gpio_chip fields are set. If is easier we can define to be just before
>> the function where the assignments were made but I don't know if it
>> can be generalized.
>
> I am curious how a generalisation should be described here.
>
>

me too :-)

Right now I identified 3 different patters:

1) When there is already a global gpio_chip structure declared and
initialized using the C99 tagged approach, i.e:

static struct gpio_chip ixp4xx_gpio_chip = {
-       .label                  = "IXP4XX_GPIO_CHIP",
+static const struct gpio_chip_ops ixp4xx_gpio_ops = {
        .direction_input        = ixp4xx_gpio_direction_input,
        .direction_output       = ixp4xx_gpio_direction_output,
        .get                    = ixp4xx_gpio_get_value,
        .set                    = ixp4xx_gpio_set_value,
+};
+
+static struct gpio_chip ixp4xx_gpio_chip = {
+       .label                  = "IXP4XX_GPIO_CHIP",
        .to_irq                 = ixp4xx_gpio_to_irq,
        .base                   = 0,
        .ngpio                  = 16,
+       .ops                    = &ixp4xx_gpio_ops,
 };

this is the easiest case in my opinion and the answer to Julia's
question about where to place the new gpio_chip_ops data structure is
easily answered since is just before the struct gpio_chip declaration.

2) When there isn't a global gpio_chip structure defined but it is
embedded in another structure and it is not statically initialized on
compile time but in a procedural on runtime, i.e:

+static const struct gpio_chip_ops scoop_gpio_ops = {
+       .set                    = scoop_gpio_set,
+       .get                    = scoop_gpio_get,
+       .direction_input        = scoop_gpio_direction_input,
+       .direction_output       = scoop_gpio_direction_output,
+};
+
 unsigned short read_scoop_reg(struct device *dev, unsigned short reg)
 {
        struct scoop_dev *sdev = dev_get_drvdata(dev);
@@ -220,10 +227,7 @@ static int scoop_probe(struct platform_device *pdev)
                devptr->gpio.label = dev_name(&pdev->dev);
                devptr->gpio.base = inf->gpio_base;
                devptr->gpio.ngpio = 12; /* PA11 = 0, PA12 = 1, etc.
up to PA22 = 11 */
-               devptr->gpio.set = scoop_gpio_set;
-               devptr->gpio.get = scoop_gpio_get;
-               devptr->gpio.direction_input = scoop_gpio_direction_input;
-               devptr->gpio.direction_output = scoop_gpio_direction_output;
+               devptr->gpio.ops = &scoop_gpio_ops;

                ret = gpiochip_add(&devptr->gpio);
                if (ret)

this one is trickier because you just can't do:

$ git grep "struct gpio_chip [_a-zA-Z][_a-zA-Z0-9]*.= {

first you have to look where the gpio_chip is declared, take that
identifier and search in which function is initialized. Also is the
question to where to place the new structure since it has to be before
it is referenced in the code. Probably a good candidate is to place it
just before the function where the gpio_chip fields are set. For what
I've been looking at spatch syntax that should be easy doable.

3) When none of the above patters are used, i.e:

+static const struct gpio_chip_ops msm_gpiolib_ops = {
+       .direction_input        = msm_gpiolib_direction_input,
+       .direction_output       = msm_gpiolib_direction_output,
+       .get                    = msm_gpiolib_get,
+       .set                    = msm_gpiolib_set,
+};
+
 #define TROUT_GPIO_BANK(name, reg_num, base_gpio, shadow_val)          \
        {                                                               \
                .chip = {                                               \
                        .label            = name,                       \
-                       .direction_input  = msm_gpiolib_direction_input,\
-                       .direction_output = msm_gpiolib_direction_output, \
-                       .get              = msm_gpiolib_get,            \
-                       .set              = msm_gpiolib_set,            \
                        .to_irq           = trout_gpio_to_irq,          \
                        .base             = base_gpio,                  \
                        .ngpio            = 8,                          \
+                       .ops              = &msm_gpiolib_ops,           \
                },                                                      \
                .reg = (void *) reg_num + TROUT_CPLD_BASE,              \
                .shadow = shadow_val,

Here a macro is used since there are many GPIO controllers so the
driver does this:

static struct msm_gpio_chip msm_gpio_banks[] = {
#if defined(CONFIG_MSM_DEBUG_UART1)
        /* H2W pins <-> UART1 */
        TROUT_GPIO_BANK("MISC2", 0x00,   TROUT_GPIO_MISC2_BASE, 0x40),
#else
        /* H2W pins <-> UART3, Bluetooth <-> UART1 */
        TROUT_GPIO_BANK("MISC2", 0x00,   TROUT_GPIO_MISC2_BASE, 0x80),
#endif
        /* I2C pull */
        TROUT_GPIO_BANK("MISC3", 0x02,   TROUT_GPIO_MISC3_BASE, 0x04),
        TROUT_GPIO_BANK("MISC4", 0x04,   TROUT_GPIO_MISC4_BASE, 0),
        /* mmdi 32k en */
        TROUT_GPIO_BANK("MISC5", 0x06,   TROUT_GPIO_MISC5_BASE, 0x04),
        TROUT_GPIO_BANK("INT2", 0x08,    TROUT_GPIO_INT2_BASE,  0),
        TROUT_GPIO_BANK("MISC1", 0x0a,   TROUT_GPIO_MISC1_BASE, 0),
        TROUT_GPIO_BANK("VIRTUAL", 0x12, TROUT_GPIO_VIRTUAL_BASE, 0),
};

or drivers that also use an array of gpio_chips too. i.e:

struct gpio_chip alchemy_gpio_chip[] = {
+static const struct gpio_chip_ops alchemy_gpio_chip_ops[] = {
        [0] = {
-               .label                  = "alchemy-gpio1",
                .direction_input        = gpio1_direction_input,
                .direction_output       = gpio1_direction_output,
                .get                    = gpio1_get,
                .set                    = gpio1_set,
-               .to_irq                 = gpio1_to_irq,
-               .base                   = ALCHEMY_GPIO1_BASE,
-               .ngpio                  = ALCHEMY_GPIO1_NUM,
        },
        [1] = {
-               .label                  = "alchemy-gpio2",
                .direction_input        = gpio2_direction_input,
                .direction_output       = gpio2_direction_output,
                .get                    = gpio2_get,
                .set                    = gpio2_set,
+       },
+};
+
+struct gpio_chip alchemy_gpio_chip[] = {
+       [0] = {
+               .label                  = "alchemy-gpio1",
+               .to_irq                 = gpio1_to_irq,
+               .base                   = ALCHEMY_GPIO1_BASE,
+               .ngpio                  = ALCHEMY_GPIO1_NUM,
+               .ops                    = &alchemy_gpio_chip_ops[0],
+       },
+       [1] = {
+               .label                  = "alchemy-gpio2",
                .to_irq                 = gpio2_to_irq,
                .base                   = ALCHEMY_GPIO2_BASE,
                .ngpio                  = ALCHEMY_GPIO2_NUM,
+               .ops                    = &alchemy_gpio_chip_ops[1],
        },
 };

But these special cases are the exception and not the norm, I'll
follow Julia's advice to start with 1) and then move to 2) and I'm ok
if I leave 3) out of the spatch and I do the changes manually to have
the complete patch.

>> But I don't want to waste your time since I still didn't have time to
>> get familiar with cocinelle/spatch syntax to get into the details.
>
> I find that the understanding of this syntax is a secondary issue. Is it more
> important to clarify the basic change process before?
>
> Do you need to consider any variations for the initialisation of the affected
> data structure?

Yes, the ones I commented above in 3)

> Is the specification precise enough so that such an algorithm can be added to a
> kind of pattern collection?
> http://refactoring.com/catalog/extractInterface.html
> http://c2.com/cgi/wiki?ExtractInterface

Great, I didn't know that this kind of refactoring pattern was named
ExtractInterface and yes that's exactly what I want to achieve here.
I'm sure other subsystems will benefit from the same semantic patch if
is generic enough so makes sense to me to be included to a demo
collection.

Probably the only change that will have to be made in the spatch is
the regular expression to filter the function names and the name of
the two data structures.

I really appreciate all the help and feedback from you both.

Thanks a lot and best regards,
Javier

[Cocci] Extract an interface with SmPL

[SF Markus Elfring]

> Great, I didn't know that this kind of refactoring pattern was named
> ExtractInterface and yes that's exactly what I want to achieve here.

Thanks for your feedback.

It will help to discuss further approaches after the goal is clear with a name
that is well-known for some software designers and developers. I hope that
experiences from other tools can eventually be reused then.


> I'm sure other subsystems will benefit from the same semantic patch if
> is generic enough so makes sense to me to be included to a demo collection.

Coccinelle provides a "small" semantic pattern collection for demonstration
purposes already.
https://github.com/coccinelle/coccinelle/tree/master/demos

Would you like to extend it with approaches for source code refactoring?


> Probably the only change that will have to be made in the spatch is
> the regular expression to filter the function names and the name of
> the two data structures.

I imagine that there are some software development challenges for the
implementation with the semantic patch language.

1. You refer to designated initializers from the standard "ISO/IEC 9899:1999".
   http://thevirtualmachinist.blogspot.de/2012/03/c99-designated-initializers.html

   Do you need to consider the case when they are not used in a source file?

2. You mentioned a few variations already. How difficult or interesting will the
handling of optional elements become?

3. Initialisation and assignment statements can vary in their number and order
generally. How would you like to treat differences here?

4. Do you need to introduce unique names for variables and involved data structures?

Regards,
Markus

[Cocci] Extract an interface with SmPL

[Julia Lawall]

> Would you like to extend it with approaches for source code refactoring?

I think it would be best that he stick to the problem at hand.

> > Probably the only change that will have to be made in the spatch is
> > the regular expression to filter the function names and the name of
> > the two data structures.
>
> I imagine that there are some software development challenges for the
> implementation with the semantic patch language.
>
> 1. You refer to designated initializers from the standard "ISO/IEC 9899:1999".
>    http://thevirtualmachinist.blogspot.de/2012/03/c99-designated-initializers.html
>
>    Do you need to consider the case when they are not used in a source file?

No idea what this question means.  One can either use an initializer or
initialize the fields of some structure in a function.  Both are
illustrated by the example patches.  But the case where there is already
an initializer seems simpler for starting.

> 2. You mentioned a few variations already. How difficult or interesting will the
> handling of optional elements become?

I think that one should focus on what is easy and hopefully what is
frequently useful first.  One can expand later, or just do the rest by
hand,

> 3. Initialisation and assignment statements can vary in their number and order
> generally. How would you like to treat differences here?

I don't think this is an issue.  The goal is just to preserve existing
behavior in this regard.  If the old structure is missing something, the
new structure will be missing it as well.

Basically, the idea would be to create the outline of the new structure,
and then fill it one by one with the fields from the old structure one by
one, if they are found.

I would imagine that there would be a number of sets of rules to move each
field from one structure to another.  This would lead to a good amount of
repeated code, but it has the benefit that the fields will end up all in
the same order, ie in the order of the rules.  Also, it seems hard to
generalize, because there is one field of function pointer type that
should not be moved.

> 4. Do you need to introduce unique names for variables and involved data structures?

I think this was discussed already.

julia

[Cocci] Extract an interface with SmPL

[Javier Martinez Canillas]

On Sat, Apr 26, 2014 at 6:06 PM, Julia Lawall <julia.lawall@lip6.fr> wrote:
>> Would you like to extend it with approaches for source code refactoring?
>
> I think it would be best that he stick to the problem at hand.
>

Agreed, I prefer to first finish this task and maybe add the spatch to
the scripts/coccinelle/api/ directory in the linux kernel source code
and also your git repo as an example and then I can work on making it
more general if that is useful. I think your project is great so I'll
be very glad to contribute as much as possible.

>> > Probably the only change that will have to be made in the spatch is
>> > the regular expression to filter the function names and the name of
>> > the two data structures.
>>
>> I imagine that there are some software development challenges for the
>> implementation with the semantic patch language.
>>
>> 1. You refer to designated initializers from the standard "ISO/IEC 9899:1999".
>>    http://thevirtualmachinist.blogspot.de/2012/03/c99-designated-initializers.html
>>
>>    Do you need to consider the case when they are not used in a source file?
>
> No idea what this question means.  One can either use an initializer or
> initialize the fields of some structure in a function.  Both are
> illustrated by the example patches.  But the case where there is already
> an initializer seems simpler for starting.
>

I think what Markus meant is if a driver could not be using designated
initializers but instead do something like this:

static struct gpio_chip foo_chip = {
        "foo",
        THIS_MODULE,
         foo_request,
         foo_free,
         foo_direction_in,
         foo_get,
         foo_direction_out,
         foo_set,
         foo_to_irq,
         true,
};

In that case then this is a bug for many reasons (structures fields
can be added or removed, there is no guarantee that the fields will be
initialized in the right order, etc) and this should have been found
on review when the driver was posted. So I even prefer this driver to
not build anymore so we can spot this issue and fix it during the
release candidate cycle.

>> 2. You mentioned a few variations already. How difficult or interesting will the
>> handling of optional elements become?
>
> I think that one should focus on what is easy and hopefully what is
> frequently useful first.  One can expand later, or just do the rest by
> hand,
>

Exactly, I understood that very well from your previous emails. Thanks
a lot again for your suggestions.

>> 3. Initialisation and assignment statements can vary in their number and order
>> generally. How would you like to treat differences here?
>
> I don't think this is an issue.  The goal is just to preserve existing
> behavior in this regard.  If the old structure is missing something, the
> new structure will be missing it as well.
>
> Basically, the idea would be to create the outline of the new structure,
> and then fill it one by one with the fields from the old structure one by
> one, if they are found.
>

Right, also most fields are optional so the driver is already using
only the subset that it needs. i.e: a GPIO chip could only support
output lines or input lines so it does not need to set all the
function pointers.

> I would imagine that there would be a number of sets of rules to move each
> field from one structure to another.  This would lead to a good amount of
> repeated code, but it has the benefit that the fields will end up all in
> the same order, ie in the order of the rules.  Also, it seems hard to
> generalize, because there is one field of function pointer type that
> should not be moved.
>

Exactly, the new gpio_chip_ops structure is defined using the const
type qualifier since the GPIO operations functions should not change
at runtime with the only exception of the .to_irq field that and is
usually modified at runtime.

>> 4. Do you need to introduce unique names for variables and involved data structures?
>
> I think this was discussed already.
>

Yes.

> julia

Best regards,
Javier