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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

>>> --- /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