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

[Luca Fancellu <Luca.Fancellu@xxxxxxx>]

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

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

Would a comment in the code be ok?


> 
>> +if [ -n "${CC_FILE}" ];
>> +then
>> +    for path in ${IGNORE_PATH_LIST}
>> +    do
>> +        if [[ ${CC_FILE} == *${path}* ]]
>> +        then
>> +            IGNORE_PATH="y"
>> +            echo "${0}: ${CC_FILE} ignored by --ignore-path matching 
>> *${path}*"
>> +        fi
>> +    done
>> +    if [ "${IGNORE_PATH}" = "n" ]
>> +    then
>> +        JDB_FILE="${OBJTREE_PATH}/$(basename "${CC_FILE}".json)"
>> +
>> +        # Prepare the Json Compilation Database for the file
>> +        create_jcd "${COMPILER} ${FORWARD_FLAGS}"
>> +
>> +        out_file="${OBJTREE_PATH}/$(basename "${CC_FILE%.c}".cppcheck.txt)"
>> +
>> +        # Check wchar size
>> +        wchar_plat_suffix="t4"
>> +        # sed prints the last occurence of -f(no-)short-wchar which is the 
>> one
>> +        # applied to the file by the compiler
>> +        wchar_option=$(echo "${FORWARD_FLAGS}" | \
>> +            sed -nre 's,.*(-f(no-)?short-wchar).*,\1,p')
>> +        if [ "${wchar_option}" = "-fshort-wchar" ]
>> +        then
>> +            wchar_plat_suffix="t2"
>> +        fi
>> +
>> +        # Select the right target platform, ARCH is generated from Xen 
>> Makefile
>> +        
>> platform="${CPPCHECK_PLAT_PATH}/${ARCH}-wchar_${wchar_plat_suffix}.xml"
> 
> Purely cosmetic, but still: Why is "t" part of the suffix rather than
> being part of the expression here (allowing e.g. a grep for "wchar_t"
> to hit here)?

No reason, I don’t have a strong objection to change it

> 
> Jan