From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from lists.xenproject.org (lists.xenproject.org [192.237.175.120]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 31CFDC433F5 for ; Wed, 27 Apr 2022 00:38:58 +0000 (UTC) Received: from list by lists.xenproject.org with outflank-mailman.314255.532227 (Exim 4.92) (envelope-from ) id 1njVhS-0001Q6-Oq; Wed, 27 Apr 2022 00:38:34 +0000 X-Outflank-Mailman: Message body and most headers restored to incoming version Received: by outflank-mailman (output) from mailman id 314255.532227; Wed, 27 Apr 2022 00:38:34 +0000 Received: from localhost ([127.0.0.1] helo=lists.xenproject.org) by lists.xenproject.org with esmtp (Exim 4.92) (envelope-from ) id 1njVhS-0001Pz-LL; Wed, 27 Apr 2022 00:38:34 +0000 Received: by outflank-mailman (input) for mailman id 314255; Wed, 27 Apr 2022 00:38:34 +0000 Received: from se1-gles-flk1-in.inumbo.com ([94.247.172.50] helo=se1-gles-flk1.inumbo.com) by lists.xenproject.org with esmtp (Exim 4.92) (envelope-from ) id 1njVhS-0001Pt-3B for xen-devel@lists.xenproject.org; Wed, 27 Apr 2022 00:38:34 +0000 Received: from dfw.source.kernel.org (dfw.source.kernel.org [139.178.84.217]) by se1-gles-flk1.inumbo.com (Halon) with ESMTPS id 5cae34aa-c5c2-11ec-8fc2-03012f2f19d4; Wed, 27 Apr 2022 02:38:32 +0200 (CEST) Received: from smtp.kernel.org (relay.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dfw.source.kernel.org (Postfix) with ESMTPS id 6926A61A71; Wed, 27 Apr 2022 00:38:30 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 57BA8C385A4; Wed, 27 Apr 2022 00:38:29 +0000 (UTC) X-BeenThere: xen-devel@lists.xenproject.org List-Id: Xen developer discussion List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Errors-To: xen-devel-bounces@lists.xenproject.org Precedence: list Sender: "Xen-devel" X-Inumbo-ID: 5cae34aa-c5c2-11ec-8fc2-03012f2f19d4 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1651019909; bh=Dbm/WTOJGlznwBy/NNY7zd7knuH35fdhKZhJ4BJatG8=; h=Date:From:To:cc:Subject:In-Reply-To:References:From; b=Cxen2MNArQ1ZpLg+VDBNQ+0KK+SUmpjcB8it0Bx8BwEb/ZxOFOpL4yTAtd1sVOm4j AMsZpAc22oVyPv5nJgr/Aw11XXr1RppnaN0k4M9bkXQFe5IAdCYWzmAmNfkHpcBdlg Renf9tphxJ+MAOm1Ly+wRWZH1rV1UFAhCV5PGZwe27IeLnrO4yPP/GHqeJ9AbTrNgu 2N597OL4liwDm7sN0XLDLOrptlF8RbAC0ZmlABUgDaFCmudBgIrmRK2Ri4aIRoQ1gR IM1OlE5uoflsD0EHCUYm7sdtHeaU3T14dKmdJzPwrg9qJyLUPmLES3P/I6hp3lKF2B 9UhuNv/2YQelg== Date: Tue, 26 Apr 2022 17:38:28 -0700 (PDT) From: Stefano Stabellini X-X-Sender: sstabellini@ubuntu-linux-20-04-desktop To: Bertrand Marquis cc: xen-devel@lists.xenproject.org, Andrew Cooper , George Dunlap , Jan Beulich , Julien Grall , Stefano Stabellini , Wei Liu , Volodymyr Babchuk , Michal Orzel Subject: Re: [PATCH v3] xen/build: Add cppcheck and cppcheck-html make rules In-Reply-To: <8a8ffce11591efb284c93214b77534fa4e4134b2.1650976647.git.bertrand.marquis@arm.com> Message-ID: References: <8a8ffce11591efb284c93214b77534fa4e4134b2.1650976647.git.bertrand.marquis@arm.com> User-Agent: Alpine 2.22 (DEB 394 2020-01-19) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII On Tue, 26 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 > Signed-off-by: Michal Orzel I tested the patch and it works fine for me. Tested-by: Stefano Stabellini In regard to the review, merge_cppcheck_reports.py looks OK now. It handles all the exceptions and errors properly. I don't have any comments on the Makefile changes, they look OK. For the headers (processor.h, config.h, kconfig.h), it is not ideal to have #ifdef CPPCHECK but given that they are the only changes needed to be able to do a full cppcheck scan of the Xen source, I'd say that it is a small price worth paying. So overall: Acked-by: Stefano Stabellini > --- > Changes in v3: > - add more checks in merge script and catch all possible exception from > the xml parser (suggested by Stefano) > 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 a bit 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 | 86 ++++++++++++++++++++++++++++ > 6 files changed, 182 insertions(+), 2 deletions(-) > create mode 100755 xen/tools/merge_cppcheck_reports.py > > diff --git a/.gitignore b/.gitignore > index c6d2c4b4f1..18ef56a780 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 ec34524ed2..255360440e 100644 > --- a/xen/Makefile > +++ b/xen/Makefile > @@ -449,7 +449,7 @@ endif # need-config > > __all: build > > -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): %: _% ; > @@ -537,15 +537,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 source > + rm -rf $(CPPCHECK_HTMLREPORT_OUTDIR) > > $(TARGET).gz: $(TARGET) > gzip -n -f -9 < $< > $@.new > @@ -619,6 +621,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 > endif # need-sub-make > > 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 > +#endif > + > #include > > #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..1c1b63ba56 > --- /dev/null > +++ b/xen/tools/merge_cppcheck_reports.py > @@ -0,0 +1,86 @@ > +#!/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 > + > + if el1.find('location') is None: return False > + if el2.find('location') is None: return False > + > + el1_location = str(el1.find('location').attrib) > + el2_location = str(el2.find('location').attrib) > + > + if el1_location != el2_location: return False > + > + return True > + > +def contain_element(new, lst): > + for elem in lst: > + if elements_equal(new, elem): > + return True > + return False > + > +def merge(files): > + try: > + result_xml_root = ElementTree.parse(files[0]).getroot() > + except: > + print("Xml parsing error in %s\n" % (files[0])) > + print("Please upgrade your cppcheck to version 2.7 or greater") > + sys.exit(1) > + insert_point = result_xml_root.findall("errors")[0] > + curr = 1 > + total = len(files) > + numelem = len(insert_point) > + for xml_file in files[1:]: > + try: > + xml_root = ElementTree.parse(xml_file).getroot() > + except: > + print("Xml parsing error in %s\n" % (xml_file)) > + print("Please upgrade your cppcheck to version 2.7 or greater") > + sys.exit(1) > + curr_elem_list = list(insert_point) > + new_elem_list = list(xml_root.findall("errors")[0]) > + for xml_error_elem in new_elem_list: > + if not contain_element(xml_error_elem, curr_elem_list): > + insert_point.insert(1,xml_error_elem) > + numelem = numelem + 1 > + curr = curr + 1 > + sys.stdout.write('\r') > + sys.stdout.write(" %d / %d" % (curr,total)) > + sys.stdout.flush() > + > + sys.stdout.write('\r\n') > + print("Done: %d elements" % (numelem)) > + return result_xml_root > + > +def run(): > + files = [] > + output = None > + for i in sys.argv[1:]: > + output = i if '.xml' in i else None > + files.append(i) if '.cppcheck' in i else None > + > + result = merge(files) > + > + if result is None: > + return > + > + if output is not None: > + ElementTree.ElementTree(result).write(output) > + else: > + print(ElementTree.tostring(result).decode('utf-8')) > + > +if __name__ == '__main__': > + run() > -- > 2.25.1 >