[PATCH v2 0/2] Cppcheck MISRA analysis improvements

[PATCH v2 0/2] Cppcheck MISRA analysis improvements

[Luca Fancellu]

This serie is adding a way to skip the check for some rules that the Xen project
has agreed to follow, this is because cppcheck reports too many false-positive
on some rules and it would be easier in a first phase to skip the check on them
and allow the tool to be mature enough before using it on the specific rules.

The serie includes also an improvement for the cppcheck report.

Luca Fancellu (2):
  xen/cppcheck: sort alphabetically cppcheck report entries
  xen/cppcheck: add parameter to skip given MISRA rules

 xen/scripts/xen_analysis/cppcheck_analysis.py |  8 +++--
 .../xen_analysis/cppcheck_report_utils.py     |  7 ++++
 xen/scripts/xen_analysis/settings.py          | 35 +++++++++++--------
 xen/tools/convert_misra_doc.py                | 28 ++++++++++-----
 4 files changed, 53 insertions(+), 25 deletions(-)

-- 
2.25.1

[PATCH v2 1/2] xen/cppcheck: sort alphabetically cppcheck report entries

[Luca Fancellu]

Sort alphabetically cppcheck report entries when producing the text
report, this will help comparing different reports and will group
together findings from the same file.

The sort operation is performed with two criteria, the first one is
sorting by misra rule, the second one is sorting by file.

Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>
---
Changes in v2:
 - Sort with two criteria, first misra rule, second filename
   (Michal, Jan)
---
---
 xen/scripts/xen_analysis/cppcheck_report_utils.py | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/xen/scripts/xen_analysis/cppcheck_report_utils.py b/xen/scripts/xen_analysis/cppcheck_report_utils.py
index 02440aefdfec..0b6cc72b9ac1 100644
--- a/xen/scripts/xen_analysis/cppcheck_report_utils.py
+++ b/xen/scripts/xen_analysis/cppcheck_report_utils.py
@@ -104,6 +104,13 @@ def cppcheck_merge_txt_fragments(fragments_list, out_txt_file, strip_paths):
                 for path in strip_paths:
                     text_report_content[i] = text_report_content[i].replace(
                                                                 path + "/", "")
+                    # Split by : separator
+                    text_report_content[i] = text_report_content[i].split(":")
+            # sort alphabetically for second field (misra rule) and as second
+            # criteria for the first field (file name)
+            text_report_content.sort(key = lambda x: (x[1], x[0]))
+            # merge back with : separator
+            text_report_content = [":".join(x) for x in text_report_content]
             # Write the final text report
             outfile.writelines(text_report_content)
     except OSError as e:
-- 
2.25.1

[PATCH v2 2/2] xen/cppcheck: add parameter to skip given MISRA rules

[Luca Fancellu]

Add parameter to skip the passed MISRA rules during the cppcheck
analysis, the rules are specified as a list of comma separated
rules with the MISRA number notation (e.g. 1.1,1.3,...).

Modify convert_misra_doc.py script to take an extra parameter
giving a list of MISRA rule to be skipped, comma separated.
While there, fix some typos in the help and print functions.

Modify settings.py and cppcheck_analysis.py to have a new
parameter (--cppcheck-skip-rules) used to specify a list of
MISRA rule to be skipped during the cppcheck analysis.

Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>
Acked-by: Stefano Stabellini <sstabellini@kernel.org>
---
Changes in v2:
 - Add Ack-by (Stefano)
---
---
 xen/scripts/xen_analysis/cppcheck_analysis.py |  8 +++--
 xen/scripts/xen_analysis/settings.py          | 35 +++++++++++--------
 xen/tools/convert_misra_doc.py                | 28 ++++++++++-----
 3 files changed, 46 insertions(+), 25 deletions(-)

diff --git a/xen/scripts/xen_analysis/cppcheck_analysis.py b/xen/scripts/xen_analysis/cppcheck_analysis.py
index 0e952a169641..cc1f403d315e 100644
--- a/xen/scripts/xen_analysis/cppcheck_analysis.py
+++ b/xen/scripts/xen_analysis/cppcheck_analysis.py
@@ -153,11 +153,15 @@ def generate_cppcheck_deps():
     if settings.cppcheck_misra:
         cppcheck_flags = cppcheck_flags + " --addon=cppcheck-misra.json"
 
+        skip_rules_arg = ""
+        if settings.cppcheck_skip_rules != "":
+            skip_rules_arg = "-s {}".format(settings.cppcheck_skip_rules)
+
         utils.invoke_command(
             "{}/convert_misra_doc.py -i {}/docs/misra/rules.rst"
-            " -o {}/cppcheck-misra.txt -j {}/cppcheck-misra.json"
+            " -o {}/cppcheck-misra.txt -j {}/cppcheck-misra.json {}"
                 .format(settings.tools_dir, settings.repo_dir,
-                        settings.outdir, settings.outdir),
+                        settings.outdir, settings.outdir, skip_rules_arg),
             False, CppcheckDepsPhaseError,
             "An error occured when running:\n{}"
         )
diff --git a/xen/scripts/xen_analysis/settings.py b/xen/scripts/xen_analysis/settings.py
index a8502e554e95..8c0d357fe0dc 100644
--- a/xen/scripts/xen_analysis/settings.py
+++ b/xen/scripts/xen_analysis/settings.py
@@ -24,6 +24,7 @@ cppcheck_binpath = "cppcheck"
 cppcheck_html = False
 cppcheck_htmlreport_binpath = "cppcheck-htmlreport"
 cppcheck_misra = False
+cppcheck_skip_rules = ""
 make_forward_args = ""
 outdir = xen_dir
 
@@ -53,20 +54,22 @@ Cppcheck report creation phase runs only when --run-cppcheck is passed to the
 script.
 
 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
-  --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
+  --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
+  --cppcheck-skip-rules=  List of MISRA rules to be skipped, comma separated.
+                          (e.g. --cppcheck-skip-rules=1.1,20.7,8.4)
+  --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], cppcheck_binpath,
                      cppcheck_htmlreport_binpath))
@@ -78,6 +81,7 @@ def parse_commandline(argv):
     global cppcheck_html
     global cppcheck_htmlreport_binpath
     global cppcheck_misra
+    global cppcheck_skip_rules
     global make_forward_args
     global outdir
     global step_get_make_vars
@@ -115,6 +119,9 @@ def parse_commandline(argv):
             cppcheck_htmlreport_binpath = args_with_content_regex.group(2)
         elif option == "--cppcheck-misra":
             cppcheck_misra = True
+        elif args_with_content_regex and \
+             args_with_content_regex.group(1) == "--cppcheck-skip-rules":
+            cppcheck_skip_rules = args_with_content_regex.group(2)
         elif option == "--distclean":
             target_distclean = True
         elif (option == "--help") or (option == "-h"):
diff --git a/xen/tools/convert_misra_doc.py b/xen/tools/convert_misra_doc.py
index 13074d8a2e91..8984ec625fa7 100755
--- a/xen/tools/convert_misra_doc.py
+++ b/xen/tools/convert_misra_doc.py
@@ -4,12 +4,14 @@
 This script is converting the misra documentation RST file into a text file
 that can be used as text-rules for cppcheck.
 Usage:
-    convert_misr_doc.py -i INPUT [-o OUTPUT] [-j JSON]
+    convert_misra_doc.py -i INPUT [-o OUTPUT] [-j JSON] [-s RULES,[...,RULES]]
 
     INPUT  - RST file containing the list of misra rules.
     OUTPUT - file to store the text output to be used by cppcheck.
              If not specified, the result will be printed to stdout.
     JSON   - cppcheck json file to be created (optional).
+    RULES  - list of rules to skip during the analysis, comma separated
+             (e.g. 1.1,1.2,1.3,...)
 """
 
 import sys, getopt, re
@@ -47,21 +49,25 @@ def main(argv):
     outfile = ''
     outstr = sys.stdout
     jsonfile = ''
+    force_skip = ''
 
     try:
-        opts, args = getopt.getopt(argv,"hi:o:j:",["input=","output=","json="])
+        opts, args = getopt.getopt(argv,"hi:o:j:s:",
+                                   ["input=","output=","json=","skip="])
     except getopt.GetoptError:
-        print('convert-misra.py -i <input> [-o <output>] [-j <json>')
+        print('convert-misra.py -i <input> [-o <output>] [-j <json>] [-s <rules>]')
         sys.exit(2)
     for opt, arg in opts:
         if opt == '-h':
-            print('convert-misra.py -i <input> [-o <output>] [-j <json>')
+            print('convert-misra.py -i <input> [-o <output>] [-j <json>] [-s <rules>]')
             print('  If output is not specified, print to stdout')
             sys.exit(1)
         elif opt in ("-i", "--input"):
             infile = arg
         elif opt in ("-o", "--output"):
             outfile = arg
+        elif opt in ("-s", "--skip"):
+            force_skip = arg
         elif opt in ("-j", "--json"):
             jsonfile = arg
 
@@ -169,14 +175,18 @@ def main(argv):
 
     skip_list = []
 
+    # Add rules to be skipped anyway
+    for r in force_skip.split(','):
+        skip_list.append(r)
+
     # Search for missing rules and add a dummy text with the rule number
     for i in misra_c2012_rules:
         for j in list(range(1,misra_c2012_rules[i]+1)):
-            if str(i) + '.' + str(j) not in rule_list:
-                outstr.write('Rule ' + str(i) + '.' + str(j) + '\n')
-                outstr.write('No description for rule ' + str(i) + '.' + str(j)
-                             + '\n')
-                skip_list.append(str(i) + '.' + str(j))
+            rule_str = str(i) + '.' + str(j)
+            if (rule_str not in rule_list) and (rule_str not in skip_list):
+                outstr.write('Rule ' + rule_str + '\n')
+                outstr.write('No description for rule ' + rule_str + '\n')
+                skip_list.append(rule_str)
 
     # Make cppcheck happy by starting the appendix
     outstr.write('Appendix B\n')
-- 
2.25.1

Re: [PATCH v2 1/2] xen/cppcheck: sort alphabetically cppcheck report entries

[Michal Orzel]

Hi Luca,

On 30/01/2023 12:01, Luca Fancellu wrote:
> 
> 
> Sort alphabetically cppcheck report entries when producing the text
> report, this will help comparing different reports and will group
> together findings from the same file.
> 
> The sort operation is performed with two criteria, the first one is
> sorting by misra rule, the second one is sorting by file.
> 
> Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>
> ---
> Changes in v2:
>  - Sort with two criteria, first misra rule, second filename
>    (Michal, Jan)
> ---
> ---
>  xen/scripts/xen_analysis/cppcheck_report_utils.py | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/xen/scripts/xen_analysis/cppcheck_report_utils.py b/xen/scripts/xen_analysis/cppcheck_report_utils.py
> index 02440aefdfec..0b6cc72b9ac1 100644
> --- a/xen/scripts/xen_analysis/cppcheck_report_utils.py
> +++ b/xen/scripts/xen_analysis/cppcheck_report_utils.py
> @@ -104,6 +104,13 @@ def cppcheck_merge_txt_fragments(fragments_list, out_txt_file, strip_paths):
>                  for path in strip_paths:
>                      text_report_content[i] = text_report_content[i].replace(
>                                                                  path + "/", "")
> +                    # Split by : separator
> +                    text_report_content[i] = text_report_content[i].split(":")
This is where the for loop body ends so it should be separated from the rest by an empty line.

> +            # sort alphabetically for second field (misra rule) and as second
The second field is not necessary a "misra rule". It is just an error id (e.g. unreadVariable).
However this is just a python script and we use cppcheck mostly for MISRA so I do not object.
 
> +            # criteria for the first field (file name)
> +            text_report_content.sort(key = lambda x: (x[1], x[0]))
> +            # merge back with : separator
> +            text_report_content = [":".join(x) for x in text_report_content]
>              # Write the final text report
>              outfile.writelines(text_report_content)
>      except OSError as e:
> --
> 2.25.1
> 

With the first remark fixed (e.g. on commit),
Reviewed-by: Michal Orzel <michal.orzel@amd.com>

~Michal

Re: [PATCH v2 1/2] xen/cppcheck: sort alphabetically cppcheck report entries

[Luca Fancellu]

> On 30 Jan 2023, at 11:22, Michal Orzel <michal.orzel@amd.com> wrote:
> 
> Hi Luca,
> 
> On 30/01/2023 12:01, Luca Fancellu wrote:
>> 
>> 
>> Sort alphabetically cppcheck report entries when producing the text
>> report, this will help comparing different reports and will group
>> together findings from the same file.
>> 
>> The sort operation is performed with two criteria, the first one is
>> sorting by misra rule, the second one is sorting by file.
>> 
>> Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>
>> ---
>> Changes in v2:
>> - Sort with two criteria, first misra rule, second filename
>>   (Michal, Jan)
>> ---
>> ---
>> xen/scripts/xen_analysis/cppcheck_report_utils.py | 7 +++++++
>> 1 file changed, 7 insertions(+)
>> 
>> diff --git a/xen/scripts/xen_analysis/cppcheck_report_utils.py b/xen/scripts/xen_analysis/cppcheck_report_utils.py
>> index 02440aefdfec..0b6cc72b9ac1 100644
>> --- a/xen/scripts/xen_analysis/cppcheck_report_utils.py
>> +++ b/xen/scripts/xen_analysis/cppcheck_report_utils.py
>> @@ -104,6 +104,13 @@ def cppcheck_merge_txt_fragments(fragments_list, out_txt_file, strip_paths):
>>                 for path in strip_paths:
>>                     text_report_content[i] = text_report_content[i].replace(
>>                                                                 path + "/", "")
>> +                    # Split by : separator
>> +                    text_report_content[i] = text_report_content[i].split(":")
> This is where the for loop body ends so it should be separated from the rest by an empty line.
> 
>> +            # sort alphabetically for second field (misra rule) and as second
> The second field is not necessary a "misra rule". It is just an error id (e.g. unreadVariable).
> However this is just a python script and we use cppcheck mostly for MISRA so I do not object.

Yes you are right, if the committer is willing to change it on commit I’ll appreciate, otherwise I can
fix it and respin the patch.

> 
>> +            # criteria for the first field (file name)
>> +            text_report_content.sort(key = lambda x: (x[1], x[0]))
>> +            # merge back with : separator
>> +            text_report_content = [":".join(x) for x in text_report_content]
>>             # Write the final text report
>>             outfile.writelines(text_report_content)
>>     except OSError as e:
>> --
>> 2.25.1
>> 
> 
> With the first remark fixed (e.g. on commit),
> Reviewed-by: Michal Orzel <michal.orzel@amd.com>

Thank you for your review

> 
> ~Michal

Re: [PATCH v2 1/2] xen/cppcheck: sort alphabetically cppcheck report entries

[Stefano Stabellini]

[-- Attachment #1: Type: text/plain, Size: 1878 bytes --]

On Mon, 30 Jan 2023, Luca Fancellu wrote:
> Sort alphabetically cppcheck report entries when producing the text
> report, this will help comparing different reports and will group
> together findings from the same file.
> 
> The sort operation is performed with two criteria, the first one is
> sorting by misra rule, the second one is sorting by file.
> 
> Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>

Acked-by: Stefano Stabellini <sstabellini@kernel.org>


> ---
> Changes in v2:
>  - Sort with two criteria, first misra rule, second filename
>    (Michal, Jan)
> ---
> ---
>  xen/scripts/xen_analysis/cppcheck_report_utils.py | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/xen/scripts/xen_analysis/cppcheck_report_utils.py b/xen/scripts/xen_analysis/cppcheck_report_utils.py
> index 02440aefdfec..0b6cc72b9ac1 100644
> --- a/xen/scripts/xen_analysis/cppcheck_report_utils.py
> +++ b/xen/scripts/xen_analysis/cppcheck_report_utils.py
> @@ -104,6 +104,13 @@ def cppcheck_merge_txt_fragments(fragments_list, out_txt_file, strip_paths):
>                  for path in strip_paths:
>                      text_report_content[i] = text_report_content[i].replace(
>                                                                  path + "/", "")
> +                    # Split by : separator
> +                    text_report_content[i] = text_report_content[i].split(":")
> +            # sort alphabetically for second field (misra rule) and as second
> +            # criteria for the first field (file name)
> +            text_report_content.sort(key = lambda x: (x[1], x[0]))
> +            # merge back with : separator
> +            text_report_content = [":".join(x) for x in text_report_content]
>              # Write the final text report
>              outfile.writelines(text_report_content)
>      except OSError as e:
> -- 
> 2.25.1
>