.gitignore | 3 ++ xen/Makefile | 75 +++++++++++++++++++++++++++- xen/arch/arm/include/asm/processor.h | 4 +- xen/include/xen/config.h | 4 ++ xen/include/xen/kconfig.h | 5 ++ xen/tools/merge_cppcheck_reports.py | 73 +++++++++++++++++++++++++++ 6 files changed, 161 insertions(+), 3 deletions(-) create mode 100755 xen/tools/merge_cppcheck_reports.py
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@arm.com>
Signed-off-by: Michal Orzel <michal.orzel@arm.com>
---
.gitignore | 3 ++
xen/Makefile | 75 +++++++++++++++++++++++++++-
xen/arch/arm/include/asm/processor.h | 4 +-
xen/include/xen/config.h | 4 ++
xen/include/xen/kconfig.h | 5 ++
xen/tools/merge_cppcheck_reports.py | 73 +++++++++++++++++++++++++++
6 files changed, 161 insertions(+), 3 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 18a4f7e101..0280d65051 100644
--- a/xen/Makefile
+++ b/xen/Makefile
@@ -336,7 +336,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): %: _% ;
@@ -424,15 +424,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
@@ -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 \
+ $(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=$@ $<
+
+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
+
+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
+ $(call if_changed,merge_cppcheck_reports)
+
+%.c.cppcheck: %.c $(BASEDIR)/include/generated/autoconf.h $(BASEDIR)/include/generated/compiler-def.h
+ $(call if_changed,cppcheck_xml)
+
+# Put this in generated headers this way it is cleaned by include/Makefile
+$(BASEDIR)/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..0b4ba73760 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)
-#if (SCTLR_EL2_SET ^ SCTLR_EL2_CLEAR) != 0xffffffffffffffffUL
+#ifndef CPPCHECK
+#if (SCTLR_EL2_SET ^ SCTLR_EL2_CLEAR) != 0xffffffffffffffffULL
#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 b76222ecf6..36e11e7133 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..ef055f6925
--- /dev/null
+++ b/xen/tools/merge_cppcheck_reports.py
@@ -0,0 +1,73 @@
+#!/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)
+
+ if el1_location != el2_location: return False
+
+ return True
+
+def remove_duplicates(xml_root_element):
+ elems_to_remove = []
+ index = 0
+ elems_list = list(xml_root_element.findall("errors")[0])
+ for elem1 in elems_list:
+ index += 1
+ for elem2 in elems_list[index:]:
+ if elements_equal(elem1, elem2) and elem2 not in elems_to_remove:
+ elems_to_remove.append(elem2)
+ continue
+
+ for elem in elems_to_remove:
+ xml_root_element.findall("errors")[0].remove(elem)
+
+def merge(files):
+ result_xml_root = None
+ for xml_file in files:
+ xml_root = ElementTree.parse(xml_file).getroot()
+ for xml_error_elem in xml_root.iter('errors'):
+ if result_xml_root is None:
+ result_xml_root = xml_root
+ insert_point = result_xml_root.findall("errors")[0]
+ else:
+ insert_point.extend(xml_error_elem)
+
+ 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
+
+ remove_duplicates(result)
+
+ if output is not None:
+ ElementTree.ElementTree(result).write(output)
+ else:
+ print(ElementTree.tostring(result).decode('utf-8'))
+
+if __name__ == '__main__':
+ run()
--
2.25.1Hi Bertrand, On 24/03/2022 11:04, Bertrand Marquis wrote: > cppcheck can be used to check Xen code quality. Is there anything we should be concerned of in the initial report? > > 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. We want to write down the minimum version of cppcheck we are going to support. My suggestion would be to use the version you picked for your development. > > 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@arm.com> > Signed-off-by: Michal Orzel <michal.orzel@arm.com> > --- > .gitignore | 3 ++ > xen/Makefile | 75 +++++++++++++++++++++++++++- > xen/arch/arm/include/asm/processor.h | 4 +- > xen/include/xen/config.h | 4 ++ > xen/include/xen/kconfig.h | 5 ++ > xen/tools/merge_cppcheck_reports.py | 73 +++++++++++++++++++++++++++ In the long term, I think it would be useful if we generate the report regularly. We might be able to hook the script that generate https://xenbits.xen.org/docs/. > diff --git a/xen/arch/arm/include/asm/processor.h b/xen/arch/arm/include/asm/processor.h > index 852b5f3c24..0b4ba73760 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) > > -#if (SCTLR_EL2_SET ^ SCTLR_EL2_CLEAR) != 0xffffffffffffffffUL > +#ifndef CPPCHECK > +#if (SCTLR_EL2_SET ^ SCTLR_EL2_CLEAR) != 0xffffffffffffffffULL > #error "Inconsistent SCTLR_EL2 set/clear bits" > #endif > +#endif Why is it necessary? Cheers, -- Julien Grall
Hi Julien, > On 24 Mar 2022, at 13:00, Julien Grall <julien@xen.org> wrote: > > Hi Bertrand, > > On 24/03/2022 11:04, Bertrand Marquis wrote: >> cppcheck can be used to check Xen code quality. > > Is there anything we should be concerned of in the initial report? Nothing major no. Michal will soon push a serie with a first set of fixes, mostly related to unused variables. > >> 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. > > We want to write down the minimum version of cppcheck we are going to support. My suggestion would be to use the version you picked for your development. Agree, 2.7 (current stable) should be the one as we had some unstable behaviours with other versions. Should I put that in the main readme ? > >> 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@arm.com> >> Signed-off-by: Michal Orzel <michal.orzel@arm.com> >> --- >> .gitignore | 3 ++ >> xen/Makefile | 75 +++++++++++++++++++++++++++- >> xen/arch/arm/include/asm/processor.h | 4 +- >> xen/include/xen/config.h | 4 ++ >> xen/include/xen/kconfig.h | 5 ++ >> xen/tools/merge_cppcheck_reports.py | 73 +++++++++++++++++++++++++++ > > In the long term, I think it would be useful if we generate the report regularly. We might be able to hook the script that generate https://xenbits.xen.org/docs/. Agree > >> diff --git a/xen/arch/arm/include/asm/processor.h b/xen/arch/arm/include/asm/processor.h >> index 852b5f3c24..0b4ba73760 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) >> -#if (SCTLR_EL2_SET ^ SCTLR_EL2_CLEAR) != 0xffffffffffffffffUL >> +#ifndef CPPCHECK >> +#if (SCTLR_EL2_SET ^ SCTLR_EL2_CLEAR) != 0xffffffffffffffffULL >> #error "Inconsistent SCTLR_EL2 set/clear bits" >> #endif >> +#endif > > Why is it necessary? The ULL was an attempt to fix but did not work. I will remove that in the next version. Here cppcheck preprocessor is wrongly generating the error for a reason that I could not understand, hence the ifdef. Cheers Bertrand > > Cheers, > > -- > Julien Grall
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
Hi Jan, > On 25 Mar 2022, at 12:43, Jan Beulich <jbeulich@suse.com> wrote: > > 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. Cppcheck is running on the preprocessed files. It has a mode to try to do all combinations of CONFIG_ but we have far to much of them and this is ending up in lots of wrong findings doing combinations which are not possible. I tried at the beginning to use obj- but failed. Now this is a lot cleaner in Makefiles thanks to Anthony’s job and I will give it an other try. > >> @@ -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 ... Those are not in the standard CFLAGS but are added to CFLAGS in sub-makefiles so I have to add them explicitly. > >> + $(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)) Ok > >> +# 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. Ok > > Also aiui you shouldn't be using $(BASEDIR) anymore, but $(srctree) > or $(objtree). Right I will fix that > >> +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)? This does not work as CPPCHECKFILES will be empty if TARGET is build by cppcheck as there will be no object file when the variable will be generated. This would require something like recalling make from make. If you have a solution for that I am open. The probability to have an object file missing (we do parse the source code is quite low so it thought this was an acceptable limitation. > >> --- 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? My attempt to have twice --include ended in bugs in cppcheck so I did that instead. An other solution was to generate an header including both and include that one. I am open to suggestion here. > >> --- 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? Cppcheck is not optimising the code but looking at the syntax so the consequence here is that cppcheck is checking some code which might be optimised out by the compiler. This is not optimal but still it should analyse properly the code. Bertrand > > Jan >
On 25.03.2022 13:57, Bertrand Marquis wrote: >> On 25 Mar 2022, at 12:43, Jan Beulich <jbeulich@suse.com> wrote: >> On 24.03.2022 12:04, Bertrand Marquis wrote: >>> --- 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? > > Cppcheck is not optimising the code but looking at the syntax so the > consequence here is that cppcheck is checking some code which might > be optimised out by the compiler. This is not optimal but still it should > analyse properly the code. Aren't you saying so merely because all uses of IS_ENABLED() in our sources so far are in if()? It would seem to me that when used in #if (as can be found in Linux, which hence means could be the case in our tree as well sooner or later) sections of code might either be skipped or syntax errors may result. Or wait - IS_ENABLED() does itself kind of rely on the respective CONFIG_* to expand to a numeric value; when expanding to a string, I guess the compiler would also warn about the resulting construct when used in if() (and reject any uses with #if). Jan
Hi Jan, > On 25 Mar 2022, at 15:10, Jan Beulich <jbeulich@suse.com> wrote: > > On 25.03.2022 13:57, Bertrand Marquis wrote: >>> On 25 Mar 2022, at 12:43, Jan Beulich <jbeulich@suse.com> wrote: >>> On 24.03.2022 12:04, Bertrand Marquis wrote: >>>> --- 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? >> >> Cppcheck is not optimising the code but looking at the syntax so the >> consequence here is that cppcheck is checking some code which might >> be optimised out by the compiler. This is not optimal but still it should >> analyse properly the code. > > Aren't you saying so merely because all uses of IS_ENABLED() in our > sources so far are in if()? It would seem to me that when used in #if > (as can be found in Linux, which hence means could be the case in our > tree as well sooner or later) sections of code might either be skipped > or syntax errors may result. Or wait - IS_ENABLED() does itself kind > of rely on the respective CONFIG_* to expand to a numeric value; when > expanding to a string, I guess the compiler would also warn about the > resulting construct when used in if() (and reject any uses with #if). I am not quite sure I am following what you are saying (or asking). I say that most use cases are if (IS_ENABLED(x)) so from the syntax point of view it is ok to not do exactly as IS_ENABLED really does. And cppcheck is checking the code not the result. Now it would be better to do it right but the point of the patch is to enable cppcheck not make it perfect on the first shot. Cheers Bertrand > > Jan >
On 25.03.2022 15:28, Bertrand Marquis wrote: >> On 25 Mar 2022, at 15:10, Jan Beulich <jbeulich@suse.com> wrote: >> On 25.03.2022 13:57, Bertrand Marquis wrote: >>>> On 25 Mar 2022, at 12:43, Jan Beulich <jbeulich@suse.com> wrote: >>>> On 24.03.2022 12:04, Bertrand Marquis wrote: >>>>> --- 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? >>> >>> Cppcheck is not optimising the code but looking at the syntax so the >>> consequence here is that cppcheck is checking some code which might >>> be optimised out by the compiler. This is not optimal but still it should >>> analyse properly the code. >> >> Aren't you saying so merely because all uses of IS_ENABLED() in our >> sources so far are in if()? It would seem to me that when used in #if >> (as can be found in Linux, which hence means could be the case in our >> tree as well sooner or later) sections of code might either be skipped >> or syntax errors may result. Or wait - IS_ENABLED() does itself kind >> of rely on the respective CONFIG_* to expand to a numeric value; when >> expanding to a string, I guess the compiler would also warn about the >> resulting construct when used in if() (and reject any uses with #if). > > I am not quite sure I am following what you are saying (or asking). I first tried to clarify what I'm concerned about, but then said that apparently there is no reason to be concerned. I'm sorry if this didn't come across quite clear enough. Bottom line - no request for any change here. Jan > I say that most use cases are if (IS_ENABLED(x)) so from the syntax point > of view it is ok to not do exactly as IS_ENABLED really does. And > cppcheck is checking the code not the result. > Now it would be better to do it right but the point of the patch is to enable > cppcheck not make it perfect on the first shot. > > Cheers > Bertrand > >> >> Jan >> >
On Thu, 24 Mar 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@arm.com>
> Signed-off-by: Michal Orzel <michal.orzel@arm.com>
Very cool, I was looking forward to this :-)
> diff --git a/xen/tools/merge_cppcheck_reports.py b/xen/tools/merge_cppcheck_reports.py
> new file mode 100755
> index 0000000000..ef055f6925
> --- /dev/null
> +++ b/xen/tools/merge_cppcheck_reports.py
> @@ -0,0 +1,73 @@
> +#!/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)
> +
> + if el1_location != el2_location: return False
> +
> + return True
> +
> +def remove_duplicates(xml_root_element):
> + elems_to_remove = []
> + index = 0
> + elems_list = list(xml_root_element.findall("errors")[0])
> + for elem1 in elems_list:
> + index += 1
> + for elem2 in elems_list[index:]:
> + if elements_equal(elem1, elem2) and elem2 not in elems_to_remove:
> + elems_to_remove.append(elem2)
> + continue
> +
> + for elem in elems_to_remove:
> + xml_root_element.findall("errors")[0].remove(elem)
> +
> +def merge(files):
> + result_xml_root = None
> + for xml_file in files:
> + xml_root = ElementTree.parse(xml_file).getroot()
Traceback (most recent call last):
File "/local/repos/xen-upstream/xen/tools/merge_cppcheck_reports.py", line 73, in <module>
run()
File "/local/repos/xen-upstream/xen/tools/merge_cppcheck_reports.py", line 60, in run
result = merge(files)
File "/local/repos/xen-upstream/xen/tools/merge_cppcheck_reports.py", line 43, in merge
xml_root = ElementTree.parse(xml_file).getroot()
File "/usr/lib/python2.7/xml/etree/ElementTree.py", line 1182, in parse
tree.parse(source, parser)
File "/usr/lib/python2.7/xml/etree/ElementTree.py", line 657, in parse
self._root = parser.close()
File "/usr/lib/python2.7/xml/etree/ElementTree.py", line 1671, in close
self._raiseerror(v)
File "/usr/lib/python2.7/xml/etree/ElementTree.py", line 1523, in _raiseerror
raise err
xml.etree.ElementTree.ParseError: no element found: line 11, column 0
make: *** [Makefile:576: xen-cppcheck.xml] Error 1
I think we should catch the xml.etree.ElementTree.ParseError exception and continue?
Hi Stefano,
On 25.03.2022 02:32, Stefano Stabellini wrote:
> On Thu, 24 Mar 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@arm.com>
>> Signed-off-by: Michal Orzel <michal.orzel@arm.com>
>
> Very cool, I was looking forward to this :-)
>
>
>> diff --git a/xen/tools/merge_cppcheck_reports.py b/xen/tools/merge_cppcheck_reports.py
>> new file mode 100755
>> index 0000000000..ef055f6925
>> --- /dev/null
>> +++ b/xen/tools/merge_cppcheck_reports.py
>> @@ -0,0 +1,73 @@
>> +#!/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)
>> +
>> + if el1_location != el2_location: return False
>> +
>> + return True
>> +
>> +def remove_duplicates(xml_root_element):
>> + elems_to_remove = []
>> + index = 0
>> + elems_list = list(xml_root_element.findall("errors")[0])
>> + for elem1 in elems_list:
>> + index += 1
>> + for elem2 in elems_list[index:]:
>> + if elements_equal(elem1, elem2) and elem2 not in elems_to_remove:
>> + elems_to_remove.append(elem2)
>> + continue
>> +
>> + for elem in elems_to_remove:
>> + xml_root_element.findall("errors")[0].remove(elem)
>> +
>> +def merge(files):
>> + result_xml_root = None
>> + for xml_file in files:
>> + xml_root = ElementTree.parse(xml_file).getroot()
>
>
> Traceback (most recent call last):
> File "/local/repos/xen-upstream/xen/tools/merge_cppcheck_reports.py", line 73, in <module>
> run()
> File "/local/repos/xen-upstream/xen/tools/merge_cppcheck_reports.py", line 60, in run
> result = merge(files)
> File "/local/repos/xen-upstream/xen/tools/merge_cppcheck_reports.py", line 43, in merge
> xml_root = ElementTree.parse(xml_file).getroot()
> File "/usr/lib/python2.7/xml/etree/ElementTree.py", line 1182, in parse
> tree.parse(source, parser)
> File "/usr/lib/python2.7/xml/etree/ElementTree.py", line 657, in parse
> self._root = parser.close()
> File "/usr/lib/python2.7/xml/etree/ElementTree.py", line 1671, in close
> self._raiseerror(v)
> File "/usr/lib/python2.7/xml/etree/ElementTree.py", line 1523, in _raiseerror
> raise err
> xml.etree.ElementTree.ParseError: no element found: line 11, column 0
> make: *** [Makefile:576: xen-cppcheck.xml] Error 1
>
> I think we should catch the xml.etree.ElementTree.ParseError exception and continue?
Well, this is of course something that we might do but this error clearly warns us that
some XML files is not well formatted and therefore is not parsable. This could mean that
you are using some old cppcheck version. Is it correct assumption?
Cheers,
Michal
On Fri, 25 Mar 2022, Michal Orzel wrote:
> On 25.03.2022 02:32, Stefano Stabellini wrote:
> > On Thu, 24 Mar 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@arm.com>
> >> Signed-off-by: Michal Orzel <michal.orzel@arm.com>
> >
> > Very cool, I was looking forward to this :-)
> >
> >
> >> diff --git a/xen/tools/merge_cppcheck_reports.py b/xen/tools/merge_cppcheck_reports.py
> >> new file mode 100755
> >> index 0000000000..ef055f6925
> >> --- /dev/null
> >> +++ b/xen/tools/merge_cppcheck_reports.py
> >> @@ -0,0 +1,73 @@
> >> +#!/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)
> >> +
> >> + if el1_location != el2_location: return False
> >> +
> >> + return True
> >> +
> >> +def remove_duplicates(xml_root_element):
> >> + elems_to_remove = []
> >> + index = 0
> >> + elems_list = list(xml_root_element.findall("errors")[0])
> >> + for elem1 in elems_list:
> >> + index += 1
> >> + for elem2 in elems_list[index:]:
> >> + if elements_equal(elem1, elem2) and elem2 not in elems_to_remove:
> >> + elems_to_remove.append(elem2)
> >> + continue
> >> +
> >> + for elem in elems_to_remove:
> >> + xml_root_element.findall("errors")[0].remove(elem)
> >> +
> >> +def merge(files):
> >> + result_xml_root = None
> >> + for xml_file in files:
> >> + xml_root = ElementTree.parse(xml_file).getroot()
> >
> >
> > Traceback (most recent call last):
> > File "/local/repos/xen-upstream/xen/tools/merge_cppcheck_reports.py", line 73, in <module>
> > run()
> > File "/local/repos/xen-upstream/xen/tools/merge_cppcheck_reports.py", line 60, in run
> > result = merge(files)
> > File "/local/repos/xen-upstream/xen/tools/merge_cppcheck_reports.py", line 43, in merge
> > xml_root = ElementTree.parse(xml_file).getroot()
> > File "/usr/lib/python2.7/xml/etree/ElementTree.py", line 1182, in parse
> > tree.parse(source, parser)
> > File "/usr/lib/python2.7/xml/etree/ElementTree.py", line 657, in parse
> > self._root = parser.close()
> > File "/usr/lib/python2.7/xml/etree/ElementTree.py", line 1671, in close
> > self._raiseerror(v)
> > File "/usr/lib/python2.7/xml/etree/ElementTree.py", line 1523, in _raiseerror
> > raise err
> > xml.etree.ElementTree.ParseError: no element found: line 11, column 0
> > make: *** [Makefile:576: xen-cppcheck.xml] Error 1
> >
> > I think we should catch the xml.etree.ElementTree.ParseError exception and continue?
>
> Well, this is of course something that we might do but this error clearly warns us that
> some XML files is not well formatted and therefore is not parsable. This could mean that
> you are using some old cppcheck version. Is it correct assumption?
I confirm it was an issue with the cppcheck version. I updated to the
latest main branch and it worked fine, thanks!
Then, I suggest we catch the exception and print an informative error
like "please upgrade your cppcheck to version xxx or greater"
Hi Stefano,
> On 25 Mar 2022, at 19:57, Stefano Stabellini <sstabellini@kernel.org> wrote:
>
> On Fri, 25 Mar 2022, Michal Orzel wrote:
>> On 25.03.2022 02:32, Stefano Stabellini wrote:
>>> On Thu, 24 Mar 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@arm.com>
>>>> Signed-off-by: Michal Orzel <michal.orzel@arm.com>
>>>
>>> Very cool, I was looking forward to this :-)
>>>
>>>
>>>> diff --git a/xen/tools/merge_cppcheck_reports.py b/xen/tools/merge_cppcheck_reports.py
>>>> new file mode 100755
>>>> index 0000000000..ef055f6925
>>>> --- /dev/null
>>>> +++ b/xen/tools/merge_cppcheck_reports.py
>>>> @@ -0,0 +1,73 @@
>>>> +#!/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)
>>>> +
>>>> + if el1_location != el2_location: return False
>>>> +
>>>> + return True
>>>> +
>>>> +def remove_duplicates(xml_root_element):
>>>> + elems_to_remove = []
>>>> + index = 0
>>>> + elems_list = list(xml_root_element.findall("errors")[0])
>>>> + for elem1 in elems_list:
>>>> + index += 1
>>>> + for elem2 in elems_list[index:]:
>>>> + if elements_equal(elem1, elem2) and elem2 not in elems_to_remove:
>>>> + elems_to_remove.append(elem2)
>>>> + continue
>>>> +
>>>> + for elem in elems_to_remove:
>>>> + xml_root_element.findall("errors")[0].remove(elem)
>>>> +
>>>> +def merge(files):
>>>> + result_xml_root = None
>>>> + for xml_file in files:
>>>> + xml_root = ElementTree.parse(xml_file).getroot()
>>>
>>>
>>> Traceback (most recent call last):
>>> File "/local/repos/xen-upstream/xen/tools/merge_cppcheck_reports.py", line 73, in <module>
>>> run()
>>> File "/local/repos/xen-upstream/xen/tools/merge_cppcheck_reports.py", line 60, in run
>>> result = merge(files)
>>> File "/local/repos/xen-upstream/xen/tools/merge_cppcheck_reports.py", line 43, in merge
>>> xml_root = ElementTree.parse(xml_file).getroot()
>>> File "/usr/lib/python2.7/xml/etree/ElementTree.py", line 1182, in parse
>>> tree.parse(source, parser)
>>> File "/usr/lib/python2.7/xml/etree/ElementTree.py", line 657, in parse
>>> self._root = parser.close()
>>> File "/usr/lib/python2.7/xml/etree/ElementTree.py", line 1671, in close
>>> self._raiseerror(v)
>>> File "/usr/lib/python2.7/xml/etree/ElementTree.py", line 1523, in _raiseerror
>>> raise err
>>> xml.etree.ElementTree.ParseError: no element found: line 11, column 0
>>> make: *** [Makefile:576: xen-cppcheck.xml] Error 1
>>>
>>> I think we should catch the xml.etree.ElementTree.ParseError exception and continue?
>>
>> Well, this is of course something that we might do but this error clearly warns us that
>> some XML files is not well formatted and therefore is not parsable. This could mean that
>> you are using some old cppcheck version. Is it correct assumption?
>
> I confirm it was an issue with the cppcheck version. I updated to the
> latest main branch and it worked fine, thanks!
>
> Then, I suggest we catch the exception and print an informative error
> like "please upgrade your cppcheck to version xxx or greater"
I will investigate that before pushing the next version.
Cheers
Bertrand
On 24/03/2022 11: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.
>
> 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@arm.com>
> Signed-off-by: Michal Orzel <michal.orzel@arm.com>
Does CPPCheck have configurable errors vs warnings? Should we wire this
into CI so we can fail builds which introduce errors that we've already
managed to purge from the codebase?
Also, please include Anthony on future patches, given the extent of
makefile changes.
~Andrew
> ---
> .gitignore | 3 ++
> xen/Makefile | 75 +++++++++++++++++++++++++++-
> xen/arch/arm/include/asm/processor.h | 4 +-
> xen/include/xen/config.h | 4 ++
> xen/include/xen/kconfig.h | 5 ++
> xen/tools/merge_cppcheck_reports.py | 73 +++++++++++++++++++++++++++
> 6 files changed, 161 insertions(+), 3 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 18a4f7e101..0280d65051 100644
> --- a/xen/Makefile
> +++ b/xen/Makefile
> @@ -336,7 +336,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): %: _% ;
> @@ -424,15 +424,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
> @@ -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 \
> + $(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=$@ $<
> +
> +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
> +
> +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
> + $(call if_changed,merge_cppcheck_reports)
> +
> +%.c.cppcheck: %.c $(BASEDIR)/include/generated/autoconf.h $(BASEDIR)/include/generated/compiler-def.h
> + $(call if_changed,cppcheck_xml)
> +
> +# Put this in generated headers this way it is cleaned by include/Makefile
> +$(BASEDIR)/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..0b4ba73760 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)
>
> -#if (SCTLR_EL2_SET ^ SCTLR_EL2_CLEAR) != 0xffffffffffffffffUL
> +#ifndef CPPCHECK
> +#if (SCTLR_EL2_SET ^ SCTLR_EL2_CLEAR) != 0xffffffffffffffffULL
> #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 b76222ecf6..36e11e7133 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..ef055f6925
> --- /dev/null
> +++ b/xen/tools/merge_cppcheck_reports.py
> @@ -0,0 +1,73 @@
> +#!/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)
> +
> + if el1_location != el2_location: return False
> +
> + return True
> +
> +def remove_duplicates(xml_root_element):
> + elems_to_remove = []
> + index = 0
> + elems_list = list(xml_root_element.findall("errors")[0])
> + for elem1 in elems_list:
> + index += 1
> + for elem2 in elems_list[index:]:
> + if elements_equal(elem1, elem2) and elem2 not in elems_to_remove:
> + elems_to_remove.append(elem2)
> + continue
> +
> + for elem in elems_to_remove:
> + xml_root_element.findall("errors")[0].remove(elem)
> +
> +def merge(files):
> + result_xml_root = None
> + for xml_file in files:
> + xml_root = ElementTree.parse(xml_file).getroot()
> + for xml_error_elem in xml_root.iter('errors'):
> + if result_xml_root is None:
> + result_xml_root = xml_root
> + insert_point = result_xml_root.findall("errors")[0]
> + else:
> + insert_point.extend(xml_error_elem)
> +
> + 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
> +
> + remove_duplicates(result)
> +
> + if output is not None:
> + ElementTree.ElementTree(result).write(output)
> + else:
> + print(ElementTree.tostring(result).decode('utf-8'))
> +
> +if __name__ == '__main__':
> + run()
On 24.03.2022 14:06, Andrew Cooper wrote: > On 24/03/2022 11: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. >> >> 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@arm.com> >> Signed-off-by: Michal Orzel <michal.orzel@arm.com> > > Does CPPCheck have configurable errors vs warnings? Should we wire this > into CI so we can fail builds which introduce errors that we've already > managed to purge from the codebase? > For cppcheck, every finding is an error as it is placed within <errors> section in XML. cppcheck differentiates different types of findings by using "severity" element which can be e.g. error, style, warning, etc. cppcheck-html uses this field to group findings into categories and present it in a nice format to be seen on a web browser. However there is currently no way to tell cppcheck to find only errors. Cheers, Michal
Hi Andrew, > On 24 Mar 2022, at 14:06, Andrew Cooper <andrew.cooper3@citrix.com> wrote: > > On 24/03/2022 11: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. >> >> 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@arm.com> >> Signed-off-by: Michal Orzel <michal.orzel@arm.com> > > Does CPPCheck have configurable errors vs warnings? Should we wire this > into CI so we can fail builds which introduce errors that we've already > managed to purge from the codebase? No we cannot configure which findings are errors or warnings. What we can (and probably will) do is create a list of cppcheck rules that we want to check and pass that to cppcheck (can be done using a text file). Once this is done and all findings are solved we should definitely add cppcheck to the CI and fail builds when errors are introduced. > > Also, please include Anthony on future patches, given the extent of > makefile changes. I will for sure. Cheers Bertrand
© 2016 - 2024 Red Hat, Inc.