Add targets to use the Coccinelle checker [Kernel]

Prev: Positive results with Transparent Hugepages at VMware
Next: x86/PCI: never allocate PCI MMIO resources below BIOS_END

From: Américo Wang on 12 May 2010 02:40

From: Joerg Roedel on 28 May 2010 03:10

On Wed, May 12, 2010 at 02:42:30PM +0800, Am�rico Wang wrote:
> On Mon, May 10, 2010 at 06:24:24PM +0200, Nicolas Palix wrote:
> >Four targets are added. Each one generates a different
> >output kind: context, patch, org, report.
> >Every SmPL file in 'scripts/coccinelle' is given to the spatch frontend
> >(located in the 'scripts' directory), and applied to the entire
> >source tree.
> >
> >Signed-off-by: Nicolas Palix <npalix(a)diku.dk>
> >Signed-off-by: Julia Lawall <julia(a)diku.dk>
>
> This is great!

Totally agreed.

> I would like to see coccinelle to be integrated with kbuild, but the
> name 'coccicheck' is really confusing, how about 'sema_check'? Which
> means 'semantic check'.

I think its okay to keep coccicheck because its the name of the tool
called. We do the same with 'make cscope'.
But since I am a bit lazy I would prefer a shorter name, probably just
'make cocci'?

> Or we can use something like for sparse, i.e. 'make C=1' etc..

Yes, this would also make sense. As a developer I could add this and
only the code I changed is checked.

Joerg

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

From: Michal Marek on 3 Jun 2010 06:00

On 10.5.2010 18:24, Nicolas Palix wrote:
> Four targets are added. Each one generates a different
> output kind: context, patch, org, report.
> Every SmPL file in 'scripts/coccinelle' is given to the spatch frontend
> (located in the 'scripts' directory), and applied to the entire
> source tree.

I think it would be good to repeat the description of the four modes
here (the 0/4 mail won't appear in the changelog).

> + @if [ "$(COCCI)" = "" ] ; then \
> + find $(srctree)/scripts/coccinelle/ \
> + -name '*.cocci' -type f \
> + -exec $(srctree)/scripts/coccinelle.sh $(COCCINELLE) $(@:coccicheck-%=%) \{} $(srctree) \; ; \
> + else \
> + $(srctree)/scripts/coccinelle.sh $(COCCINELLE) $(@:coccicheck-%=%) $(COCCI) $(srctree) ; \
> + fi

Please sort the output of 'find' or use a shell glob instead. IMO the
order of tests should be stable (and thus easily diff-able) and not
change when you e.g. switch branches.

> +if [ -x "$SPATCH" ]; then
> +
> + echo "Processing `basename $COCCI` with option(s) \"$(OPT)\""

/home/mmarek/linux-2.6/scripts/coccinelle.sh: line 13: OPT: command not
found

Please remove the parentheses around OPT.

> + echo 'Message example to submit a patch:'
> + sed -e '/\/\/\//!d' -e 's|^///||' $COCCI
> +
> + echo ' The semantic patch that makes this change is available'
> + echo " in $FILE."
> + echo ''
> + echo ' More information about semantic patching is available at'
> + echo ' http://coccinelle.lip6.fr/'
> + echo ''
> +
> + $SPATCH -D $MODE -very_quiet -sp_file $COCCI $OPT -dir $DIR
> +
> +else
> + echo 'spatch is part of the Coccinelle project and is available at http://coccinelle.lip6.fr/'
> +fi

If 'spatch' is not available, you should exit with an error.

Thanks,
Michal

PS: Sorry for the delay, I didn't get around to to reviewing the v2 of
your patch early enough and then I was offline for two weeks.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

From: Sam Ravnborg on 3 Jun 2010 06:30

On Mon, May 10, 2010 at 06:24:24PM +0200, Nicolas Palix wrote:
> Four targets are added. Each one generates a different
> output kind: context, patch, org, report.
> Every SmPL file in 'scripts/coccinelle' is given to the spatch frontend
> (located in the 'scripts' directory), and applied to the entire
> source tree.

A little late feedback.
IIRC all points raised at first review has been addressed - good.

>
> Signed-off-by: Nicolas Palix <npalix(a)diku.dk>
> Signed-off-by: Julia Lawall <julia(a)diku.dk>
> ---
> MAINTAINERS | 10 ++++++++++
> Makefile | 20 ++++++++++++++++++--

The top-level Makefile is already in a very bad shape.
Adding more stuff there is never good.

How about adding some infrastructure to support the various tools?
Something along these lines:

All targets named %check will call a shell script:
scripts/%check.sh

"make help" will call a script named:
scripts/helpcheck.sh

helpcheck.sh will contain all the ugly details
about help for the various targets.

We need to rename a few seldomly used targets to do this.
Sample patch for top-level Makefile:

diff --git a/Makefile b/Makefile
index efdc3d0..40bf83e 100644
--- a/Makefile
+++ b/Makefile
@@ -412,7 +412,7 @@ endif
# of make so .config is not included in this case either (for *config).

no-dot-config-targets := clean mrproper distclean \
- cscope TAGS tags help %docs check% \
+ cscope TAGS tags help %docs %check \
include/linux/version.h headers_% \
kernelrelease kernelversion

@@ -1024,10 +1024,6 @@ include/linux/version.h: $(srctree)/Makefile FORCE
include/generated/utsrelease.h: include/config/kernel.release FORCE
$(call filechk,utsrelease.h)

-PHONY += headerdep
-headerdep:
- $(Q)find include/ -name '*.h' | xargs --max-args 1 scripts/headerdep.pl
-
# ---------------------------------------------------------------------------

PHONY += depend dep
@@ -1273,13 +1269,9 @@ help:
echo ' (default: $(INSTALL_HDR_PATH))'; \
echo ''
@echo 'Static analysers'
- @echo ' checkstack - Generate a list of stack hogs'
- @echo ' namespacecheck - Name space analysis on compiled kernel'
- @echo ' versioncheck - Sanity check on version.h usage'
- @echo ' includecheck - Check for duplicate included header files'
+ @$(CONFIG_SHELL)$(srctree)/scripts/helpcheck.sh
@echo ' export_report - List the usages of all exported symbols'
@echo ' headers_check - Sanity check on exported headers'
- @echo ' headerdep - Detect inclusion cycles in headers'; \
echo ''
@echo 'Kernel packaging:'
@$(MAKE) $(build)=$(package-dir) help
@@ -1427,20 +1419,10 @@ tags TAGS cscope: FORCE
$(call cmd,tags)

# Scripts to check various things for consistency
+# Call a helper script in scripts/ to do the actual job
# ---------------------------------------------------------------------------
-
-includecheck:
- find * $(RCS_FIND_IGNORE) \
- -name '*.[hcS]' -type f -print | sort \
- | xargs $(PERL) -w $(srctree)/scripts/checkincludes.pl
-
-versioncheck:
- find * $(RCS_FIND_IGNORE) \
- -name '*.[hcS]' -type f -print | sort \
- | xargs $(PERL) -w $(srctree)/scripts/checkversion.pl
-
-namespacecheck:
- $(PERL) $(srctree)/scripts/namespace.pl
+%check:
+ $(Q)$(CONFIG_SHELL)$(srctree)/scripts/$@.sh

export_report:
$(PERL) $(srctree)/scripts/export_report.pl
@@ -1448,20 +1430,7 @@ export_report:
endif #ifeq ($(config-targets),1)
endif #ifeq ($(mixed-targets),1)

-PHONY += checkstack kernelrelease kernelversion
-
-# UML needs a little special treatment here. It wants to use the host
-# toolchain, so needs $(SUBARCH) passed to checkstack.pl. Everyone
-# else wants $(ARCH), including people doing cross-builds, which means
-# that $(SUBARCH) doesn't work here.
-ifeq ($(ARCH), um)
-CHECKSTACK_ARCH := $(SUBARCH)
-else
-CHECKSTACK_ARCH := $(ARCH)
-endif
-checkstack:
- $(OBJDUMP) -d vmlinux $$(find . -name '*.ko') | \
- $(PERL) $(src)/scripts/checkstack.pl $(CHECKSTACK_ARCH)
+PHONY += kernelrelease kernelversion

kernelrelease:
$(if $(wildcard include/config/kernel.release), $(Q)echo $(KERNELRELEASE), \

A nice simplification of the top-level Mkefile.
and an opportunity to document some of the
magic shell commands used by the various tools.

If we do this I know the scripts will sum up to a lot more
lines but doing it distributed is better.
And some of the lines would be comments that explains what is
going on.

An additinal benefit is that we can avoid touching
the top-level Makefile next time we add a new
%check target.

As for cocci this would have the the effect that
new scipts can be made as drop-in - no changes
in any files. Just add the files to the
correct directory.

Sam
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

From: Nicolas Palix on 4 Jun 2010 06:00

On Thursday 03 June 2010 12:23:15 Sam Ravnborg wrote:
> On Mon, May 10, 2010 at 06:24:24PM +0200, Nicolas Palix wrote:
> > Four targets are added. Each one generates a different
> > output kind: context, patch, org, report.
> > Every SmPL file in 'scripts/coccinelle' is given to the spatch frontend
> > (located in the 'scripts' directory), and applied to the entire
> > source tree.
>
> A little late feedback.
> IIRC all points raised at first review has been addressed - good.
>
> >
> > Signed-off-by: Nicolas Palix <npalix(a)diku.dk>
> > Signed-off-by: Julia Lawall <julia(a)diku.dk>
> > ---
> > MAINTAINERS | 10 ++++++++++
> > Makefile | 20 ++++++++++++++++++--
>
> The top-level Makefile is already in a very bad shape.
> Adding more stuff there is never good.
>
> How about adding some infrastructure to support the various tools?
> Something along these lines:
>
> All targets named %check will call a shell script:
> scripts/%check.sh
>
> "make help" will call a script named:
> scripts/helpcheck.sh
>
> helpcheck.sh will contain all the ugly details
> about help for the various targets.
>
> We need to rename a few seldomly used targets to do this.
> Sample patch for top-level Makefile:
>
> diff --git a/Makefile b/Makefile
> index efdc3d0..40bf83e 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -412,7 +412,7 @@ endif
> # of make so .config is not included in this case either (for *config).
>
> no-dot-config-targets := clean mrproper distclean \
> - cscope TAGS tags help %docs check% \
> + cscope TAGS tags help %docs %check \
> include/linux/version.h headers_% \
> kernelrelease kernelversion
>
> @@ -1024,10 +1024,6 @@ include/linux/version.h: $(srctree)/Makefile FORCE
> include/generated/utsrelease.h: include/config/kernel.release FORCE
> $(call filechk,utsrelease.h)
>
> -PHONY += headerdep
> -headerdep:
> - $(Q)find include/ -name '*.h' | xargs --max-args 1 scripts/headerdep.pl
> -
> # ---------------------------------------------------------------------------
>
> PHONY += depend dep
> @@ -1273,13 +1269,9 @@ help:
> echo ' (default: $(INSTALL_HDR_PATH))'; \
> echo ''
> @echo 'Static analysers'
> - @echo ' checkstack - Generate a list of stack hogs'
> - @echo ' namespacecheck - Name space analysis on compiled kernel'
> - @echo ' versioncheck - Sanity check on version.h usage'
> - @echo ' includecheck - Check for duplicate included header files'
> + @$(CONFIG_SHELL)$(srctree)/scripts/helpcheck.sh
> @echo ' export_report - List the usages of all exported symbols'
> @echo ' headers_check - Sanity check on exported headers'
> - @echo ' headerdep - Detect inclusion cycles in headers'; \
> echo ''
> @echo 'Kernel packaging:'
> @$(MAKE) $(build)=$(package-dir) help
> @@ -1427,20 +1419,10 @@ tags TAGS cscope: FORCE
> $(call cmd,tags)
>
> # Scripts to check various things for consistency
> +# Call a helper script in scripts/ to do the actual job
> # ---------------------------------------------------------------------------
> -
> -includecheck:
> - find * $(RCS_FIND_IGNORE) \
> - -name '*.[hcS]' -type f -print | sort \
> - | xargs $(PERL) -w $(srctree)/scripts/checkincludes.pl
> -
> -versioncheck:
> - find * $(RCS_FIND_IGNORE) \
> - -name '*.[hcS]' -type f -print | sort \
> - | xargs $(PERL) -w $(srctree)/scripts/checkversion.pl
> -
> -namespacecheck:
> - $(PERL) $(srctree)/scripts/namespace.pl
> +%check:
> + $(Q)$(CONFIG_SHELL)$(srctree)/scripts/$@.sh
>
> export_report:
> $(PERL) $(srctree)/scripts/export_report.pl
> @@ -1448,20 +1430,7 @@ export_report:
> endif #ifeq ($(config-targets),1)
> endif #ifeq ($(mixed-targets),1)
>
> -PHONY += checkstack kernelrelease kernelversion
> -
> -# UML needs a little special treatment here. It wants to use the host
> -# toolchain, so needs $(SUBARCH) passed to checkstack.pl. Everyone
> -# else wants $(ARCH), including people doing cross-builds, which means
> -# that $(SUBARCH) doesn't work here.
> -ifeq ($(ARCH), um)
> -CHECKSTACK_ARCH := $(SUBARCH)
> -else
> -CHECKSTACK_ARCH := $(ARCH)
> -endif
> -checkstack:
> - $(OBJDUMP) -d vmlinux $$(find . -name '*.ko') | \
> - $(PERL) $(src)/scripts/checkstack.pl $(CHECKSTACK_ARCH)
> +PHONY += kernelrelease kernelversion
>
> kernelrelease:
> $(if $(wildcard include/config/kernel.release), $(Q)echo $(KERNELRELEASE), \
>
>
>
> A nice simplification of the top-level Mkefile.
> and an opportunity to document some of the
> magic shell commands used by the various tools.
>
> If we do this I know the scripts will sum up to a lot more
> lines but doing it distributed is better.
> And some of the lines would be comments that explains what is
> going on.
>
> An additinal benefit is that we can avoid touching
> the top-level Makefile next time we add a new
> %check target.
>
> As for cocci this would have the the effect that
> new scipts can be made as drop-in - no changes
> in any files. Just add the files to the
> correct directory.
>
> Sam
>

Hi,

I am not sure to understand your suggestion.

Should I make this change and integrate it into my patch series
or it is something to do later ?

In doing so, I will have to select the 'mode' in a different way.
I was thinking of renaming my targets in one of the following forms
or using an environment variable:

make cocci-report-check
make report-coccicheck
make coccicheck MODE=report

Do you have any preference ?

--
Nicolas Palix
Tel: (+33) 1 44 27 87 25
Tel: (+33) 6 81 07 91 72
Web: http://www.diku.dk/~npalix/
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

First | Prev | Next | Last
Pages: 1 2 3 4
Prev: Positive results with Transparent Hugepages at VMware
Next: x86/PCI: never allocate PCI MMIO resources below BIOS_END