[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


  • To: Jan Beulich <jbeulich@xxxxxxxx>
  • From: Bertrand Marquis <Bertrand.Marquis@xxxxxxx>
  • Date: Fri, 25 Mar 2022 12:57:30 +0000
  • Accept-language: en-GB, en-US
  • Arc-authentication-results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=arm.com; dmarc=pass action=none header.from=arm.com; dkim=pass header.d=arm.com; arc=none
  • Arc-message-signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-AntiSpam-MessageData-ChunkCount:X-MS-Exchange-AntiSpam-MessageData-0:X-MS-Exchange-AntiSpam-MessageData-1; bh=sG7olXZlyxTVSGAoKTZiPEHJCPRagZz/ItshextdSCM=; b=jAiObvibv/46gtyolpZUP1hKt5EbTSg3wivRhoJuM1vOHHDCiB7WZ3lh8UR2OyJBvxdtjXoMhLTI+y+s55/OIclh3fRPABx+C2hKOZM6bybIE3sUHC5+7HbtbD4aIzCLdsy8jgCpRmzpssdbQtXVcfXFavuY/TwWL4z6jP/60ua01ctjNaKI3ag2Y7X4yFj9dcCN5+39btPvxTyTk7MMLNmyiAju29VuKKLFQmh0UUmDyLgCRw/zd1DLE8PqbebdROTTu4ct9XisgmeWg3uHERGlNIcZE7wmaWk06UWHTudbnPTe3xzdyaW9xLlMxQQRGGTDgBcs7Q1vNt8erpDIcQ==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=eRnaOKCFxGZSwQYgzHQRXPT3REFYDquSPS/aDT3Zcg4lR+VeyEPZCMgFfpnDSIhHtbIGIea3FLEyJr8MVXK9q0orosrFf/U8gQtm62xREyXLqKiftim9ka951Rp14WMwC0qucxEV6s377+ZuMsL3+ERTfcvuNszybDHD8UlJQHqcE6DAWt/tSaq0RtjVxFgsWSwyxfBg7CQRuqBjtjw2cBqIL9W6ZPLwC6iZUVWsZeynGDC7Q0tquOTJQIPygT67Vjg75g7ee7pd+ZSU0YpEXM+nV6xIsEIS0lXYxm/pnOCKliv5hIv0PpjDLrJoPy0fMxLYXw/nGF92ANjAPSLiEA==
  • Authentication-results-original: dkim=none (message not signed) header.d=none;dmarc=none action=none header.from=arm.com;
  • Cc: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>, George Dunlap <george.dunlap@xxxxxxxxxx>, Julien Grall <julien@xxxxxxx>, Stefano Stabellini <sstabellini@xxxxxxxxxx>, Wei Liu <wl@xxxxxxx>, Volodymyr Babchuk <Volodymyr_Babchuk@xxxxxxxx>, Michal Orzel <Michal.Orzel@xxxxxxx>, "xen-devel@xxxxxxxxxxxxxxxxxxxx" <xen-devel@xxxxxxxxxxxxxxxxxxxx>
  • Delivery-date: Fri, 25 Mar 2022 12:58:06 +0000
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>
  • Nodisclaimer: true
  • Original-authentication-results: dkim=none (message not signed) header.d=none;dmarc=none action=none header.from=arm.com;
  • Thread-index: AQHYP279DnP7J8Edsk+NG7Po4Qnn9azP/EyAgAAUmoA=
  • Thread-topic: [RFC PATCH] xen/build: Add cppcheck and cppcheck-html make rules

Hi Jan,


> On 25 Mar 2022, at 12:43, Jan Beulich <jbeulich@xxxxxxxx> 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
> 


 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.