* [PATCH] checkpatch: Test for kmalloc/memset(0) pairs @ 2011-03-18 2:52 Steven Rostedt 2011-03-18 3:15 ` Jonathan Corbet 0 siblings, 1 reply; 28+ messages in thread From: Steven Rostedt @ 2011-03-18 2:52 UTC (permalink / raw) To: LKML; +Cc: Andy Whitcroft, Dave Jones, Andrew Morton The use of kzalloc() is preferred over kmalloc/memset(0) pairs. When a match is made with "memset(p, 0, s);" a search back through the patch hunk is made looking for "p = kmalloc(s,". If that is found, then a warning is given, suggesting to use kzalloc() instead. Signed-off-by: Steven Rostedt <rostedt@goodmis.org> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl index 58848e3..f28f0e3 100755 --- a/scripts/checkpatch.pl +++ b/scripts/checkpatch.pl @@ -2902,6 +2902,22 @@ sub process { $line =~ /DEVICE_ATTR.*S_IWUGO/ ) { WARN("Exporting world writable files is usually an error. Consider more restrictive permissions.\n" . $herecurr); } + +# The use of kzalloc() is preferred over kmalloc/memset(0) pairs. + if ($line =~ /\smemset\s*\((\S*)\s*,\s*(?:0x0|0)\s*,\s*(\S*)\s*\);/) { + my $ptr = $1; + my $size = $2; + + for (my $i = $linenr-2; $i >= 0; $i--) { + next if ($lines[$i] =~ /^-/); # ignore deletions + last if ($lines[$i] =~ /^\@\@/); + if ($lines[$i] =~ /\s(\S*)\s*=\s*kmalloc\((\S*)\,/ && + $1 eq $ptr && $2 eq $size) { + WARN("use kzalloc() instead of kmalloc/memset(p,0,size) pair\n" + . $herecurr); + } + } + } } # If we have no input at all, then there is nothing to report on ^ permalink raw reply related [flat|nested] 28+ messages in thread
* Re: [PATCH] checkpatch: Test for kmalloc/memset(0) pairs 2011-03-18 2:52 [PATCH] checkpatch: Test for kmalloc/memset(0) pairs Steven Rostedt @ 2011-03-18 3:15 ` Jonathan Corbet 2011-03-18 3:32 ` Steven Rostedt 2011-03-19 19:39 ` Dave Jones 0 siblings, 2 replies; 28+ messages in thread From: Jonathan Corbet @ 2011-03-18 3:15 UTC (permalink / raw) To: Steven Rostedt; +Cc: LKML, Andy Whitcroft, Dave Jones, Andrew Morton On Thu, 17 Mar 2011 22:52:24 -0400 Steven Rostedt <rostedt@goodmis.org> wrote: > The use of kzalloc() is preferred over kmalloc/memset(0) pairs. > > When a match is made with "memset(p, 0, s);" a search back through the > patch hunk is made looking for "p = kmalloc(s,". If that is found, then > a warning is given, suggesting to use kzalloc() instead. The Coccinelle stuff already has a lot of this kind of test. See, for example, scripts/coccinelle/api/alloc/kzalloc-simple.cocci. Suppose there is some way all this nice analysis infrastructure could be integrated instead of duplicated? Or am I just a crazy dreamer? jon ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH] checkpatch: Test for kmalloc/memset(0) pairs 2011-03-18 3:15 ` Jonathan Corbet @ 2011-03-18 3:32 ` Steven Rostedt 2011-03-20 3:40 ` Américo Wang 2011-03-19 19:39 ` Dave Jones 1 sibling, 1 reply; 28+ messages in thread From: Steven Rostedt @ 2011-03-18 3:32 UTC (permalink / raw) To: Jonathan Corbet; +Cc: LKML, Andy Whitcroft, Dave Jones, Andrew Morton On Thu, 2011-03-17 at 21:15 -0600, Jonathan Corbet wrote: > On Thu, 17 Mar 2011 22:52:24 -0400 > Steven Rostedt <rostedt@goodmis.org> wrote: > > > The use of kzalloc() is preferred over kmalloc/memset(0) pairs. > > > > When a match is made with "memset(p, 0, s);" a search back through the > > patch hunk is made looking for "p = kmalloc(s,". If that is found, then > > a warning is given, suggesting to use kzalloc() instead. > > The Coccinelle stuff already has a lot of this kind of test. See, for > example, scripts/coccinelle/api/alloc/kzalloc-simple.cocci. Suppose > there is some way all this nice analysis infrastructure could be > integrated instead of duplicated? Or am I just a crazy dreamer? > Hmm, to me Coccinelle is sorta the big hammer approach to this. It can be used to analyze the kernel code that exists. checkpatch is made to stop developers from adding new crap. That said, I wonder if there is a way to make one use information from the other. Perhaps we can make checkpatch read the Concinelle rules. Honestly, I've never used Concinelle and have no idea how to use it. Looking at their web site there's a PDF on how to use something called "spatch" but it seems more complex than checkpatch. But if we can teach checkpatch to read the rule files, then maybe that would be beneficial. -- Steve ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH] checkpatch: Test for kmalloc/memset(0) pairs 2011-03-18 3:32 ` Steven Rostedt @ 2011-03-20 3:40 ` Américo Wang 2011-03-20 7:02 ` Pekka Enberg 0 siblings, 1 reply; 28+ messages in thread From: Américo Wang @ 2011-03-20 3:40 UTC (permalink / raw) To: Steven Rostedt Cc: Jonathan Corbet, LKML, Andy Whitcroft, Dave Jones, Andrew Morton On Fri, Mar 18, 2011 at 11:32 AM, Steven Rostedt <rostedt@goodmis.org> wrote: > > But if we can teach checkpatch to read the rule files, then maybe that > would be beneficial. > The problem is Perl doesn't really understand C, while Coccinelle does. ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH] checkpatch: Test for kmalloc/memset(0) pairs 2011-03-20 3:40 ` Américo Wang @ 2011-03-20 7:02 ` Pekka Enberg 2011-03-20 8:01 ` Julia Lawall 0 siblings, 1 reply; 28+ messages in thread From: Pekka Enberg @ 2011-03-20 7:02 UTC (permalink / raw) To: Américo Wang Cc: Steven Rostedt, Jonathan Corbet, LKML, Andy Whitcroft, Dave Jones, Andrew Morton, Julia Lawall Hi, On Fri, Mar 18, 2011 at 11:32 AM, Steven Rostedt <rostedt@goodmis.org> wrote: >> But if we can teach checkpatch to read the rule files, then maybe that >> would be beneficial. On Sun, Mar 20, 2011 at 5:40 AM, Américo Wang <xiyou.wangcong@gmail.com> wrote: > The problem is Perl doesn't really understand C, while Coccinelle does. It would be nice if Coccinelle would be even more easy to setup and use during development. Like with Dave, Coccinelle has been on my todo list forever. ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH] checkpatch: Test for kmalloc/memset(0) pairs 2011-03-20 7:02 ` Pekka Enberg @ 2011-03-20 8:01 ` Julia Lawall 2011-03-20 9:02 ` Pekka Enberg 0 siblings, 1 reply; 28+ messages in thread From: Julia Lawall @ 2011-03-20 8:01 UTC (permalink / raw) To: Pekka Enberg Cc: Américo Wang, Steven Rostedt, Jonathan Corbet, LKML, Andy Whitcroft, Dave Jones, Andrew Morton, Nicolas Palix [-- Attachment #1: Type: TEXT/PLAIN, Size: 1200 bytes --] On Sun, 20 Mar 2011, Pekka Enberg wrote: > Hi, > > On Fri, Mar 18, 2011 at 11:32 AM, Steven Rostedt <rostedt@goodmis.org> wrote: > >> But if we can teach checkpatch to read the rule files, then maybe that > >> would be beneficial. > > On Sun, Mar 20, 2011 at 5:40 AM, Américo Wang <xiyou.wangcong@gmail.com> wrote: > > The problem is Perl doesn't really understand C, while Coccinelle does. > > It would be nice if Coccinelle would be even more easy to setup and > use during development. Like with Dave, Coccinelle has been on my todo > list forever. You have to get and set up Coccinelle on your own. However, it is part of many Linux distributions, including ubuntu. Afterwards, you can run make coccicheck to apply the semantic patches that are included in the kernel to the entire kernel. There are some options to eg restrict the choice of rule, the choice of considered Linux files, and the format of the output. Where possible we have taken decisions that are common with what is done elsewhere in the make file. Documentation is in Documentation/coccinelle.txt Suggestions for how to make it easier to use or the documentation more understandable are welcome. thanks, julia ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH] checkpatch: Test for kmalloc/memset(0) pairs 2011-03-20 8:01 ` Julia Lawall @ 2011-03-20 9:02 ` Pekka Enberg 2011-03-20 9:17 ` Julia Lawall 0 siblings, 1 reply; 28+ messages in thread From: Pekka Enberg @ 2011-03-20 9:02 UTC (permalink / raw) To: Julia Lawall Cc: Américo Wang, Steven Rostedt, Jonathan Corbet, LKML, Andy Whitcroft, Dave Jones, Andrew Morton, Nicolas Palix, Ingo Molnar Hi Julia, On Sun, Mar 20, 2011 at 10:01 AM, Julia Lawall <julia@diku.dk> wrote: > Suggestions for how to make it easier to use or the documentation more > understandable are welcome. The benefit of scripts/checkpatch.pl is that it doesn't require any setting up to do. I'm personally less likely to use Coccinelle (and Sparse for that matter) on boxes where the software is not installed. I'm not sure how other people feel about it, but I'd personally love to see tools/coccinelle and tools/sparse. As for something more concrete, I guess this is what I'm mostly interested in: penberg@jaguar:~/src/linux$ make C=1 CHECK="scripts/coccicheck" mm/slub.o I guess it'd be good to document that for 'make help' because now you need to dig through Documentation/coccinelle.txt to find it. P.S. It seems there's a bug somewhere because the above command fails miserably for me: CHK include/linux/version.h CHK include/generated/utsrelease.h CALL scripts/checksyscalls.sh CHECK mm/slub.c File "/home/penberg/src/linux/scripts/coccinelle/api/memdup_user.cocci", line 32, column 5, charpos = 747 around = '<+...', whole content = - <+... when != goto l2; Fatal error: exception Lexer_cocci.Lexical("invalid in a nonempty context: <+...") File "/home/penberg/src/linux/scripts/coccinelle/api/memdup_user.cocci", line 32, column 5, charpos = 747 around = '<+...', whole content = - <+... when != goto l2; Fatal error: exception Lexer_cocci.Lexical("invalid in a nonempty context: <+...") File "/home/penberg/src/linux/scripts/coccinelle/api/memdup_user.cocci", line 32, column 5, charpos = 747 around = '<+...', whole content = - <+... when != goto l2; Fatal error: exception Lexer_cocci.Lexical("invalid in a nonempty context: <+...") File "/home/penberg/src/linux/scripts/coccinelle/api/memdup_user.cocci", line 32, column 5, charpos = 747 around = '<+...', whole content = - <+... when != goto l2; Fatal error: exception Lexer_cocci.Lexical("invalid in a nonempty context: <+...") make[1]: *** [mm/slub.o] Error 1 make: *** [mm/slub.o] Error 2 penberg@jaguar:~/src/linux$ dpkg -l|grep coccinelle ii coccinelle 0.2.2.deb-2 semantic patching tool for C [ I'm on Ubuntu 10.10. ] Pekka ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH] checkpatch: Test for kmalloc/memset(0) pairs 2011-03-20 9:02 ` Pekka Enberg @ 2011-03-20 9:17 ` Julia Lawall 2011-03-20 10:54 ` Ingo Molnar 2011-03-24 15:34 ` Aneesh Kumar K. V 0 siblings, 2 replies; 28+ messages in thread From: Julia Lawall @ 2011-03-20 9:17 UTC (permalink / raw) To: Pekka Enberg Cc: Américo Wang, Steven Rostedt, Jonathan Corbet, LKML, Andy Whitcroft, Dave Jones, Andrew Morton, Nicolas Palix, Ingo Molnar On Sun, 20 Mar 2011, Pekka Enberg wrote: > Hi Julia, > > On Sun, Mar 20, 2011 at 10:01 AM, Julia Lawall <julia@diku.dk> wrote: > > Suggestions for how to make it easier to use or the documentation more > > understandable are welcome. > > The benefit of scripts/checkpatch.pl is that it doesn't require any > setting up to do. I'm personally less likely to use Coccinelle (and > Sparse for that matter) on boxes where the software is not installed. > I'm not sure how other people feel about it, but I'd personally love > to see tools/coccinelle and tools/sparse. This was discussed before, and it was felt that perhaps 75000 lines of ocaml code was not really appropriate for the Linux source tree, and also that it would too much complicate our development process. One reason for using multiple machines would be to work on multiple architectures. But Coccinelle is not sensitive to the architecture on which it is run, so perhaps you do't need to have it installed everywhere. > As for something more concrete, I guess this is what I'm mostly interested in: > > penberg@jaguar:~/src/linux$ make C=1 CHECK="scripts/coccicheck" mm/slub.o > > I guess it'd be good to document that for 'make help' because now you > need to dig through Documentation/coccinelle.txt to find it. OK, thanks. We will look into that. > P.S. It seems there's a bug somewhere because the above command fails > miserably for me: > > CHK include/linux/version.h > CHK include/generated/utsrelease.h > CALL scripts/checksyscalls.sh > CHECK mm/slub.c > File "/home/penberg/src/linux/scripts/coccinelle/api/memdup_user.cocci", > line 32, column 5, charpos = 747 > around = '<+...', whole content = - <+... when != goto l2; > Fatal error: exception Lexer_cocci.Lexical("invalid in a nonempty > context: <+...") > File "/home/penberg/src/linux/scripts/coccinelle/api/memdup_user.cocci", > line 32, column 5, charpos = 747 > around = '<+...', whole content = - <+... when != goto l2; > Fatal error: exception Lexer_cocci.Lexical("invalid in a nonempty > context: <+...") > File "/home/penberg/src/linux/scripts/coccinelle/api/memdup_user.cocci", > line 32, column 5, charpos = 747 > around = '<+...', whole content = - <+... when != goto l2; > Fatal error: exception Lexer_cocci.Lexical("invalid in a nonempty > context: <+...") > File "/home/penberg/src/linux/scripts/coccinelle/api/memdup_user.cocci", > line 32, column 5, charpos = 747 > around = '<+...', whole content = - <+... when != goto l2; > Fatal error: exception Lexer_cocci.Lexical("invalid in a nonempty > context: <+...") > make[1]: *** [mm/slub.o] Error 1 > make: *** [mm/slub.o] Error 2 > > penberg@jaguar:~/src/linux$ dpkg -l|grep coccinelle > ii coccinelle 0.2.2.deb-2 > semantic patching tool for C > > [ I'm on Ubuntu 10.10. ] Indeed that one seems to be quite out of date. You can get the most recent version here: https://launchpad.net/~npalix/+archive/coccinelle julia ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH] checkpatch: Test for kmalloc/memset(0) pairs 2011-03-20 9:17 ` Julia Lawall @ 2011-03-20 10:54 ` Ingo Molnar 2011-03-20 11:06 ` Pekka Enberg ` (3 more replies) 2011-03-24 15:34 ` Aneesh Kumar K. V 1 sibling, 4 replies; 28+ messages in thread From: Ingo Molnar @ 2011-03-20 10:54 UTC (permalink / raw) To: Julia Lawall Cc: Pekka Enberg, Américo Wang, Steven Rostedt, Jonathan Corbet, LKML, Andy Whitcroft, Dave Jones, Andrew Morton, Nicolas Palix, Thomas Gleixner * Julia Lawall <julia@diku.dk> wrote: > On Sun, 20 Mar 2011, Pekka Enberg wrote: > > > Hi Julia, > > > > On Sun, Mar 20, 2011 at 10:01 AM, Julia Lawall <julia@diku.dk> wrote: > > > Suggestions for how to make it easier to use or the documentation more > > > understandable are welcome. > > > > The benefit of scripts/checkpatch.pl is that it doesn't require any > > setting up to do. I'm personally less likely to use Coccinelle (and > > Sparse for that matter) on boxes where the software is not installed. > > I'm not sure how other people feel about it, but I'd personally love > > to see tools/coccinelle and tools/sparse. > > This was discussed before, and it was felt that perhaps 75000 lines of > ocaml code was not really appropriate for the Linux source tree, [...] We have drivers and rare architectures that have a (much) larger size and which are only useful to <0.01% of Linux users. Coccinelle on the other hand is useful to 100% of Linux users. We even have C++, perl and python code in the kernel repo so there's no language bigotry either - use the language that works best for your project. > [...] and also that it would too much complicate our development process. 'our' as in the Linux kernel development process? I really don't think it's an issue - see above. or 'our' as in Coccinelle development process? When we brought tools/perf/ into the kernel repo all it forced on us were sane Git commits and a predictable, (modulo-) 3 months release/stabilization cycle. Both constraints served the quality of the perf project very well - but of course your milage may vary. > One reason for using multiple machines would be to work on multiple > architectures. But Coccinelle is not sensitive to the architecture on > which it is run, so perhaps you do't need to have it installed everywhere. I think the point Pekka tried to make is to have it integrated into the kbuild mechanism as well at a certain point. That way it's very easy to use it and we maintainers could require frequent patch submitters to use those tools to check the quality of their patches. Right now i cannot require that, as it's not part of the kernel repo. Requiring a checkpatch.pl check is much easier, as it's available to everyone who is writing kernel patches. 'The power of the default' is a very strong force. Also, IIRC Thomas also sometimes uses Coccinelle to generate *patches*, so having it in the kernel would also help that kind of usage. > > File "/home/penberg/src/linux/scripts/coccinelle/api/memdup_user.cocci", > > line 32, column 5, charpos = 747 > > around = '<+...', whole content = - <+... when != goto l2; > > Fatal error: exception Lexer_cocci.Lexical("invalid in a nonempty > > context: <+...") > > make[1]: *** [mm/slub.o] Error 1 > > make: *** [mm/slub.o] Error 2 > > > > penberg@jaguar:~/src/linux$ dpkg -l|grep coccinelle > > ii coccinelle 0.2.2.deb-2 > > semantic patching tool for C > > > > [ I'm on Ubuntu 10.10. ] > > Indeed that one seems to be quite out of date. You can get the most > recent version here: https://launchpad.net/~npalix/+archive/coccinelle With tools/coccinelle/ you would never run into such problems of distributing the latest stable version to your fellow kernel developers: it would always be available in tools/coccinelle/. Integration, synergy, availability, distribution and half a dozen other buzzwords come to mind as to why it's a good idea to have kernel-focused tools hosted in the kernel repo :-) IMO it's an option to consider. Thanks, Ingo ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH] checkpatch: Test for kmalloc/memset(0) pairs 2011-03-20 10:54 ` Ingo Molnar @ 2011-03-20 11:06 ` Pekka Enberg 2011-03-20 11:32 ` Nicolas Palix 2011-03-20 11:09 ` Alexey Dobriyan ` (2 subsequent siblings) 3 siblings, 1 reply; 28+ messages in thread From: Pekka Enberg @ 2011-03-20 11:06 UTC (permalink / raw) To: Ingo Molnar Cc: Julia Lawall, Américo Wang, Steven Rostedt, Jonathan Corbet, LKML, Andy Whitcroft, Dave Jones, Andrew Morton, Nicolas Palix, Thomas Gleixner Hi, On Sun, Mar 20, 2011 at 12:54 PM, Ingo Molnar <mingo@elte.hu> wrote: >> Indeed that one seems to be quite out of date. You can get the most >> recent version here: https://launchpad.net/~npalix/+archive/coccinelle > > With tools/coccinelle/ you would never run into such problems of distributing > the latest stable version to your fellow kernel developers: it would always be > available in tools/coccinelle/. > > Integration, synergy, availability, distribution and half a dozen other > buzzwords come to mind as to why it's a good idea to have kernel-focused > tools hosted in the kernel repo :-) > > IMO it's an option to consider. That's my thinking too. Yes, 80 KLOC of OCaml in the kernel tree sounds crazy but I think the practical advantages might be enough to justify it. Btw, would git-submodule be something to consider here? ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH] checkpatch: Test for kmalloc/memset(0) pairs 2011-03-20 11:06 ` Pekka Enberg @ 2011-03-20 11:32 ` Nicolas Palix 2011-03-20 12:00 ` Julia Lawall 0 siblings, 1 reply; 28+ messages in thread From: Nicolas Palix @ 2011-03-20 11:32 UTC (permalink / raw) To: Pekka Enberg Cc: Ingo Molnar, Julia Lawall, Américo Wang, Steven Rostedt, Jonathan Corbet, LKML, Andy Whitcroft, Dave Jones, Andrew Morton, Thomas Gleixner Hi, On Sun, Mar 20, 2011 at 12:06 PM, Pekka Enberg <penberg@kernel.org> wrote: > Hi, > > On Sun, Mar 20, 2011 at 12:54 PM, Ingo Molnar <mingo@elte.hu> wrote: >>> Indeed that one seems to be quite out of date. You can get the most >>> recent version here: https://launchpad.net/~npalix/+archive/coccinelle >> >> With tools/coccinelle/ you would never run into such problems of distributing >> the latest stable version to your fellow kernel developers: it would always be >> available in tools/coccinelle/. >> >> Integration, synergy, availability, distribution and half a dozen other >> buzzwords come to mind as to why it's a good idea to have kernel-focused >> tools hosted in the kernel repo :-) Our usage is mainly kernel-focused but not the tool. It is C-program focused and we have used it on other programs like Wine, OpenSSL, VLC. Others use it on other projects like Davecot or close-source projets. So, IMHO Coccinelle should no be part of Linux. Integrating kernel-focused SmPL scripts is on the other hand a great idea to check the kernel and to ease kernel developer life. It is what have been done so far. It is certainly possible to improve that, at least by adding more and more scripts. >> >> IMO it's an option to consider. > > That's my thinking too. Yes, 80 KLOC of OCaml in the kernel tree > sounds crazy but I think the practical advantages might be enough to > justify it. Btw, would git-submodule be something to consider here? > At every RC, we push the Coccinelle code on github. Using git-submodule seems the way to go thus. Moreover, it will ease the maintenance of scripts as we may assume users have one of the latest versions. -- Nicolas Palix http://sardes.inrialpes.fr/~npalix/ ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH] checkpatch: Test for kmalloc/memset(0) pairs 2011-03-20 11:32 ` Nicolas Palix @ 2011-03-20 12:00 ` Julia Lawall 0 siblings, 0 replies; 28+ messages in thread From: Julia Lawall @ 2011-03-20 12:00 UTC (permalink / raw) To: Nicolas Palix Cc: Pekka Enberg, Ingo Molnar, Américo Wang, Steven Rostedt, Jonathan Corbet, LKML, Andy Whitcroft, Dave Jones, Andrew Morton, Thomas Gleixner [-- Attachment #1: Type: TEXT/PLAIN, Size: 1963 bytes --] On Sun, 20 Mar 2011, Nicolas Palix wrote: > Hi, > > On Sun, Mar 20, 2011 at 12:06 PM, Pekka Enberg <penberg@kernel.org> wrote: > > Hi, > > > > On Sun, Mar 20, 2011 at 12:54 PM, Ingo Molnar <mingo@elte.hu> wrote: > >>> Indeed that one seems to be quite out of date. You can get the most > >>> recent version here: https://launchpad.net/~npalix/+archive/coccinelle > >> > >> With tools/coccinelle/ you would never run into such problems of distributing > >> the latest stable version to your fellow kernel developers: it would always be > >> available in tools/coccinelle/. > >> > >> Integration, synergy, availability, distribution and half a dozen other > >> buzzwords come to mind as to why it's a good idea to have kernel-focused > >> tools hosted in the kernel repo :-) > > Our usage is mainly kernel-focused but not the tool. It is C-program focused > and we have used it on other programs like Wine, OpenSSL, VLC. Others > use it on other projects like Davecot or close-source projets. > So, IMHO Coccinelle should no be part of Linux. > > Integrating kernel-focused SmPL scripts is on the other hand a great idea > to check the kernel and to ease kernel developer life. > It is what have been done so far. It is certainly possible to improve that, > at least by adding more and more scripts. > > >> > >> IMO it's an option to consider. > > > > That's my thinking too. Yes, 80 KLOC of OCaml in the kernel tree > > sounds crazy but I think the practical advantages might be enough to > > justify it. Btw, would git-submodule be something to consider here? > > > > At every RC, we push the Coccinelle code on github. Using git-submodule > seems the way to go thus. Moreover, it will ease the maintenance of > scripts as we may assume users have one of the latest versions. The latter is indeed the more serious problem. But even if the Coccinelle code is available, the person will still have to have ocaml installed to be able to compile it. julia ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH] checkpatch: Test for kmalloc/memset(0) pairs 2011-03-20 10:54 ` Ingo Molnar 2011-03-20 11:06 ` Pekka Enberg @ 2011-03-20 11:09 ` Alexey Dobriyan 2011-03-20 12:38 ` Julia Lawall 2011-03-20 14:48 ` Alejandro Riveira Fernández 3 siblings, 0 replies; 28+ messages in thread From: Alexey Dobriyan @ 2011-03-20 11:09 UTC (permalink / raw) To: Ingo Molnar Cc: Julia Lawall, Pekka Enberg, Américo Wang, Steven Rostedt, Jonathan Corbet, LKML, Andy Whitcroft, Dave Jones, Andrew Morton, Nicolas Palix, Thomas Gleixner Can we tell "0" token from "sizeof(struct {})" in C compiler? This would solve memset(,0); issue quite nicely. ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH] checkpatch: Test for kmalloc/memset(0) pairs 2011-03-20 10:54 ` Ingo Molnar 2011-03-20 11:06 ` Pekka Enberg 2011-03-20 11:09 ` Alexey Dobriyan @ 2011-03-20 12:38 ` Julia Lawall 2011-03-20 14:48 ` Alejandro Riveira Fernández 3 siblings, 0 replies; 28+ messages in thread From: Julia Lawall @ 2011-03-20 12:38 UTC (permalink / raw) To: Ingo Molnar Cc: Pekka Enberg, Américo Wang, Steven Rostedt, Jonathan Corbet, LKML, Andy Whitcroft, Dave Jones, Andrew Morton, Nicolas Palix, Thomas Gleixner > > [...] and also that it would too much complicate our development process. > > 'our' as in the Linux kernel development process? I really don't think it's an > issue - see above. > > or 'our' as in Coccinelle development process? When we brought tools/perf/ into > the kernel repo all it forced on us were sane Git commits and a predictable, > (modulo-) 3 months release/stabilization cycle. Both constraints served the > quality of the perf project very well - but of course your milage may vary. Yes, I meant the Coccinelle development process. > > One reason for using multiple machines would be to work on multiple > > architectures. But Coccinelle is not sensitive to the architecture on > > which it is run, so perhaps you do't need to have it installed everywhere. > > I think the point Pekka tried to make is to have it integrated into the kbuild > mechanism as well at a certain point. That way it's very easy to use it and we > maintainers could require frequent patch submitters to use those tools to check > the quality of their patches. Right now i cannot require that, as it's not part > of the kernel repo. Requiring a checkpatch.pl check is much easier, as it's > available to everyone who is writing kernel patches. There remains the problem that if it is just the sources that are part of the kernel, the user has to have the ocaml compiler installed. julia ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH] checkpatch: Test for kmalloc/memset(0) pairs 2011-03-20 10:54 ` Ingo Molnar ` (2 preceding siblings ...) 2011-03-20 12:38 ` Julia Lawall @ 2011-03-20 14:48 ` Alejandro Riveira Fernández 2011-03-21 13:13 ` Steven Rostedt 3 siblings, 1 reply; 28+ messages in thread From: Alejandro Riveira Fernández @ 2011-03-20 14:48 UTC (permalink / raw) To: Ingo Molnar Cc: Julia Lawall, Pekka Enberg, Américo Wang, Steven Rostedt, Jonathan Corbet, LKML, Andy Whitcroft, Dave Jones, Andrew Morton, Nicolas Palix, Thomas Gleixner El Sun, 20 Mar 2011 11:54:12 +0100 Ingo Molnar <mingo@elte.hu> escribió: > > I think the point Pekka tried to make is to have it integrated into the kbuild > mechanism as well at a certain point. That way it's very easy to use it and we > maintainers could require frequent patch submitters to use those tools to check > the quality of their patches. Right now i cannot require that, as it's not part > of the kernel repo. Requiring a checkpatch.pl check is much easier, as it's > available to everyone who is writing kernel patches. But a perl interpreter is not shipped with the kernel, is it? neither a posix shell or a python interpreter ... The scripts are already shipped with the kernel, the "interpreter" have to be installed in the dev machine like you have to install perl, make, gcc, etc just my two cents as a bystander :) > > Thanks, > > Ingo > -- ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH] checkpatch: Test for kmalloc/memset(0) pairs 2011-03-20 14:48 ` Alejandro Riveira Fernández @ 2011-03-21 13:13 ` Steven Rostedt 0 siblings, 0 replies; 28+ messages in thread From: Steven Rostedt @ 2011-03-21 13:13 UTC (permalink / raw) To: Alejandro Riveira Fernández Cc: Ingo Molnar, Julia Lawall, Pekka Enberg, Américo Wang, Jonathan Corbet, LKML, Andy Whitcroft, Dave Jones, Andrew Morton, Nicolas Palix, Thomas Gleixner On Sun, 2011-03-20 at 15:48 +0100, Alejandro Riveira Fernández wrote: > El Sun, 20 Mar 2011 11:54:12 +0100 > Ingo Molnar <mingo@elte.hu> escribió: > > But a perl interpreter is not shipped with the kernel, is it? neither a > posix shell or a python interpreter ... > The scripts are already shipped with the kernel, the "interpreter" have > to be installed in the dev machine like you have to install perl, make, > gcc, etc We don't ship gcc either. Note, there are some cases where you need a perl interpreter to build the kernel. -- Steve ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH] checkpatch: Test for kmalloc/memset(0) pairs 2011-03-20 9:17 ` Julia Lawall 2011-03-20 10:54 ` Ingo Molnar @ 2011-03-24 15:34 ` Aneesh Kumar K. V 2011-03-24 16:08 ` Julia Lawall 1 sibling, 1 reply; 28+ messages in thread From: Aneesh Kumar K. V @ 2011-03-24 15:34 UTC (permalink / raw) To: Julia Lawall, Pekka Enberg Cc: Américo Wang, Steven Rostedt, Jonathan Corbet, LKML, Andy Whitcroft, Dave Jones, Andrew Morton, Nicolas Palix, Ingo Molnar On Sun, 20 Mar 2011 10:17:07 +0100 (CET), Julia Lawall <julia@diku.dk> wrote: > On Sun, 20 Mar 2011, Pekka Enberg wrote: > > > Hi Julia, > > > > On Sun, Mar 20, 2011 at 10:01 AM, Julia Lawall <julia@diku.dk> wrote: > > > Suggestions for how to make it easier to use or the documentation more > > > understandable are welcome. > > > > The benefit of scripts/checkpatch.pl is that it doesn't require any > > setting up to do. I'm personally less likely to use Coccinelle (and > > Sparse for that matter) on boxes where the software is not installed. > > I'm not sure how other people feel about it, but I'd personally love > > to see tools/coccinelle and tools/sparse. > > This was discussed before, and it was felt that perhaps 75000 lines of > ocaml code was not really appropriate for the Linux source tree, and also > that it would too much complicate our development process. > > One reason for using multiple machines would be to work on multiple > architectures. But Coccinelle is not sensitive to the architecture on > which it is run, so perhaps you do't need to have it installed everywhere. > > > As for something more concrete, I guess this is what I'm mostly interested in: > > > > penberg@jaguar:~/src/linux$ make C=1 CHECK="scripts/coccicheck" mm/slub.o > > > > I guess it'd be good to document that for 'make help' because now you > > need to dig through Documentation/coccinelle.txt to find it. > > OK, thanks. We will look into that. > > > P.S. It seems there's a bug somewhere because the above command fails > > miserably for me: > > > > CHK include/linux/version.h > > CHK include/generated/utsrelease.h > > CALL scripts/checksyscalls.sh > > CHECK mm/slub.c > > File "/home/penberg/src/linux/scripts/coccinelle/api/memdup_user.cocci", > > line 32, column 5, charpos = 747 > > around = '<+...', whole content = - <+... when != goto l2; > > Fatal error: exception Lexer_cocci.Lexical("invalid in a nonempty > > context: <+...") > > File "/home/penberg/src/linux/scripts/coccinelle/api/memdup_user.cocci", > > line 32, column 5, charpos = 747 > > around = '<+...', whole content = - <+... when != goto l2; > > Fatal error: exception Lexer_cocci.Lexical("invalid in a nonempty > > context: <+...") > > File "/home/penberg/src/linux/scripts/coccinelle/api/memdup_user.cocci", > > line 32, column 5, charpos = 747 > > around = '<+...', whole content = - <+... when != goto l2; > > Fatal error: exception Lexer_cocci.Lexical("invalid in a nonempty > > context: <+...") > > File "/home/penberg/src/linux/scripts/coccinelle/api/memdup_user.cocci", > > line 32, column 5, charpos = 747 > > around = '<+...', whole content = - <+... when != goto l2; > > Fatal error: exception Lexer_cocci.Lexical("invalid in a nonempty > > context: <+...") > > make[1]: *** [mm/slub.o] Error 1 > > make: *** [mm/slub.o] Error 2 > > > > penberg@jaguar:~/src/linux$ dpkg -l|grep coccinelle > > ii coccinelle 0.2.2.deb-2 > > semantic patching tool for C > > > > [ I'm on Ubuntu 10.10. ] > > Indeed that one seems to be quite out of date. You can get the most > recent version here: https://launchpad.net/~npalix/+archive/coccinelle I never got a working version of coccinelle with scripts in the kernel. Even the ppa version doesn't work for me. make C=2 CHECK=scripts/coccicheck fs/namei.o ... .... CHECK scripts/mod/empty.c File "/home/opensource/sources/kernels/linux-2.6/scripts/coccinelle/api/memdup_user.cocci", line 32, column 5, charpos = 747 around = '<+...', whole content = - <+... when != goto l2; Fatal error: exception Lexer_cocci.Lexical("invalid in a nonempty context: <+...") File "/home/opensource/sources/kernels/linux-2.6/scripts/coccinelle/api/memdup_user.cocci", line 32, column 5, charpos = 747 around = '<+...', whole content = - <+... when != goto l2; Fatal error: exception Lexer_cocci.Lexical("invalid in a nonempty context: <+...") File "/home/opensource/sources/kernels/linux-2.6/scripts/coccinelle/api/memdup_user.cocci", line 32, column 5, charpos = 747 around = '<+...', whole content = - <+... when != goto l2; Fatal error: exception Lexer_cocci.Lexical("invalid in a nonempty context: <+...") File "/home/opensource/sources/kernels/linux-2.6/scripts/coccinelle/api/memdup_user.cocci", line 32, column 5, charpos = 747 around = '<+...', whole content = - <+... when != goto l2; Fatal error: exception Lexer_cocci.Lexical("invalid in a nonempty context: <+...") ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH] checkpatch: Test for kmalloc/memset(0) pairs 2011-03-24 15:34 ` Aneesh Kumar K. V @ 2011-03-24 16:08 ` Julia Lawall 2011-03-24 16:10 ` Nicolas Palix 2011-03-24 17:28 ` Aneesh Kumar K. V 0 siblings, 2 replies; 28+ messages in thread From: Julia Lawall @ 2011-03-24 16:08 UTC (permalink / raw) To: Aneesh Kumar K. V Cc: Pekka Enberg, Américo Wang, Steven Rostedt, Jonathan Corbet, LKML, Andy Whitcroft, Dave Jones, Andrew Morton, Nicolas Palix, Ingo Molnar On Thu, 24 Mar 2011, Aneesh Kumar K. V wrote: > On Sun, 20 Mar 2011 10:17:07 +0100 (CET), Julia Lawall <julia@diku.dk> wrote: > > On Sun, 20 Mar 2011, Pekka Enberg wrote: > > > > > Hi Julia, > > > > > > On Sun, Mar 20, 2011 at 10:01 AM, Julia Lawall <julia@diku.dk> wrote: > > > > Suggestions for how to make it easier to use or the documentation more > > > > understandable are welcome. > > > > > > The benefit of scripts/checkpatch.pl is that it doesn't require any > > > setting up to do. I'm personally less likely to use Coccinelle (and > > > Sparse for that matter) on boxes where the software is not installed. > > > I'm not sure how other people feel about it, but I'd personally love > > > to see tools/coccinelle and tools/sparse. > > > > This was discussed before, and it was felt that perhaps 75000 lines of > > ocaml code was not really appropriate for the Linux source tree, and also > > that it would too much complicate our development process. > > > > One reason for using multiple machines would be to work on multiple > > architectures. But Coccinelle is not sensitive to the architecture on > > which it is run, so perhaps you do't need to have it installed everywhere. > > > > > As for something more concrete, I guess this is what I'm mostly interested in: > > > > > > penberg@jaguar:~/src/linux$ make C=1 CHECK="scripts/coccicheck" mm/slub.o > > > > > > I guess it'd be good to document that for 'make help' because now you > > > need to dig through Documentation/coccinelle.txt to find it. > > > > OK, thanks. We will look into that. > > > > > P.S. It seems there's a bug somewhere because the above command fails > > > miserably for me: > > > > > > CHK include/linux/version.h > > > CHK include/generated/utsrelease.h > > > CALL scripts/checksyscalls.sh > > > CHECK mm/slub.c > > > File "/home/penberg/src/linux/scripts/coccinelle/api/memdup_user.cocci", > > > line 32, column 5, charpos = 747 > > > around = '<+...', whole content = - <+... when != goto l2; > > > Fatal error: exception Lexer_cocci.Lexical("invalid in a nonempty > > > context: <+...") > > > File "/home/penberg/src/linux/scripts/coccinelle/api/memdup_user.cocci", > > > line 32, column 5, charpos = 747 > > > around = '<+...', whole content = - <+... when != goto l2; > > > Fatal error: exception Lexer_cocci.Lexical("invalid in a nonempty > > > context: <+...") > > > File "/home/penberg/src/linux/scripts/coccinelle/api/memdup_user.cocci", > > > line 32, column 5, charpos = 747 > > > around = '<+...', whole content = - <+... when != goto l2; > > > Fatal error: exception Lexer_cocci.Lexical("invalid in a nonempty > > > context: <+...") > > > File "/home/penberg/src/linux/scripts/coccinelle/api/memdup_user.cocci", > > > line 32, column 5, charpos = 747 > > > around = '<+...', whole content = - <+... when != goto l2; > > > Fatal error: exception Lexer_cocci.Lexical("invalid in a nonempty > > > context: <+...") > > > make[1]: *** [mm/slub.o] Error 1 > > > make: *** [mm/slub.o] Error 2 > > > > > > penberg@jaguar:~/src/linux$ dpkg -l|grep coccinelle > > > ii coccinelle 0.2.2.deb-2 > > > semantic patching tool for C > > > > > > [ I'm on Ubuntu 10.10. ] > > > > Indeed that one seems to be quite out of date. You can get the most > > recent version here: https://launchpad.net/~npalix/+archive/coccinelle > > I never got a working version of coccinelle with scripts in the kernel. > Even the ppa version doesn't work for me. > > make C=2 CHECK=scripts/coccicheck fs/namei.o > ... > .... > CHECK scripts/mod/empty.c > File "/home/opensource/sources/kernels/linux-2.6/scripts/coccinelle/api/memdup_user.cocci", line 32, column 5, charpos = 747 > around = '<+...', whole content = - <+... when != goto l2; > Fatal error: exception Lexer_cocci.Lexical("invalid in a nonempty context: <+...") > File "/home/opensource/sources/kernels/linux-2.6/scripts/coccinelle/api/memdup_user.cocci", line 32, column 5, charpos = 747 > around = '<+...', whole content = - <+... when != goto l2; > Fatal error: exception Lexer_cocci.Lexical("invalid in a nonempty context: <+...") > File "/home/opensource/sources/kernels/linux-2.6/scripts/coccinelle/api/memdup_user.cocci", line 32, column 5, charpos = 747 > around = '<+...', whole content = - <+... when != goto l2; > Fatal error: exception Lexer_cocci.Lexical("invalid in a nonempty context: <+...") > File "/home/opensource/sources/kernels/linux-2.6/scripts/coccinelle/api/memdup_user.cocci", line 32, column 5, charpos = 747 > around = '<+...', whole content = - <+... when != goto l2; > Fatal error: exception Lexer_cocci.Lexical("invalid in a nonempty context: <+...") That's quite strange. Could you check that you are using the ppa version and not the old version? spatch -version should give you 0.2.5-rc8 julia ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH] checkpatch: Test for kmalloc/memset(0) pairs 2011-03-24 16:08 ` Julia Lawall @ 2011-03-24 16:10 ` Nicolas Palix 2011-03-24 18:30 ` Arnaud Lacombe 2011-03-24 17:28 ` Aneesh Kumar K. V 1 sibling, 1 reply; 28+ messages in thread From: Nicolas Palix @ 2011-03-24 16:10 UTC (permalink / raw) To: Julia Lawall Cc: Aneesh Kumar K. V, Pekka Enberg, Américo Wang, Steven Rostedt, Jonathan Corbet, LKML, Andy Whitcroft, Dave Jones, Andrew Morton, Ingo Molnar On Thu, Mar 24, 2011 at 5:08 PM, Julia Lawall <julia@diku.dk> wrote: > On Thu, 24 Mar 2011, Aneesh Kumar K. V wrote: > >> On Sun, 20 Mar 2011 10:17:07 +0100 (CET), Julia Lawall <julia@diku.dk> wrote: >> > On Sun, 20 Mar 2011, Pekka Enberg wrote: >> > >> > > Hi Julia, >> > > >> > > On Sun, Mar 20, 2011 at 10:01 AM, Julia Lawall <julia@diku.dk> wrote: >> > > > Suggestions for how to make it easier to use or the documentation more >> > > > understandable are welcome. >> > > >> > > The benefit of scripts/checkpatch.pl is that it doesn't require any >> > > setting up to do. I'm personally less likely to use Coccinelle (and >> > > Sparse for that matter) on boxes where the software is not installed. >> > > I'm not sure how other people feel about it, but I'd personally love >> > > to see tools/coccinelle and tools/sparse. >> > >> > This was discussed before, and it was felt that perhaps 75000 lines of >> > ocaml code was not really appropriate for the Linux source tree, and also >> > that it would too much complicate our development process. >> > >> > One reason for using multiple machines would be to work on multiple >> > architectures. But Coccinelle is not sensitive to the architecture on >> > which it is run, so perhaps you do't need to have it installed everywhere. >> > >> > > As for something more concrete, I guess this is what I'm mostly interested in: >> > > >> > > penberg@jaguar:~/src/linux$ make C=1 CHECK="scripts/coccicheck" mm/slub.o >> > > >> > > I guess it'd be good to document that for 'make help' because now you >> > > need to dig through Documentation/coccinelle.txt to find it. >> > >> > OK, thanks. We will look into that. >> > >> > > P.S. It seems there's a bug somewhere because the above command fails >> > > miserably for me: >> > > >> > > CHK include/linux/version.h >> > > CHK include/generated/utsrelease.h >> > > CALL scripts/checksyscalls.sh >> > > CHECK mm/slub.c >> > > File "/home/penberg/src/linux/scripts/coccinelle/api/memdup_user.cocci", >> > > line 32, column 5, charpos = 747 >> > > around = '<+...', whole content = - <+... when != goto l2; >> > > Fatal error: exception Lexer_cocci.Lexical("invalid in a nonempty >> > > context: <+...") >> > > File "/home/penberg/src/linux/scripts/coccinelle/api/memdup_user.cocci", >> > > line 32, column 5, charpos = 747 >> > > around = '<+...', whole content = - <+... when != goto l2; >> > > Fatal error: exception Lexer_cocci.Lexical("invalid in a nonempty >> > > context: <+...") >> > > File "/home/penberg/src/linux/scripts/coccinelle/api/memdup_user.cocci", >> > > line 32, column 5, charpos = 747 >> > > around = '<+...', whole content = - <+... when != goto l2; >> > > Fatal error: exception Lexer_cocci.Lexical("invalid in a nonempty >> > > context: <+...") >> > > File "/home/penberg/src/linux/scripts/coccinelle/api/memdup_user.cocci", >> > > line 32, column 5, charpos = 747 >> > > around = '<+...', whole content = - <+... when != goto l2; >> > > Fatal error: exception Lexer_cocci.Lexical("invalid in a nonempty >> > > context: <+...") >> > > make[1]: *** [mm/slub.o] Error 1 >> > > make: *** [mm/slub.o] Error 2 >> > > >> > > penberg@jaguar:~/src/linux$ dpkg -l|grep coccinelle >> > > ii coccinelle 0.2.2.deb-2 >> > > semantic patching tool for C >> > > >> > > [ I'm on Ubuntu 10.10. ] >> > >> > Indeed that one seems to be quite out of date. You can get the most >> > recent version here: https://launchpad.net/~npalix/+archive/coccinelle >> >> I never got a working version of coccinelle with scripts in the kernel. >> Even the ppa version doesn't work for me. >> >> make C=2 CHECK=scripts/coccicheck fs/namei.o >> ... >> .... >> CHECK scripts/mod/empty.c >> File "/home/opensource/sources/kernels/linux-2.6/scripts/coccinelle/api/memdup_user.cocci", line 32, column 5, charpos = 747 >> around = '<+...', whole content = - <+... when != goto l2; >> Fatal error: exception Lexer_cocci.Lexical("invalid in a nonempty context: <+...") >> File "/home/opensource/sources/kernels/linux-2.6/scripts/coccinelle/api/memdup_user.cocci", line 32, column 5, charpos = 747 >> around = '<+...', whole content = - <+... when != goto l2; >> Fatal error: exception Lexer_cocci.Lexical("invalid in a nonempty context: <+...") >> File "/home/opensource/sources/kernels/linux-2.6/scripts/coccinelle/api/memdup_user.cocci", line 32, column 5, charpos = 747 >> around = '<+...', whole content = - <+... when != goto l2; >> Fatal error: exception Lexer_cocci.Lexical("invalid in a nonempty context: <+...") >> File "/home/opensource/sources/kernels/linux-2.6/scripts/coccinelle/api/memdup_user.cocci", line 32, column 5, charpos = 747 >> around = '<+...', whole content = - <+... when != goto l2; >> Fatal error: exception Lexer_cocci.Lexical("invalid in a nonempty context: <+...") > > That's quite strange. Could you check that you are using the ppa version > and not the old version? spatch -version should give you 0.2.5-rc8 Indeed... I just checked and I got not error. npalix@penpen:~/Build/linux$ make C=2 CHECK=scripts/coccicheck COCCI="scripts/coccinelle/api/memdup_user.cocci" fs/namei.o CHK include/linux/version.h CHK include/generated/utsrelease.h CALL scripts/checksyscalls.sh CHECK scripts/mod/empty.c CHECK fs/namei.c npalix@penpen:~/Build/linux$ make C=2 CHECK=scripts/coccicheck fs/namei.o CHK include/linux/version.h CHK include/generated/utsrelease.h CALL scripts/checksyscalls.sh CHECK scripts/mod/empty.c CHECK fs/namei.c npalix@penpen:~/Build/linux$ spatch -version spatch version 0.2.5-rc8 with Python support > > julia > -- Nicolas Palix http://sardes.inrialpes.fr/~npalix/ ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH] checkpatch: Test for kmalloc/memset(0) pairs 2011-03-24 16:10 ` Nicolas Palix @ 2011-03-24 18:30 ` Arnaud Lacombe 2011-03-24 20:39 ` Julia Lawall 0 siblings, 1 reply; 28+ messages in thread From: Arnaud Lacombe @ 2011-03-24 18:30 UTC (permalink / raw) To: Nicolas Palix Cc: Julia Lawall, Aneesh Kumar K. V, Pekka Enberg, Américo Wang, Steven Rostedt, Jonathan Corbet, LKML, Andy Whitcroft, Dave Jones, Andrew Morton, Ingo Molnar Hi, 2011/3/24 Nicolas Palix <Nicolas.Palix@inria.fr>: > On Thu, Mar 24, 2011 at 5:08 PM, Julia Lawall <julia@diku.dk> wrote: >> On Thu, 24 Mar 2011, Aneesh Kumar K. V wrote: >> >>> On Sun, 20 Mar 2011 10:17:07 +0100 (CET), Julia Lawall <julia@diku.dk> wrote: >>> > On Sun, 20 Mar 2011, Pekka Enberg wrote: >>> > >>> > > Hi Julia, >>> > > >>> > > On Sun, Mar 20, 2011 at 10:01 AM, Julia Lawall <julia@diku.dk> wrote: >>> > > > Suggestions for how to make it easier to use or the documentation more >>> > > > understandable are welcome. >>> > > >>> > > The benefit of scripts/checkpatch.pl is that it doesn't require any >>> > > setting up to do. I'm personally less likely to use Coccinelle (and >>> > > Sparse for that matter) on boxes where the software is not installed. >>> > > I'm not sure how other people feel about it, but I'd personally love >>> > > to see tools/coccinelle and tools/sparse. >>> > >>> > This was discussed before, and it was felt that perhaps 75000 lines of >>> > ocaml code was not really appropriate for the Linux source tree, and also >>> > that it would too much complicate our development process. >>> > >>> > One reason for using multiple machines would be to work on multiple >>> > architectures. But Coccinelle is not sensitive to the architecture on >>> > which it is run, so perhaps you do't need to have it installed everywhere. >>> > >>> > > As for something more concrete, I guess this is what I'm mostly interested in: >>> > > >>> > > penberg@jaguar:~/src/linux$ make C=1 CHECK="scripts/coccicheck" mm/slub.o >>> > > >>> > > I guess it'd be good to document that for 'make help' because now you >>> > > need to dig through Documentation/coccinelle.txt to find it. >>> > >>> > OK, thanks. We will look into that. >>> > >>> > > P.S. It seems there's a bug somewhere because the above command fails >>> > > miserably for me: >>> > > >>> > > CHK include/linux/version.h >>> > > CHK include/generated/utsrelease.h >>> > > CALL scripts/checksyscalls.sh >>> > > CHECK mm/slub.c >>> > > File "/home/penberg/src/linux/scripts/coccinelle/api/memdup_user.cocci", >>> > > line 32, column 5, charpos = 747 >>> > > around = '<+...', whole content = - <+... when != goto l2; >>> > > Fatal error: exception Lexer_cocci.Lexical("invalid in a nonempty >>> > > context: <+...") >>> > > File "/home/penberg/src/linux/scripts/coccinelle/api/memdup_user.cocci", >>> > > line 32, column 5, charpos = 747 >>> > > around = '<+...', whole content = - <+... when != goto l2; >>> > > Fatal error: exception Lexer_cocci.Lexical("invalid in a nonempty >>> > > context: <+...") >>> > > File "/home/penberg/src/linux/scripts/coccinelle/api/memdup_user.cocci", >>> > > line 32, column 5, charpos = 747 >>> > > around = '<+...', whole content = - <+... when != goto l2; >>> > > Fatal error: exception Lexer_cocci.Lexical("invalid in a nonempty >>> > > context: <+...") >>> > > File "/home/penberg/src/linux/scripts/coccinelle/api/memdup_user.cocci", >>> > > line 32, column 5, charpos = 747 >>> > > around = '<+...', whole content = - <+... when != goto l2; >>> > > Fatal error: exception Lexer_cocci.Lexical("invalid in a nonempty >>> > > context: <+...") >>> > > make[1]: *** [mm/slub.o] Error 1 >>> > > make: *** [mm/slub.o] Error 2 >>> > > >>> > > penberg@jaguar:~/src/linux$ dpkg -l|grep coccinelle >>> > > ii coccinelle 0.2.2.deb-2 >>> > > semantic patching tool for C >>> > > >>> > > [ I'm on Ubuntu 10.10. ] >>> > >>> > Indeed that one seems to be quite out of date. You can get the most >>> > recent version here: https://launchpad.net/~npalix/+archive/coccinelle >>> >>> I never got a working version of coccinelle with scripts in the kernel. >>> Even the ppa version doesn't work for me. >>> >>> make C=2 CHECK=scripts/coccicheck fs/namei.o >>> ... >>> .... >>> CHECK scripts/mod/empty.c >>> File "/home/opensource/sources/kernels/linux-2.6/scripts/coccinelle/api/memdup_user.cocci", line 32, column 5, charpos = 747 >>> around = '<+...', whole content = - <+... when != goto l2; >>> Fatal error: exception Lexer_cocci.Lexical("invalid in a nonempty context: <+...") >>> File "/home/opensource/sources/kernels/linux-2.6/scripts/coccinelle/api/memdup_user.cocci", line 32, column 5, charpos = 747 >>> around = '<+...', whole content = - <+... when != goto l2; >>> Fatal error: exception Lexer_cocci.Lexical("invalid in a nonempty context: <+...") >>> File "/home/opensource/sources/kernels/linux-2.6/scripts/coccinelle/api/memdup_user.cocci", line 32, column 5, charpos = 747 >>> around = '<+...', whole content = - <+... when != goto l2; >>> Fatal error: exception Lexer_cocci.Lexical("invalid in a nonempty context: <+...") >>> File "/home/opensource/sources/kernels/linux-2.6/scripts/coccinelle/api/memdup_user.cocci", line 32, column 5, charpos = 747 >>> around = '<+...', whole content = - <+... when != goto l2; >>> Fatal error: exception Lexer_cocci.Lexical("invalid in a nonempty context: <+...") >> >> That's quite strange. Could you check that you are using the ppa version >> and not the old version? spatch -version should give you 0.2.5-rc8 > > Indeed... I just checked and I got not error. > > npalix@penpen:~/Build/linux$ make C=2 CHECK=scripts/coccicheck > COCCI="scripts/coccinelle/api/memdup_user.cocci" fs/namei.o > CHK include/linux/version.h > CHK include/generated/utsrelease.h > CALL scripts/checksyscalls.sh > CHECK scripts/mod/empty.c > CHECK fs/namei.c > npalix@penpen:~/Build/linux$ make C=2 CHECK=scripts/coccicheck > fs/namei.o CHK include/linux/version.h > CHK include/generated/utsrelease.h > CALL scripts/checksyscalls.sh > CHECK scripts/mod/empty.c > CHECK fs/namei.c > npalix@penpen:~/Build/linux$ spatch -version > spatch version 0.2.5-rc8 with Python support > So does it mean that you do not even have a stable grammar ? Do you at least offer any guarantee that a script made for version X will still work in version X+1 ? Thanks, - Arnaud ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH] checkpatch: Test for kmalloc/memset(0) pairs 2011-03-24 18:30 ` Arnaud Lacombe @ 2011-03-24 20:39 ` Julia Lawall 0 siblings, 0 replies; 28+ messages in thread From: Julia Lawall @ 2011-03-24 20:39 UTC (permalink / raw) To: Arnaud Lacombe Cc: Nicolas Palix, Aneesh Kumar K. V, Pekka Enberg, Américo Wang, Steven Rostedt, Jonathan Corbet, LKML, Andy Whitcroft, Dave Jones, Andrew Morton, Ingo Molnar > So does it mean that you do not even have a stable grammar ? Do you at > least offer any guarantee that a script made for version X will still > work in version X+1 ? The inability to put - on <+... ...+> was a bug. So I don't know if fixing a bug would be considered a sign of an unstable grammar. There was also one deliberate change in the grammar some rc's ago related to the use of commas in sequences (parameter lists etc). But otherwise, the changes in the grammar have been additions, as people eg ask for new kinds of metavariables. But Coccinelle is a research prototype under development, not a product. So I'm not sure it is appropriate to say that we guarantee anything. julia ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH] checkpatch: Test for kmalloc/memset(0) pairs 2011-03-24 16:08 ` Julia Lawall 2011-03-24 16:10 ` Nicolas Palix @ 2011-03-24 17:28 ` Aneesh Kumar K. V 1 sibling, 0 replies; 28+ messages in thread From: Aneesh Kumar K. V @ 2011-03-24 17:28 UTC (permalink / raw) To: Julia Lawall Cc: Pekka Enberg, Américo Wang, Steven Rostedt, Jonathan Corbet, LKML, Andy Whitcroft, Dave Jones, Andrew Morton, Nicolas Palix, Ingo Molnar On Thu, 24 Mar 2011 17:08:01 +0100 (CET), Julia Lawall <julia@diku.dk> wrote: > On Thu, 24 Mar 2011, Aneesh Kumar K. V wrote: > > > On Sun, 20 Mar 2011 10:17:07 +0100 (CET), Julia Lawall <julia@diku.dk> wrote: > > > On Sun, 20 Mar 2011, Pekka Enberg wrote: > > > > > > > Hi Julia, > > > > > > > > On Sun, Mar 20, 2011 at 10:01 AM, Julia Lawall <julia@diku.dk> wrote: > > > > > Suggestions for how to make it easier to use or the documentation more > > > > > understandable are welcome. > > > > > > > > The benefit of scripts/checkpatch.pl is that it doesn't require any > > > > setting up to do. I'm personally less likely to use Coccinelle (and > > > > Sparse for that matter) on boxes where the software is not installed. > > > > I'm not sure how other people feel about it, but I'd personally love > > > > to see tools/coccinelle and tools/sparse. > > > > > > This was discussed before, and it was felt that perhaps 75000 lines of > > > ocaml code was not really appropriate for the Linux source tree, and also > > > that it would too much complicate our development process. > > > > > > One reason for using multiple machines would be to work on multiple > > > architectures. But Coccinelle is not sensitive to the architecture on > > > which it is run, so perhaps you do't need to have it installed everywhere. > > > > > > > As for something more concrete, I guess this is what I'm mostly interested in: > > > > > > > > penberg@jaguar:~/src/linux$ make C=1 CHECK="scripts/coccicheck" mm/slub.o > > > > > > > > I guess it'd be good to document that for 'make help' because now you > > > > need to dig through Documentation/coccinelle.txt to find it. > > > > > > OK, thanks. We will look into that. > > > > > > > P.S. It seems there's a bug somewhere because the above command fails > > > > miserably for me: > > > > > > > > CHK include/linux/version.h > > > > CHK include/generated/utsrelease.h > > > > CALL scripts/checksyscalls.sh > > > > CHECK mm/slub.c > > > > File "/home/penberg/src/linux/scripts/coccinelle/api/memdup_user.cocci", > > > > line 32, column 5, charpos = 747 > > > > around = '<+...', whole content = - <+... when != goto l2; > > > > Fatal error: exception Lexer_cocci.Lexical("invalid in a nonempty > > > > context: <+...") > > > > File "/home/penberg/src/linux/scripts/coccinelle/api/memdup_user.cocci", > > > > line 32, column 5, charpos = 747 > > > > around = '<+...', whole content = - <+... when != goto l2; > > > > Fatal error: exception Lexer_cocci.Lexical("invalid in a nonempty > > > > context: <+...") > > > > File "/home/penberg/src/linux/scripts/coccinelle/api/memdup_user.cocci", > > > > line 32, column 5, charpos = 747 > > > > around = '<+...', whole content = - <+... when != goto l2; > > > > Fatal error: exception Lexer_cocci.Lexical("invalid in a nonempty > > > > context: <+...") > > > > File "/home/penberg/src/linux/scripts/coccinelle/api/memdup_user.cocci", > > > > line 32, column 5, charpos = 747 > > > > around = '<+...', whole content = - <+... when != goto l2; > > > > Fatal error: exception Lexer_cocci.Lexical("invalid in a nonempty > > > > context: <+...") > > > > make[1]: *** [mm/slub.o] Error 1 > > > > make: *** [mm/slub.o] Error 2 > > > > > > > > penberg@jaguar:~/src/linux$ dpkg -l|grep coccinelle > > > > ii coccinelle 0.2.2.deb-2 > > > > semantic patching tool for C > > > > > > > > [ I'm on Ubuntu 10.10. ] > > > > > > Indeed that one seems to be quite out of date. You can get the most > > > recent version here: https://launchpad.net/~npalix/+archive/coccinelle > > > > I never got a working version of coccinelle with scripts in the kernel. > > Even the ppa version doesn't work for me. > > > > make C=2 CHECK=scripts/coccicheck fs/namei.o > > ... > > .... > > CHECK scripts/mod/empty.c > > File "/home/opensource/sources/kernels/linux-2.6/scripts/coccinelle/api/memdup_user.cocci", line 32, column 5, charpos = 747 > > around = '<+...', whole content = - <+... when != goto l2; > > Fatal error: exception Lexer_cocci.Lexical("invalid in a nonempty context: <+...") > > File "/home/opensource/sources/kernels/linux-2.6/scripts/coccinelle/api/memdup_user.cocci", line 32, column 5, charpos = 747 > > around = '<+...', whole content = - <+... when != goto l2; > > Fatal error: exception Lexer_cocci.Lexical("invalid in a nonempty context: <+...") > > File "/home/opensource/sources/kernels/linux-2.6/scripts/coccinelle/api/memdup_user.cocci", line 32, column 5, charpos = 747 > > around = '<+...', whole content = - <+... when != goto l2; > > Fatal error: exception Lexer_cocci.Lexical("invalid in a nonempty context: <+...") > > File "/home/opensource/sources/kernels/linux-2.6/scripts/coccinelle/api/memdup_user.cocci", line 32, column 5, charpos = 747 > > around = '<+...', whole content = - <+... when != goto l2; > > Fatal error: exception Lexer_cocci.Lexical("invalid in a nonempty context: <+...") > > That's quite strange. Could you check that you are using the ppa version > and not the old version? spatch -version should give you 0.2.5-rc8 missed an apt-get update between add-apt-repository and apt-get install -aneesh ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH] checkpatch: Test for kmalloc/memset(0) pairs 2011-03-18 3:15 ` Jonathan Corbet 2011-03-18 3:32 ` Steven Rostedt @ 2011-03-19 19:39 ` Dave Jones 2011-03-21 13:26 ` Steven Rostedt 1 sibling, 1 reply; 28+ messages in thread From: Dave Jones @ 2011-03-19 19:39 UTC (permalink / raw) To: Jonathan Corbet; +Cc: Steven Rostedt, LKML, Andy Whitcroft, Andrew Morton On Thu, Mar 17, 2011 at 09:15:48PM -0600, Jonathan Corbet wrote: > On Thu, 17 Mar 2011 22:52:24 -0400 > Steven Rostedt <rostedt@goodmis.org> wrote: > > > The use of kzalloc() is preferred over kmalloc/memset(0) pairs. > > > > When a match is made with "memset(p, 0, s);" a search back through the > > patch hunk is made looking for "p = kmalloc(s,". If that is found, then > > a warning is given, suggesting to use kzalloc() instead. > > The Coccinelle stuff already has a lot of this kind of test. See, for > example, scripts/coccinelle/api/alloc/kzalloc-simple.cocci. Suppose > there is some way all this nice analysis infrastructure could be > integrated instead of duplicated? Or am I just a crazy dreamer? Maybe. Coccinelle is one of those tools that seems to be perpetually on my "to look into" list that I never get around to. Something that has crossed my mind over the last few days was the idea of splitting checkpatch into two tools. One for checking CodingStyle issues, and one for checking for actual code problems like the memset example. The motivation for such is that I think it's pretty clear that many maintainers never run checkpatch on patches they queue up before pushing to Linus... $ scripts/checkpatch.pl ~/Mail/upstream/2.6.39/head-March-18-2011 | wc -l 2361 The bulk of this is all "missing space here" "don't put a space there" type fluff that most maintainers just don't care about. Any valuable warnings are lost in the noise. If we had a separate tool to check for real flaws, (or even a way to suppress the stylistic warnings from the existing one) maybe more maintainers would run their changes through it. I dunno, maybe I'm just a crazy dreamer too, but I think many people have written checkpatch off as useless in its current incarnation. Dave ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH] checkpatch: Test for kmalloc/memset(0) pairs 2011-03-19 19:39 ` Dave Jones @ 2011-03-21 13:26 ` Steven Rostedt 2011-03-21 13:29 ` Steven Rostedt ` (2 more replies) 0 siblings, 3 replies; 28+ messages in thread From: Steven Rostedt @ 2011-03-21 13:26 UTC (permalink / raw) To: Dave Jones; +Cc: Jonathan Corbet, LKML, Andy Whitcroft, Andrew Morton On Sat, 2011-03-19 at 15:39 -0400, Dave Jones wrote: > Something that has crossed my mind over the last few days was the idea > of splitting checkpatch into two tools. > One for checking CodingStyle issues, and one for checking for actual > code problems like the memset example. > > The motivation for such is that I think it's pretty clear that many maintainers > never run checkpatch on patches they queue up before pushing to Linus... > > $ scripts/checkpatch.pl ~/Mail/upstream/2.6.39/head-March-18-2011 | wc -l > 2361 > I run it on all my patches. But there are some warnings that I ignore. Sometimes I don't split the 80char lines if doing so makes the code even uglier. > The bulk of this is all "missing space here" "don't put a space there" type > fluff that most maintainers just don't care about. Any valuable warnings > are lost in the noise. If we had a separate tool to check for real flaws, > (or even a way to suppress the stylistic warnings from the existing one) > maybe more maintainers would run their changes through it. As I replied with my phone, having a suppress warnings would be nice. checkpatch -e patch where -e is errors only? > > I dunno, maybe I'm just a crazy dreamer too, but I think many people > have written checkpatch off as useless in its current incarnation. Well I and I'm sure Ingo use it quite a bit. I'm sure there are others that do too. I would really like to disable warnings, as the patches to my magic macros break all sorts of checkpatch formatting rules, but real errors may still exist. And because of that, I seldom use checkpatch on patches that modify those macros. Note, I've even found myself running checkpatch on non Linux code too ;) -- Steve ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH] checkpatch: Test for kmalloc/memset(0) pairs 2011-03-21 13:26 ` Steven Rostedt @ 2011-03-21 13:29 ` Steven Rostedt 2011-03-21 13:55 ` Borislav Petkov 2011-03-22 19:58 ` checkpatch: introduce --nocs to disable CodingStyle warnings Dave Jones 2 siblings, 0 replies; 28+ messages in thread From: Steven Rostedt @ 2011-03-21 13:29 UTC (permalink / raw) To: Dave Jones; +Cc: Jonathan Corbet, LKML, Andy Whitcroft, Andrew Morton Also, there seems to be one important thing missing from this thread. Can we please apply my patch? It may be months or years before we decide how to handle this checkpatch vs Coccinelle and I think it is important that we flag this as a warning/error. -- Steve ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH] checkpatch: Test for kmalloc/memset(0) pairs 2011-03-21 13:26 ` Steven Rostedt 2011-03-21 13:29 ` Steven Rostedt @ 2011-03-21 13:55 ` Borislav Petkov 2011-03-22 19:58 ` checkpatch: introduce --nocs to disable CodingStyle warnings Dave Jones 2 siblings, 0 replies; 28+ messages in thread From: Borislav Petkov @ 2011-03-21 13:55 UTC (permalink / raw) To: Steven Rostedt Cc: Dave Jones, Jonathan Corbet, LKML, Andy Whitcroft, Andrew Morton On Mon, Mar 21, 2011 at 09:26:20AM -0400, Steven Rostedt wrote: > On Sat, 2011-03-19 at 15:39 -0400, Dave Jones wrote: > > > Something that has crossed my mind over the last few days was the idea > > of splitting checkpatch into two tools. > > One for checking CodingStyle issues, and one for checking for actual > > code problems like the memset example. > > > > The motivation for such is that I think it's pretty clear that many maintainers > > never run checkpatch on patches they queue up before pushing to Linus... > > > > $ scripts/checkpatch.pl ~/Mail/upstream/2.6.39/head-March-18-2011 | wc -l > > 2361 > > > > I run it on all my patches. But there are some warnings that I ignore. > Sometimes I don't split the 80char lines if doing so makes the code even > uglier. Same here, certain warnings checkpatch spits are plain dumb in certain situations. What we want is to fix checkpatch to warn/error out only on real issues which are valid 100% of the cases and stick all the remaining, not-always-an-issue stuff which deserves a warning only sometimes behind a "--pedantic" or "--extended-checks" option or whatever. > > The bulk of this is all "missing space here" "don't put a space there" type > > fluff that most maintainers just don't care about. Any valuable warnings > > are lost in the noise. If we had a separate tool to check for real flaws, > > (or even a way to suppress the stylistic warnings from the existing one) > > maybe more maintainers would run their changes through it. > > As I replied with my phone, having a suppress warnings would be nice. > > checkpatch -e patch > > where -e is errors only? Yeah, let's enable the errors-only checking by default, i.e. no args and if you want additional ones, you need to switch on an option. > > > > I dunno, maybe I'm just a crazy dreamer too, but I think many people > > have written checkpatch off as useless in its current incarnation. > > Well I and I'm sure Ingo use it quite a bit. I'm sure there are others > that do too. Yep, I have it as a pre-commit hook in git and it helps a lot. However, sometimes I need to do '--no-verify' on false positives. -- Regards/Gruss, Boris. Advanced Micro Devices GmbH Einsteinring 24, 85609 Dornach General Managers: Alberto Bozzo, Andrew Bowd Registration: Dornach, Gemeinde Aschheim, Landkreis Muenchen Registergericht Muenchen, HRB Nr. 43632 ^ permalink raw reply [flat|nested] 28+ messages in thread
* checkpatch: introduce --nocs to disable CodingStyle warnings. 2011-03-21 13:26 ` Steven Rostedt 2011-03-21 13:29 ` Steven Rostedt 2011-03-21 13:55 ` Borislav Petkov @ 2011-03-22 19:58 ` Dave Jones 2011-03-22 20:21 ` Joe Perches 2 siblings, 1 reply; 28+ messages in thread From: Dave Jones @ 2011-03-22 19:58 UTC (permalink / raw) To: Steven Rostedt; +Cc: Jonathan Corbet, LKML, Andy Whitcroft, Andrew Morton > I would really like to disable warnings, as the patches to my magic > macros break all sorts of checkpatch formatting rules, but real errors > may still exist. How about this ? running checkpatch with --nocs will now only print non-CodingStyle related warnings. I may have missed some of the conversions, but this seems to silence the more egregious 'noise'. Signed-off-by: Dave Jones <davej@redhat.com> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl index 58848e3..a2b0086 100755 --- a/scripts/checkpatch.pl +++ b/scripts/checkpatch.pl @@ -19,6 +19,7 @@ my $tree = 1; my $chk_signoff = 1; my $chk_patch = 1; my $tst_only; +my $nocs = 0; my $emacs = 0; my $terse = 0; my $file = 0; @@ -55,6 +56,7 @@ Options: is all off) --test-only=WORD report only warnings/errors containing WORD literally + --nocs don't display warnings about CodingStyle. -h, --help, --version display this help and exit When FILE is - read standard input. @@ -80,6 +82,7 @@ GetOptions( 'debug=s' => \%debug, 'test-only=s' => \$tst_only, + 'nocs' => \$nocs, 'h|help' => \$help, 'version' => \$help ) or help(1); @@ -1113,6 +1116,22 @@ sub WARN { our $cnt_warn++; } } +sub CS_ERROR { + if ($nocs == 0) { + if (report("ERROR: $_[0]\n")) { + our $clean = 0; + our $cnt_error++; + } + } +} +sub CS_WARN { + if ($nocs == 0) { + if (report("WARNING: $_[0]\n")) { + our $clean = 0; + our $cnt_warn++; + } + } +} sub CHK { if ($check && report("CHECK: $_[0]\n")) { our $clean = 0; @@ -1417,11 +1436,11 @@ sub process { #trailing whitespace if ($line =~ /^\+.*\015/) { my $herevet = "$here\n" . cat_vet($rawline) . "\n"; - ERROR("DOS line endings\n" . $herevet); + CS_ERROR("DOS line endings\n" . $herevet); } elsif ($rawline =~ /^\+.*\S\s+$/ || $rawline =~ /^\+\s+$/) { my $herevet = "$here\n" . cat_vet($rawline) . "\n"; - ERROR("trailing whitespace\n" . $herevet); + CS_ERROR("trailing whitespace\n" . $herevet); $rpt_cleaners = 1; } @@ -1466,17 +1485,17 @@ sub process { $line =~ /^\+\s*"[^"]*"\s*(?:\s*|,|\)\s*;)\s*$/) && $length > 80) { - WARN("line over 80 characters\n" . $herecurr); + CS_WARN("line over 80 characters\n" . $herecurr); } # check for spaces before a quoted newline if ($rawline =~ /^.*\".*\s\\n/) { - WARN("unnecessary whitespace before a quoted newline\n" . $herecurr); + CS_WARN("unnecessary whitespace before a quoted newline\n" . $herecurr); } # check for adding lines without a newline. if ($line =~ /^\+/ && defined $lines[$linenr] && $lines[$linenr] =~ /^\\ No newline at end of file/) { - WARN("adding a line without newline at end of file\n" . $herecurr); + CS_WARN("adding a line without newline at end of file\n" . $herecurr); } # Blackfin: use hi/lo macros @@ -1499,14 +1518,14 @@ sub process { if ($rawline =~ /^\+\s* \t\s*\S/ || $rawline =~ /^\+\s* \s*/) { my $herevet = "$here\n" . cat_vet($rawline) . "\n"; - ERROR("code indent should use tabs where possible\n" . $herevet); + CS_ERROR("code indent should use tabs where possible\n" . $herevet); $rpt_cleaners = 1; } # check for space before tabs. if ($rawline =~ /^\+/ && $rawline =~ / \t/) { my $herevet = "$here\n" . cat_vet($rawline) . "\n"; - WARN("please, no space before tabs\n" . $herevet); + CS_WARN("please, no space before tabs\n" . $herevet); } # check for spaces at the beginning of a line. @@ -1516,7 +1535,7 @@ sub process { # 3) hanging labels if ($rawline =~ /^\+ / && $line !~ /\+ *(?:$;|#|$Ident:)/) { my $herevet = "$here\n" . cat_vet($rawline) . "\n"; - WARN("please, no spaces at the start of a line\n" . $herevet); + CS_WARN("please, no spaces at the start of a line\n" . $herevet); } # check we are in a valid C source file if not then ignore this hunk @@ -1623,7 +1642,7 @@ sub process { } } if ($err ne '') { - ERROR("switch and case should be at the same indent\n$hereline$err"); + CS_ERROR("switch and case should be at the same indent\n$hereline$err"); } } @@ -1651,7 +1670,7 @@ sub process { #print "pre<$pre_ctx>\nline<$line>\nctx<$ctx>\nnext<$lines[$ctx_ln - 1]>\n"; if ($ctx !~ /{\s*/ && defined($lines[$ctx_ln -1]) && $lines[$ctx_ln - 1] =~ /^\+\s*{/) { - ERROR("that open brace { should be on the previous line\n" . + CS_ERROR("that open brace { should be on the previous line\n" . "$here\n$ctx\n$rawlines[$ctx_ln - 1]\n"); } if ($level == 0 && $pre_ctx !~ /}\s*while\s*\($/ && @@ -1790,7 +1809,7 @@ sub process { # check for initialisation to aggregates open brace on the next line if ($line =~ /^.\s*{/ && $prevline =~ /(?:^|[^=])=\s*$/) { - ERROR("that open brace { should be on the previous line\n" . $hereprev); + CS_ERROR("that open brace { should be on the previous line\n" . $hereprev); } # @@ -1808,7 +1827,7 @@ sub process { # no C99 // comments if ($line =~ m{//}) { - ERROR("do not use C99 // comments\n" . $herecurr); + CS_ERROR("do not use C99 // comments\n" . $herecurr); } # Remove C99 comments. $line =~ s@//.*@@; @@ -1911,7 +1930,7 @@ sub process { #print "from<$from> to<$to>\n"; if ($from ne $to) { - ERROR("\"(foo$from)\" should be \"(foo$to)\"\n" . $herecurr); + CS_ERROR("\"(foo$from)\" should be \"(foo$to)\"\n" . $herecurr); } } elsif ($line =~ m{\b$NonptrType(\s*(?:$Modifier\b\s*|\*\s*)+)($Ident)}) { my ($from, $to, $ident) = ($1, $1, $2); @@ -1928,7 +1947,7 @@ sub process { #print "from<$from> to<$to> ident<$ident>\n"; if ($from ne $to && $ident !~ /^$Modifier$/) { - ERROR("\"foo${from}bar\" should be \"foo${to}bar\"\n" . $herecurr); + CS_ERROR("\"foo${from}bar\" should be \"foo${to}bar\"\n" . $herecurr); } } @@ -1970,18 +1989,18 @@ sub process { # or if closed on same line if (($line=~/$Type\s*$Ident\(.*\).*\s{/) and !($line=~/\#\s*define.*do\s{/) and !($line=~/}/)) { - ERROR("open brace '{' following function declarations go on the next line\n" . $herecurr); + CS_ERROR("open brace '{' following function declarations go on the next line\n" . $herecurr); } # open braces for enum, union and struct go on the same line. if ($line =~ /^.\s*{/ && $prevline =~ /^.\s*(?:typedef\s+)?(enum|union|struct)(?:\s+$Ident)?\s*$/) { - ERROR("open brace '{' following $1 go on the same line\n" . $hereprev); + CS_ERROR("open brace '{' following $1 go on the same line\n" . $hereprev); } # missing space after union, struct or enum definition if ($line =~ /^.\s*(?:typedef\s+)?(enum|union|struct)(?:\s+$Ident)?(?:\s+$Ident)?[=\{]/) { - WARN("missing space after $1 definition\n" . $herecurr); + CS_WARN("missing space after $1 definition\n" . $herecurr); } # check for spacing round square brackets; allowed: @@ -1993,7 +2012,7 @@ sub process { if ($prefix !~ /$Type\s+$/ && ($where != 0 || $prefix !~ /^.\s+$/) && $prefix !~ /{\s+$/) { - ERROR("space prohibited before open square bracket '['\n" . $herecurr); + CS_ERROR("space prohibited before open square bracket '['\n" . $herecurr); } } @@ -2024,7 +2043,7 @@ sub process { } elsif ($ctx =~ /$Type$/) { } else { - WARN("space prohibited between function name and open parenthesis '('\n" . $herecurr); + CS_WARN("space prohibited between function name and open parenthesis '('\n" . $herecurr); } } # Check operator spacing. @@ -2098,7 +2117,7 @@ sub process { } elsif ($op eq ';') { if ($ctx !~ /.x[WEBC]/ && $cc !~ /^\\/ && $cc !~ /^;/) { - ERROR("space required after that '$op' $at\n" . $hereptr); + CS_ERROR("space required after that '$op' $at\n" . $hereptr); } # // is a comment @@ -2109,13 +2128,13 @@ sub process { # : when part of a bitfield } elsif ($op eq '->' || $opv eq ':B') { if ($ctx =~ /Wx.|.xW/) { - ERROR("spaces prohibited around that '$op' $at\n" . $hereptr); + CS_ERROR("spaces prohibited around that '$op' $at\n" . $hereptr); } # , must have a space on the right. } elsif ($op eq ',') { if ($ctx !~ /.x[WEC]/ && $cc !~ /^}/) { - ERROR("space required after that '$op' $at\n" . $hereptr); + CS_ERROR("space required after that '$op' $at\n" . $hereptr); } # '*' as part of a type definition -- reported already. @@ -2129,26 +2148,26 @@ sub process { $opv eq '*U' || $opv eq '-U' || $opv eq '&U' || $opv eq '&&U') { if ($ctx !~ /[WEBC]x./ && $ca !~ /(?:\)|!|~|\*|-|\&|\||\+\+|\-\-|\{)$/) { - ERROR("space required before that '$op' $at\n" . $hereptr); + CS_ERROR("space required before that '$op' $at\n" . $hereptr); } if ($op eq '*' && $cc =~/\s*$Modifier\b/) { # A unary '*' may be const } elsif ($ctx =~ /.xW/) { - ERROR("space prohibited after that '$op' $at\n" . $hereptr); + CS_ERROR("space prohibited after that '$op' $at\n" . $hereptr); } # unary ++ and unary -- are allowed no space on one side. } elsif ($op eq '++' or $op eq '--') { if ($ctx !~ /[WEOBC]x[^W]/ && $ctx !~ /[^W]x[WOBEC]/) { - ERROR("space required one side of that '$op' $at\n" . $hereptr); + CS_ERROR("space required one side of that '$op' $at\n" . $hereptr); } if ($ctx =~ /Wx[BE]/ || ($ctx =~ /Wx./ && $cc =~ /^;/)) { - ERROR("space prohibited before that '$op' $at\n" . $hereptr); + CS_ERROR("space prohibited before that '$op' $at\n" . $hereptr); } if ($ctx =~ /ExW/) { - ERROR("space prohibited after that '$op' $at\n" . $hereptr); + CS_ERROR("space prohibited after that '$op' $at\n" . $hereptr); } @@ -2160,7 +2179,7 @@ sub process { $op eq '%') { if ($ctx =~ /Wx[^WCE]|[^WCE]xW/) { - ERROR("need consistent spacing around '$op' $at\n" . + CS_ERROR("need consistent spacing around '$op' $at\n" . $hereptr); } @@ -2168,7 +2187,7 @@ sub process { # terminating a case value or a label. } elsif ($opv eq ':C' || $opv eq ':L') { if ($ctx =~ /Wx./) { - ERROR("space prohibited before that '$op' $at\n" . $hereptr); + CS_ERROR("space prohibited before that '$op' $at\n" . $hereptr); } # All the others need spaces both sides. @@ -2191,7 +2210,7 @@ sub process { } if ($ok == 0) { - ERROR("spaces required around that '$op' $at\n" . $hereptr); + CS_ERROR("spaces required around that '$op' $at\n" . $hereptr); } } $off += length($elements[$n + 1]); @@ -2221,38 +2240,38 @@ sub process { #need space before brace following if, while, etc if (($line =~ /\(.*\){/ && $line !~ /\($Type\){/) || $line =~ /do{/) { - ERROR("space required before the open brace '{'\n" . $herecurr); + CS_ERROR("space required before the open brace '{'\n" . $herecurr); } # closing brace should have a space following it when it has anything # on the line if ($line =~ /}(?!(?:,|;|\)))\S/) { - ERROR("space required after that close brace '}'\n" . $herecurr); + CS_ERROR("space required after that close brace '}'\n" . $herecurr); } # check spacing on square brackets if ($line =~ /\[\s/ && $line !~ /\[\s*$/) { - ERROR("space prohibited after that open square bracket '['\n" . $herecurr); + CS_ERROR("space prohibited after that open square bracket '['\n" . $herecurr); } if ($line =~ /\s\]/) { - ERROR("space prohibited before that close square bracket ']'\n" . $herecurr); + CS_ERROR("space prohibited before that close square bracket ']'\n" . $herecurr); } # check spacing on parentheses if ($line =~ /\(\s/ && $line !~ /\(\s*(?:\\)?$/ && $line !~ /for\s*\(\s+;/) { - ERROR("space prohibited after that open parenthesis '('\n" . $herecurr); + CS_ERROR("space prohibited after that open parenthesis '('\n" . $herecurr); } if ($line =~ /(\s+)\)/ && $line !~ /^.\s*\)/ && $line !~ /for\s*\(.*;\s+\)/ && $line !~ /:\s+\)/) { - ERROR("space prohibited before that close parenthesis ')'\n" . $herecurr); + CS_ERROR("space prohibited before that close parenthesis ')'\n" . $herecurr); } #goto labels aren't indented, allow a single space however if ($line=~/^.\s+[A-Za-z\d_]+:(?![0-9]+)/ and !($line=~/^. [A-Za-z\d_]+:/) and !($line=~/^.\s+default:/)) { - WARN("labels should not be indented\n" . $herecurr); + CS_WARN("labels should not be indented\n" . $herecurr); } # Return is not a function. @@ -2271,10 +2290,10 @@ sub process { } #print "value<$value>\n"; if ($value =~ /^\s*(?:$Ident|-?$Constant)\s*$/) { - ERROR("return is not a function, parentheses are not required\n" . $herecurr); + CS_ERROR("return is not a function, parentheses are not required\n" . $herecurr); } elsif ($spacing !~ /\s+/) { - ERROR("space required before the open parenthesis '('\n" . $herecurr); + CS_ERROR("space required before the open parenthesis '('\n" . $herecurr); } } # Return of what appears to be an errno should normally be -'ve @@ -2287,7 +2306,7 @@ sub process { # Need a space before open parenthesis after if, while etc if ($line=~/\b(if|while|for|switch)\(/) { - ERROR("space required before the open parenthesis '('\n" . $herecurr); + CS_ERROR("space required before the open parenthesis '('\n" . $herecurr); } # Check for illegal assignment in if conditional -- and check for trailing @@ -2337,7 +2356,7 @@ sub process { $stat_real = "[...]\n$stat_real"; } - ERROR("trailing statements should be on next line\n" . $herecurr . $stat_real); + CS_ERROR("trailing statements should be on next line\n" . $herecurr . $stat_real); } } @@ -2383,7 +2402,7 @@ sub process { # indent level to be relevant to each other. if ($prevline=~/}\s*$/ and $line=~/^.\s*else\s*/ and $previndent == $indent) { - ERROR("else should follow close brace '}'\n" . $hereprev); + CS_ERROR("else should follow close brace '}'\n" . $hereprev); } if ($prevline=~/}\s*$/ and $line=~/^.\s*while\s*/ and @@ -2396,7 +2415,7 @@ sub process { $s =~ s/\n.*//g; if ($s =~ /^\s*;/) { - ERROR("while should follow close brace '}'\n" . $hereprev); + CS_ERROR("while should follow close brace '}'\n" . $hereprev); } } @@ -2576,7 +2595,7 @@ sub process { } } if ($seen && !$allowed) { - WARN("braces {} are not necessary for any arm of this statement\n" . $herectx); + CS_WARN("braces {} are not necessary for any arm of this statement\n" . $herectx); } } } @@ -2630,7 +2649,7 @@ sub process { $herectx .= raw_line($linenr, $n) . "\n";; } - WARN("braces {} are not necessary for single statement blocks\n" . $herectx); + CS_WARN("braces {} are not necessary for single statement blocks\n" . $herectx); } } @@ -2699,7 +2718,7 @@ sub process { # warn about spacing in #ifdefs if ($line =~ /^.\s*\#\s*(ifdef|ifndef|elif)\s\s+/) { - ERROR("exactly one space required after that #$1\n" . $herecurr); + CS_ERROR("exactly one space required after that #$1\n" . $herecurr); } # check for spinlock_t definitions without a comment. @@ -2723,24 +2742,24 @@ sub process { # Check that the storage class is at the beginning of a declaration if ($line =~ /\b$Storage\b/ && $line !~ /^.\s*$Storage\b/) { - WARN("storage class should be at the beginning of the declaration\n" . $herecurr) + CS_WARN("storage class should be at the beginning of the declaration\n" . $herecurr) } # check the location of the inline attribute, that it is between # storage class and type. if ($line =~ /\b$Type\s+$Inline\b/ || $line =~ /\b$Inline\s+$Storage\b/) { - ERROR("inline keyword should sit between storage class and type\n" . $herecurr); + CS_ERROR("inline keyword should sit between storage class and type\n" . $herecurr); } # Check for __inline__ and __inline, prefer inline if ($line =~ /\b(__inline__|__inline)\b/) { - WARN("plain inline is preferred over $1\n" . $herecurr); + CS_WARN("plain inline is preferred over $1\n" . $herecurr); } # Check for __attribute__ packed, prefer __packed if ($line =~ /\b__attribute__\s*\(\s*\(.*\bpacked\b/) { - WARN("__packed is preferred over __attribute__((packed))\n" . $herecurr); + CS_WARN("__packed is preferred over __attribute__((packed))\n" . $herecurr); } # check for sizeof(&) @@ -2791,7 +2810,7 @@ sub process { # check for multiple semicolons if ($line =~ /;\s*;\s*$/) { - WARN("Statements terminations use 1 semicolon\n" . $herecurr); + CS_WARN("Statements terminations use 1 semicolon\n" . $herecurr); } # check for gcc specific __FUNCTION__ @@ -2945,6 +2964,9 @@ sub process { print "NOTE: whitespace errors detected, you may wish to use scripts/cleanpatch or\n"; print " scripts/cleanfile\n\n"; } + if ($nocs == 1) { + print "You should rerun without the --nocs switch before patch submission\n"; + } } if ($clean == 1 && $quiet == 0) { ^ permalink raw reply related [flat|nested] 28+ messages in thread
* Re: checkpatch: introduce --nocs to disable CodingStyle warnings. 2011-03-22 19:58 ` checkpatch: introduce --nocs to disable CodingStyle warnings Dave Jones @ 2011-03-22 20:21 ` Joe Perches 0 siblings, 0 replies; 28+ messages in thread From: Joe Perches @ 2011-03-22 20:21 UTC (permalink / raw) To: Dave Jones Cc: Steven Rostedt, Jonathan Corbet, Andy Whitcroft, LKML, Andrew Morton On Tue, 2011-03-22 at 15:58 -0400, Dave Jones wrote: > How about this ? running checkpatch with --nocs will now only print > non-CodingStyle related warnings. I may have missed some of the > conversions, but this seems to silence the more egregious 'noise'. > Signed-off-by: Dave Jones <davej@redhat.com> Hi Dave. Trivia only: > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl > index 58848e3..a2b0086 100755 > --- a/scripts/checkpatch.pl > +++ b/scripts/checkpatch.pl > @@ -19,6 +19,7 @@ my $tree = 1; > my $chk_signoff = 1; > my $chk_patch = 1; > my $tst_only; > +my $nocs = 0; To match the other checkpatch control flag uses, I think this would be better as: my $chk_style = 1; > @@ -55,6 +56,7 @@ Options: + --style display warnings about CodingStyle. etc... > @@ -80,6 +82,7 @@ GetOptions( > > 'debug=s' => \%debug, > 'test-only=s' => \$tst_only, > + 'nocs' => \$nocs, 'style!' => \$chk_style, etc... And the command line use would be --nostyle cheers, Joe ^ permalink raw reply [flat|nested] 28+ messages in thread
end of thread, other threads:[~2011-03-24 20:41 UTC | newest] Thread overview: 28+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2011-03-18 2:52 [PATCH] checkpatch: Test for kmalloc/memset(0) pairs Steven Rostedt 2011-03-18 3:15 ` Jonathan Corbet 2011-03-18 3:32 ` Steven Rostedt 2011-03-20 3:40 ` Américo Wang 2011-03-20 7:02 ` Pekka Enberg 2011-03-20 8:01 ` Julia Lawall 2011-03-20 9:02 ` Pekka Enberg 2011-03-20 9:17 ` Julia Lawall 2011-03-20 10:54 ` Ingo Molnar 2011-03-20 11:06 ` Pekka Enberg 2011-03-20 11:32 ` Nicolas Palix 2011-03-20 12:00 ` Julia Lawall 2011-03-20 11:09 ` Alexey Dobriyan 2011-03-20 12:38 ` Julia Lawall 2011-03-20 14:48 ` Alejandro Riveira Fernández 2011-03-21 13:13 ` Steven Rostedt 2011-03-24 15:34 ` Aneesh Kumar K. V 2011-03-24 16:08 ` Julia Lawall 2011-03-24 16:10 ` Nicolas Palix 2011-03-24 18:30 ` Arnaud Lacombe 2011-03-24 20:39 ` Julia Lawall 2011-03-24 17:28 ` Aneesh Kumar K. V 2011-03-19 19:39 ` Dave Jones 2011-03-21 13:26 ` Steven Rostedt 2011-03-21 13:29 ` Steven Rostedt 2011-03-21 13:55 ` Borislav Petkov 2011-03-22 19:58 ` checkpatch: introduce --nocs to disable CodingStyle warnings Dave Jones 2011-03-22 20:21 ` Joe Perches
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).