Re: [RFC PATCH] xen/build: Add cppcheck and cppcheck-html make rules

[Bertrand Marquis <Bertrand.Marquis@xxxxxxx>]

Hi Stefano,

> On 25 Mar 2022, at 19:57, Stefano Stabellini <sstabellini@xxxxxxxxxx> wrote:
> 
> On Fri, 25 Mar 2022, Michal Orzel wrote:
>> On 25.03.2022 02:32, Stefano Stabellini wrote:
>>> On Thu, 24 Mar 2022, Bertrand Marquis wrote:
>>>> cppcheck can be used to check Xen code quality.
>>>> 
>>>> To create a report do "make cppcheck" on a built tree adding any options
>>>> you added during the process you used to build xen (like CROSS_COMPILE
>>>> or XEN_TARGET_ARCH). This will generate an xml report xen-cppcheck.xml.
>>>> 
>>>> To create a html report do "make cppcheck-html" in the same way and a
>>>> full report to be seen in a browser will be generated in
>>>> cppcheck-htmlreport/index.html.
>>>> 
>>>> For better results it is recommended to build your own cppcheck from the
>>>> latest sources that you can find at [1].
>>>> Development and result analysis has been done with cppcheck 2.7.
>>>> 
>>>> The Makefile rule is searching for all C files which have been compiled
>>>> (ie which have a generated .o file) and is running cppcheck on all of
>>>> them using the current configuration of xen so only the code actually
>>>> compiled is checked.
>>>> 
>>>> A new tool is introduced to merge all cppcheck reports into one global
>>>> report including all findings and removing duplicates.
>>>> 
>>>> Some extra variables can be used to customize the report:
>>>> - CPPCHECK can be used to give the full path to the cppcheck binary to
>>>> use (default is to use the one from the standard path).
>>>> - CPPCHECK_HTMLREPORT can be used to give the full path to
>>>> cppcheck-htmlreport (default is to use the one from the standard path).
>>>> 
>>>> This has been tested on several arm configurations (x86 should work but
>>>> has not been tested).
>>>> 
>>>> [1] https://cppcheck.sourceforge.io/
>>>> 
>>>> Signed-off-by: Bertrand Marquis <bertrand.marquis@xxxxxxx>
>>>> Signed-off-by: Michal Orzel <michal.orzel@xxxxxxx>
>>> 
>>> Very cool, I was looking forward to this :-)
>>> 
>>> 
>>>> diff --git a/xen/tools/merge_cppcheck_reports.py 
>>>> b/xen/tools/merge_cppcheck_reports.py
>>>> new file mode 100755
>>>> index 0000000000..ef055f6925
>>>> --- /dev/null
>>>> +++ b/xen/tools/merge_cppcheck_reports.py
>>>> @@ -0,0 +1,73 @@
>>>> +#!/usr/bin/env python
>>>> +
>>>> +"""
>>>> +This script acts as a tool to merge XML files created by cppcheck.
>>>> +Usage:
>>>> +    merge_cppcheck_reports.py [FILES] [OUTPUT]
>>>> +
>>>> +    FILES  - list of XML files with extension .cppcheck
>>>> +    OUTPUT - file to store results (with .xml extension).
>>>> +             If not specified, the script will print results to stdout.
>>>> +"""
>>>> +
>>>> +import sys
>>>> +from xml.etree import ElementTree
>>>> +
>>>> +def elements_equal(el1, el2):
>>>> +    if type(el1) != type(el2): return False
>>>> +
>>>> +    el1_location = str(el1.find('location').attrib)
>>>> +    el2_location = str(el2.find('location').attrib)
>>>> +
>>>> +    if el1_location != el2_location: return False
>>>> +
>>>> +    return True
>>>> +
>>>> +def remove_duplicates(xml_root_element):
>>>> +    elems_to_remove = []
>>>> +    index = 0
>>>> +    elems_list = list(xml_root_element.findall("errors")[0])
>>>> +    for elem1 in elems_list:
>>>> +        index += 1
>>>> +        for elem2 in elems_list[index:]:
>>>> +            if elements_equal(elem1, elem2) and elem2 not in 
>>>> elems_to_remove:
>>>> +                elems_to_remove.append(elem2)
>>>> +                continue
>>>> +
>>>> +    for elem in elems_to_remove:
>>>> +        xml_root_element.findall("errors")[0].remove(elem)
>>>> +
>>>> +def merge(files):
>>>> +    result_xml_root = None
>>>> +    for xml_file in files:
>>>> +        xml_root = ElementTree.parse(xml_file).getroot()
>>> 
>>> 
>>> Traceback (most recent call last):
>>>  File "/local/repos/xen-upstream/xen/tools/merge_cppcheck_reports.py", line 
>>> 73, in <module>
>>>    run()
>>>  File "/local/repos/xen-upstream/xen/tools/merge_cppcheck_reports.py", line 
>>> 60, in run
>>>    result = merge(files)
>>>  File "/local/repos/xen-upstream/xen/tools/merge_cppcheck_reports.py", line 
>>> 43, in merge
>>>    xml_root = ElementTree.parse(xml_file).getroot()
>>>  File "/usr/lib/python2.7/xml/etree/ElementTree.py", line 1182, in parse
>>>    tree.parse(source, parser)
>>>  File "/usr/lib/python2.7/xml/etree/ElementTree.py", line 657, in parse
>>>    self._root = parser.close()
>>>  File "/usr/lib/python2.7/xml/etree/ElementTree.py", line 1671, in close
>>>    self._raiseerror(v)
>>>  File "/usr/lib/python2.7/xml/etree/ElementTree.py", line 1523, in 
>>> _raiseerror
>>>    raise err
>>> xml.etree.ElementTree.ParseError: no element found: line 11, column 0
>>> make: *** [Makefile:576: xen-cppcheck.xml] Error 1
>>> 
>>> I think we should catch the xml.etree.ElementTree.ParseError exception and 
>>> continue?
>> 
>> Well, this is of course something that we might do but this error clearly 
>> warns us that
>> some XML files is not well formatted and therefore is not parsable. This 
>> could mean that
>> you are using some old cppcheck version. Is it correct assumption?
> 
> I confirm it was an issue with the cppcheck version. I updated to the
> latest main branch and it worked fine, thanks!
> 
> Then, I suggest we catch the exception and print an informative error
> like "please upgrade your cppcheck to version xxx or greater"

I will investigate that before pushing the next version.

Cheers
Bertrand