Re: [PATCH 2/4] xen/scripts: add cppcheck tool to the xen-analysis.py script

[Luca Fancellu <Luca.Fancellu@xxxxxxx>]

> On 29 Nov 2022, at 09:42, Jan Beulich <jbeulich@xxxxxxxx> wrote:
> 
> On 28.11.2022 16:37, Luca Fancellu wrote:
>>> On 28 Nov 2022, at 15:19, Jan Beulich <jbeulich@xxxxxxxx> wrote:
>>> On 28.11.2022 15:10, Luca Fancellu wrote:
>>>> Change cppcheck invocation method by using the xen-analysis.py
>>>> script using the arguments --run-cppcheck.
>>>> 
>>>> Now cppcheck analysis will build Xen while the analysis is performed
>>>> on the source files, it will produce a text report and an additional
>>>> html output when the script is called with --cppcheck-html.
>>>> 
>>>> With this patch cppcheck will benefit of platform configuration files
>>>> that will help it to understand the target of the compilation and
>>>> improve the analysis.
>>>> 
>>>> To do so:
>>>> - remove cppcheck rules from Makefile and move them to the script.
>>>> - Update xen-analysis.py with the code to integrate cppcheck.
>>>> - merge the script merge_cppcheck_reports.py into the xen-analysis
>>>>  script package and rework the code to integrate it.
>>>> - add platform configuration files for cppcheck..
>>>> - add cppcheck-cc.sh script that is a wrapper for cppcheck and it's
>>>>  used as Xen compiler, it will intercept all flags given from the
>>>>  make build system and it will execute cppcheck on the compiled
>>>>  file together with the file compilation.
>>>> - guarded hypercall-defs.c with CPPCHECK define because cppcheck
>>>>  gets confused as the file does not contain c code.
>>>> - add false-positive-cppcheck.json file
>>>> - update documentation.
>>>> - update .gitignore
>>>> 
>>>> Signed-off-by: Luca Fancellu <luca.fancellu@xxxxxxx>
>>> 
>>> Just two and a half questions, not a full review:
>>> 
>>>> ---
>>>> .gitignore                                    |   8 +-
>>>> docs/misra/cppcheck.txt                       |  27 +-
>>>> docs/misra/documenting-violations.rst         |   7 +-
>>>> docs/misra/false-positive-cppcheck.json       |  12 +
>>>> docs/misra/xen-static-analysis.rst            |  42 ++-
>>>> xen/Makefile                                  | 116 +-------
>>>> xen/include/hypercall-defs.c                  |   9 +
>>>> xen/scripts/xen-analysis.py                   |  18 +-
>>>> xen/scripts/xen_analysis/cppcheck_analysis.py | 272 ++++++++++++++++++
>>>> .../xen_analysis/cppcheck_report_utils.py     | 130 +++++++++
>>>> xen/scripts/xen_analysis/generic_analysis.py  |  21 +-
>>>> xen/scripts/xen_analysis/settings.py          |  77 ++++-
>>>> xen/scripts/xen_analysis/utils.py             |  21 +-
>>>> xen/tools/cppcheck-cc.sh                      | 223 ++++++++++++++
>>>> xen/tools/cppcheck-plat/arm32-wchar_t4.xml    |  17 ++
>>>> xen/tools/cppcheck-plat/arm64-wchar_t2.xml    |  17 ++
>>>> xen/tools/cppcheck-plat/arm64-wchar_t4.xml    |  17 ++
>>>> xen/tools/cppcheck-plat/x86_64-wchar_t2.xml   |  17 ++
>>>> xen/tools/cppcheck-plat/x86_64-wchar_t4.xml   |  17 ++
>>> 
>>> What are these last five files about? There's nothing about them in
>>> the description afaics.
>> 
>> They are cppcheck platform configuration files, they help cppcheck to 
>> understand
>> the size of the types depending on the target of the compilation.
>> 
>> This section in the commit message is to introduce them:
>> 
>> With this patch cppcheck will benefit of platform configuration files
>> that will help it to understand the target of the compilation and
>> improve the analysis.
>> 
>> Do you think I should say it differently? Or maybe say that they reside in 
>> xen/tools/cppcheck-plat/ ?
> 
> Perhaps (I didn't read that paragraph as relating to _anything_ in
> tree), e.g.:
> 
> With this patch cppcheck will benefit from platform configuration files
> that will help it to understand the target of the compilation and
> improve the analysis. These are XML files placed in
> xen/tools/cppcheck-plat/, describing ... (I don't know what to put here).
> 
> Please write the description here such that people not familiar with
> cppcheck (or more generally with any external tool) can still follow
> what you're talking about and what the patch is doing.

Ok I can modify the description to add more details

> 
>>>> --- /dev/null
>>>> +++ b/xen/scripts/xen_analysis/cppcheck_analysis.py
>>>> @@ -0,0 +1,272 @@
>>>> +#!/usr/bin/env python3
>>>> +
>>>> +import os, re, shutil
>>>> +from . import settings, utils, cppcheck_report_utils
>>>> +
>>>> +class GetMakeVarsPhaseError(Exception):
>>>> +    pass
>>>> +
>>>> +class CppcheckDepsPhaseError(Exception):
>>>> +    pass
>>>> +
>>>> +class CppcheckReportPhaseError(Exception):
>>>> +    pass
>>>> +
>>>> +CPPCHECK_BUILD_DIR = "build-dir-cppcheck"
>>>> +CPPCHECK_HTMLREPORT_OUTDIR = "cppcheck-htmlreport"
>>>> +CPPCHECK_REPORT_OUTDIR = "cppcheck-report"
>>>> +cppcheck_extra_make_args = ""
>>>> +xen_cc = ""
>>>> +
>>>> +def get_make_vars():
>>>> +    global xen_cc
>>>> +    invoke_make = utils.invoke_command(
>>>> +            "make -C {} {} export-variable-CC"
>>>> +                .format(settings.xen_dir, settings.make_forward_args),
>>>> +            True, GetMakeVarsPhaseError,
>>>> +            "Error occured retrieving make vars:\n{}"
>>>> +        )
>>>> +
>>>> +    cc_var_regex = re.search('^CC=(.*)$', invoke_make, flags=re.M)
>>>> +    if cc_var_regex:
>>>> +        xen_cc = cc_var_regex.group(1)
>>>> +
>>>> +    if xen_cc == "":
>>>> +        raise GetMakeVarsPhaseError("CC variable not found in Xen make 
>>>> output")
>>> 
>>> What use is CC without CFLAGS? Once again the description could do
>>> with containing some information on what's going on here, and why
>>> you need to export any variables in the first place.
>> 
>> We don’t need CFLAGS here, we need only CC to generate 
>> include/generated/compiler-def.h and
>> to pass CC to the cppcheck-cc.sh --compiler argument.
> 
> Hmm, I see that include/generated/compiler-def.h is generated already
> now without any use of CFLAGS. Which looks suspicious to me. Sadly
> the uses in xen/Makefile are lacking any details on what this is for,
> and Bertrand's commit introducing it doesn't explain its purpose
> either. Maybe again something entirely obvious to people knowing
> cppcheck sufficiently well ...
> 
>> Would a comment in the code be ok?
> 
> Not sure (yet).
> 
> Jan