Re: [Cocci] [PATCH 1/1] scripts/coccinelle: use BIT() macro if possible
Hello Julia, On Sun, Apr 27, 2014 at 11:36 AM, Julia Lawall julia.law...@lip6.fr wrote: On Sun, 27 Apr 2014, Javier Martinez Canillas wrote: Hello Wolfram, Thanks a lot for your feedback. On Sun, Apr 27, 2014 at 7:14 AM, Wolfram Sang w...@the-dreams.de wrote: On Sun, Apr 27, 2014 at 02:29:46AM +0200, Javier Martinez Canillas wrote: Using the BIT() macro instead of manually shifting bits makes the code less error prone and also more readable. Does it? It is a taste thing, yet I don't think it makes the code that much more readable that it is worth changing the whole tree. I believe there is a reason for that macro but yes I agree with you that is a matter of taste and the it shouldn't be enforced. I'm doing a big refactoring for the GPIO subsystem and was told to use coccinelle so this patch was part of my learning. I posted it because I thought that it could be useful but I don't mind the patch to be dropped if that is not the case. Perhaps it could be useful in files that already use BIT somewhere? Well the semantic patch already has a rule that checks if the file includes linux/bitops.h so files that don't include this header will be skipped. I've checked and in most cases when that header is included is because at least the BIT macro is used once on the file. My guess is that the original author included the header and used the macro but other people modifying the file after its original creation just used 1 n instead. But as I said, I've no strong opinion about this patch. I just used to learn the basics of SmPL and to cleanup a driver I maintain and thought it was a good touch to post it in case more people find it useful. julia Thanks a lot and best regards, Javier ___ Cocci mailing list Cocci@systeme.lip6.fr https://systeme.lip6.fr/mailman/listinfo/cocci
Re: [Cocci] [PATCH 1/1] scripts/coccinelle: use BIT() macro if possible
On Sun, Apr 27, 2014 at 12:29 PM, Julia Lawall julia.law...@lip6.fr wrote: On Sun, 27 Apr 2014, Javier Martinez Canillas wrote: Using the BIT() macro instead of manually shifting bits makes the code less error prone and also more readable. Signed-off-by: Javier Martinez Canillas jav...@dowhile0.org --- An example of the patches that can be obtained with this spatch: http://www.mail-archive.com/linux-gpio@vger.kernel.org/msg02722.html scripts/coccinelle/api/bit.cocci | 25 + 1 file changed, 25 insertions(+) create mode 100644 scripts/coccinelle/api/bit.cocci diff --git a/scripts/coccinelle/api/bit.cocci b/scripts/coccinelle/api/bit.cocci new file mode 100644 index 000..a5df73a --- /dev/null +++ b/scripts/coccinelle/api/bit.cocci @@ -0,0 +1,25 @@ +// Use the macro BIT() macro if possible +// +// Confidence: High +// Copyright (C) 2014 Javier Martinez Canillas. GPLv2. +// URL: http://coccinelle.lip6.fr/ +// Options: --include-headers + +@hasbitops@ +@@ + +#include linux/bitops.h Here you could say: @usesbit@ @@ BIT(...) +@depends on hasbitops@ and then here it would be @depends on hasbitops usesbit@ julia Thanks a lot for the feedback, I'll send a v2 of the patch then. Best regards, Javier ___ Cocci mailing list Cocci@systeme.lip6.fr https://systeme.lip6.fr/mailman/listinfo/cocci
[Cocci] [PATCH v2 1/1] scripts/coccinelle: use BIT macro if used
Using the BIT() macro instead of manually shifting bits makes the code less error prone. If is more readable is a matter of taste so only replace if the file is already using this macro. Signed-off-by: Javier Martinez Canillas jav...@dowhile0.org --- Changes since v1: - Add a rule that checks if the file is already using this macro as suggested by Julia Lawall scripts/coccinelle/api/bit.cocci | 30 ++ 1 file changed, 30 insertions(+) create mode 100644 scripts/coccinelle/api/bit.cocci diff --git a/scripts/coccinelle/api/bit.cocci b/scripts/coccinelle/api/bit.cocci new file mode 100644 index 000..a02cfd3 --- /dev/null +++ b/scripts/coccinelle/api/bit.cocci @@ -0,0 +1,30 @@ +// Use the BIT() macro if is already used +// +// Confidence: High +// Copyright (C) 2014 Javier Martinez Canillas. GPLv2. +// URL: http://coccinelle.lip6.fr/ +// Options: --include-headers + +@hasbitops@ +@@ + +#include linux/bitops.h + +@usesbit@ +@@ + +BIT(...) + +@depends on hasbitops usesbit@ +expression E; +@@ + +- 1 E ++ BIT(E) + +@depends on hasbitops usesbit@ +expression E; +@@ + +- BIT((E)) ++ BIT(E) -- 1.9.1 ___ Cocci mailing list Cocci@systeme.lip6.fr https://systeme.lip6.fr/mailman/listinfo/cocci
Re: [Cocci] Extract an interface with SmPL
On Sat, Apr 26, 2014 at 6:06 PM, Julia Lawall julia.law...@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
___
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci
Re: [Cocci] semantic patch feasibility
On Fri, Apr 25, 2014 at 6:24 PM, Julia Lawall julia.law...@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 mailing list Cocci@systeme.lip6.fr https://systeme.lip6.fr/mailman/listinfo/cocci
Re: [Cocci] semantic patch feasibility
Hello Markus, On Fri, Apr 25, 2014 at 4:30 PM, SF Markus Elfring elfr...@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 mailing list Cocci@systeme.lip6.fr https://systeme.lip6.fr/mailman/listinfo/cocci
Re: [Cocci] semantic patch feasibility
Hello Julia, On Fri, Apr 25, 2014 at 8:03 PM, Julia Lawall julia.law...@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 mailing list Cocci@systeme.lip6.fr https://systeme.lip6.fr/mailman/listinfo/cocci
Re: [Cocci] semantic patch feasibility
Hello Julia, On Fri, Apr 25, 2014 at 6:23 PM, Julia Lawall julia.law...@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 mailing list Cocci@systeme.lip6.fr https://systeme.lip6.fr/mailman/listinfo/cocci
Re: [Cocci] semantic patch feasibility
Hello Julia, On Fri, Apr 25, 2014 at 11:25 PM, Julia Lawall julia.law...@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.law...@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 mailing list Cocci@systeme.lip6.fr https://systeme.lip6.fr/mailman/listinfo/cocci
