All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/2] xen/misra: create exclusion file list
@ 2023-02-14  8:56 Luca Fancellu
  2023-02-14  8:56 ` [PATCH 1/2] xen/cppcheck: add a way to exclude files from the scan Luca Fancellu
  2023-02-14  8:56 ` [PATCH 2/2] xen/misra: add entries to exclude-list.json Luca Fancellu
  0 siblings, 2 replies; 15+ messages in thread
From: Luca Fancellu @ 2023-02-14  8:56 UTC (permalink / raw)
  To: xen-devel
  Cc: wei.chen, bertrand.marquis, Andrew Cooper, George Dunlap,
	Jan Beulich, Julien Grall, Stefano Stabellini, Michal Orzel,
	Wei Liu

This serie is introducing an exclusion list for the misra analysis, at the
moment only cppcheck can benefit from it because it's the tool where we
control every step and configuration.

Exclude a file from the analysis is the last step we should do, but sometime it
is unavoidable because we can't do direct changes to it to fix/deviate from
findings.
So we declare the file(s) here and we leave the burden of the misra compliance
to the final user.

Luca Fancellu (2):
  xen/cppcheck: add a way to exclude files from the scan
  xen/misra: add entries to exclude-list.json

 docs/misra/exclude-list.json                  | 209 ++++++++++++++++++
 docs/misra/exclude-list.rst                   |  44 ++++
 xen/scripts/xen_analysis/cppcheck_analysis.py |  21 +-
 .../xen_analysis/exclusion_file_list.py       |  79 +++++++
 4 files changed, 351 insertions(+), 2 deletions(-)
 create mode 100644 docs/misra/exclude-list.json
 create mode 100644 docs/misra/exclude-list.rst
 create mode 100644 xen/scripts/xen_analysis/exclusion_file_list.py

-- 
2.25.1



^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH 1/2] xen/cppcheck: add a way to exclude files from the scan
  2023-02-14  8:56 [PATCH 0/2] xen/misra: create exclusion file list Luca Fancellu
@ 2023-02-14  8:56 ` Luca Fancellu
  2023-02-14  9:51   ` Jan Beulich
  2023-02-14  8:56 ` [PATCH 2/2] xen/misra: add entries to exclude-list.json Luca Fancellu
  1 sibling, 1 reply; 15+ messages in thread
From: Luca Fancellu @ 2023-02-14  8:56 UTC (permalink / raw)
  To: xen-devel
  Cc: wei.chen, bertrand.marquis, Andrew Cooper, George Dunlap,
	Jan Beulich, Julien Grall, Stefano Stabellini, Michal Orzel,
	Wei Liu

Add a way to exclude files from the scan, in this way we can skip
some findings from the report on those files that Xen doesn't own.

To do that, introduce the exclude-list.json file under docs/misra,
this file will be populated with relative path to the files/folder
to be excluded.
Introduce a new module, exclusion_file_list.py, to deal with the
exclusion list file and use the new module in cppcheck_analysis.py
to take a list of excluded paths to update the suppression list of
cppcheck.
Modified --suppress flag for cppcheck tool to remove
unmatchedSuppression findings for those external file that are
listed but for example not built for the current architecture.

Add documentation for the file.

Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>
---
 docs/misra/exclude-list.json                  |  4 +
 docs/misra/exclude-list.rst                   | 44 +++++++++++
 xen/scripts/xen_analysis/cppcheck_analysis.py | 21 ++++-
 .../xen_analysis/exclusion_file_list.py       | 79 +++++++++++++++++++
 4 files changed, 146 insertions(+), 2 deletions(-)
 create mode 100644 docs/misra/exclude-list.json
 create mode 100644 docs/misra/exclude-list.rst
 create mode 100644 xen/scripts/xen_analysis/exclusion_file_list.py

diff --git a/docs/misra/exclude-list.json b/docs/misra/exclude-list.json
new file mode 100644
index 000000000000..1fb0ac67747b
--- /dev/null
+++ b/docs/misra/exclude-list.json
@@ -0,0 +1,4 @@
+{
+    "version": "1.0",
+    "content": []
+}
diff --git a/docs/misra/exclude-list.rst b/docs/misra/exclude-list.rst
new file mode 100644
index 000000000000..969539c46beb
--- /dev/null
+++ b/docs/misra/exclude-list.rst
@@ -0,0 +1,44 @@
+.. SPDX-License-Identifier: CC-BY-4.0
+
+Exclude file list for xen-analysis script
+=========================================
+
+The code analysis is performed on the Xen codebase for both MISRA checkers and
+static analysis checkers, there are some files however that needs to be removed
+from the findings report because they are not owned by Xen and they must be kept
+in sync with their origin (completely or even partially), hence we can't easily
+fix findings or deviate from them.
+
+For this reason the file docs/misra/exclude-list.json is used to exclude every
+entry listed in that file from the final report.
+Currently only the cppcheck analysis will use this file.
+
+Here is an example of the exclude-list.json file::
+
+|{
+|    "version": "1.0",
+|    "content": [
+|        {
+|            "rel_path": "relative/path/from/xen/file",
+|            "comment": "This file is originated from ..."
+|        },
+|        {
+|            "rel_path": "relative/path/from/xen/folder/*",
+|            "comment": "This folder is a library"
+|        },
+|        {
+|            "rel_path": "relative/path/from/xen/mem*.c",
+|            "comment": "memcpy.c, memory.c and memcmp.c are from the outside"
+|        }
+|    ]
+|}
+
+Here is an explanation of the fields inside an object of the "content" array:
+ - rel_path: it is the relative path from the Xen folder to the file/folder that
+   needs to be excluded from the analysis report, it can contain a wildcard to
+   match more than one file/folder at the time. This field is mandatory.
+ - comment: an optional comment to explain why the file is removed from the
+   analysis.
+
+To ease the review and the modifications of the entries, they shall be listed in
+alphabetical order referring to the rel_path field.
diff --git a/xen/scripts/xen_analysis/cppcheck_analysis.py b/xen/scripts/xen_analysis/cppcheck_analysis.py
index cc1f403d315e..b681dca90836 100644
--- a/xen/scripts/xen_analysis/cppcheck_analysis.py
+++ b/xen/scripts/xen_analysis/cppcheck_analysis.py
@@ -1,7 +1,8 @@
 #!/usr/bin/env python3
 
 import os, re, shutil
-from . import settings, utils, cppcheck_report_utils
+from . import settings, utils, cppcheck_report_utils, exclusion_file_list
+from .exclusion_file_list import ExclusionFileListError
 
 class GetMakeVarsPhaseError(Exception):
     pass
@@ -50,6 +51,22 @@ def __generate_suppression_list(out_file):
             # header for cppcheck
             supplist_file.write("*:*generated/compiler-def.h\n")
 
+            try:
+                exclusion_file = \
+                    "{}/docs/misra/exclude-list.json".format(settings.repo_dir)
+                exclusion_list = \
+                    exclusion_file_list.load_exclusion_file_list(exclusion_file)
+            except ExclusionFileListError as e:
+                    raise CppcheckDepsPhaseError(
+                            "Issue with reading file {}: {}"
+                                .format(exclusion_file, e)
+                          )
+
+            # Append excluded files to the suppression list, using * as error id
+            # to suppress every findings on that file
+            for path in exclusion_list:
+                supplist_file.write("*:{}\n".format(path))
+
             for entry in headers:
                 filename = entry["file"]
                 try:
@@ -128,7 +145,7 @@ def generate_cppcheck_deps():
  --relative-paths={}
  --inline-suppr
  --suppressions-list={}/suppression-list.txt
- --suppress='unmatchedSuppression:*generated/compiler-def.h'
+ --suppress='unmatchedSuppression:*'
  --include={}/include/xen/config.h
  -DCPPCHECK
 """.format(settings.outdir, CPPCHECK_BUILD_DIR, settings.xen_dir,
diff --git a/xen/scripts/xen_analysis/exclusion_file_list.py b/xen/scripts/xen_analysis/exclusion_file_list.py
new file mode 100644
index 000000000000..4a47a90f5944
--- /dev/null
+++ b/xen/scripts/xen_analysis/exclusion_file_list.py
@@ -0,0 +1,79 @@
+#!/usr/bin/env python3
+
+import os, glob, json
+from . import settings
+
+class ExclusionFileListError(Exception):
+    pass
+
+
+def __cppcheck_path_exclude_syntax(path):
+    # Prepending * to the relative path to match every path where the Xen
+    # codebase could be
+    path = "*" + path
+
+    # Check if the path is to a folder without the wildcard at the end
+    if not (path.endswith(".c") or path.endswith(".h") or path.endswith("*")):
+        # The path is to a folder, if it doesn't have the final /, add it
+        if not path.endswith("/"):
+            path = path + "/"
+        # Since the path is a folder, add a wildcard to the end so that
+        # cppcheck will remove every issue related with this path
+        path = path + "*"
+
+    return path
+
+
+# Reads the exclusion file list and returns a list of relative path to be
+# excluded.
+def load_exclusion_file_list(input_file):
+    ret = []
+    try:
+        with open(input_file, "rt") as handle:
+            content = json.load(handle)
+            entries = content['content']
+    except json.JSONDecodeError as e:
+        raise ExclusionFileListError(
+                "JSON decoding error in file {}: {}".format(input_file, e)
+        )
+    except KeyError:
+        raise ExclusionFileListError(
+            "Malformed JSON file: content field not found!"
+        )
+    except Exception as e:
+        raise ExclusionFileListError(
+                "Can't open file {}: {}".format(input_file, e)
+        )
+
+    for entry in entries:
+        try:
+            path = entry['rel_path']
+        except KeyError:
+            raise ExclusionFileListError(
+                "Malformed JSON entry: rel_path field not found!"
+            )
+        abs_path = settings.xen_dir + "/" + path
+        check_path = [abs_path]
+
+        # If the path contains wildcards, solve them
+        if '*' in abs_path:
+            check_path = glob.glob(abs_path)
+
+        # Check that the path exists
+        for filepath_object in check_path:
+            if not os.path.exists(filepath_object):
+                raise ExclusionFileListError(
+                    "Malformed path: {} refers to {} that does not exists"
+                    .format(path, filepath_object)
+                )
+
+        if settings.analysis_tool == "cppcheck":
+            path = __cppcheck_path_exclude_syntax(path)
+        else:
+            raise ExclusionFileListError(
+                "Unimplemented for {}!".format(settings.analysis_tool)
+            )
+
+        ret.append(path)
+
+    return ret
-- 
2.25.1



^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH 2/2] xen/misra: add entries to exclude-list.json
  2023-02-14  8:56 [PATCH 0/2] xen/misra: create exclusion file list Luca Fancellu
  2023-02-14  8:56 ` [PATCH 1/2] xen/cppcheck: add a way to exclude files from the scan Luca Fancellu
@ 2023-02-14  8:56 ` Luca Fancellu
  2023-02-14  9:54   ` Jan Beulich
  1 sibling, 1 reply; 15+ messages in thread
From: Luca Fancellu @ 2023-02-14  8:56 UTC (permalink / raw)
  To: xen-devel
  Cc: wei.chen, bertrand.marquis, Andrew Cooper, George Dunlap,
	Jan Beulich, Julien Grall, Stefano Stabellini, Michal Orzel,
	Wei Liu

Add entries to the exclude-list.json for those files that need to be
excluded from the analysis scan.

Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>
Signed-off-by: Michal Orzel <michal.orzel@amd.com>
---
This list is originated from Michal's work here:
https://patchwork.kernel.org/project/xen-devel/patch/20221116092032.4423-1-michal.orzel@amd.com/#25123099
the only files removed are the *arm/smmu* that, if I understand
correctly, we would like to include in the scan.
---
---
 docs/misra/exclude-list.json | 207 ++++++++++++++++++++++++++++++++++-
 1 file changed, 206 insertions(+), 1 deletion(-)

diff --git a/docs/misra/exclude-list.json b/docs/misra/exclude-list.json
index 1fb0ac67747b..3a6dc0809af5 100644
--- a/docs/misra/exclude-list.json
+++ b/docs/misra/exclude-list.json
@@ -1,4 +1,209 @@
 {
     "version": "1.0",
-    "content": []
+    "content": [
+        {
+            "rel_path": "arch/arm/arm64/cpufeature.c"
+        },
+        {
+            "rel_path": "arch/arm/arm64/insn.c"
+        },
+        {
+            "rel_path": "arch/arm/arm64/lib/find_next_bit.c"
+        },
+        {
+            "rel_path": "arch/x86/acpi/boot.c"
+        },
+        {
+            "rel_path": "arch/x86/acpi/cpu_idle.c"
+        },
+        {
+            "rel_path": "arch/x86/acpi/cpufreq/cpufreq.c"
+        },
+        {
+            "rel_path": "arch/x86/acpi/cpuidle_menu.c"
+        },
+        {
+            "rel_path": "arch/x86/acpi/lib.c"
+        },
+        {
+            "rel_path": "arch/x86/acpi/power.c"
+        },
+        {
+            "rel_path": "arch/x86/cpu/amd.c"
+        },
+        {
+            "rel_path": "arch/x86/cpu/centaur.c"
+        },
+        {
+            "rel_path": "arch/x86/cpu/common.c"
+        },
+        {
+            "rel_path": "arch/x86/cpu/hygon.c"
+        },
+        {
+            "rel_path": "arch/x86/cpu/intel.c"
+        },
+        {
+            "rel_path": "arch/x86/cpu/intel_cacheinfo.c"
+        },
+        {
+            "rel_path": "arch/x86/cpu/mcheck/mce-apei.c"
+        },
+        {
+            "rel_path": "arch/x86/cpu/mcheck/non-fatal.c"
+        },
+        {
+            "rel_path": "arch/x86/cpu/mtrr/*"
+        },
+        {
+            "rel_path": "arch/x86/cpu/mwait-idle.c"
+        },
+        {
+            "rel_path": "arch/x86/delay.c"
+        },
+        {
+            "rel_path": "arch/x86/dmi_scan.c"
+        },
+        {
+            "rel_path": "arch/x86/domain.c"
+        },
+        {
+            "rel_path": "arch/x86/genapic/*"
+        },
+        {
+            "rel_path": "arch/x86/i387.c"
+        },
+        {
+            "rel_path": "arch/x86/irq.c"
+        },
+        {
+            "rel_path": "arch/x86/mpparse.c"
+        },
+        {
+            "rel_path": "arch/x86/srat.c"
+        },
+        {
+            "rel_path": "arch/x86/time.c"
+        },
+        {
+            "rel_path": "arch/x86/traps.c"
+        },
+        {
+            "rel_path": "arch/x86/usercopy.c"
+        },
+        {
+            "rel_path": "arch/x86/x86_64/mmconf-fam10h.c"
+        },
+        {
+            "rel_path": "common/bitmap.c"
+        },
+        {
+            "rel_path": "common/bunzip2.c"
+        },
+        {
+            "rel_path": "common/cpu.c"
+        },
+        {
+            "rel_path": "common/earlycpio.c"
+        },
+        {
+            "rel_path": "common/inflate.c"
+        },
+        {
+            "rel_path": "common/libfdt/*"
+        },
+        {
+            "rel_path": "common/lz4/decompress.c"
+        },
+        {
+            "rel_path": "common/notifier.c"
+        },
+        {
+            "rel_path": "common/radix-tree.c"
+        },
+        {
+            "rel_path": "common/rcupdate.c"
+        },
+        {
+            "rel_path": "common/softirq.c"
+        },
+        {
+            "rel_path": "common/tasklet.c"
+        },
+        {
+            "rel_path": "common/ubsan/ubsan.c"
+        },
+        {
+            "rel_path": "common/un*.c"
+        },
+        {
+            "rel_path": "common/vsprintf.c"
+        },
+        {
+            "rel_path": "common/xz/*"
+        },
+        {
+            "rel_path": "common/zstd/*"
+        },
+        {
+            "rel_path": "crypto/rijndael.c"
+        },
+        {
+            "rel_path": "crypto/vmac.c"
+        },
+        {
+            "rel_path": "drivers/acpi/apei/*"
+        },
+        {
+            "rel_path": "drivers/acpi/hwregs.c"
+        },
+        {
+            "rel_path": "drivers/acpi/numa.c"
+        },
+        {
+            "rel_path": "drivers/acpi/osl.c"
+        },
+        {
+            "rel_path": "drivers/acpi/reboot.c"
+        },
+        {
+            "rel_path": "drivers/acpi/tables.c"
+        },
+        {
+            "rel_path": "drivers/acpi/tables/*"
+        },
+        {
+            "rel_path": "drivers/acpi/utilities/*"
+        },
+        {
+            "rel_path": "drivers/char/ehci-dbgp.c"
+        },
+        {
+            "rel_path": "drivers/char/xhci-dbc.c"
+        },
+        {
+            "rel_path": "drivers/cpufreq/*"
+        },
+        {
+            "rel_path": "drivers/video/font_*"
+        },
+        {
+            "rel_path": "lib/list-sort.c"
+        },
+        {
+            "rel_path": "lib/mem*.c"
+        },
+        {
+            "rel_path": "lib/rbtree.c"
+        },
+        {
+            "rel_path": "lib/str*.c"
+        },
+        {
+            "rel_path": "lib/xxhash32.c"
+        },
+        {
+            "rel_path": "lib/xxhash64.c"
+        }
+    ]
 }
-- 
2.25.1



^ permalink raw reply related	[flat|nested] 15+ messages in thread

* Re: [PATCH 1/2] xen/cppcheck: add a way to exclude files from the scan
  2023-02-14  8:56 ` [PATCH 1/2] xen/cppcheck: add a way to exclude files from the scan Luca Fancellu
@ 2023-02-14  9:51   ` Jan Beulich
  2023-02-14 10:02     ` Luca Fancellu
  0 siblings, 1 reply; 15+ messages in thread
From: Jan Beulich @ 2023-02-14  9:51 UTC (permalink / raw)
  To: Luca Fancellu
  Cc: wei.chen, bertrand.marquis, Andrew Cooper, George Dunlap,
	Julien Grall, Stefano Stabellini, Michal Orzel, Wei Liu,
	xen-devel

On 14.02.2023 09:56, Luca Fancellu wrote:
> Add a way to exclude files from the scan, in this way we can skip
> some findings from the report on those files that Xen doesn't own.
> 
> To do that, introduce the exclude-list.json file under docs/misra,
> this file will be populated with relative path to the files/folder
> to be excluded.
> Introduce a new module, exclusion_file_list.py, to deal with the
> exclusion list file and use the new module in cppcheck_analysis.py
> to take a list of excluded paths to update the suppression list of
> cppcheck.
> Modified --suppress flag for cppcheck tool to remove
> unmatchedSuppression findings for those external file that are
> listed but for example not built for the current architecture.
> 
> Add documentation for the file.
> 
> Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>
> ---
>  docs/misra/exclude-list.json                  |  4 +
>  docs/misra/exclude-list.rst                   | 44 +++++++++++
>  xen/scripts/xen_analysis/cppcheck_analysis.py | 21 ++++-
>  .../xen_analysis/exclusion_file_list.py       | 79 +++++++++++++++++++
>  4 files changed, 146 insertions(+), 2 deletions(-)
>  create mode 100644 docs/misra/exclude-list.json
>  create mode 100644 docs/misra/exclude-list.rst
>  create mode 100644 xen/scripts/xen_analysis/exclusion_file_list.py

As I think I have asked for before: Can new files please avoid underscores
in their names, when dashes do quite fine? Or does Python have special
restrictions?

> --- a/xen/scripts/xen_analysis/cppcheck_analysis.py
> +++ b/xen/scripts/xen_analysis/cppcheck_analysis.py
> @@ -1,7 +1,8 @@
>  #!/usr/bin/env python3
>  
>  import os, re, shutil
> -from . import settings, utils, cppcheck_report_utils
> +from . import settings, utils, cppcheck_report_utils, exclusion_file_list
> +from .exclusion_file_list import ExclusionFileListError
>  
>  class GetMakeVarsPhaseError(Exception):
>      pass
> @@ -50,6 +51,22 @@ def __generate_suppression_list(out_file):
>              # header for cppcheck
>              supplist_file.write("*:*generated/compiler-def.h\n")
>  
> +            try:
> +                exclusion_file = \
> +                    "{}/docs/misra/exclude-list.json".format(settings.repo_dir)
> +                exclusion_list = \
> +                    exclusion_file_list.load_exclusion_file_list(exclusion_file)
> +            except ExclusionFileListError as e:
> +                    raise CppcheckDepsPhaseError(
> +                            "Issue with reading file {}: {}"
> +                                .format(exclusion_file, e)
> +                          )

My Python isn't very good, but isn't indentation one level too deep
here?

Jan


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 2/2] xen/misra: add entries to exclude-list.json
  2023-02-14  8:56 ` [PATCH 2/2] xen/misra: add entries to exclude-list.json Luca Fancellu
@ 2023-02-14  9:54   ` Jan Beulich
  2023-02-14 22:25     ` Stefano Stabellini
  0 siblings, 1 reply; 15+ messages in thread
From: Jan Beulich @ 2023-02-14  9:54 UTC (permalink / raw)
  To: Luca Fancellu
  Cc: wei.chen, bertrand.marquis, Andrew Cooper, George Dunlap,
	Julien Grall, Stefano Stabellini, Michal Orzel, Wei Liu,
	xen-devel

On 14.02.2023 09:56, Luca Fancellu wrote:
> --- a/docs/misra/exclude-list.json
> +++ b/docs/misra/exclude-list.json
> @@ -1,4 +1,209 @@
>  {
>      "version": "1.0",
> -    "content": []
> +    "content": [
> +        {
> +            "rel_path": "arch/arm/arm64/cpufeature.c"
> +        },
> +        {
> +            "rel_path": "arch/arm/arm64/insn.c"
> +        },
> +        {
> +            "rel_path": "arch/arm/arm64/lib/find_next_bit.c"
> +        },
> +        {
> +            "rel_path": "arch/x86/acpi/boot.c"
> +        },
> +        {
> +            "rel_path": "arch/x86/acpi/cpu_idle.c"
> +        },
> +        {
> +            "rel_path": "arch/x86/acpi/cpufreq/cpufreq.c"
> +        },
> +        {
> +            "rel_path": "arch/x86/acpi/cpuidle_menu.c"
> +        },
> +        {
> +            "rel_path": "arch/x86/acpi/lib.c"
> +        },
> +        {
> +            "rel_path": "arch/x86/acpi/power.c"
> +        },
> +        {
> +            "rel_path": "arch/x86/cpu/amd.c"
> +        },
> +        {
> +            "rel_path": "arch/x86/cpu/centaur.c"
> +        },
> +        {
> +            "rel_path": "arch/x86/cpu/common.c"
> +        },
> +        {
> +            "rel_path": "arch/x86/cpu/hygon.c"
> +        },
> +        {
> +            "rel_path": "arch/x86/cpu/intel.c"
> +        },
> +        {
> +            "rel_path": "arch/x86/cpu/intel_cacheinfo.c"
> +        },
> +        {
> +            "rel_path": "arch/x86/cpu/mcheck/mce-apei.c"
> +        },
> +        {
> +            "rel_path": "arch/x86/cpu/mcheck/non-fatal.c"
> +        },
> +        {
> +            "rel_path": "arch/x86/cpu/mtrr/*"
> +        },
> +        {
> +            "rel_path": "arch/x86/cpu/mwait-idle.c"
> +        },
> +        {
> +            "rel_path": "arch/x86/delay.c"
> +        },
> +        {
> +            "rel_path": "arch/x86/dmi_scan.c"
> +        },
> +        {
> +            "rel_path": "arch/x86/domain.c"
> +        },
> +        {
> +            "rel_path": "arch/x86/genapic/*"
> +        },
> +        {
> +            "rel_path": "arch/x86/i387.c"
> +        },
> +        {
> +            "rel_path": "arch/x86/irq.c"
> +        },
> +        {
> +            "rel_path": "arch/x86/mpparse.c"
> +        },
> +        {
> +            "rel_path": "arch/x86/srat.c"
> +        },
> +        {
> +            "rel_path": "arch/x86/time.c"
> +        },
> +        {
> +            "rel_path": "arch/x86/traps.c"
> +        },
> +        {
> +            "rel_path": "arch/x86/usercopy.c"
> +        },
> +        {
> +            "rel_path": "arch/x86/x86_64/mmconf-fam10h.c"
> +        },
> +        {
> +            "rel_path": "common/bitmap.c"
> +        },
> +        {
> +            "rel_path": "common/bunzip2.c"
> +        },
> +        {
> +            "rel_path": "common/cpu.c"
> +        },
> +        {
> +            "rel_path": "common/earlycpio.c"
> +        },
> +        {
> +            "rel_path": "common/inflate.c"
> +        },
> +        {
> +            "rel_path": "common/libfdt/*"
> +        },
> +        {
> +            "rel_path": "common/lz4/decompress.c"
> +        },
> +        {
> +            "rel_path": "common/notifier.c"
> +        },
> +        {
> +            "rel_path": "common/radix-tree.c"
> +        },
> +        {
> +            "rel_path": "common/rcupdate.c"
> +        },
> +        {
> +            "rel_path": "common/softirq.c"
> +        },
> +        {
> +            "rel_path": "common/tasklet.c"
> +        },
> +        {
> +            "rel_path": "common/ubsan/ubsan.c"
> +        },
> +        {
> +            "rel_path": "common/un*.c"
> +        },
> +        {
> +            "rel_path": "common/vsprintf.c"
> +        },
> +        {
> +            "rel_path": "common/xz/*"
> +        },
> +        {
> +            "rel_path": "common/zstd/*"
> +        },
> +        {
> +            "rel_path": "crypto/rijndael.c"
> +        },
> +        {
> +            "rel_path": "crypto/vmac.c"
> +        },
> +        {
> +            "rel_path": "drivers/acpi/apei/*"
> +        },
> +        {
> +            "rel_path": "drivers/acpi/hwregs.c"
> +        },
> +        {
> +            "rel_path": "drivers/acpi/numa.c"
> +        },
> +        {
> +            "rel_path": "drivers/acpi/osl.c"
> +        },
> +        {
> +            "rel_path": "drivers/acpi/reboot.c"
> +        },
> +        {
> +            "rel_path": "drivers/acpi/tables.c"
> +        },
> +        {
> +            "rel_path": "drivers/acpi/tables/*"
> +        },
> +        {
> +            "rel_path": "drivers/acpi/utilities/*"
> +        },
> +        {
> +            "rel_path": "drivers/char/ehci-dbgp.c"
> +        },
> +        {
> +            "rel_path": "drivers/char/xhci-dbc.c"
> +        },
> +        {
> +            "rel_path": "drivers/cpufreq/*"
> +        },
> +        {
> +            "rel_path": "drivers/video/font_*"
> +        },
> +        {
> +            "rel_path": "lib/list-sort.c"
> +        },
> +        {
> +            "rel_path": "lib/mem*.c"
> +        },
> +        {
> +            "rel_path": "lib/rbtree.c"
> +        },
> +        {
> +            "rel_path": "lib/str*.c"
> +        },
> +        {
> +            "rel_path": "lib/xxhash32.c"
> +        },
> +        {
> +            "rel_path": "lib/xxhash64.c"
> +        }
> +    ]
>  }

Patch 1's example has a "comment" field, which no entry makes use of.
Without that, how does it become clear _why_ a particular file is to
be excluded? The patch description here also doesn't provide any
justification ...

Jan


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 1/2] xen/cppcheck: add a way to exclude files from the scan
  2023-02-14  9:51   ` Jan Beulich
@ 2023-02-14 10:02     ` Luca Fancellu
  0 siblings, 0 replies; 15+ messages in thread
From: Luca Fancellu @ 2023-02-14 10:02 UTC (permalink / raw)
  To: Jan Beulich
  Cc: Wei Chen, Bertrand Marquis, Andrew Cooper, George Dunlap,
	Julien Grall, Stefano Stabellini, Michal Orzel, Wei Liu,
	xen-devel



> On 14 Feb 2023, at 09:51, Jan Beulich <jbeulich@suse.com> wrote:
> 
> On 14.02.2023 09:56, Luca Fancellu wrote:
>> Add a way to exclude files from the scan, in this way we can skip
>> some findings from the report on those files that Xen doesn't own.
>> 
>> To do that, introduce the exclude-list.json file under docs/misra,
>> this file will be populated with relative path to the files/folder
>> to be excluded.
>> Introduce a new module, exclusion_file_list.py, to deal with the
>> exclusion list file and use the new module in cppcheck_analysis.py
>> to take a list of excluded paths to update the suppression list of
>> cppcheck.
>> Modified --suppress flag for cppcheck tool to remove
>> unmatchedSuppression findings for those external file that are
>> listed but for example not built for the current architecture.
>> 
>> Add documentation for the file.
>> 
>> Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>
>> ---
>> docs/misra/exclude-list.json                  |  4 +
>> docs/misra/exclude-list.rst                   | 44 +++++++++++
>> xen/scripts/xen_analysis/cppcheck_analysis.py | 21 ++++-
>> .../xen_analysis/exclusion_file_list.py       | 79 +++++++++++++++++++
>> 4 files changed, 146 insertions(+), 2 deletions(-)
>> create mode 100644 docs/misra/exclude-list.json
>> create mode 100644 docs/misra/exclude-list.rst
>> create mode 100644 xen/scripts/xen_analysis/exclusion_file_list.py
> 
> As I think I have asked for before: Can new files please avoid underscores
> in their names, when dashes do quite fine? Or does Python have special
> restrictions?

From the python coding style:

Modules should have short, all-lowercase names. Underscores can be used in
the module name if it improves readability. Python packages should also have
short, all-lowercase names, although the use of underscores is discouraged.

In this case I think it improves readability

> 
>> --- a/xen/scripts/xen_analysis/cppcheck_analysis.py
>> +++ b/xen/scripts/xen_analysis/cppcheck_analysis.py
>> @@ -1,7 +1,8 @@
>> #!/usr/bin/env python3
>> 
>> import os, re, shutil
>> -from . import settings, utils, cppcheck_report_utils
>> +from . import settings, utils, cppcheck_report_utils, exclusion_file_list
>> +from .exclusion_file_list import ExclusionFileListError
>> 
>> class GetMakeVarsPhaseError(Exception):
>>     pass
>> @@ -50,6 +51,22 @@ def __generate_suppression_list(out_file):
>>             # header for cppcheck
>>             supplist_file.write("*:*generated/compiler-def.h\n")
>> 
>> +            try:
>> +                exclusion_file = \
>> +                    "{}/docs/misra/exclude-list.json".format(settings.repo_dir)
>> +                exclusion_list = \
>> +                    exclusion_file_list.load_exclusion_file_list(exclusion_file)
>> +            except ExclusionFileListError as e:
>> +                    raise CppcheckDepsPhaseError(
>> +                            "Issue with reading file {}: {}"
>> +                                .format(exclusion_file, e)
>> +                          )
> 
> My Python isn't very good, but isn't indentation one level too deep
> here?

Good catch, I’ll fix it

> 
> Jan



^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 2/2] xen/misra: add entries to exclude-list.json
  2023-02-14  9:54   ` Jan Beulich
@ 2023-02-14 22:25     ` Stefano Stabellini
  2023-02-15  8:56       ` Julien Grall
  0 siblings, 1 reply; 15+ messages in thread
From: Stefano Stabellini @ 2023-02-14 22:25 UTC (permalink / raw)
  To: Jan Beulich
  Cc: Luca Fancellu, wei.chen, bertrand.marquis, Andrew Cooper,
	George Dunlap, Julien Grall, Stefano Stabellini, Michal Orzel,
	Wei Liu, xen-devel

On Tue, 14 Feb 2023, Jan Beulich wrote:
> On 14.02.2023 09:56, Luca Fancellu wrote:
> > --- a/docs/misra/exclude-list.json
> > +++ b/docs/misra/exclude-list.json
> > @@ -1,4 +1,209 @@
> >  {
> >      "version": "1.0",
> > -    "content": []
> > +    "content": [
> > +        {
> > +            "rel_path": "arch/arm/arm64/cpufeature.c"
> > +        },
> > +        {
> > +            "rel_path": "arch/arm/arm64/insn.c"
> > +        },
> > +        {
> > +            "rel_path": "arch/arm/arm64/lib/find_next_bit.c"
> > +        },
> > +        {
> > +            "rel_path": "arch/x86/acpi/boot.c"
> > +        },
> > +        {
> > +            "rel_path": "arch/x86/acpi/cpu_idle.c"
> > +        },
> > +        {
> > +            "rel_path": "arch/x86/acpi/cpufreq/cpufreq.c"
> > +        },
> > +        {
> > +            "rel_path": "arch/x86/acpi/cpuidle_menu.c"
> > +        },
> > +        {
> > +            "rel_path": "arch/x86/acpi/lib.c"
> > +        },
> > +        {
> > +            "rel_path": "arch/x86/acpi/power.c"
> > +        },
> > +        {
> > +            "rel_path": "arch/x86/cpu/amd.c"
> > +        },
> > +        {
> > +            "rel_path": "arch/x86/cpu/centaur.c"
> > +        },
> > +        {
> > +            "rel_path": "arch/x86/cpu/common.c"
> > +        },
> > +        {
> > +            "rel_path": "arch/x86/cpu/hygon.c"
> > +        },
> > +        {
> > +            "rel_path": "arch/x86/cpu/intel.c"
> > +        },
> > +        {
> > +            "rel_path": "arch/x86/cpu/intel_cacheinfo.c"
> > +        },
> > +        {
> > +            "rel_path": "arch/x86/cpu/mcheck/mce-apei.c"
> > +        },
> > +        {
> > +            "rel_path": "arch/x86/cpu/mcheck/non-fatal.c"
> > +        },
> > +        {
> > +            "rel_path": "arch/x86/cpu/mtrr/*"
> > +        },
> > +        {
> > +            "rel_path": "arch/x86/cpu/mwait-idle.c"
> > +        },
> > +        {
> > +            "rel_path": "arch/x86/delay.c"
> > +        },
> > +        {
> > +            "rel_path": "arch/x86/dmi_scan.c"
> > +        },
> > +        {
> > +            "rel_path": "arch/x86/domain.c"
> > +        },
> > +        {
> > +            "rel_path": "arch/x86/genapic/*"
> > +        },
> > +        {
> > +            "rel_path": "arch/x86/i387.c"
> > +        },
> > +        {
> > +            "rel_path": "arch/x86/irq.c"
> > +        },
> > +        {
> > +            "rel_path": "arch/x86/mpparse.c"
> > +        },
> > +        {
> > +            "rel_path": "arch/x86/srat.c"
> > +        },
> > +        {
> > +            "rel_path": "arch/x86/time.c"
> > +        },
> > +        {
> > +            "rel_path": "arch/x86/traps.c"
> > +        },
> > +        {
> > +            "rel_path": "arch/x86/usercopy.c"
> > +        },
> > +        {
> > +            "rel_path": "arch/x86/x86_64/mmconf-fam10h.c"
> > +        },
> > +        {
> > +            "rel_path": "common/bitmap.c"
> > +        },
> > +        {
> > +            "rel_path": "common/bunzip2.c"
> > +        },
> > +        {
> > +            "rel_path": "common/cpu.c"
> > +        },
> > +        {
> > +            "rel_path": "common/earlycpio.c"
> > +        },
> > +        {
> > +            "rel_path": "common/inflate.c"
> > +        },
> > +        {
> > +            "rel_path": "common/libfdt/*"
> > +        },
> > +        {
> > +            "rel_path": "common/lz4/decompress.c"
> > +        },
> > +        {
> > +            "rel_path": "common/notifier.c"
> > +        },
> > +        {
> > +            "rel_path": "common/radix-tree.c"
> > +        },
> > +        {
> > +            "rel_path": "common/rcupdate.c"
> > +        },
> > +        {
> > +            "rel_path": "common/softirq.c"
> > +        },
> > +        {
> > +            "rel_path": "common/tasklet.c"
> > +        },
> > +        {
> > +            "rel_path": "common/ubsan/ubsan.c"
> > +        },
> > +        {
> > +            "rel_path": "common/un*.c"
> > +        },
> > +        {
> > +            "rel_path": "common/vsprintf.c"
> > +        },
> > +        {
> > +            "rel_path": "common/xz/*"
> > +        },
> > +        {
> > +            "rel_path": "common/zstd/*"
> > +        },
> > +        {
> > +            "rel_path": "crypto/rijndael.c"
> > +        },
> > +        {
> > +            "rel_path": "crypto/vmac.c"
> > +        },
> > +        {
> > +            "rel_path": "drivers/acpi/apei/*"
> > +        },
> > +        {
> > +            "rel_path": "drivers/acpi/hwregs.c"
> > +        },
> > +        {
> > +            "rel_path": "drivers/acpi/numa.c"
> > +        },
> > +        {
> > +            "rel_path": "drivers/acpi/osl.c"
> > +        },
> > +        {
> > +            "rel_path": "drivers/acpi/reboot.c"
> > +        },
> > +        {
> > +            "rel_path": "drivers/acpi/tables.c"
> > +        },
> > +        {
> > +            "rel_path": "drivers/acpi/tables/*"
> > +        },
> > +        {
> > +            "rel_path": "drivers/acpi/utilities/*"
> > +        },
> > +        {
> > +            "rel_path": "drivers/char/ehci-dbgp.c"
> > +        },
> > +        {
> > +            "rel_path": "drivers/char/xhci-dbc.c"
> > +        },
> > +        {
> > +            "rel_path": "drivers/cpufreq/*"
> > +        },
> > +        {
> > +            "rel_path": "drivers/video/font_*"
> > +        },
> > +        {
> > +            "rel_path": "lib/list-sort.c"
> > +        },
> > +        {
> > +            "rel_path": "lib/mem*.c"
> > +        },
> > +        {
> > +            "rel_path": "lib/rbtree.c"
> > +        },
> > +        {
> > +            "rel_path": "lib/str*.c"
> > +        },
> > +        {
> > +            "rel_path": "lib/xxhash32.c"
> > +        },
> > +        {
> > +            "rel_path": "lib/xxhash64.c"
> > +        }
> > +    ]
> >  }
> 
> Patch 1's example has a "comment" field, which no entry makes use of.
> Without that, how does it become clear _why_ a particular file is to
> be excluded? The patch description here also doesn't provide any
> justification ...

It would be good to have a couple of pre-canned justifications as the
reason for excluding one file could be different from the reason for
excluding another file. Some of the reasons:

- imported from Linux
- too many false positives


That said, we don't necessarily need to know the exact reason for
excluding one file to be able to start scanning xen with cppcheck
automatically. If someone wants to remove a file from the exclude list
in the future they just need to show that cppcheck does a good job at
scanning the file and we can handle the number of violations.

I take that with this exclude list, not accounting for rule 20.7,
cppcheck reports zero violations overall?


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 2/2] xen/misra: add entries to exclude-list.json
  2023-02-14 22:25     ` Stefano Stabellini
@ 2023-02-15  8:56       ` Julien Grall
  2023-02-15 23:49         ` Stefano Stabellini
  0 siblings, 1 reply; 15+ messages in thread
From: Julien Grall @ 2023-02-15  8:56 UTC (permalink / raw)
  To: Stefano Stabellini, Jan Beulich
  Cc: Luca Fancellu, wei.chen, bertrand.marquis, Andrew Cooper,
	George Dunlap, Michal Orzel, Wei Liu, xen-devel

Hi Stefano,

On 14/02/2023 22:25, Stefano Stabellini wrote:
>> Patch 1's example has a "comment" field, which no entry makes use of.
>> Without that, how does it become clear _why_ a particular file is to
>> be excluded? The patch description here also doesn't provide any
>> justification ...
> 
> It would be good to have a couple of pre-canned justifications as the
> reason for excluding one file could be different from the reason for
> excluding another file. Some of the reasons:

I think the reasons should be ambiguous. This is ...

> - imported from Linux

... the case here but...

This reason is pretty clear to me but...

> - too many false positives

... not here. What is too many?

> 
> 
> That said, we don't necessarily need to know the exact reason for
> excluding one file to be able to start scanning xen with cppcheck
> automatically. If someone wants to remove a file from the exclude list
> in the future they just need to show that cppcheck does a good job at
> scanning the file and we can handle the number of violations.

I disagree. A good reasoning from the start will be helpful to decide 
when we can remove a file from the list. Furthermore, we need to set 
good example for any new file we want to exclude.

Furthermore, if we exclude some files, then it will be difficult for the 
reviewers to know when they can be removed from the list. What if this 
is fine with CPPCheck but not EClair (or any other)?

> 
> I take that with this exclude list, not accounting for rule 20.7,
> cppcheck reports zero violations overall?

So the goal right now is to have zero violations from the start? 
Shouldn't it instead be that we don't increase the number violations?

If so, we would want a baseline including the files that have "too many 
violation". The advantage is it will be easier to reduce the violations 
in small chunk and the reviewer can confirm that a patch help.

Cheers,

-- 
Julien Grall


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 2/2] xen/misra: add entries to exclude-list.json
  2023-02-15  8:56       ` Julien Grall
@ 2023-02-15 23:49         ` Stefano Stabellini
  2023-02-16  8:19           ` Jan Beulich
  0 siblings, 1 reply; 15+ messages in thread
From: Stefano Stabellini @ 2023-02-15 23:49 UTC (permalink / raw)
  To: Julien Grall
  Cc: Stefano Stabellini, Jan Beulich, Luca Fancellu, wei.chen,
	bertrand.marquis, Andrew Cooper, George Dunlap, Michal Orzel,
	Wei Liu, xen-devel

Hi Julien and all,

Summarizing here on the list what I had with Julien and Bertrand this
morning.


On Wed, 15 Feb 2023, Julien Grall wrote:
> Hi Stefano,
> 
> On 14/02/2023 22:25, Stefano Stabellini wrote:
> > > Patch 1's example has a "comment" field, which no entry makes use of.
> > > Without that, how does it become clear _why_ a particular file is to
> > > be excluded? The patch description here also doesn't provide any
> > > justification ...
> > 
> > It would be good to have a couple of pre-canned justifications as the
> > reason for excluding one file could be different from the reason for
> > excluding another file. Some of the reasons:
> 
> I think the reasons should be ambiguous. This is ...
> 
> > - imported from Linux
> 
> ... the case here but...
> 
> This reason is pretty clear to me but...
> 
> > - too many false positives
> 
> ... not here. What is too many?
>
> > That said, we don't necessarily need to know the exact reason for
> > excluding one file to be able to start scanning xen with cppcheck
> > automatically. If someone wants to remove a file from the exclude list
> > in the future they just need to show that cppcheck does a good job at
> > scanning the file and we can handle the number of violations.
> 
> I disagree. A good reasoning from the start will be helpful to decide when we
> can remove a file from the list. Furthermore, we need to set good example for
> any new file we want to exclude.
> 
> Furthermore, if we exclude some files, then it will be difficult for the
> reviewers to know when they can be removed from the list. What if this is fine
> with CPPCheck but not EClair (or any other)?

Yes, the reason would help. In previous incarnations of this work, there
was a request for detailed information on external files, such as:
- where this file is coming from
- if coming from Linux, which version of Linux
- maintenance status
- coding style

But this is not what you are asking. You are only asking for a reason
and "imported from Linux" would be good enough. Please correct me if I
am wrong.

If that is the case, I think it is doable. Most of these files would end
up with "imported from Linux" as a reason. That would be fine.

 
> > I take that with this exclude list, not accounting for rule 20.7,
> > cppcheck reports zero violations overall?
> 
> So the goal right now is to have zero violations from the start? Shouldn't it
> instead be that we don't increase the number violations?
> 
> If so, we would want a baseline including the files that have "too many
> violation". The advantage is it will be easier to reduce the violations in
> small chunk and the reviewer can confirm that a patch help.

Yes, we want to be able to compared and see if a patch introduces new
violations, but that's not easy to do with cppcheck and it would help a
lot if we had a cleaner baseline with only few violations. Otherwise
there is too much noise.


To help make progress faster, I took a stab at writing an
exclude-list.json with plausible reasons that should drastically reduce
the number of violations reported by cppcheck, see below.

With the below:

- There are a total of only 18 violations on x86 (Once we ignore the
  unusedStructMember reports that look bogus. We should add
  unusedStructMember to the rules exclusion list.)

- There are a total of only 12 violations on ARM64.

- You can repro my findings, applying this series, updating
  docs/misra/exclude-list.json, and calling
  ./scripts/xen-analysis.py --cppcheck-bin=/local/repos/cppcheck/cppcheck --run-cppcheck -- CROSS_COMPILE="x86_64-linux-gnu-" XEN_TARGET_ARCH=x86_64



{
    "version": "1.0",
    "content": [
        {
            "rel_path": "common/bunzip2.c",
            "comment" : "imported from Linux"
        },
        {
            "rel_path": "common/libfdt/*",
            "comment" : "imported from Linux"
        },
        {
            "rel_path": "common/livepatch_elf.c",
            "comment" : "not in scope initially as it generates many violations and it is not enabled in safety configurations"
        },
        {
            "rel_path": "common/lzo.c",
            "comment" : "imported from Linux"
        },
        {
            "rel_path": "common/symbols-dummy.c",
            "comment" : "MISRA is not relevant to this file"
        },
        {
            "rel_path": "common/xz/*",
            "comment" : "imported from Linux"
        },
        {
            "rel_path": "common/zstd/*",
            "comment" : "imported from Linux"
        },
        {
            "rel_path": "common/unlz4.c",
            "comment" : "imported from Linux"
        },
        {
            "rel_path": "common/unlzma.c",
            "comment" : "imported from Linux"
        },
        {
            "rel_path": "common/unlzo.c",
            "comment" : "imported from Linux"
        },
        {
            "rel_path": "common/version.c",
            "comment" : "cppcheck cannot understand the tricks with linker symbols"
        },
        {
            "rel_path": "include/xen/lib.h",
            "comment" : "imported from Linux"
        },
        {
            "rel_path": "include/xen/sort.h",
            "comment" : "imported from Linux"
        },
        {
            "rel_path": "lib/list-sort.c",
            "comment" : "imported from Linux"
        },
        {
            "rel_path": "lib/rbtree.c",
            "comment" : "imported from Linux"
        },
        {
            "rel_path": "lib/xxhash32.c",
            "comment" : "imported from Linux"
        },
        {
            "rel_path": "lib/xxhash64.c",
            "comment" : "imported from Linux"
        },
        {
            "rel_path": "xsm/flask/*",
            "comment" : "not in scope initially as it generates many violations and it is not enabled in safety configurations"
        }
    ]
}


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 2/2] xen/misra: add entries to exclude-list.json
  2023-02-15 23:49         ` Stefano Stabellini
@ 2023-02-16  8:19           ` Jan Beulich
  2023-02-16  9:20             ` Luca Fancellu
  0 siblings, 1 reply; 15+ messages in thread
From: Jan Beulich @ 2023-02-16  8:19 UTC (permalink / raw)
  To: Stefano Stabellini
  Cc: Luca Fancellu, wei.chen, bertrand.marquis, Andrew Cooper,
	George Dunlap, Michal Orzel, Wei Liu, xen-devel, Julien Grall

On 16.02.2023 00:49, Stefano Stabellini wrote:
> On Wed, 15 Feb 2023, Julien Grall wrote:
>> On 14/02/2023 22:25, Stefano Stabellini wrote:
>>>> Patch 1's example has a "comment" field, which no entry makes use of.
>>>> Without that, how does it become clear _why_ a particular file is to
>>>> be excluded? The patch description here also doesn't provide any
>>>> justification ...
>>>
>>> It would be good to have a couple of pre-canned justifications as the
>>> reason for excluding one file could be different from the reason for
>>> excluding another file. Some of the reasons:
>>
>> I think the reasons should be ambiguous. This is ...
>>
>>> - imported from Linux
>>
>> ... the case here but...
>>
>> This reason is pretty clear to me but...
>>
>>> - too many false positives
>>
>> ... not here. What is too many?
>>
>>> That said, we don't necessarily need to know the exact reason for
>>> excluding one file to be able to start scanning xen with cppcheck
>>> automatically. If someone wants to remove a file from the exclude list
>>> in the future they just need to show that cppcheck does a good job at
>>> scanning the file and we can handle the number of violations.
>>
>> I disagree. A good reasoning from the start will be helpful to decide when we
>> can remove a file from the list. Furthermore, we need to set good example for
>> any new file we want to exclude.
>>
>> Furthermore, if we exclude some files, then it will be difficult for the
>> reviewers to know when they can be removed from the list. What if this is fine
>> with CPPCheck but not EClair (or any other)?
> 
> Yes, the reason would help. In previous incarnations of this work, there
> was a request for detailed information on external files, such as:
> - where this file is coming from
> - if coming from Linux, which version of Linux
> - maintenance status
> - coding style
> 
> But this is not what you are asking. You are only asking for a reason
> and "imported from Linux" would be good enough. Please correct me if I
> am wrong.

I guess you mean s/would/could/. Personally I find "imported from Linux"
as an entirely unacceptable justification: Why would the origin of a file
matter on whether it has violations? Dealing with the violations may be
more cumbersome (because preferably the adjustments would go to the
original files first). Yet not dealing with them - especially if there
are many - reduces the benefit of the work we do quite a bit, because it
may leave much more work for downstreams to do to actually be able to do
any certification. That may go to the extent of questioning why we would
bother dealing with a few dozen violations if hundreds remain but are
hidden.

Jan


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 2/2] xen/misra: add entries to exclude-list.json
  2023-02-16  8:19           ` Jan Beulich
@ 2023-02-16  9:20             ` Luca Fancellu
  2023-02-17  1:44               ` Stefano Stabellini
  0 siblings, 1 reply; 15+ messages in thread
From: Luca Fancellu @ 2023-02-16  9:20 UTC (permalink / raw)
  To: Jan Beulich
  Cc: Stefano Stabellini, Wei Chen, Bertrand Marquis, Andrew Cooper,
	George Dunlap, Michal Orzel, Wei Liu, xen-devel, Julien Grall



> On 16 Feb 2023, at 08:19, Jan Beulich <jbeulich@suse.com> wrote:
> 
> On 16.02.2023 00:49, Stefano Stabellini wrote:
>> On Wed, 15 Feb 2023, Julien Grall wrote:
>>> On 14/02/2023 22:25, Stefano Stabellini wrote:
>>>>> Patch 1's example has a "comment" field, which no entry makes use of.
>>>>> Without that, how does it become clear _why_ a particular file is to
>>>>> be excluded? The patch description here also doesn't provide any
>>>>> justification ...
>>>> 
>>>> It would be good to have a couple of pre-canned justifications as the
>>>> reason for excluding one file could be different from the reason for
>>>> excluding another file. Some of the reasons:
>>> 
>>> I think the reasons should be ambiguous. This is ...
>>> 
>>>> - imported from Linux
>>> 
>>> ... the case here but...
>>> 
>>> This reason is pretty clear to me but...
>>> 
>>>> - too many false positives
>>> 
>>> ... not here. What is too many?
>>> 
>>>> That said, we don't necessarily need to know the exact reason for
>>>> excluding one file to be able to start scanning xen with cppcheck
>>>> automatically. If someone wants to remove a file from the exclude list
>>>> in the future they just need to show that cppcheck does a good job at
>>>> scanning the file and we can handle the number of violations.
>>> 
>>> I disagree. A good reasoning from the start will be helpful to decide when we
>>> can remove a file from the list. Furthermore, we need to set good example for
>>> any new file we want to exclude.
>>> 
>>> Furthermore, if we exclude some files, then it will be difficult for the
>>> reviewers to know when they can be removed from the list. What if this is fine
>>> with CPPCheck but not EClair (or any other)?
>> 
>> Yes, the reason would help. In previous incarnations of this work, there
>> was a request for detailed information on external files, such as:
>> - where this file is coming from
>> - if coming from Linux, which version of Linux
>> - maintenance status
>> - coding style
>> 
>> But this is not what you are asking. You are only asking for a reason
>> and "imported from Linux" would be good enough. Please correct me if I
>> am wrong.
> 
> I guess you mean s/would/could/. Personally I find "imported from Linux"
> as an entirely unacceptable justification: Why would the origin of a file
> matter on whether it has violations? Dealing with the violations may be
> more cumbersome (because preferably the adjustments would go to the
> original files first). Yet not dealing with them - especially if there
> are many - reduces the benefit of the work we do quite a bit, because it
> may leave much more work for downstreams to do to actually be able to do
> any certification. That may go to the extent of questioning why we would
> bother dealing with a few dozen violations if hundreds remain but are
> hidden.

Hi Jan,

my personal opinion is that we can’t handle them for files that needs to be kept
in sync from an external source, because we can’t justify findings or tag false
positives, if we are lucky we might fix the non compliances but even in that case
there might be times when the fix goes through and cases where the fix is objected
by the maintainers just because their project is not following the MISRA standard.

On our side, what we can do is track these files and from time to time check that
they are still eligible to backport, because when they are not anymore we could
just port them to Xen coding style and start doing direct changes.


@Stefano/@Bertrand/@Julien thanks for your effort on the list, yesterday morning
I’ve also had a look on the Michal’s file list and I’ve tracked down the origin, here
my findings, maybe we could merge your list with mine?


{
    "version": "1.0",
    "content": [
        {
            "rel_path": "arch/arm/arm64/cpufeature.c",
            "comment": "Imported from Linux"
        },
        {
            "rel_path": "arch/arm/arm64/insn.c",
            "comment": "Imported on Linux"
        },
        {
            "rel_path": "arch/arm/arm64/lib/find_next_bit.c",
            "comment": "Imported from Linux"
        },
        {
            "rel_path": "arch/x86/acpi/boot.c",
            "comment": "Imported from Linux"
        },
        {
            "rel_path": "arch/x86/acpi/cpu_idle.c",
            "comment": "Imported from Linux"
        },
        {
            "rel_path": "arch/x86/acpi/cpufreq/cpufreq.c",
            "comment": "Imported from Linux"
        },
        {
            "rel_path": "arch/x86/acpi/cpuidle_menu.c",
            "comment": "Imported from Linux"
        },
        {
            "rel_path": "arch/x86/acpi/lib.c",
            "comment": "Imported from Linux"
        },
        {
            "rel_path": "arch/x86/cpu/amd.c",
            "comment": "Imported from Linux"
        },
        {
            "rel_path": "arch/x86/cpu/centaur.c",
            "comment": "Imported from Linux"
        },
        {
            "rel_path": "arch/x86/cpu/common.c",
            "comment": "Imported from Linux"
        },
        {
            "rel_path": "arch/x86/cpu/hygon.c",
            "comment": "Imported from Linux"
        },
        {
            "rel_path": "arch/x86/cpu/intel.c",
            "comment": "Imported from Linux"
        },
        {
            "rel_path": "arch/x86/cpu/intel_cacheinfo.c",
            "comment": "Imported from Linux"
        },
        {
            "rel_path": "arch/x86/cpu/mcheck/non-fatal.c",
            "comment": "Imported from Linux"
        },
        {
            "rel_path": "arch/x86/cpu/mtrr/*",
            "comment": "Imported from Linux"
        },
        {
            "rel_path": "arch/x86/cpu/mwait-idle.c",
            "comment": "Imported from Linux"
        },
        {
            "rel_path": "arch/x86/delay.c",
            "comment": "Imported from Linux"
        },
        {
            "rel_path": "arch/x86/dmi_scan.c",
            "comment": "Imported from Linux"
        },
        {
            "rel_path": "arch/x86/mpparse.c",
            "comment": "Imported from Linux"
        },
        {
            "rel_path": "arch/x86/srat.c",
            "comment": "Imported from Linux"
        },
        {
            "rel_path": "arch/x86/time.c",
            "comment": "Imported from Linux"
        },
        {
            "rel_path": "arch/x86/x86_64/mmconf-fam10h.c",
            "comment": "Imported from Linux"
        },
        {
            "rel_path": "common/bitmap.c",
            "comment": "Imported from Linux"
        },
        {
            "rel_path": "common/bunzip2.c",
            "comment": "Imported from Linux"
        },
        {
            "rel_path": "common/earlycpio.c",
            "comment": "Imported from Linux"
        },
        {
            "rel_path": "common/inflate.c",
            "comment": "Imported from Linux"
        },
        {
            "rel_path": "common/libfdt/*",
            "comment": "External library"
        },
        {
            "rel_path": "common/lz4/decompress.c",
            "comment": "Imported from Linux"
        },
        {
            "rel_path": "common/radix-tree.c",
            "comment": "Imported from Linux"
        },
        {
            "rel_path": "common/ubsan/ubsan.c",
            "comment": "Imported from Linux"
        },
        {
            "rel_path": "common/un*.c",
            "comment": "unlz4.c implementation by Yann Collet, the others un* are from Linux"
        },
        {
            "rel_path": "common/xz/*",
            "comment": "Imported from Linux"
        },
        {
            "rel_path": "common/zstd/*",
            "comment": "Imported from Linux"
        },
        {
            "rel_path": "crypto/*",
            "comment": "Origin is external and documented in crypto/README.source"
        },
        {
            "rel_path": "drivers/acpi/apei/*",
            "comment": "Imported from Linux"
        },
        {
            "rel_path": "drivers/acpi/hwregs.c",
            "comment": "Imported from Linux"
        },
        {
            "rel_path": "drivers/acpi/numa.c",
            "comment": "Imported from Linux"
        },
        {
            "rel_path": "drivers/acpi/osl.c",
            "comment": "Imported from Linux"
        },
        {
            "rel_path": "drivers/acpi/tables.c",
            "comment": "Imported from Linux"
        },
        {
            "rel_path": "drivers/acpi/tables/*",
            "comment": "Imported from Linux"
        },
        {
            "rel_path": "drivers/acpi/utilities/*",
            "comment": "Imported from Linux"
        },
        {
            "rel_path": "drivers/video/font_*",
            "comment": "Imported from Linux"
        },
        {
            "rel_path": "lib/list-sort.c",
            "comment": "Imported from Linux"
        },
        {
            "rel_path": "lib/rbtree.c",
            "comment": "Imported from Linux"
        },
        {
            "rel_path": "lib/xxhash*.c",
            "comment": "Imported from Linux"
        }
    ]
}


Cheers,
Luca

> 
> Jan


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 2/2] xen/misra: add entries to exclude-list.json
  2023-02-16  9:20             ` Luca Fancellu
@ 2023-02-17  1:44               ` Stefano Stabellini
  2023-02-17  7:15                 ` Jan Beulich
  2023-02-24 12:00                 ` Luca Fancellu
  0 siblings, 2 replies; 15+ messages in thread
From: Stefano Stabellini @ 2023-02-17  1:44 UTC (permalink / raw)
  To: Luca Fancellu
  Cc: Jan Beulich, Stefano Stabellini, Wei Chen, Bertrand Marquis,
	Andrew Cooper, George Dunlap, Michal Orzel, Wei Liu, xen-devel,
	Julien Grall

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

On Thu, 16 Feb 2023, Luca Fancellu wrote:
> > On 16 Feb 2023, at 08:19, Jan Beulich <jbeulich@suse.com> wrote:
> > On 16.02.2023 00:49, Stefano Stabellini wrote:
> >> On Wed, 15 Feb 2023, Julien Grall wrote:
> >>> On 14/02/2023 22:25, Stefano Stabellini wrote:
> >>>>> Patch 1's example has a "comment" field, which no entry makes use of.
> >>>>> Without that, how does it become clear _why_ a particular file is to
> >>>>> be excluded? The patch description here also doesn't provide any
> >>>>> justification ...
> >>>> 
> >>>> It would be good to have a couple of pre-canned justifications as the
> >>>> reason for excluding one file could be different from the reason for
> >>>> excluding another file. Some of the reasons:
> >>> 
> >>> I think the reasons should be ambiguous. This is ...
> >>> 
> >>>> - imported from Linux
> >>> 
> >>> ... the case here but...
> >>> 
> >>> This reason is pretty clear to me but...
> >>> 
> >>>> - too many false positives
> >>> 
> >>> ... not here. What is too many?
> >>> 
> >>>> That said, we don't necessarily need to know the exact reason for
> >>>> excluding one file to be able to start scanning xen with cppcheck
> >>>> automatically. If someone wants to remove a file from the exclude list
> >>>> in the future they just need to show that cppcheck does a good job at
> >>>> scanning the file and we can handle the number of violations.
> >>> 
> >>> I disagree. A good reasoning from the start will be helpful to decide when we
> >>> can remove a file from the list. Furthermore, we need to set good example for
> >>> any new file we want to exclude.
> >>> 
> >>> Furthermore, if we exclude some files, then it will be difficult for the
> >>> reviewers to know when they can be removed from the list. What if this is fine
> >>> with CPPCheck but not EClair (or any other)?
> >> 
> >> Yes, the reason would help. In previous incarnations of this work, there
> >> was a request for detailed information on external files, such as:
> >> - where this file is coming from
> >> - if coming from Linux, which version of Linux
> >> - maintenance status
> >> - coding style
> >> 
> >> But this is not what you are asking. You are only asking for a reason
> >> and "imported from Linux" would be good enough. Please correct me if I
> >> am wrong.
> > 
> > I guess you mean s/would/could/. Personally I find "imported from Linux"
> > as an entirely unacceptable justification: Why would the origin of a file
> > matter on whether it has violations? Dealing with the violations may be
> > more cumbersome (because preferably the adjustments would go to the
> > original files first). Yet not dealing with them - especially if there
> > are many - reduces the benefit of the work we do quite a bit, because it
> > may leave much more work for downstreams to do to actually be able to do
> > any certification. That may go to the extent of questioning why we would
> > bother dealing with a few dozen violations if hundreds remain but are
> > hidden.

Yes, we need to figure out a way to deal with these files. However, at
the moment they are getting in the way of easier low hanging fruits.

One "easy" low hanging fruit is to use cppcheck to scan new patches for
new MISRA violations. That would give immediate benefits to the
community. It is not easy to "diff" results with cppcheck and in any
case it would help a lot if we had a cleaner baseline because it would
make it far easier to read the results and make sense of them.

The goal of this exclusion list is to give us that: a cleaner baseline
so that we can make progress faster on low hanging fruits. This list is
not meant to be fixed and stay unchanged for a long time. In fact, it
could even live under automation/ as part of the gitlab-ci test that
triggers cppcheck, if we prefer.


> Hi Jan,
> 
> my personal opinion is that we can’t handle them for files that needs to be kept
> in sync from an external source, because we can’t justify findings or tag false
> positives, if we are lucky we might fix the non compliances but even in that case
> there might be times when the fix goes through and cases where the fix is objected
> by the maintainers just because their project is not following the MISRA standard.
> 
> On our side, what we can do is track these files and from time to time check that
> they are still eligible to backport, because when they are not anymore we could
> just port them to Xen coding style and start doing direct changes.
> 
> 
> @Stefano/@Bertrand/@Julien thanks for your effort on the list, yesterday morning
> I’ve also had a look on the Michal’s file list and I’ve tracked down the origin, here
> my findings, maybe we could merge your list with mine?

Yes to merge the two lists, but I think we should keep only items that we
actually need for the sake of having a cleaner baseline. So I would only
add things we need today to reduce the noise on cppcheck results
(excluding 20.7 and also excluding unusedStructMember which I think we
should probably stop scanning with cppcheck altogether).

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 2/2] xen/misra: add entries to exclude-list.json
  2023-02-17  1:44               ` Stefano Stabellini
@ 2023-02-17  7:15                 ` Jan Beulich
  2023-02-24 12:00                 ` Luca Fancellu
  1 sibling, 0 replies; 15+ messages in thread
From: Jan Beulich @ 2023-02-17  7:15 UTC (permalink / raw)
  To: Stefano Stabellini, Luca Fancellu
  Cc: Wei Chen, Bertrand Marquis, Andrew Cooper, George Dunlap,
	Michal Orzel, Wei Liu, xen-devel, Julien Grall

On 17.02.2023 02:44, Stefano Stabellini wrote:
> On Thu, 16 Feb 2023, Luca Fancellu wrote:
>>> On 16 Feb 2023, at 08:19, Jan Beulich <jbeulich@suse.com> wrote:
>>> On 16.02.2023 00:49, Stefano Stabellini wrote:
>>>> On Wed, 15 Feb 2023, Julien Grall wrote:
>>>>> On 14/02/2023 22:25, Stefano Stabellini wrote:
>>>>>>> Patch 1's example has a "comment" field, which no entry makes use of.
>>>>>>> Without that, how does it become clear _why_ a particular file is to
>>>>>>> be excluded? The patch description here also doesn't provide any
>>>>>>> justification ...
>>>>>>
>>>>>> It would be good to have a couple of pre-canned justifications as the
>>>>>> reason for excluding one file could be different from the reason for
>>>>>> excluding another file. Some of the reasons:
>>>>>
>>>>> I think the reasons should be ambiguous. This is ...
>>>>>
>>>>>> - imported from Linux
>>>>>
>>>>> ... the case here but...
>>>>>
>>>>> This reason is pretty clear to me but...
>>>>>
>>>>>> - too many false positives
>>>>>
>>>>> ... not here. What is too many?
>>>>>
>>>>>> That said, we don't necessarily need to know the exact reason for
>>>>>> excluding one file to be able to start scanning xen with cppcheck
>>>>>> automatically. If someone wants to remove a file from the exclude list
>>>>>> in the future they just need to show that cppcheck does a good job at
>>>>>> scanning the file and we can handle the number of violations.
>>>>>
>>>>> I disagree. A good reasoning from the start will be helpful to decide when we
>>>>> can remove a file from the list. Furthermore, we need to set good example for
>>>>> any new file we want to exclude.
>>>>>
>>>>> Furthermore, if we exclude some files, then it will be difficult for the
>>>>> reviewers to know when they can be removed from the list. What if this is fine
>>>>> with CPPCheck but not EClair (or any other)?
>>>>
>>>> Yes, the reason would help. In previous incarnations of this work, there
>>>> was a request for detailed information on external files, such as:
>>>> - where this file is coming from
>>>> - if coming from Linux, which version of Linux
>>>> - maintenance status
>>>> - coding style
>>>>
>>>> But this is not what you are asking. You are only asking for a reason
>>>> and "imported from Linux" would be good enough. Please correct me if I
>>>> am wrong.
>>>
>>> I guess you mean s/would/could/. Personally I find "imported from Linux"
>>> as an entirely unacceptable justification: Why would the origin of a file
>>> matter on whether it has violations? Dealing with the violations may be
>>> more cumbersome (because preferably the adjustments would go to the
>>> original files first). Yet not dealing with them - especially if there
>>> are many - reduces the benefit of the work we do quite a bit, because it
>>> may leave much more work for downstreams to do to actually be able to do
>>> any certification. That may go to the extent of questioning why we would
>>> bother dealing with a few dozen violations if hundreds remain but are
>>> hidden.
> 
> Yes, we need to figure out a way to deal with these files. However, at
> the moment they are getting in the way of easier low hanging fruits.
> 
> One "easy" low hanging fruit is to use cppcheck to scan new patches for
> new MISRA violations. That would give immediate benefits to the
> community. It is not easy to "diff" results with cppcheck and in any
> case it would help a lot if we had a cleaner baseline because it would
> make it far easier to read the results and make sense of them.
> 
> The goal of this exclusion list is to give us that: a cleaner baseline
> so that we can make progress faster on low hanging fruits. This list is
> not meant to be fixed and stay unchanged for a long time. In fact, it
> could even live under automation/ as part of the gitlab-ci test that
> triggers cppcheck, if we prefer.

Okay, that sounds reasonable to me as an intermediate goal. But then
description of the change and justifications should also state so imo
(for the latter e.g. "from Linux, ignore for now").

Jan


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 2/2] xen/misra: add entries to exclude-list.json
  2023-02-17  1:44               ` Stefano Stabellini
  2023-02-17  7:15                 ` Jan Beulich
@ 2023-02-24 12:00                 ` Luca Fancellu
  2023-02-27 22:19                   ` Stefano Stabellini
  1 sibling, 1 reply; 15+ messages in thread
From: Luca Fancellu @ 2023-02-24 12:00 UTC (permalink / raw)
  To: Stefano Stabellini
  Cc: Jan Beulich, Wei Chen, Bertrand Marquis, Andrew Cooper,
	George Dunlap, Michal Orzel, Wei Liu, xen-devel, Julien Grall

Hi Stefano,

>> Hi Jan,
>> 
>> my personal opinion is that we can’t handle them for files that needs to be kept
>> in sync from an external source, because we can’t justify findings or tag false
>> positives, if we are lucky we might fix the non compliances but even in that case
>> there might be times when the fix goes through and cases where the fix is objected
>> by the maintainers just because their project is not following the MISRA standard.
>> 
>> On our side, what we can do is track these files and from time to time check that
>> they are still eligible to backport, because when they are not anymore we could
>> just port them to Xen coding style and start doing direct changes.
>> 
>> 
>> @Stefano/@Bertrand/@Julien thanks for your effort on the list, yesterday morning
>> I’ve also had a look on the Michal’s file list and I’ve tracked down the origin, here
>> my findings, maybe we could merge your list with mine?
> 
> Yes to merge the two lists, but I think we should keep only items that we
> actually need for the sake of having a cleaner baseline. So I would only
> add things we need today to reduce the noise on cppcheck results
> (excluding 20.7 and also excluding unusedStructMember which I think we
> should probably stop scanning with cppcheck altogether).

Yes I thought about excluding unusedStructMember, I see there are a lot of findings on x86
which are not real findings, it’s just that the tool has not the complete picture.

Here the ways are two:
1) exclude globally unusedStructMember
2) exclude unusedStructMember only on some selected files (available only on cppcheck)
    this requires some work to add a field to this list, like “cppcheck-error-exclude” and a list,
    comma separated, of error-id to be suppressed from the corresponding file.

Regarding the list, I merged your list with mine (and Michal’s work) to create a complete list,
I think it’s better to have it complete because all those files are external and even if today we don’t
have findings for some of these files, we could have some in the future, and since we know today 
that we can’t do direct changes to them, in my opinion it’s better to list them now instead of forgetting
them later.

I left out for now these files to start a discussion for them, because I think they should be included in
the analysis:

common/symbols-dummy.c:
  this file accepts direct changes, cppcheck complains about misra-c2012-8.4 but I think it is right, also
  Coverity complains about the same findings, they can be fixed adding declarations on symbols.h I think
  and removing the declarations from symbol.c module

common/version.c:
  Apart from unusedStructMember, cppcheck is confused only on 2 findings that compares one local symbol
  and one linker defined symbol, could we have just one tag to suppress those two findings instead of removing
  completely the file? This file is under our control, so we could push changes.

include/xen/lib.h:
  Findings are from the bsearch function, which is derived from Linux, but I can see the codestyle is the Xen style
  and it seems (I might be wrong) that it has diverged from Linux, so we might do changes and fix the findings that
  are correct, void* arithmetic is working because gcc make it work assigning a sizeof of 1, using char* pointers we
  have the same result without having the undefined behaviour (correct me if I am wrong).

include/xen/sort.h:
  Also this one is derived from Linux, but seems that we converted it to our coding style and we can do direct changes,
  the same reasoning about void* pointers arithmetic applies here.


What are your thoughts?

Here the merged list, capturing also Jan comments:

{
   "version": "1.0",
   "content": [
       {
           "rel_path": "arch/arm/arm64/cpufeature.c",
           "comment": "Imported from Linux, ignore for now"
       },
       {
           "rel_path": "arch/arm/arm64/insn.c",
           "comment": "Imported on Linux"
       },
       {
           "rel_path": "arch/arm/arm64/lib/find_next_bit.c",
           "comment": "Imported from Linux, ignore for now"
       },
       {
           "rel_path": "arch/x86/acpi/boot.c",
           "comment": "Imported from Linux, ignore for now"
       },
       {
           "rel_path": "arch/x86/acpi/cpu_idle.c",
           "comment": "Imported from Linux, ignore for now"
       },
       {
           "rel_path": "arch/x86/acpi/cpufreq/cpufreq.c",
           "comment": "Imported from Linux, ignore for now"
       },
       {
           "rel_path": "arch/x86/acpi/cpuidle_menu.c",
           "comment": "Imported from Linux, ignore for now"
       },
       {
           "rel_path": "arch/x86/acpi/lib.c",
           "comment": "Imported from Linux, ignore for now"
       },
       {
           "rel_path": "arch/x86/cpu/amd.c",
           "comment": "Imported from Linux, ignore for now"
       },
       {
           "rel_path": "arch/x86/cpu/centaur.c",
           "comment": "Imported from Linux, ignore for now"
       },
       {
           "rel_path": "arch/x86/cpu/common.c",
           "comment": "Imported from Linux, ignore for now"
       },
       {
           "rel_path": "arch/x86/cpu/hygon.c",
           "comment": "Imported from Linux, ignore for now"
       },
       {
           "rel_path": "arch/x86/cpu/intel.c",
           "comment": "Imported from Linux, ignore for now"
       },
       {
           "rel_path": "arch/x86/cpu/intel_cacheinfo.c",
           "comment": "Imported from Linux, ignore for now"
       },
       {
           "rel_path": "arch/x86/cpu/mcheck/non-fatal.c",
           "comment": "Imported from Linux, ignore for now"
       },
       {
           "rel_path": "arch/x86/cpu/mtrr/*",
           "comment": "Imported from Linux, ignore for now"
       },
       {
           "rel_path": "arch/x86/cpu/mwait-idle.c",
           "comment": "Imported from Linux, ignore for now"
       },
       {
           "rel_path": "arch/x86/delay.c",
           "comment": "Imported from Linux, ignore for now"
       },
       {
           "rel_path": "arch/x86/dmi_scan.c",
           "comment": "Imported from Linux, ignore for now"
       },
       {
           "rel_path": "arch/x86/mpparse.c",
           "comment": "Imported from Linux, ignore for now"
       },
       {
           "rel_path": "arch/x86/srat.c",
           "comment": "Imported from Linux, ignore for now"
       },
       {
           "rel_path": "arch/x86/time.c",
           "comment": "Imported from Linux, ignore for now"
       },
       {
           "rel_path": "arch/x86/x86_64/mmconf-fam10h.c",
           "comment": "Imported from Linux, ignore for now"
       },
       {
           "rel_path": "common/bitmap.c",
           "comment": "Imported from Linux, ignore for now"
       },
       {
           "rel_path": "common/bunzip2.c",
           "comment": "Imported from Linux, ignore for now"
       },
       {
           "rel_path": "common/earlycpio.c",
           "comment": "Imported from Linux, ignore for now"
       },
       {
           "rel_path": "common/inflate.c",
           "comment": "Imported from Linux, ignore for now"
       },
       {
           "rel_path": "common/libfdt/*",
           "comment": "External library"
       },
       {
           "rel_path": "common/livepatch_elf.c",
           "comment" : "Not in scope initially as it generates many violations and it is not enabled in safety configurations"
       },
       {
           "rel_path": "common/lzo.c",
           "comment" : "Imported from Linux, ignore for now"
       },
       {
           "rel_path": "common/lz4/decompress.c",
           "comment": "Imported from Linux, ignore for now"
       },
       {
           "rel_path": "common/radix-tree.c",
           "comment": "Imported from Linux, ignore for now"
       },
       {
           "rel_path": "common/ubsan/ubsan.c",
           "comment": "Imported from Linux, ignore for now"
       },
       {
           "rel_path": "common/un*.c",
           "comment": "unlz4.c implementation by Yann Collet, the others un* are from Linux, ignore for now"
       },
       {
           "rel_path": "common/xz/*",
           "comment": "Imported from Linux, ignore for now"
       },
       {
           "rel_path": "common/zstd/*",
           "comment": "Imported from Linux, ignore for now"
       },
       {
           "rel_path": "crypto/*",
           "comment": "Origin is external and documented in crypto/README.source"
       },
       {
           "rel_path": "drivers/acpi/apei/*",
           "comment": "Imported from Linux, ignore for now"
       },
       {
           "rel_path": "drivers/acpi/hwregs.c",
           "comment": "Imported from Linux, ignore for now"
       },
       {
           "rel_path": "drivers/acpi/numa.c",
           "comment": "Imported from Linux, ignore for now"
       },
       {
           "rel_path": "drivers/acpi/osl.c",
           "comment": "Imported from Linux, ignore for now"
       },
       {
           "rel_path": "drivers/acpi/tables.c",
           "comment": "Imported from Linux, ignore for now"
       },
       {
           "rel_path": "drivers/acpi/tables/*",
           "comment": "Imported from Linux, ignore for now"
       },
       {
           "rel_path": "drivers/acpi/utilities/*",
           "comment": "Imported from Linux, ignore for now"
       },
       {
           "rel_path": "drivers/video/font_*",
           "comment": "Imported from Linux, ignore for now"
       },
       {
           "rel_path": "lib/list-sort.c",
           "comment": "Imported from Linux, ignore for now"
       },
       {
           "rel_path": "lib/rbtree.c",
           "comment": "Imported from Linux, ignore for now"
       },
       {
           "rel_path": "lib/xxhash*.c",
           "comment": "Imported from Linux, ignore for now"
       },
       {
           "rel_path": "xsm/flask/*",
           "comment" : "Not in scope initially as it generates many violations and it is not enabled in safety configurations"
       }
   ]
}



^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 2/2] xen/misra: add entries to exclude-list.json
  2023-02-24 12:00                 ` Luca Fancellu
@ 2023-02-27 22:19                   ` Stefano Stabellini
  0 siblings, 0 replies; 15+ messages in thread
From: Stefano Stabellini @ 2023-02-27 22:19 UTC (permalink / raw)
  To: Luca Fancellu
  Cc: Stefano Stabellini, Jan Beulich, Wei Chen, Bertrand Marquis,
	Andrew Cooper, George Dunlap, Michal Orzel, Wei Liu, xen-devel,
	Julien Grall

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

On Fri, 24 Feb 2023, Luca Fancellu wrote:
> Hi Stefano,
> 
> >> Hi Jan,
> >> 
> >> my personal opinion is that we can’t handle them for files that needs to be kept
> >> in sync from an external source, because we can’t justify findings or tag false
> >> positives, if we are lucky we might fix the non compliances but even in that case
> >> there might be times when the fix goes through and cases where the fix is objected
> >> by the maintainers just because their project is not following the MISRA standard.
> >> 
> >> On our side, what we can do is track these files and from time to time check that
> >> they are still eligible to backport, because when they are not anymore we could
> >> just port them to Xen coding style and start doing direct changes.
> >> 
> >> 
> >> @Stefano/@Bertrand/@Julien thanks for your effort on the list, yesterday morning
> >> I’ve also had a look on the Michal’s file list and I’ve tracked down the origin, here
> >> my findings, maybe we could merge your list with mine?
> > 
> > Yes to merge the two lists, but I think we should keep only items that we
> > actually need for the sake of having a cleaner baseline. So I would only
> > add things we need today to reduce the noise on cppcheck results
> > (excluding 20.7 and also excluding unusedStructMember which I think we
> > should probably stop scanning with cppcheck altogether).
> 
> Yes I thought about excluding unusedStructMember, I see there are a lot of findings on x86
> which are not real findings, it’s just that the tool has not the complete picture.
> 
> Here the ways are two:
> 1) exclude globally unusedStructMember
> 2) exclude unusedStructMember only on some selected files (available only on cppcheck)
>     this requires some work to add a field to this list, like “cppcheck-error-exclude” and a list,
>     comma separated, of error-id to be suppressed from the corresponding file.

For now, I would exclude globally


> Regarding the list, I merged your list with mine (and Michal’s work) to create a complete list,
> I think it’s better to have it complete because all those files are external and even if today we don’t
> have findings for some of these files, we could have some in the future, and since we know today 
> that we can’t do direct changes to them, in my opinion it’s better to list them now instead of forgetting
> them later.
> 
> I left out for now these files to start a discussion for them, because I think they should be included in
> the analysis:
> 
> common/symbols-dummy.c:
>   this file accepts direct changes, cppcheck complains about misra-c2012-8.4 but I think it is right, also
>   Coverity complains about the same findings, they can be fixed adding declarations on symbols.h I think
>   and removing the declarations from symbol.c module
> 
> common/version.c:
>   Apart from unusedStructMember, cppcheck is confused only on 2 findings that compares one local symbol
>   and one linker defined symbol, could we have just one tag to suppress those two findings instead of removing
>   completely the file? This file is under our control, so we could push changes.
> 
> include/xen/lib.h:
>   Findings are from the bsearch function, which is derived from Linux, but I can see the codestyle is the Xen style
>   and it seems (I might be wrong) that it has diverged from Linux, so we might do changes and fix the findings that
>   are correct, void* arithmetic is working because gcc make it work assigning a sizeof of 1, using char* pointers we
>   have the same result without having the undefined behaviour (correct me if I am wrong).
> 
> include/xen/sort.h:
>   Also this one is derived from Linux, but seems that we converted it to our coding style and we can do direct changes,
>   the same reasoning about void* pointers arithmetic applies here.
> 
> 
> What are your thoughts?

I am good with this


> Here the merged list, capturing also Jan comments:
> 
> {
>    "version": "1.0",
>    "content": [
>        {
>            "rel_path": "arch/arm/arm64/cpufeature.c",
>            "comment": "Imported from Linux, ignore for now"
>        },
>        {
>            "rel_path": "arch/arm/arm64/insn.c",
>            "comment": "Imported on Linux"
>        },
>        {
>            "rel_path": "arch/arm/arm64/lib/find_next_bit.c",
>            "comment": "Imported from Linux, ignore for now"
>        },
>        {
>            "rel_path": "arch/x86/acpi/boot.c",
>            "comment": "Imported from Linux, ignore for now"
>        },
>        {
>            "rel_path": "arch/x86/acpi/cpu_idle.c",
>            "comment": "Imported from Linux, ignore for now"
>        },
>        {
>            "rel_path": "arch/x86/acpi/cpufreq/cpufreq.c",
>            "comment": "Imported from Linux, ignore for now"
>        },
>        {
>            "rel_path": "arch/x86/acpi/cpuidle_menu.c",
>            "comment": "Imported from Linux, ignore for now"
>        },
>        {
>            "rel_path": "arch/x86/acpi/lib.c",
>            "comment": "Imported from Linux, ignore for now"
>        },
>        {
>            "rel_path": "arch/x86/cpu/amd.c",
>            "comment": "Imported from Linux, ignore for now"
>        },
>        {
>            "rel_path": "arch/x86/cpu/centaur.c",
>            "comment": "Imported from Linux, ignore for now"
>        },
>        {
>            "rel_path": "arch/x86/cpu/common.c",
>            "comment": "Imported from Linux, ignore for now"
>        },
>        {
>            "rel_path": "arch/x86/cpu/hygon.c",
>            "comment": "Imported from Linux, ignore for now"
>        },
>        {
>            "rel_path": "arch/x86/cpu/intel.c",
>            "comment": "Imported from Linux, ignore for now"
>        },
>        {
>            "rel_path": "arch/x86/cpu/intel_cacheinfo.c",
>            "comment": "Imported from Linux, ignore for now"
>        },
>        {
>            "rel_path": "arch/x86/cpu/mcheck/non-fatal.c",
>            "comment": "Imported from Linux, ignore for now"
>        },
>        {
>            "rel_path": "arch/x86/cpu/mtrr/*",
>            "comment": "Imported from Linux, ignore for now"
>        },
>        {
>            "rel_path": "arch/x86/cpu/mwait-idle.c",
>            "comment": "Imported from Linux, ignore for now"
>        },
>        {
>            "rel_path": "arch/x86/delay.c",
>            "comment": "Imported from Linux, ignore for now"
>        },
>        {
>            "rel_path": "arch/x86/dmi_scan.c",
>            "comment": "Imported from Linux, ignore for now"
>        },
>        {
>            "rel_path": "arch/x86/mpparse.c",
>            "comment": "Imported from Linux, ignore for now"
>        },
>        {
>            "rel_path": "arch/x86/srat.c",
>            "comment": "Imported from Linux, ignore for now"
>        },
>        {
>            "rel_path": "arch/x86/time.c",
>            "comment": "Imported from Linux, ignore for now"
>        },
>        {
>            "rel_path": "arch/x86/x86_64/mmconf-fam10h.c",
>            "comment": "Imported from Linux, ignore for now"
>        },
>        {
>            "rel_path": "common/bitmap.c",
>            "comment": "Imported from Linux, ignore for now"
>        },
>        {
>            "rel_path": "common/bunzip2.c",
>            "comment": "Imported from Linux, ignore for now"
>        },
>        {
>            "rel_path": "common/earlycpio.c",
>            "comment": "Imported from Linux, ignore for now"
>        },
>        {
>            "rel_path": "common/inflate.c",
>            "comment": "Imported from Linux, ignore for now"
>        },
>        {
>            "rel_path": "common/libfdt/*",
>            "comment": "External library"
>        },
>        {
>            "rel_path": "common/livepatch_elf.c",
>            "comment" : "Not in scope initially as it generates many violations and it is not enabled in safety configurations"
>        },
>        {
>            "rel_path": "common/lzo.c",
>            "comment" : "Imported from Linux, ignore for now"
>        },
>        {
>            "rel_path": "common/lz4/decompress.c",
>            "comment": "Imported from Linux, ignore for now"
>        },
>        {
>            "rel_path": "common/radix-tree.c",
>            "comment": "Imported from Linux, ignore for now"
>        },
>        {
>            "rel_path": "common/ubsan/ubsan.c",
>            "comment": "Imported from Linux, ignore for now"
>        },
>        {
>            "rel_path": "common/un*.c",
>            "comment": "unlz4.c implementation by Yann Collet, the others un* are from Linux, ignore for now"
>        },
>        {
>            "rel_path": "common/xz/*",
>            "comment": "Imported from Linux, ignore for now"
>        },
>        {
>            "rel_path": "common/zstd/*",
>            "comment": "Imported from Linux, ignore for now"
>        },
>        {
>            "rel_path": "crypto/*",
>            "comment": "Origin is external and documented in crypto/README.source"
>        },
>        {
>            "rel_path": "drivers/acpi/apei/*",
>            "comment": "Imported from Linux, ignore for now"
>        },
>        {
>            "rel_path": "drivers/acpi/hwregs.c",
>            "comment": "Imported from Linux, ignore for now"
>        },
>        {
>            "rel_path": "drivers/acpi/numa.c",
>            "comment": "Imported from Linux, ignore for now"
>        },
>        {
>            "rel_path": "drivers/acpi/osl.c",
>            "comment": "Imported from Linux, ignore for now"
>        },
>        {
>            "rel_path": "drivers/acpi/tables.c",
>            "comment": "Imported from Linux, ignore for now"
>        },
>        {
>            "rel_path": "drivers/acpi/tables/*",
>            "comment": "Imported from Linux, ignore for now"
>        },
>        {
>            "rel_path": "drivers/acpi/utilities/*",
>            "comment": "Imported from Linux, ignore for now"
>        },
>        {
>            "rel_path": "drivers/video/font_*",
>            "comment": "Imported from Linux, ignore for now"
>        },
>        {
>            "rel_path": "lib/list-sort.c",
>            "comment": "Imported from Linux, ignore for now"
>        },
>        {
>            "rel_path": "lib/rbtree.c",
>            "comment": "Imported from Linux, ignore for now"
>        },
>        {
>            "rel_path": "lib/xxhash*.c",
>            "comment": "Imported from Linux, ignore for now"
>        },
>        {
>            "rel_path": "xsm/flask/*",
>            "comment" : "Not in scope initially as it generates many violations and it is not enabled in safety configurations"
>        }
>    ]
> }
> 
> 
> 

^ permalink raw reply	[flat|nested] 15+ messages in thread

end of thread, other threads:[~2023-02-27 22:20 UTC | newest]

Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-02-14  8:56 [PATCH 0/2] xen/misra: create exclusion file list Luca Fancellu
2023-02-14  8:56 ` [PATCH 1/2] xen/cppcheck: add a way to exclude files from the scan Luca Fancellu
2023-02-14  9:51   ` Jan Beulich
2023-02-14 10:02     ` Luca Fancellu
2023-02-14  8:56 ` [PATCH 2/2] xen/misra: add entries to exclude-list.json Luca Fancellu
2023-02-14  9:54   ` Jan Beulich
2023-02-14 22:25     ` Stefano Stabellini
2023-02-15  8:56       ` Julien Grall
2023-02-15 23:49         ` Stefano Stabellini
2023-02-16  8:19           ` Jan Beulich
2023-02-16  9:20             ` Luca Fancellu
2023-02-17  1:44               ` Stefano Stabellini
2023-02-17  7:15                 ` Jan Beulich
2023-02-24 12:00                 ` Luca Fancellu
2023-02-27 22:19                   ` Stefano Stabellini

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.