> On 29 Nov 2022, at 09:42, Jan Beulich <jbeul...@suse.com> wrote:
>
> On 28.11.2022 16:37, Luca Fancellu wrote:
>>> On 28 Nov 2022, at 15:19, Jan Beulich <jbeul...@suse.com> 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.fance...@arm.com>
>>>
>>> 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
