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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

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

> +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)?

Jan