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

[Luca Fancellu <Luca.Fancellu@xxxxxxx>]

> On 6 Dec 2022, at 17:06, Stefano Stabellini <sstabellini@xxxxxxxxxx> wrote:
> 
> On Tue, 6 Dec 2022, Luca Fancellu wrote:
>> Hi Stefano,
>>>> 
>>>> +++ b/docs/misra/false-positive-cppcheck.json
>>>> @@ -0,0 +1,12 @@
>>>> +{
>>>> +    "version": "1.0",
>>>> +    "content": [
>>>> +        {
>>>> +            "id": "SAF-0-false-positive-cppcheck",
>>>> +            "violation-id": "",
>>>> +            "tool-version": "",
>>>> +            "name": "Sentinel",
>>>> +            "text": "Next ID to be used"
>>>> +        }
>>>> +    ]
>>>> +}
>>> 
>>> I think we need to add to the cppcheck document how to figure out the
>>> cppcheck id for a given violation in the html report
>> 
>> I’m planning to send some patches with cppcheck false positive fixes, would 
>> them be enough?
>> 
>> We already have a section in documenting-violation.rst on how to document 
>> the finding, for
>> cppcheck it’s just a matter to get the text report, do you think it’s better 
>> to add a part to that section
>> on how to locate the cppcheck violation id from its text report?
> 
> Examples would certainly help a lot. Looking at the html results it
> wasn't clear to me what the violation-id actually was. It took me a few
> tries to understand that "shadowVariable" was the cppcheck violation-id.
> 
> Maybe just add: look under the column "Defect ID" amoung the html
> results to find the violation-id, such as "variableScope".

I was thinking about showing where to locate the violation ID from the text 
report, do you think it’s better
to give an example from the HTML report instead?

So far I have added this part to the bottom of documenting-violations.rst:

Also, the same tag can be used on other symbols from the linker that are
declared in the codebase, because the justification holds for them too.

A possible violation found by Cppcheck can be handled in the same way, from the
cppcheck report it is possible to identify the violation id:

| include/public/arch-arm.h(226,0):misra-c2012-20.7:style:Expressions resulting 
from the expansion of macro parameters shall be enclosed in parentheses (Misra 
rule 20.7)

Given the violation id "misra-c2012-20.7", we can follow the procedure above to
justify the finding.



> 
> 
>>>> diff --git a/xen/scripts/xen_analysis/generic_analysis.py 
>>>> b/xen/scripts/xen_analysis/generic_analysis.py
>>>> index 0b470c4ecf7d..94122aebace0 100644
>>>> --- a/xen/scripts/xen_analysis/generic_analysis.py
>>>> +++ b/xen/scripts/xen_analysis/generic_analysis.py
>>>> @@ -1,7 +1,7 @@
>>>> #!/usr/bin/env python3
>>>> 
>>>> -import os, subprocess
>>>> -from . import settings, utils, tag_database
>>>> +import os
>>>> +from . import settings, utils, tag_database, cppcheck_analysis
>>>> 
>>>> class ParseTagPhaseError(Exception):
>>>>    pass
>>>> @@ -60,18 +60,13 @@ def parse_xen_tags():
>>>> 
>>>> 
>>>> def build_xen():
>>>> -    try:
>>>> -        subprocess.run(
>>>> -            "make -C {} {} build"
>>>> -                .format(settings.xen_dir, settings.make_forward_args),
>>>> -            shell=True, check=True
>>>> +    utils.invoke_command(
>>>> +            "make -C {} {} {} build"
>>>> +                .format(settings.xen_dir, settings.make_forward_args,
>>>> +                        cppcheck_analysis.cppcheck_extra_make_args),
>>>> +            False, BuildPhaseError,
>>>> +            "Build error occured when running:\n{}"
>>>>        )
>>>> -    except (subprocess.CalledProcessError, subprocess.SubprocessError)  
>>>> as e:
>>>> -        excp = BuildPhaseError(
>>>> -                "Build error occured when running:\n{}".format(e.cmd)
>>>> -            )
>>>> -        excp.errorcode = e.returncode if hasattr(e, 'returncode') else 1
>>>> -        raise excp
>>> 
>>> Any reason why we can't have utils.invoke_command directly in patch #1?
>> 
>> There was only one invocation, so I left that as it was, now if I change it 
>> I think I will lost your
>> Tested-by and ack, do you want me to put also in the first patch?
> 
> Yes I think it is fine. I plan to test again your next version anyway

Ok I’ll do the modification

> 
> 
>>>> def clean_analysis_artifacts():
>>>> diff --git a/xen/scripts/xen_analysis/settings.py 
>>>> b/xen/scripts/xen_analysis/settings.py
>>>> index 947dfa2d50af..bd1faafe79a3 100644
>>>> --- a/xen/scripts/xen_analysis/settings.py
>>>> +++ b/xen/scripts/xen_analysis/settings.py
>>>> @@ -7,14 +7,23 @@ xen_dir = os.path.realpath(module_dir + "/../..")
>>>> repo_dir = os.path.realpath(xen_dir + "/..")
>>>> tools_dir = os.path.realpath(xen_dir + "/tools")
>>>> 
>>>> +step_get_make_vars = False
>>>> step_parse_tags = True
>>>> +step_cppcheck_deps = False
>>>> step_build_xen = True
>>>> +step_cppcheck_report = False
>>>> step_clean_analysis = True
>>>> +step_distclean_analysis = False
>>>> 
>>>> target_build = False
>>>> target_clean = False
>>>> +target_distclean = False
>>>> 
>>>> analysis_tool = ""
>>>> +cppcheck_binpath = "cppcheck"
>>>> +cppcheck_html = False
>>>> +cppcheck_htmlreport_binpath = "cppcheck-htmlreport"
>>>> +cppcheck_misra = False
>>>> make_forward_args = ""
>>>> outdir = xen_dir
>>>> 
>>>> @@ -26,29 +35,47 @@ Usage: {} [OPTION] ... [-- [make arguments]]
>>>> This script runs the analysis on the Xen codebase.
>>>> 
>>>> Options:
>>>> -  --build-only    Run only the commands to build Xen with the optional 
>>>> make
>>>> -                  arguments passed to the script
>>>> -  --clean-only    Run only the commands to clean the analysis artifacts
>>>> -  -h, --help      Print this help
>>>> -  --no-build      Skip the build Xen phase
>>>> -  --no-clean      Don\'t clean the analysis artifacts on exit
>>>> -  --run-coverity  Run the analysis for the Coverity tool
>>>> -  --run-eclair    Run the analysis for the Eclair tool
>>>> +  --build-only          Run only the commands to build Xen with the 
>>>> optional
>>>> +                        make arguments passed to the script
>>>> +  --clean-only          Run only the commands to clean the analysis 
>>>> artifacts
>>>> +  --cppcheck-bin=       Path to the cppcheck binary (Default: {})
>>>> +  --cppcheck-html       Produce an additional HTML output report for 
>>>> Cppcheck
>>>> +  --cppcheck-html-bin=  Path to the cppcheck-html binary (Default: {})
>>>> +  --cppcheck-misra      Activate the Cppcheck MISRA analysis
>>>> +  --distclean           Clean analysis artifacts and reports
>>>> +  -h, --help            Print this help
>>>> +  --no-build            Skip the build Xen phase
>>>> +  --no-clean            Don\'t clean the analysis artifacts on exit
>>>> +  --run-coverity        Run the analysis for the Coverity tool
>>>> +  --run-cppcheck        Run the Cppcheck analysis tool on Xen
>>>> +  --run-eclair          Run the analysis for the Eclair tool
>>>> """
>>>> -    print(msg.format(sys.argv[0]))
>>>> +    print(msg.format(sys.argv[0], cppcheck_binpath,
>>>> +                     cppcheck_htmlreport_binpath))
>>>> 
>>>> 
>>>> def parse_commandline(argv):
>>>>    global analysis_tool
>>>> +    global cppcheck_binpath
>>>> +    global cppcheck_html
>>>> +    global cppcheck_htmlreport_binpath
>>>> +    global cppcheck_misra
>>>>    global make_forward_args
>>>>    global outdir
>>>> +    global step_get_make_vars
>>>>    global step_parse_tags
>>>> +    global step_cppcheck_deps
>>>>    global step_build_xen
>>>> +    global step_cppcheck_report
>>>>    global step_clean_analysis
>>>> +    global step_distclean_analysis
>>>>    global target_build
>>>>    global target_clean
>>>> +    global target_distclean
>>>>    forward_to_make = False
>>>>    for option in argv:
>>>> +        args_with_content_regex = re.match(r'^(--[a-z]+[a-z-]*)=(.*)$', 
>>>> option)
>>>> +
>>>>        if forward_to_make:
>>>>            # Intercept outdir
>>>>            outdir_regex = re.match("^O=(.*)$", option)
>>>> @@ -60,6 +87,18 @@ def parse_commandline(argv):
>>>>            target_build = True
>>>>        elif option == "--clean-only":
>>>>            target_clean = True
>>>> +        elif args_with_content_regex and \
>>>> +             args_with_content_regex.group(1) == "--cppcheck-bin":
>>>> +            cppcheck_binpath = args_with_content_regex.group(2)
>>>> +        elif option == "--cppcheck-html":
>>>> +            cppcheck_html = True
>>>> +        elif args_with_content_regex and \
>>>> +             args_with_content_regex.group(1) == "--cppcheck-html-bin":
>>>> +            cppcheck_htmlreport_binpath = args_with_content_regex.group(2)
>>>> +        elif option == "--cppcheck-misra":
>>>> +            cppcheck_misra = True
>>>> +        elif option == "--distclean":
>>>> +            target_distclean = True
>>>>        elif (option == "--help") or (option == "-h"):
>>>>            help()
>>>>            sys.exit(0)
>>>> @@ -69,6 +108,11 @@ def parse_commandline(argv):
>>>>            step_clean_analysis = False
>>>>        elif (option == "--run-coverity") or (option == "--run-eclair"):
>>>>            analysis_tool = option[6:]
>>>> +        elif (option == "--run-cppcheck"):
>>>> +            analysis_tool = "cppcheck"
>>>> +            step_get_make_vars = True
>>>> +            step_cppcheck_deps = True
>>>> +            step_cppcheck_report = True
>>>>        elif option == "--":
>>>>            forward_to_make = True
>>>>        else:
>>>> @@ -76,13 +120,23 @@ def parse_commandline(argv):
>>>>            help()
>>>>            sys.exit(1)
>>>> 
>>>> -    if target_build and target_clean:
>>>> -        print("--build-only is not compatible with --clean-only 
>>>> argument.")
>>>> +    if target_build and (target_clean or target_distclean):
>>>> +        print("--build-only is not compatible with 
>>>> --clean-only/--distclean "
>>>> +              "argument.")
>>>>        sys.exit(1)
>>>> 
>>>> +    if target_distclean:
>>>> +        # Implicit activation of clean target
>>>> +        target_clean = True
>>>> +
>>>> +        step_distclean_analysis = True
>>>> +
>>>>    if target_clean:
>>>> +        step_get_make_vars = False
>>>>        step_parse_tags = False
>>>> +        step_cppcheck_deps = False
>>>>        step_build_xen = False
>>>> +        step_cppcheck_report = False
>>>>        step_clean_analysis = True
>>>>        return
>>>> 
>>>> @@ -95,3 +149,4 @@ def parse_commandline(argv):
>>>>        step_parse_tags = False
>>>>        step_build_xen = True
>>>>        step_clean_analysis = False
>>>> +        step_cppcheck_report = False
>>> 
>>> I think that target_build should not say anything about
>>> step_cppcheck_report.
>>> 
>>> - if one wants to just do a regular build, they can do "make xen"
>>> - if one is calling xen-analysis.py --cppcheck-html --run-cppcheck
>>> --build-only, it means that they want the build done, not the cleaning
>>> done, not the tags substitution. If they also add --cppcheck-html and
>>> --run-cppcheck, then it means that they also want the cppcheck report
>>> produced. --build-only still makes sense because they don't want the
>>> cleaning done and don't want the tag substitution.
>>> 
>>> Does it make sense to you as well?
>>> 
>>> 
>>> If it does, I think we also need to add a note in the help message from
>>> xen_analysis because it is not clear. So basically:
>>> 
>>> <nothing>: tags, build, clean [, cppcheck]
>>> --no-clean: tags, build [, cppcheck]
>>> --build-only: build [, cppcheck]
>>> --no-build: tags
>>> --clean-only: clean
>>> 
>>> Did I get it right?
>> 
>> Ok I can leave the report generation with the build-only, I will also 
>> explain better
>> In the help
> 
> Thank you
> 
> 
>>>> +
>>>> +function create_jcd() {
>>>> +    local line="${1}"
>>>> +    local arg_num=0
>>>> +    local same_line=0
>>>> +
>>>> +    {
>>>> +        echo -e -n "[\n"
>>> 
>>> Everywhere in this bash function: there is no point in passing -n and
>>> then adding \n at the end. You might as well do this:
>>> 
>>> echo -e "["
>>> 
>>> Also, you'll find that in most cases, you don't need -e either, which
>>> simplifies it to:
>>> 
>>> echo "["
>>> 
>>> That's better right? :-)  Of course feel free to use -e when you have
>>> escape and -n when you don't want \n in the output
>> 
>> Yeah I guess they come from copy paste, I can use the right arguments when 
>> needed