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

[Bertrand Marquis <Bertrand.Marquis@xxxxxxx>]

Hi Stefano,

> On 16 Apr 2022, at 00:42, Stefano Stabellini <sstabellini@xxxxxxxxxx> wrote:
> 
> On Wed, 13 Apr 2022, 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.
>> 
>> A new tool is introduced to merge all cppcheck reports into one global
>> report including all findings and removing duplicates.
>> 
>> Some extra variables can be used to customize the report:
>> - CPPCHECK can be used to give the full path to the cppcheck binary to
>> use (default is to use the one from the standard path).
>> - CPPCHECK_HTMLREPORT can be used to give the full path to
>> cppcheck-htmlreport (default is to use the one from the standard path).
>> 
>> This has been tested on several arm configurations (x86 should work but
>> has not been tested).
>> 
>> [1] https://cppcheck.sourceforge.io/
>> 
>> Signed-off-by: Bertrand Marquis <bertrand.marquis@xxxxxxx>
>> Signed-off-by: Michal Orzel <michal.orzel@xxxxxxx>
> 
> This is better, thanks! I don't have a lot of comments as it looks
> pretty OK to me already. Only one comment on the python script.
> 
> 
>> ---
>> Changes in v2:
>> - catch parsing errors during merge and tell the user to update cppcheck
>> - fix alignments in Makefile
>> - use srctree and objtree instead of BASEDIR
>> - remove SCTLR invalid change
>> - add a check of cppcheck version and mandate version 2.7 or greater
>> - rework the merge script to make it faster and display progress
>> ---
>> .gitignore                           |  3 +
>> xen/Makefile                         | 84 +++++++++++++++++++++++++++-
>> xen/arch/arm/include/asm/processor.h |  2 +
>> xen/include/xen/config.h             |  4 ++
>> xen/include/xen/kconfig.h            |  5 ++
>> xen/tools/merge_cppcheck_reports.py  | 83 +++++++++++++++++++++++++++
>> 6 files changed, 179 insertions(+), 2 deletions(-)
>> create mode 100755 xen/tools/merge_cppcheck_reports.py
>> 
>> diff --git a/.gitignore b/.gitignore
>> index d425be4bd9..0d2d60b8f1 100644
>> --- a/.gitignore
>> +++ b/.gitignore
>> @@ -7,6 +7,7 @@
>> *.o
>> *.d
>> *.d2
>> +*.c.cppcheck
>> *.opic
>> *.a
>> *.so
>> @@ -296,6 +297,7 @@ xen/.banner
>> xen/.config
>> xen/.config.old
>> xen/.xen.elf32
>> +xen/xen-cppcheck.xml
>> xen/System.map
>> xen/arch/x86/boot/mkelf32
>> xen/arch/x86/boot/cmdline.S
>> @@ -316,6 +318,7 @@ xen/arch/*/efi/runtime.c
>> xen/arch/*/include/asm/asm-offsets.h
>> xen/common/config_data.S
>> xen/common/config.gz
>> +xen/cppcheck-htmlreport
>> xen/include/headers*.chk
>> xen/include/compat/*
>> xen/include/config/
>> diff --git a/xen/Makefile b/xen/Makefile
>> index ebc7854c08..734f9f2b4f 100644
>> --- a/xen/Makefile
>> +++ b/xen/Makefile
>> @@ -335,7 +335,7 @@ export CFLAGS_UBSAN
>> 
>> endif # need-config
>> 
>> -main-targets := build install uninstall clean distclean MAP
>> +main-targets := build install uninstall clean distclean MAP cppcheck 
>> cppcheck-html
>> .PHONY: $(main-targets)
>> ifneq ($(XEN_TARGET_ARCH),x86_32)
>> $(main-targets): %: _% ;
>> @@ -423,15 +423,17 @@ _clean:
>>      $(Q)$(MAKE) $(clean)=tools/kconfig
>>      find . \( -name "*.o" -o -name ".*.d" -o -name ".*.d2" \
>>              -o -name ".*.o.tmp" -o -name "*~" -o -name "core" \
>> -            -o -name '*.lex.c' -o -name '*.tab.[ch]' \
>> +            -o -name '*.lex.c' -o -name '*.tab.[ch]' -o -name 
>> '*.c.cppcheck' \
>>              -o -name "*.gcno" -o -name ".*.cmd" -o -name "lib.a" \) -exec 
>> rm -f {} \;
>>      rm -f include/asm $(TARGET) $(TARGET).gz $(TARGET).efi 
>> $(TARGET).efi.map $(TARGET)-syms $(TARGET)-syms.map
>>      rm -f asm-offsets.s arch/*/include/asm/asm-offsets.h
>>      rm -f .banner .allconfig.tmp include/xen/compile.h
>> +    rm -f xen-cppcheck.xml
>> 
>> .PHONY: _distclean
>> _distclean: clean
>>      rm -f tags TAGS cscope.files cscope.in.out cscope.out cscope.po.out 
>> GTAGS GPATH GRTAGS GSYMS .config
>> +    rm -rf $(CPPCHECK_HTMLREPORT_OUTDIR)
>> 
>> $(TARGET).gz: $(TARGET)
>>      gzip -n -f -9 < $< > $@.new
>> @@ -505,6 +507,84 @@ 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=$(srctree)/include/xen/config.h \
>> +                 -I $(srctree)/xsm/flask/include \
>> +                 -I $(srctree)/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 $(objtree)/%.o,$(srctree)/%.c, \
>> +                 $(filter-out $(objtree)/tools/%, \
>> +                 $(shell find $(objtree) -name "*.o"))))
>> +
>> +quiet_cmd_cppcheck_xml = CPPCHECK $(patsubst $(srctree)/%,%,$<)
>> +cmd_cppcheck_xml = $(CPPCHECK) -v -q --xml $(CPPCHECKFLAGS) \
>> +                   --output-file=$@ $<
>> +
>> +quiet_cmd_merge_cppcheck_reports = CPPCHECK-MERGE $@
>> +cmd_merge_cppcheck_reports = $(srctree)/tools/merge_cppcheck_reports.py $^ 
>> $@
>> +
>> +quiet_cmd_cppcheck_html = CPPCHECK-HTML $<
>> +cmd_cppcheck_html = $(CPPCHECK_HTMLREPORT) --file=$< 
>> --source-dir=$(srctree) \
>> +                    --report-dir=$(CPPCHECK_HTMLREPORT_OUTDIR) --title=Xen
>> +
>> +PHONY += _cppcheck _cppcheck-html cppcheck-version
>> +
>> +_cppcheck-html: xen-cppcheck.xml
>> +    $(call if_changed,cppcheck_html)
>> +
>> +_cppcheck: xen-cppcheck.xml
>> +
>> +xen-cppcheck.xml: $(patsubst 
>> $(srctree)/%.c,$(objtree)/%.c.cppcheck,$(CPPCHECKFILES))
>> +ifeq ($(CPPCHECKFILES),)
>> +    $(error Please build Xen before running cppcheck)
>> +endif
>> +    $(call if_changed,merge_cppcheck_reports)
>> +
>> +$(objtree)/%.c.cppcheck: $(srctree)/%.c 
>> $(objtree)/include/generated/autoconf.h 
>> $(objtree)/include/generated/compiler-def.h | cppcheck-version
>> +    $(call if_changed,cppcheck_xml)
>> +
>> +cppcheck-version:
>> +ifeq ($(shell which $(CPPCHECK)),)
>> +    $(error Cannot find cppcheck executable: $(CPPCHECK))
>> +endif
>> +ifeq ($(shell $(CPPCHECK) --version | awk '{print ($$2 < 2.7)}'),1)
>> +    $(error Please upgrade your cppcheck to version 2.7 or greater)
>> +endif
>> +
>> +# Put this in generated headers this way it is cleaned by include/Makefile
>> +$(objtree)/include/generated/compiler-def.h:
>> +    $(Q)$(CC) -dM -E -o $@ - < /dev/null
>> +
>> endif #config-build
>> 
>> PHONY += FORCE
>> diff --git a/xen/arch/arm/include/asm/processor.h 
>> b/xen/arch/arm/include/asm/processor.h
>> index 852b5f3c24..ef37cfa16f 100644
>> --- a/xen/arch/arm/include/asm/processor.h
>> +++ b/xen/arch/arm/include/asm/processor.h
>> @@ -219,9 +219,11 @@
>>                          SCTLR_Axx_ELx_A    | SCTLR_Axx_ELx_C   |\
>>                          SCTLR_Axx_ELx_WXN  | SCTLR_Axx_ELx_EE)
>> 
>> +#ifndef CPPCHECK
>> #if (SCTLR_EL2_SET ^ SCTLR_EL2_CLEAR) != 0xffffffffffffffffUL
>> #error "Inconsistent SCTLR_EL2 set/clear bits"
>> #endif
>> +#endif
>> 
>> #endif
>> 
>> diff --git a/xen/include/xen/config.h b/xen/include/xen/config.h
>> index 85c6f59be9..d888b2314d 100644
>> --- 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
>> +
>> #include <xen/kconfig.h>
>> 
>> #ifndef __ASSEMBLY__
>> diff --git a/xen/include/xen/kconfig.h b/xen/include/xen/kconfig.h
>> index 4d58c5bb3c..a717b0819c 100644
>> --- 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
>> 
>> #endif /* __XEN_KCONFIG_H */
>> diff --git a/xen/tools/merge_cppcheck_reports.py 
>> b/xen/tools/merge_cppcheck_reports.py
>> new file mode 100755
>> index 0000000000..eabf558768
>> --- /dev/null
>> +++ b/xen/tools/merge_cppcheck_reports.py
>> @@ -0,0 +1,83 @@
>> +#!/usr/bin/env python
>> +
>> +"""
>> +This script acts as a tool to merge XML files created by cppcheck.
>> +Usage:
>> +    merge_cppcheck_reports.py [FILES] [OUTPUT]
>> +
>> +    FILES  - list of XML files with extension .cppcheck
>> +    OUTPUT - file to store results (with .xml extension).
>> +             If not specified, the script will print results to stdout.
>> +"""
>> +
>> +import sys
>> +from xml.etree import ElementTree
>> +
>> +def elements_equal(el1, el2):
>> +    if type(el1) != type(el2): return False
>> +
>> +    el1_location = str(el1.find('location').attrib)
>> +    el2_location = str(el2.find('location').attrib)
> 
> I managed to repro another stacktrace:
> 
>  CPPCHECK-MERGE xen-cppcheck.xml
> 123 / 191Traceback (most recent call last):
>  File "./tools/merge_cppcheck_reports.py", line 83, in <module>
>    run()
>  File "./tools/merge_cppcheck_reports.py", line 72, in run
>    result = merge(files)
>  File "./tools/merge_cppcheck_reports.py", line 53, in merge
>    if not contain_element(xml_error_elem, curr_elem_list):
>  File "./tools/merge_cppcheck_reports.py", line 28, in contain_element
>    if elements_equal(new, elem):
>  File "./tools/merge_cppcheck_reports.py", line 19, in elements_equal
>    el1_location = str(el1.find('location').attrib)
> AttributeError: 'NoneType' object has no attribute 'attrib'
> make: *** [Makefile:685: xen-cppcheck.xml] Error 1
> 
> It must be a natural talent :-)
> 
> Should we have a try/except also here as well?
> 
> FYI I added:
> 
>    if el1.find('location') is None: return False
>    if el2.find('location') is None: return False
> 
> And that solved the issue for me

You are very talented to trigger the errors (I do not have any on my side).
Anyway I will include this and make the try/except to catch any kind of errors 
in v3.

Thanks a lot for the deep testing :-)

Cheers
Bertrand