Re: [RFC PATCH] xen/build: Add cppcheck and cppcheck-html make rules

[Jan Beulich <jbeulich@xxxxxxxx>]

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