[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [RFC PATCH] xen/build: Add cppcheck and cppcheck-html make rules
On 24.03.2022 12:04, Bertrand Marquis wrote: > cppcheck can be used to check Xen code quality. > > To create a report do "make cppcheck" on a built tree adding any options > you added during the process you used to build xen (like CROSS_COMPILE > or XEN_TARGET_ARCH). This will generate an xml report xen-cppcheck.xml. > > To create a html report do "make cppcheck-html" in the same way and a > full report to be seen in a browser will be generated in > cppcheck-htmlreport/index.html. > > For better results it is recommended to build your own cppcheck from the > latest sources that you can find at [1]. > Development and result analysis has been done with cppcheck 2.7. > > The Makefile rule is searching for all C files which have been compiled > (ie which have a generated .o file) and is running cppcheck on all of > them using the current configuration of xen so only the code actually > compiled is checked. Why this restriction? It means there are multiple runs needed in case files are touched by a patch which can't both be built at the same time (e.g. ones under multiple xen/arch/*/). In addition, by going from .o files, you also require (and yes, you say so) that the tree has been built before. I think you would instead want to go from the collective set of $(obj-y), $(obj-n), and $(obj-), traversing the tree suitably. > @@ -511,6 +513,75 @@ cloc: > done; \ > done | cloc --list-file=- > > +# What cppcheck command to use. > +# To get proper results, it is recommended to build cppcheck manually from > the > +# latest source and use CPPCHECK to give the full path to the built version. > +CPPCHECK ?= cppcheck > + > +# What cppcheck-htmlreport to use. > +# If you give the full path to a self compiled cppcheck, this should be set > +# to the full path to cppcheck-html in the htmlreport directory of cppcheck. > +# On recent distribution, this is available in the standard path. > +CPPCHECK_HTMLREPORT ?= cppcheck-htmlreport > + > +# By default we generate the report in cppcheck-htmlreport directory in the > +# build directory. This can be changed by giving a directory in this > variable. > +CPPCHECK_HTMLREPORT_OUTDIR ?= cppcheck-htmlreport > + > +# Compile flags to pass to cppcheck: > +# - include directories and defines Xen Makefile is passing (from CFLAGS) > +# - include config.h as this is passed directly to the compiler. > +# - define CPPCHECK as we use to disable or enable some specific part of the > +# code to solve some cppcheck issues. > +# - explicitely enable some cppcheck checks as we do not want to use "all" > +# which includes unusedFunction which gives wrong positives as we check > file > +# per file. > +# > +# Compiler defines are in compiler-def.h which is included in config.h > +# > +CPPCHECKFLAGS := -DCPPCHECK --max-ctu-depth=10 \ > + --enable=style,information,missingInclude \ > + --include=$(BASEDIR)/include/xen/config.h \ > + -I $(BASEDIR)/xsm/flask/include \ > + -I $(BASEDIR)/include/xen/libfdt \ Why ware these two -I necessary? Shouldn't they derive cleanly ... > + $(filter -D% -I%,$(CFLAGS)) ... here? As to style (also applicable further down) I think it would help if you didn't use tabs and if you aligned things, e.g. CPPCHECKFLAGS := -DCPPCHECK --max-ctu-depth=10 \ --enable=style,information,missingInclude \ --include=$(BASEDIR)/include/xen/config.h \ -I $(BASEDIR)/xsm/flask/include \ -I $(BASEDIR)/include/xen/libfdt \ $(filter -D% -I%,$(CFLAGS)) > +# We need to find all C files (as we are not checking assembly files) so > +# we find all generated .o files which have a .c corresponding file. > +CPPCHECKFILES := $(wildcard $(patsubst %.o,%.c, $(filter-out > $(BASEDIR)/tools/%,$(shell find $(BASEDIR) -name "*.o")))) > + > +quiet_cmd_cppcheck_xml = CPPCHECK $(patsubst $(BASEDIR)/%,%,$<) > +cmd_cppcheck_xml = $(CPPCHECK) -v -q --xml $(CPPCHECKFLAGS) \ > + --output-file=$@ $< As per the earlier comment (just to give another example) I think this would want to be cmd_cppcheck_xml = $(CPPCHECK) -v -q --xml $(CPPCHECKFLAGS) \ --output-file=$@ $< (i.e. with continue options aligned with the first one). This is even more noticable with ... > +quiet_cmd_merge_cppcheck_reports = CPPCHECK-MERGE $@ > +cmd_merge_cppcheck_reports = $(BASEDIR)/tools/merge_cppcheck_reports.py $^ $@ > + > +quiet_cmd_cppcheck_html = CPPCHECK-HTML $< > +cmd_cppcheck_html = $(CPPCHECK_HTMLREPORT) --file=$< --source-dir=$(BASEDIR) > \ > + > --report-dir=$(CPPCHECK_HTMLREPORT_OUTDIR) \ > + > --title=Xen ... needlessly long lines like these ones. Also aiui you shouldn't be using $(BASEDIR) anymore, but $(srctree) or $(objtree). > +PHONY += _cppcheck _cppcheck-html > + > +_cppcheck-html: xen-cppcheck.xml > + $(call if_changed,cppcheck_html) > + > +_cppcheck: xen-cppcheck.xml > + > +xen-cppcheck.xml: $(patsubst %.c,%.c.cppcheck,$(CPPCHECKFILES)) > +ifeq ($(CPPCHECKFILES),) > + $(error Please build Xen before running cppcheck) > +endif Besides the requirement being enforced here to have _some_ .o files, ... > + $(call if_changed,merge_cppcheck_reports) > + > +%.c.cppcheck: %.c $(BASEDIR)/include/generated/autoconf.h > $(BASEDIR)/include/generated/compiler-def.h ... doesn't the dependency on autoconf.h here point out another issue: Don't you require the build to be up-to-date? If this dependency really is to be retained, should you perhaps make the new goal depend on $(TARGET), thus forcing a build to occur (perhaps just an incremental one)? > --- a/xen/include/xen/config.h > +++ b/xen/include/xen/config.h > @@ -7,6 +7,10 @@ > #ifndef __XEN_CONFIG_H__ > #define __XEN_CONFIG_H__ > > +#ifdef CPPCHECK > +#include <generated/compiler-def.h> > +#endif Could you leave this file untouched and have the generated file included by passing another --include= in CPPCHECKFLAGS? > --- a/xen/include/xen/kconfig.h > +++ b/xen/include/xen/kconfig.h > @@ -8,6 +8,10 @@ > * these only work with boolean option. > */ > > +/* cppcheck is failing to parse the macro so use a dummy one */ > +#ifdef CPPCHECK > +#define IS_ENABLED(option) option > +#else > /* > * Getting something that works in C and CPP for an arg that may or may > * not be defined is tricky. Here, if we have "#define CONFIG_BOOGER 1" > @@ -27,5 +31,6 @@ > * otherwise. > */ > #define IS_ENABLED(option) config_enabled(option) > +#endif What are the consequences of this workaround on the code actually being checked? Does this perhaps lead to certain portions of code being skipped while checking? Jan
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |