* [RFC PATCH 1/4] xen/Makefile: add analysis-coverity and analysis-eclair
2022-11-07 10:47 [RFC PATCH 0/4] Static analyser finding deviation Luca Fancellu
@ 2022-11-07 10:47 ` Luca Fancellu
2022-11-07 16:35 ` Jan Beulich
2022-11-14 16:25 ` Anthony PERARD
2022-11-07 10:47 ` [RFC PATCH 2/4] xen/Makefile: add analysis-cppcheck and analysis-cppcheck-html Luca Fancellu
` (2 subsequent siblings)
3 siblings, 2 replies; 30+ messages in thread
From: Luca Fancellu @ 2022-11-07 10:47 UTC (permalink / raw)
To: xen-devel
Cc: bertrand.marquis, wei.chen, Andrew Cooper, George Dunlap,
Jan Beulich, Julien Grall, Stefano Stabellini, Wei Liu
Add new targets to makefile, analysis-{coverity,eclair} that will:
- Create a tag database using a new tool called xenfusa-gen-tags.py
- Get every file with the FuSa tag SAF- in-code comment, create a
copy of it as <file>.safparse and substituting the tags with
proprietary tool syntax in-code comments using the database.
- build Xen, coverity and eclair are capable of intercepting the
compiler invocation on every build file so the only action from
them is to run these new targets, the file they will analyse will
automatically contain understandable suppression in-code comment
for them.
- call analysis-clean to restore original files.
In case of any error, the user needs to manually run the target
analysis-clean to restore the original files, before that step, any
following run of analysis-{coverity,eclair} will stop and won't
overwrite the original files.
Add in docs/misra/ the files safe.json and
false-positive-{coverity,eclair}.json that are JSON files containing
the data structures for the justifications, they are used by the
xenfusa-gen-tags.py to create the substitution list.
Add docs/misra/documenting-violations.rst to explain how to add
justifications.
Add files to .gitignore and update clean rule content in Makefile.
Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>
---
.gitignore | 2 +
docs/misra/documenting-violations.rst | 172 ++++++++++++++++++++++++
docs/misra/false-positive-coverity.json | 12 ++
docs/misra/false-positive-eclair.json | 12 ++
docs/misra/safe.json | 11 ++
xen/Makefile | 50 ++++++-
xen/tools/xenfusa-gen-tags.py | 81 +++++++++++
7 files changed, 338 insertions(+), 2 deletions(-)
create mode 100644 docs/misra/documenting-violations.rst
create mode 100644 docs/misra/false-positive-coverity.json
create mode 100644 docs/misra/false-positive-eclair.json
create mode 100644 docs/misra/safe.json
create mode 100755 xen/tools/xenfusa-gen-tags.py
diff --git a/.gitignore b/.gitignore
index 418bdfaebf36..b48e1e20c4fc 100644
--- a/.gitignore
+++ b/.gitignore
@@ -10,6 +10,7 @@
*.c.cppcheck
*.opic
*.a
+*.safparse
*.so
*.so.[0-9]*
*.bin
@@ -314,6 +315,7 @@ xen/xsm/flask/policy.*
xen/xsm/flask/xenpolicy-*
tools/flask/policy/policy.conf
tools/flask/policy/xenpolicy-*
+xen/*.sed
xen/xen
xen/xen-cppcheck.xml
xen/xen-syms
diff --git a/docs/misra/documenting-violations.rst b/docs/misra/documenting-violations.rst
new file mode 100644
index 000000000000..3430abfaa177
--- /dev/null
+++ b/docs/misra/documenting-violations.rst
@@ -0,0 +1,172 @@
+.. SPDX-License-Identifier: CC-BY-4.0
+
+Documenting violations
+======================
+
+Static analysers are used on the Xen codebase for both static analysis and MISRA
+compliance.
+There might be the need to suppress some findings instead of fixing them and
+many tools permit the usage of in-code comments that suppress findings so that
+they are not shown in the final report.
+
+Xen includes a tool capable of translating a specific comment used in its
+codebase to the right proprietary in-code comment understandable by the selected
+analyser that suppress its finding.
+
+In the Xen codebase, these tags will be used to document and suppress findings:
+
+ - SAF-X-safe: This tag means that the next line of code contains a finding, but
+ the non compliance to the checker is analysed and demonstrated to be safe.
+ - SAF-X-false-positive-<tool>: This tag means that the next line of code
+ contains a finding, but the finding is a bug of the tool.
+
+SAF stands for Static Analyser Finding, the X is a placeholder for a positive
+number that starts from zero, the number after SAF- shall be incremental and
+unique, base ten notation and without leading zeros.
+
+Entries in the database shall never be removed, even if they are not used
+anymore in the code (if a patch is removing or modifying the faulty line).
+This is to make sure that numbers are not reused which could lead to conflicts
+with old branches or misleading justifications.
+
+An entry can be reused in multiple places in the code to suppress a finding if
+and only if the justification holds for the same non-compliance to the coding
+standard.
+
+An orphan entry, that is an entry who was justifying a finding in the code, but
+later that code was removed and there is no other use of that entry in the code,
+can be reused as long as the justification for the finding holds. This is done
+to avoid the allocation of a new entry with exactly the same justification, that
+would lead to waste of space and maintenance issues of the database.
+
+The files where to store all the justifications are in xen/docs/misra/ and are
+named as safe.json and false-positive-<tool>.json, they have JSON format.
+
+Here is an example to add a new justification in safe.json::
+
+|{
+| "version": "1.0",
+| "content": [
+| {
+| "id": "SAF-0-safe",
+| "analyser": {
+| "coverity": "misra_c_2012_rule_20_7_violation",
+| "eclair": "MC3R1.R20.7"
+| },
+| "name": "R20.7 C macro parameters not used as expression",
+| "text": "The macro parameters used in this [...]"
+| },
+| {
+| "id": "SAF-1-safe",
+| "analyser": {},
+| "name": "Sentinel",
+| "text": "Next ID to be used"
+| }
+| ]
+|}
+
+Here is an example to add a new justification in false-positive-<tool>.json::
+
+|{
+| "version": "1.0",
+| "content": [
+| {
+| "id": "SAF-0-false-positive-<tool>",
+| "analyser": {
+| "<tool>": "<proprietary-id>"
+| },
+| "tool-version": "<version>",
+| "name": "R20.7 [...]",
+| "text": "[...]"
+| },
+| {
+| "id": "SAF-1-false-positive-<tool>",
+| "analyser": {},
+| "tool-version": "",
+| "name": "Sentinel",
+| "text": "Next ID to be used"
+| }
+| ]
+|}
+
+To document a finding, just add another block {[...]} before the sentinel block,
+using the id contained in the sentinel block and increment by one the number
+contained in the id of the sentinel block.
+
+Here an explanation of the field inside an object of the "content" array:
+ - id: it is a unique string that is used to refer to the finding, many finding
+ can be tagged with the same id, if the justification holds for any applied
+ case.
+ It tells the tool to substitute a Xen in-code comment having this structure:
+ /* SAF-0-safe [...] \*/
+ - analyser: it is an object containing pair of key-value strings, the key is
+ the analyser, so it can be coverity or eclair, the value is the proprietary
+ id corresponding on the finding, for example when coverity is used as
+ analyser, the tool will translate the Xen in-code coment in this way:
+ /* SAF-0-safe [...] \*/ -> /* coverity[misra_c_2012_rule_20_7_violation] \*/
+ if the object doesn't have a key-value, then the corresponding in-code
+ comment won't be translated.
+ - name: a simple name for the finding
+ - text: a proper justification to turn off the finding.
+
+
+Justification example
+---------------------
+
+Here an example of the usage of the in-code comment tags to suppress a finding
+for the Rule 8.6:
+
+Eclair reports it in its web report, file xen/include/xen/kernel.h, line 68:
+
+| MC3R1.R8.6 for program 'xen/xen-syms', variable '_start' has no definition
+
+Also coverity reports it, here is an extract of the finding:
+
+| xen/include/xen/kernel.h:68:
+| 1. misra_c_2012_rule_8_6_violation: Function "_start" is declared but never
+ defined.
+
+The analysers are complaining because we have this in xen/include/xen/kernel.h
+at line 68::
+
+| extern char _start[], _end[], start[];
+
+Those are symbols exported by the linker, hence we will need to have a proper
+deviation for this finding.
+
+We will prepare our entry in the safe.json database::
+
+|{
+| "version": "1.0",
+| "content": [
+| {
+| [...]
+| },
+| {
+| "id": "SAF-1-safe",
+| "analyser": {
+| "eclair": "MC3R1.R8.6",
+| "coverity": "misra_c_2012_rule_8_6_violation"
+| },
+| "name": "Rule 8.6: linker script defined symbols",
+| "text": "It is safe to declare this symbol because it is defined in the linker script."
+| },
+| {
+| "id": "SAF-2-safe",
+| "analyser": {},
+| "name": "Sentinel",
+| "text": "Next ID to be used"
+| }
+| ]
+|}
+
+And we will use the proper tag above the violation line::
+
+| /* SAF-1-safe R8.6 linker defined symbols */
+| extern char _start[], _end[], start[];
+
+This entry will fix also the violation on _end and start, because they are on
+the same line and the same "violation ID".
+
+Also, the same tag can be used on other symbols from the linker that are
+declared in the codebase, because the justification holds for them too.
diff --git a/docs/misra/false-positive-coverity.json b/docs/misra/false-positive-coverity.json
new file mode 100644
index 000000000000..f8e6a014acb5
--- /dev/null
+++ b/docs/misra/false-positive-coverity.json
@@ -0,0 +1,12 @@
+{
+ "version": "1.0",
+ "content": [
+ {
+ "id": "SAF-0-false-positive-coverity",
+ "analyser": {},
+ "tool-version": "",
+ "name": "Sentinel",
+ "text": "Next ID to be used"
+ }
+ ]
+}
diff --git a/docs/misra/false-positive-eclair.json b/docs/misra/false-positive-eclair.json
new file mode 100644
index 000000000000..63d00e160f9c
--- /dev/null
+++ b/docs/misra/false-positive-eclair.json
@@ -0,0 +1,12 @@
+{
+ "version": "1.0",
+ "content": [
+ {
+ "id": "SAF-0-false-positive-eclair",
+ "analyser": {},
+ "tool-version": "",
+ "name": "Sentinel",
+ "text": "Next ID to be used"
+ }
+ ]
+}
diff --git a/docs/misra/safe.json b/docs/misra/safe.json
new file mode 100644
index 000000000000..e079d3038120
--- /dev/null
+++ b/docs/misra/safe.json
@@ -0,0 +1,11 @@
+{
+ "version": "1.0",
+ "content": [
+ {
+ "id": "SAF-0-safe",
+ "analyser": {},
+ "name": "Sentinel",
+ "text": "Next ID to be used"
+ }
+ ]
+}
diff --git a/xen/Makefile b/xen/Makefile
index 9d0df5e2c543..3b8d1acd1697 100644
--- a/xen/Makefile
+++ b/xen/Makefile
@@ -457,7 +457,8 @@ endif # need-config
__all: build
-main-targets := build install uninstall clean distclean MAP cppcheck cppcheck-html
+main-targets := build install uninstall clean distclean MAP cppcheck \
+ cppcheck-html analysis-coverity analysis-eclair
.PHONY: $(main-targets)
ifneq ($(XEN_TARGET_ARCH),x86_32)
$(main-targets): %: _% ;
@@ -572,7 +573,7 @@ _clean:
rm -f $(TARGET).efi $(TARGET).efi.map $(TARGET).efi.stripped
rm -f asm-offsets.s arch/*/include/asm/asm-offsets.h
rm -f .banner .allconfig.tmp include/xen/compile.h
- rm -f cppcheck-misra.* xen-cppcheck.xml
+ rm -f cppcheck-misra.* xen-cppcheck.xml *.sed
.PHONY: _distclean
_distclean: clean
@@ -757,6 +758,51 @@ cppcheck-version:
$(objtree)/include/generated/compiler-def.h:
$(Q)$(CC) -dM -E -o $@ - < /dev/null
+JUSTIFICATION_FILES := $(XEN_ROOT)/docs/misra/safe.json \
+ $(XEN_ROOT)/docs/misra/false-positive-$$*.json
+
+# The following command is using grep to find all files that contains a comment
+# containing "SAF-<anything>" on a single line.
+# %.safparse will be the original files saved from the build system, these files
+# will be restored at the end of the analysis step
+PARSE_FILE_LIST := $(addsuffix .safparse,$(filter-out %.safparse,\
+$(shell grep -ERl '^[[:blank:]]*\/\*[[:space:]]+SAF-.*\*\/$$' $(srctree))))
+
+.PRECIOUS: $(PARSE_FILE_LIST) $(objtree)/%.sed
+
+.SECONDEXPANSION:
+$(objtree)/%.sed: $(JUSTIFICATION_FILES) $(srctree)/tools/xenfusa-gen-tags.py
+ $(PYTHON) $(srctree)/tools/xenfusa-gen-tags.py \
+ $(foreach file, $(filter %.json, $^), --input $(file)) --output $@ \
+ --tool $*
+
+%.safparse: %
+# Create a copy of the original file (-p preserves also timestamp)
+ $(Q)if [ -f "$@" ]; then \
+ echo "Found $@, please check the integrity of $*"; \
+ exit 1; \
+ fi
+ $(Q)cp -p "$*" "$@"
+
+analysis-parse-tags-%: $(PARSE_FILE_LIST) $(objtree)/%.sed
+ $(Q)for file in $(patsubst %.safparse,%,$(PARSE_FILE_LIST)); do \
+ sed -i -f "$(objtree)/$*.sed" "$${file}"; \
+ done
+
+analysis-build-%: analysis-parse-tags-%
+ $(MAKE) O=$(abs_objtree) -f $(srctree)/Makefile build
+
+analysis-clean:
+# Reverts the original file (-p preserves also timestamp)
+ $(Q)find $(srctree) -type f -name "*.safparse" -print | \
+ while IFS= read file; do \
+ cp -p "$${file}" "$${file%.safparse}"; \
+ rm -f "$${file}"; \
+ done
+
+_analysis-%: analysis-build-%
+ $(Q)$(MAKE) O=$(abs_objtree) -f $(srctree)/Makefile analysis-clean
+
endif #config-build
endif # need-sub-make
diff --git a/xen/tools/xenfusa-gen-tags.py b/xen/tools/xenfusa-gen-tags.py
new file mode 100755
index 000000000000..4ab8c0f07a52
--- /dev/null
+++ b/xen/tools/xenfusa-gen-tags.py
@@ -0,0 +1,81 @@
+#!/usr/bin/env python
+
+import sys, getopt, json
+
+def help():
+ print('Usage: {} [OPTION] ...'.format(sys.argv[0]))
+ print('')
+ print('This script converts the justification file to a set of sed rules')
+ print('that will replace generic tags from Xen codebase in-code comments')
+ print('to in-code comments having the proprietary syntax for the selected')
+ print('tool.')
+ print('')
+ print('Options:')
+ print(' -i/--input Json file containing the justifications, can be')
+ print(' passed multiple times for multiple files')
+ print(' -o/--output Sed file containing the substitution rules')
+ print(' -t/--tool Tool that will use the in-code comments')
+ print('')
+
+# This is the dictionary for the rules that translates to proprietary comments:
+# - cppcheck: /* cppcheck-suppress[id] */
+# - coverity: /* coverity[id] */
+# - eclair: /* -E> hide id 1 "" */
+# Add entries to support more analyzers
+tool_syntax = {
+ "cppcheck":"s,^.*/*[[:space:]]*TAG.*$,/* cppcheck-suppress[VID] */,g",
+ "coverity":"s,^.*/*[[:space:]]*TAG.*$,/* coverity[VID] */,g",
+ "eclair":"s,^.*/*[[:space:]]*TAG.*$,/* -E> hide VID 1 \"\" */,g"
+}
+
+def main(argv):
+ infiles = []
+ justifications = []
+ outfile = ''
+ tool = ''
+
+ try:
+ opts, args = getopt.getopt(argv,"hi:o:t:",["input=","output=","tool="])
+ except getopt.GetoptError:
+ help()
+ sys.exit(2)
+ for opt, arg in opts:
+ if opt == '-h':
+ help()
+ sys.exit(0)
+ elif opt in ("-i", "--input"):
+ infiles.append(arg)
+ elif opt in ("-o", "--output"):
+ outfile = arg
+ elif opt in ("-t", "--tool"):
+ tool = arg
+
+ # Open all input files
+ for file in infiles:
+ try:
+ handle = open(file, 'rt')
+ content = json.load(handle)
+ justifications = justifications + content['content']
+ handle.close()
+ except json.JSONDecodeError:
+ print('JSON decoding error in file: ' + file)
+ except:
+ print('Error opening ' + file)
+ sys.exit(1)
+
+ try:
+ outstr = open(outfile, "w")
+ except:
+ print('Error creating ' + outfile)
+ sys.exit(1)
+
+ for j in justifications:
+ if tool in j['analyser']:
+ comment=tool_syntax[tool].replace("TAG",j['id'])
+ comment=comment.replace("VID",j['analyser'][tool])
+ outstr.write('{}\n'.format(comment))
+
+ outstr.close()
+
+if __name__ == "__main__":
+ main(sys.argv[1:])
\ No newline at end of file
--
2.17.1
^ permalink raw reply related [flat|nested] 30+ messages in thread* Re: [RFC PATCH 1/4] xen/Makefile: add analysis-coverity and analysis-eclair
2022-11-07 10:47 ` [RFC PATCH 1/4] xen/Makefile: add analysis-coverity and analysis-eclair Luca Fancellu
@ 2022-11-07 16:35 ` Jan Beulich
2022-11-08 10:59 ` Luca Fancellu
2022-11-14 16:25 ` Anthony PERARD
1 sibling, 1 reply; 30+ messages in thread
From: Jan Beulich @ 2022-11-07 16:35 UTC (permalink / raw)
To: Luca Fancellu
Cc: bertrand.marquis, wei.chen, Andrew Cooper, George Dunlap,
Julien Grall, Stefano Stabellini, Wei Liu, xen-devel
On 07.11.2022 11:47, Luca Fancellu wrote:
> +Here is an example to add a new justification in false-positive-<tool>.json::
With <tool> already present in the name, ...
> +|{
> +| "version": "1.0",
> +| "content": [
> +| {
> +| "id": "SAF-0-false-positive-<tool>",
> +| "analyser": {
> +| "<tool>": "<proprietary-id>"
... can we avoid the redundancy here? Perhaps ...
> +| },
> +| "tool-version": "<version>",
... it could be
"analyser": {
"<version>": "<proprietary-id>"
},
? It's not really clear to me though how a false positive would be
correctly recorded which is present over a range of versions.
> --- a/xen/Makefile
> +++ b/xen/Makefile
> @@ -457,7 +457,8 @@ endif # need-config
>
> __all: build
>
> -main-targets := build install uninstall clean distclean MAP cppcheck cppcheck-html
> +main-targets := build install uninstall clean distclean MAP cppcheck \
> + cppcheck-html analysis-coverity analysis-eclair
> .PHONY: $(main-targets)
> ifneq ($(XEN_TARGET_ARCH),x86_32)
> $(main-targets): %: _% ;
> @@ -572,7 +573,7 @@ _clean:
> rm -f $(TARGET).efi $(TARGET).efi.map $(TARGET).efi.stripped
> rm -f asm-offsets.s arch/*/include/asm/asm-offsets.h
> rm -f .banner .allconfig.tmp include/xen/compile.h
> - rm -f cppcheck-misra.* xen-cppcheck.xml
> + rm -f cppcheck-misra.* xen-cppcheck.xml *.sed
Is *.sed perhaps a little too wide? But yes, we can of course deal with that
in case any *.sed file appears in the source tree.
> @@ -757,6 +758,51 @@ cppcheck-version:
> $(objtree)/include/generated/compiler-def.h:
> $(Q)$(CC) -dM -E -o $@ - < /dev/null
>
> +JUSTIFICATION_FILES := $(XEN_ROOT)/docs/misra/safe.json \
> + $(XEN_ROOT)/docs/misra/false-positive-$$*.json
> +
> +# The following command is using grep to find all files that contains a comment
> +# containing "SAF-<anything>" on a single line.
> +# %.safparse will be the original files saved from the build system, these files
> +# will be restored at the end of the analysis step
> +PARSE_FILE_LIST := $(addsuffix .safparse,$(filter-out %.safparse,\
> +$(shell grep -ERl '^[[:blank:]]*\/\*[[:space:]]+SAF-.*\*\/$$' $(srctree))))
Please indent such line continuations. And then isn't this going to risk
matching non-source files as well? Perhaps you want to restrict this to
*.c and *.h?
> +.PRECIOUS: $(PARSE_FILE_LIST) $(objtree)/%.sed
> +
> +.SECONDEXPANSION:
I have to admit that I'm a little worried of this living relatively early in
the script.
> +$(objtree)/%.sed: $(JUSTIFICATION_FILES) $(srctree)/tools/xenfusa-gen-tags.py
> + $(PYTHON) $(srctree)/tools/xenfusa-gen-tags.py \
> + $(foreach file, $(filter %.json, $^), --input $(file)) --output $@ \
> + --tool $*
To reduce redundancy, how about
$(objtree)/%.sed: $(srctree)/tools/xenfusa-gen-tags.py $(JUSTIFICATION_FILES)
$(PYTHON) $< --output $@ --tool $* \
$(foreach file, $(filter %.json, $^), --input $(file))
?
> +%.safparse: %
For this to not be overly widely matching, maybe better
$(PARSE_FILE_LIST): %.safparse: %
?
> +# Create a copy of the original file (-p preserves also timestamp)
> + $(Q)if [ -f "$@" ]; then \
> + echo "Found $@, please check the integrity of $*"; \
> + exit 1; \
> + fi
> + $(Q)cp -p "$*" "$@"
While you use the full source name as the stem, I still think $< would be
more clear to use here.
To limit work done, could this me "mv" instead of "cp -p", and then ...
> +analysis-parse-tags-%: $(PARSE_FILE_LIST) $(objtree)/%.sed
> + $(Q)for file in $(patsubst %.safparse,%,$(PARSE_FILE_LIST)); do \
> + sed -i -f "$(objtree)/$*.sed" "$${file}"; \
... with then using
sed -f "$(objtree)/$*.sed" "$${file}.safparse" >"$${file}"
here? This would then also have source consistent between prereqs and
rule.
> + done
> +
> +analysis-build-%: analysis-parse-tags-%
> + $(MAKE) O=$(abs_objtree) -f $(srctree)/Makefile build
This rule doesn't use the stem, so I'm struggling to understand what
this is about.
> +analysis-clean:
> +# Reverts the original file (-p preserves also timestamp)
> + $(Q)find $(srctree) -type f -name "*.safparse" -print | \
> + while IFS= read file; do \
> + cp -p "$${file}" "$${file%.safparse}"; \
> + rm -f "$${file}"; \
Why not "mv"?
> + done
> +
> +_analysis-%: analysis-build-%
> + $(Q)$(MAKE) O=$(abs_objtree) -f $(srctree)/Makefile analysis-clean
Again no use of the stem, plus here I wonder if this may not lead to
people invoking "analysis-clean" without having said anything about
cleaning on their command line.
> --- /dev/null
> +++ b/xen/tools/xenfusa-gen-tags.py
> @@ -0,0 +1,81 @@
> +#!/usr/bin/env python
> +
> +import sys, getopt, json
> +
> +def help():
> + print('Usage: {} [OPTION] ...'.format(sys.argv[0]))
> + print('')
> + print('This script converts the justification file to a set of sed rules')
> + print('that will replace generic tags from Xen codebase in-code comments')
> + print('to in-code comments having the proprietary syntax for the selected')
> + print('tool.')
> + print('')
> + print('Options:')
> + print(' -i/--input Json file containing the justifications, can be')
> + print(' passed multiple times for multiple files')
> + print(' -o/--output Sed file containing the substitution rules')
> + print(' -t/--tool Tool that will use the in-code comments')
> + print('')
> +
> +# This is the dictionary for the rules that translates to proprietary comments:
> +# - cppcheck: /* cppcheck-suppress[id] */
> +# - coverity: /* coverity[id] */
> +# - eclair: /* -E> hide id 1 "" */
> +# Add entries to support more analyzers
> +tool_syntax = {
> + "cppcheck":"s,^.*/*[[:space:]]*TAG.*$,/* cppcheck-suppress[VID] */,g",
> + "coverity":"s,^.*/*[[:space:]]*TAG.*$,/* coverity[VID] */,g",
> + "eclair":"s,^.*/*[[:space:]]*TAG.*$,/* -E> hide VID 1 \"\" */,g"
> +}
> +
> +def main(argv):
> + infiles = []
> + justifications = []
> + outfile = ''
> + tool = ''
> +
> + try:
> + opts, args = getopt.getopt(argv,"hi:o:t:",["input=","output=","tool="])
> + except getopt.GetoptError:
> + help()
> + sys.exit(2)
> + for opt, arg in opts:
> + if opt == '-h':
> + help()
> + sys.exit(0)
> + elif opt in ("-i", "--input"):
> + infiles.append(arg)
> + elif opt in ("-o", "--output"):
> + outfile = arg
> + elif opt in ("-t", "--tool"):
> + tool = arg
> +
> + # Open all input files
> + for file in infiles:
> + try:
> + handle = open(file, 'rt')
> + content = json.load(handle)
> + justifications = justifications + content['content']
> + handle.close()
> + except json.JSONDecodeError:
> + print('JSON decoding error in file: ' + file)
> + except:
> + print('Error opening ' + file)
> + sys.exit(1)
> +
> + try:
> + outstr = open(outfile, "w")
> + except:
> + print('Error creating ' + outfile)
> + sys.exit(1)
> +
> + for j in justifications:
> + if tool in j['analyser']:
> + comment=tool_syntax[tool].replace("TAG",j['id'])
> + comment=comment.replace("VID",j['analyser'][tool])
> + outstr.write('{}\n'.format(comment))
> +
> + outstr.close()
> +
> +if __name__ == "__main__":
> + main(sys.argv[1:])
> \ No newline at end of file
Nit: ^^^
Jan
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [RFC PATCH 1/4] xen/Makefile: add analysis-coverity and analysis-eclair
2022-11-07 16:35 ` Jan Beulich
@ 2022-11-08 10:59 ` Luca Fancellu
2022-11-08 11:48 ` Jan Beulich
0 siblings, 1 reply; 30+ messages in thread
From: Luca Fancellu @ 2022-11-08 10:59 UTC (permalink / raw)
To: Jan Beulich
Cc: Bertrand Marquis, Wei Chen, Andrew Cooper, George Dunlap,
Julien Grall, Stefano Stabellini, Wei Liu, xen-devel
Hi Jan
>
> On 07.11.2022 11:47, Luca Fancellu wrote:
>> +Here is an example to add a new justification in false-positive-<tool>.json::
>
> With <tool> already present in the name, ...
>
>> +|{
>> +| "version": "1.0",
>> +| "content": [
>> +| {
>> +| "id": "SAF-0-false-positive-<tool>",
>> +| "analyser": {
>> +| "<tool>": "<proprietary-id>"
>
> ... can we avoid the redundancy here? Perhaps ...
>
>> +| },
>> +| "tool-version": "<version>",
>
> ... it could be
>
> "analyser": {
> "<version>": "<proprietary-id>"
> },
Yes it’s a bit redundant but it helps re-using the same tool we use for safe.json
>
> ? It's not really clear to me though how a false positive would be
> correctly recorded which is present over a range of versions.
We could put a range in "tool-version”: “<verision-old> - <version-new>"
>
>> --- a/xen/Makefile
>> +++ b/xen/Makefile
>> @@ -457,7 +457,8 @@ endif # need-config
>>
>> __all: build
>>
>> -main-targets := build install uninstall clean distclean MAP cppcheck cppcheck-html
>> +main-targets := build install uninstall clean distclean MAP cppcheck \
>> + cppcheck-html analysis-coverity analysis-eclair
>> .PHONY: $(main-targets)
>> ifneq ($(XEN_TARGET_ARCH),x86_32)
>> $(main-targets): %: _% ;
>> @@ -572,7 +573,7 @@ _clean:
>> rm -f $(TARGET).efi $(TARGET).efi.map $(TARGET).efi.stripped
>> rm -f asm-offsets.s arch/*/include/asm/asm-offsets.h
>> rm -f .banner .allconfig.tmp include/xen/compile.h
>> - rm -f cppcheck-misra.* xen-cppcheck.xml
>> + rm -f cppcheck-misra.* xen-cppcheck.xml *.sed
>
> Is *.sed perhaps a little too wide? But yes, we can of course deal with that
> in case any *.sed file appears in the source tree.
>
>> @@ -757,6 +758,51 @@ cppcheck-version:
>> $(objtree)/include/generated/compiler-def.h:
>> $(Q)$(CC) -dM -E -o $@ - < /dev/null
>>
>> +JUSTIFICATION_FILES := $(XEN_ROOT)/docs/misra/safe.json \
>> + $(XEN_ROOT)/docs/misra/false-positive-$$*.json
>> +
>> +# The following command is using grep to find all files that contains a comment
>> +# containing "SAF-<anything>" on a single line.
>> +# %.safparse will be the original files saved from the build system, these files
>> +# will be restored at the end of the analysis step
>> +PARSE_FILE_LIST := $(addsuffix .safparse,$(filter-out %.safparse,\
>> +$(shell grep -ERl '^[[:blank:]]*\/\*[[:space:]]+SAF-.*\*\/$$' $(srctree))))
>
> Please indent such line continuations. And then isn't this going to risk
> matching non-source files as well? Perhaps you want to restrict this to
> *.c and *.h?
Yes, how about this, it will filter out *.safparse files while keeping in only .h and .c:
PARSE_FILE_LIST := $(addsuffix .safparse,$(filter %.c %.h,\
$(shell grep -ERl '^[[:blank:]]*\/\*[[:space:]]+SAF-.*\*\/$$' $(srctree))))
>
>> +.PRECIOUS: $(PARSE_FILE_LIST) $(objtree)/%.sed
>> +
>> +.SECONDEXPANSION:
>
> I have to admit that I'm a little worried of this living relatively early in
> the script.
>
>> +$(objtree)/%.sed: $(JUSTIFICATION_FILES) $(srctree)/tools/xenfusa-gen-tags.py
>> + $(PYTHON) $(srctree)/tools/xenfusa-gen-tags.py \
>> + $(foreach file, $(filter %.json, $^), --input $(file)) --output $@ \
>> + --tool $*
>
> To reduce redundancy, how about
>
> $(objtree)/%.sed: $(srctree)/tools/xenfusa-gen-tags.py $(JUSTIFICATION_FILES)
> $(PYTHON) $< --output $@ --tool $* \
> $(foreach file, $(filter %.json, $^), --input $(file))
>
> ?
Yes it sounds better
>
>> +%.safparse: %
>
> For this to not be overly widely matching, maybe better
>
> $(PARSE_FILE_LIST): %.safparse: %
>
> ?
Yes very sensible
>
>> +# Create a copy of the original file (-p preserves also timestamp)
>> + $(Q)if [ -f "$@" ]; then \
>> + echo "Found $@, please check the integrity of $*"; \
>> + exit 1; \
>> + fi
>> + $(Q)cp -p "$*" "$@"
>
> While you use the full source name as the stem, I still think $< would be
> more clear to use here.
Agree
>
> To limit work done, could this me "mv" instead of "cp -p", and then ...
>
>> +analysis-parse-tags-%: $(PARSE_FILE_LIST) $(objtree)/%.sed
>> + $(Q)for file in $(patsubst %.safparse,%,$(PARSE_FILE_LIST)); do \
>> + sed -i -f "$(objtree)/$*.sed" "$${file}"; \
>
> ... with then using
>
> sed -f "$(objtree)/$*.sed" "$${file}.safparse" >"$${file}"
>
> here? This would then also have source consistent between prereqs and
> rule.
We saw that mv is not preserving the timestamp of the file, instead we would like to preserve
it, for this reason we used cp -p
>
>> + done
>> +
>> +analysis-build-%: analysis-parse-tags-%
>> + $(MAKE) O=$(abs_objtree) -f $(srctree)/Makefile build
>
> This rule doesn't use the stem, so I'm struggling to understand what
> this is about.
Yes, here my aim was to catch analysis-build-{eclair,coverity}, here I see that if the user has a typo
the rule will run anyway, but it will be stopped by the dependency chain because at the end we have:
JUSTIFICATION_FILES := $(XEN_ROOT)/docs/misra/safe.json \
$(XEN_ROOT)/docs/misra/false-positive-$$*.json
That will give an error because $(XEN_ROOT)/docs/misra/false-positive-<typo>.json does not exists.
If you think it is not enough, what if I reduce the scope of the rule like this?
_analysis-coverity _analysis-eclair: _analysis-%: analysis-build-%
Or, if you are still worried about “analysis-build-%: analysis-parse-tags-%”, then I can do something
like this:
analysis-supported-coverity analysis-supported-eclair:
@echo > /dev/null
analysis-supported-%:
@error Unsupported analysis tool @*
analysis-build-%: analysis-parse-tags-% | analysis-supported-%
$(MAKE) O=$(abs_objtree) -f $(srctree)/Makefile build
[…]
_analysis-%: analysis-build-% | analysis-supported-%
$(Q)$(MAKE) O=$(abs_objtree) -f $(srctree)/Makefile analysis-clean
>
>> +analysis-clean:
>> +# Reverts the original file (-p preserves also timestamp)
>> + $(Q)find $(srctree) -type f -name "*.safparse" -print | \
>> + while IFS= read file; do \
>> + cp -p "$${file}" "$${file%.safparse}"; \
>> + rm -f "$${file}"; \
>
> Why not "mv"?
>
>> + done
>> +
>> +_analysis-%: analysis-build-%
>> + $(Q)$(MAKE) O=$(abs_objtree) -f $(srctree)/Makefile analysis-clean
>
> Again no use of the stem, plus here I wonder if this may not lead to
> people invoking "analysis-clean" without having said anything about
> cleaning on their command line.
In any case, the cleaning process is very safe and does not clean anything that was not dirty before,
so in case of typos, it’s just like a nop.
>
>> --- /dev/null
>> +++ b/xen/tools/xenfusa-gen-tags.py
>> @@ -0,0 +1,81 @@
>> +#!/usr/bin/env python
>> +
>> +import sys, getopt, json
>> +
>> +def help():
>> + print('Usage: {} [OPTION] ...'.format(sys.argv[0]))
>> + print('')
>> + print('This script converts the justification file to a set of sed rules')
>> + print('that will replace generic tags from Xen codebase in-code comments')
>> + print('to in-code comments having the proprietary syntax for the selected')
>> + print('tool.')
>> + print('')
>> + print('Options:')
>> + print(' -i/--input Json file containing the justifications, can be')
>> + print(' passed multiple times for multiple files')
>> + print(' -o/--output Sed file containing the substitution rules')
>> + print(' -t/--tool Tool that will use the in-code comments')
>> + print('')
>> +
>> +# This is the dictionary for the rules that translates to proprietary comments:
>> +# - cppcheck: /* cppcheck-suppress[id] */
>> +# - coverity: /* coverity[id] */
>> +# - eclair: /* -E> hide id 1 "" */
>> +# Add entries to support more analyzers
>> +tool_syntax = {
>> + "cppcheck":"s,^.*/*[[:space:]]*TAG.*$,/* cppcheck-suppress[VID] */,g",
>> + "coverity":"s,^.*/*[[:space:]]*TAG.*$,/* coverity[VID] */,g",
>> + "eclair":"s,^.*/*[[:space:]]*TAG.*$,/* -E> hide VID 1 \"\" */,g"
>> +}
>> +
>> +def main(argv):
>> + infiles = []
>> + justifications = []
>> + outfile = ''
>> + tool = ''
>> +
>> + try:
>> + opts, args = getopt.getopt(argv,"hi:o:t:",["input=","output=","tool="])
>> + except getopt.GetoptError:
>> + help()
>> + sys.exit(2)
>> + for opt, arg in opts:
>> + if opt == '-h':
>> + help()
>> + sys.exit(0)
>> + elif opt in ("-i", "--input"):
>> + infiles.append(arg)
>> + elif opt in ("-o", "--output"):
>> + outfile = arg
>> + elif opt in ("-t", "--tool"):
>> + tool = arg
>> +
>> + # Open all input files
>> + for file in infiles:
>> + try:
>> + handle = open(file, 'rt')
>> + content = json.load(handle)
>> + justifications = justifications + content['content']
>> + handle.close()
>> + except json.JSONDecodeError:
>> + print('JSON decoding error in file: ' + file)
>> + except:
>> + print('Error opening ' + file)
>> + sys.exit(1)
>> +
>> + try:
>> + outstr = open(outfile, "w")
>> + except:
>> + print('Error creating ' + outfile)
>> + sys.exit(1)
>> +
>> + for j in justifications:
>> + if tool in j['analyser']:
>> + comment=tool_syntax[tool].replace("TAG",j['id'])
>> + comment=comment.replace("VID",j['analyser'][tool])
>> + outstr.write('{}\n'.format(comment))
>> +
>> + outstr.close()
>> +
>> +if __name__ == "__main__":
>> + main(sys.argv[1:])
>> \ No newline at end of file
>
> Nit: ^^^
Will fix
>
> Jan
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [RFC PATCH 1/4] xen/Makefile: add analysis-coverity and analysis-eclair
2022-11-08 10:59 ` Luca Fancellu
@ 2022-11-08 11:48 ` Jan Beulich
2022-11-08 14:00 ` Luca Fancellu
0 siblings, 1 reply; 30+ messages in thread
From: Jan Beulich @ 2022-11-08 11:48 UTC (permalink / raw)
To: Luca Fancellu
Cc: Bertrand Marquis, Wei Chen, Andrew Cooper, George Dunlap,
Julien Grall, Stefano Stabellini, Wei Liu, xen-devel
On 08.11.2022 11:59, Luca Fancellu wrote:
>> On 07.11.2022 11:47, Luca Fancellu wrote:
>>> +Here is an example to add a new justification in false-positive-<tool>.json::
>>
>> With <tool> already present in the name, ...
>>
>>> +|{
>>> +| "version": "1.0",
>>> +| "content": [
>>> +| {
>>> +| "id": "SAF-0-false-positive-<tool>",
>>> +| "analyser": {
>>> +| "<tool>": "<proprietary-id>"
>>
>> ... can we avoid the redundancy here? Perhaps ...
>>
>>> +| },
>>> +| "tool-version": "<version>",
>>
>> ... it could be
>>
>> "analyser": {
>> "<version>": "<proprietary-id>"
>> },
>
> Yes it’s a bit redundant but it helps re-using the same tool we use for safe.json
I guess the tool could also be made cope without much effort.
>>> @@ -757,6 +758,51 @@ cppcheck-version:
>>> $(objtree)/include/generated/compiler-def.h:
>>> $(Q)$(CC) -dM -E -o $@ - < /dev/null
>>>
>>> +JUSTIFICATION_FILES := $(XEN_ROOT)/docs/misra/safe.json \
>>> + $(XEN_ROOT)/docs/misra/false-positive-$$*.json
>>> +
>>> +# The following command is using grep to find all files that contains a comment
>>> +# containing "SAF-<anything>" on a single line.
>>> +# %.safparse will be the original files saved from the build system, these files
>>> +# will be restored at the end of the analysis step
>>> +PARSE_FILE_LIST := $(addsuffix .safparse,$(filter-out %.safparse,\
>>> +$(shell grep -ERl '^[[:blank:]]*\/\*[[:space:]]+SAF-.*\*\/$$' $(srctree))))
>>
>> Please indent such line continuations. And then isn't this going to risk
>> matching non-source files as well? Perhaps you want to restrict this to
>> *.c and *.h?
>
> Yes, how about this, it will filter out *.safparse files while keeping in only .h and .c:
>
> PARSE_FILE_LIST := $(addsuffix .safparse,$(filter %.c %.h,\
> $(shell grep -ERl '^[[:blank:]]*\/\*[[:space:]]+SAF-.*\*\/$$' $(srctree))))
That's better, but still means touching all files by grep despite now
only a subset really looked for. If I was to use the new goals on a
more or less regular basis, I'd expect that this enumeration of files
doesn't read _much_ more stuff from disk than is actually necessary.
>> To limit work done, could this me "mv" instead of "cp -p", and then ...
>>
>>> +analysis-parse-tags-%: $(PARSE_FILE_LIST) $(objtree)/%.sed
>>> + $(Q)for file in $(patsubst %.safparse,%,$(PARSE_FILE_LIST)); do \
>>> + sed -i -f "$(objtree)/$*.sed" "$${file}"; \
>>
>> ... with then using
>>
>> sed -f "$(objtree)/$*.sed" "$${file}.safparse" >"$${file}"
>>
>> here? This would then also have source consistent between prereqs and
>> rule.
>
> We saw that mv is not preserving the timestamp of the file, instead we would like to preserve
> it, for this reason we used cp -p
Buggy mv? It certainly doesn't alter timestamps here, and I don't think
the spec allows for it doing so (at least when it doesn't need to resort
to copying to deal with cross-volume moves, but those can't happen here).
>>> + done
>>> +
>>> +analysis-build-%: analysis-parse-tags-%
>>> + $(MAKE) O=$(abs_objtree) -f $(srctree)/Makefile build
>>
>> This rule doesn't use the stem, so I'm struggling to understand what
>> this is about.
>
> Yes, here my aim was to catch analysis-build-{eclair,coverity}, here I see that if the user has a typo
> the rule will run anyway, but it will be stopped by the dependency chain because at the end we have:
>
> JUSTIFICATION_FILES := $(XEN_ROOT)/docs/misra/safe.json \
> $(XEN_ROOT)/docs/misra/false-positive-$$*.json
>
> That will give an error because $(XEN_ROOT)/docs/misra/false-positive-<typo>.json does not exists.
>
> If you think it is not enough, what if I reduce the scope of the rule like this?
>
> _analysis-coverity _analysis-eclair: _analysis-%: analysis-build-%
But then, without using the stem, how does it know whether to do an
Eclair or a Coverity run?
> Or, if you are still worried about “analysis-build-%: analysis-parse-tags-%”, then I can do something
> like this:
>
> analysis-supported-coverity analysis-supported-eclair:
> @echo > /dev/null
>
> analysis-supported-%:
> @error Unsupported analysis tool @*
>
> analysis-build-%: analysis-parse-tags-% | analysis-supported-%
> $(MAKE) O=$(abs_objtree) -f $(srctree)/Makefile build
If I'm not mistaken support for | doesn't exist in make 3.80 (the
minimum version we require to be used).
>>> +analysis-clean:
>>> +# Reverts the original file (-p preserves also timestamp)
>>> + $(Q)find $(srctree) -type f -name "*.safparse" -print | \
>>> + while IFS= read file; do \
>>> + cp -p "$${file}" "$${file%.safparse}"; \
>>> + rm -f "$${file}"; \
>>
>> Why not "mv"?
>>
>>> + done
>>> +
>>> +_analysis-%: analysis-build-%
>>> + $(Q)$(MAKE) O=$(abs_objtree) -f $(srctree)/Makefile analysis-clean
>>
>> Again no use of the stem, plus here I wonder if this may not lead to
>> people invoking "analysis-clean" without having said anything about
>> cleaning on their command line.
>
> In any case, the cleaning process is very safe and does not clean anything that was not dirty before,
> so in case of typos, it’s just like a nop.
People may put transient files in their trees. Of course they need to be
aware that when they specify a "clean" target their files may be deleted.
But without any "clean" target specified nothing should be removed.
Jan
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [RFC PATCH 1/4] xen/Makefile: add analysis-coverity and analysis-eclair
2022-11-08 11:48 ` Jan Beulich
@ 2022-11-08 14:00 ` Luca Fancellu
2022-11-08 15:49 ` Jan Beulich
0 siblings, 1 reply; 30+ messages in thread
From: Luca Fancellu @ 2022-11-08 14:00 UTC (permalink / raw)
To: Jan Beulich
Cc: Bertrand Marquis, Wei Chen, Andrew Cooper, George Dunlap,
Julien Grall, Stefano Stabellini, Wei Liu, xen-devel
> On 8 Nov 2022, at 11:48, Jan Beulich <jbeulich@suse.com> wrote:
>
> On 08.11.2022 11:59, Luca Fancellu wrote:
>>> On 07.11.2022 11:47, Luca Fancellu wrote:
>>>> +Here is an example to add a new justification in false-positive-<tool>.json::
>>>
>>> With <tool> already present in the name, ...
>>>
>>>> +|{
>>>> +| "version": "1.0",
>>>> +| "content": [
>>>> +| {
>>>> +| "id": "SAF-0-false-positive-<tool>",
>>>> +| "analyser": {
>>>> +| "<tool>": "<proprietary-id>"
>>>
>>> ... can we avoid the redundancy here? Perhaps ...
>>>
>>>> +| },
>>>> +| "tool-version": "<version>",
>>>
>>> ... it could be
>>>
>>> "analyser": {
>>> "<version>": "<proprietary-id>"
>>> },
>>
>> Yes it’s a bit redundant but it helps re-using the same tool we use for safe.json
>
> I guess the tool could also be made cope without much effort.
I can modify the script to take an additional parameter to distinguish between safe.json
and false-positive-*.json, then call twice the script and append the result to the .sed file.
>
>>>> @@ -757,6 +758,51 @@ cppcheck-version:
>>>> $(objtree)/include/generated/compiler-def.h:
>>>> $(Q)$(CC) -dM -E -o $@ - < /dev/null
>>>>
>>>> +JUSTIFICATION_FILES := $(XEN_ROOT)/docs/misra/safe.json \
>>>> + $(XEN_ROOT)/docs/misra/false-positive-$$*.json
>>>> +
>>>> +# The following command is using grep to find all files that contains a comment
>>>> +# containing "SAF-<anything>" on a single line.
>>>> +# %.safparse will be the original files saved from the build system, these files
>>>> +# will be restored at the end of the analysis step
>>>> +PARSE_FILE_LIST := $(addsuffix .safparse,$(filter-out %.safparse,\
>>>> +$(shell grep -ERl '^[[:blank:]]*\/\*[[:space:]]+SAF-.*\*\/$$' $(srctree))))
>>>
>>> Please indent such line continuations. And then isn't this going to risk
>>> matching non-source files as well? Perhaps you want to restrict this to
>>> *.c and *.h?
>>
>> Yes, how about this, it will filter out *.safparse files while keeping in only .h and .c:
>>
>> PARSE_FILE_LIST := $(addsuffix .safparse,$(filter %.c %.h,\
>> $(shell grep -ERl '^[[:blank:]]*\/\*[[:space:]]+SAF-.*\*\/$$' $(srctree))))
>
> That's better, but still means touching all files by grep despite now
> only a subset really looked for. If I was to use the new goals on a
> more or less regular basis, I'd expect that this enumeration of files
> doesn't read _much_ more stuff from disk than is actually necessary.
Ok would it be ok?
PARSE_FILE_LIST := $(addsuffix .safparse,$(shell grep -ERl --include=\*.h \
--include=\*.c '^[[:blank:]]*\/\*[[:space:]]+SAF-.*\*\/$$' $(srctree)))
>
>>> To limit work done, could this me "mv" instead of "cp -p", and then ...
>>>
>>>> +analysis-parse-tags-%: $(PARSE_FILE_LIST) $(objtree)/%.sed
>>>> + $(Q)for file in $(patsubst %.safparse,%,$(PARSE_FILE_LIST)); do \
>>>> + sed -i -f "$(objtree)/$*.sed" "$${file}"; \
>>>
>>> ... with then using
>>>
>>> sed -f "$(objtree)/$*.sed" "$${file}.safparse" >"$${file}"
>>>
>>> here? This would then also have source consistent between prereqs and
>>> rule.
>>
>> We saw that mv is not preserving the timestamp of the file, instead we would like to preserve
>> it, for this reason we used cp -p
>
> Buggy mv? It certainly doesn't alter timestamps here, and I don't think
> the spec allows for it doing so (at least when it doesn't need to resort
> to copying to deal with cross-volume moves, but those can't happen here).
Yes you are right, my assumption was wrong, I will change the code as you suggested.
>
>>>> + done
>>>> +
>>>> +analysis-build-%: analysis-parse-tags-%
>>>> + $(MAKE) O=$(abs_objtree) -f $(srctree)/Makefile build
>>>
>>> This rule doesn't use the stem, so I'm struggling to understand what
>>> this is about.
>>
>> Yes, here my aim was to catch analysis-build-{eclair,coverity}, here I see that if the user has a typo
>> the rule will run anyway, but it will be stopped by the dependency chain because at the end we have:
>>
>> JUSTIFICATION_FILES := $(XEN_ROOT)/docs/misra/safe.json \
>> $(XEN_ROOT)/docs/misra/false-positive-$$*.json
>>
>> That will give an error because $(XEN_ROOT)/docs/misra/false-positive-<typo>.json does not exists.
>>
>> If you think it is not enough, what if I reduce the scope of the rule like this?
>>
>> _analysis-coverity _analysis-eclair: _analysis-%: analysis-build-%
>
> But then, without using the stem, how does it know whether to do an
> Eclair or a Coverity run?
Sorry I think I’m a bit lost here, the makefile is working on both analysis-coverity and analysis-eclair
because the % is solving in coverity or eclair depending on which the makefile has in input, it is not complaining
so I guess it works.
Do you see something not working? If so, are you able to provide a piece of code for that to make me understand?
>
>> Or, if you are still worried about “analysis-build-%: analysis-parse-tags-%”, then I can do something
>> like this:
>>
>> analysis-supported-coverity analysis-supported-eclair:
>> @echo > /dev/null
>>
>> analysis-supported-%:
>> @error Unsupported analysis tool @*
>>
>> analysis-build-%: analysis-parse-tags-% | analysis-supported-%
>> $(MAKE) O=$(abs_objtree) -f $(srctree)/Makefile build
>
> If I'm not mistaken support for | doesn't exist in make 3.80 (the
> minimum version we require to be used).
IDK, we use order-only prerequisite already in the Makefile.
>
>>>> +analysis-clean:
>>>> +# Reverts the original file (-p preserves also timestamp)
>>>> + $(Q)find $(srctree) -type f -name "*.safparse" -print | \
>>>> + while IFS= read file; do \
>>>> + cp -p "$${file}" "$${file%.safparse}"; \
>>>> + rm -f "$${file}"; \
>>>
>>> Why not "mv"?
>>>
>>>> + done
>>>> +
>>>> +_analysis-%: analysis-build-%
>>>> + $(Q)$(MAKE) O=$(abs_objtree) -f $(srctree)/Makefile analysis-clean
>>>
>>> Again no use of the stem, plus here I wonder if this may not lead to
>>> people invoking "analysis-clean" without having said anything about
>>> cleaning on their command line.
>>
>> In any case, the cleaning process is very safe and does not clean anything that was not dirty before,
>> so in case of typos, it’s just like a nop.
>
> People may put transient files in their trees. Of course they need to be
> aware that when they specify a "clean" target their files may be deleted.
> But without any "clean" target specified nothing should be removed.
*.safparse files are not supposed to be used freely by user in their tree, those
files will be removed only if the user calls the “analysis-clean” target or if the
analysis-coverity or analysis-eclair reaches the end (a process that creates *.safparse).
There is no other way to trigger the “analysis-clean” unintentionally, so I’m not sure about
the modification you would like to see there.
>
> Jan
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [RFC PATCH 1/4] xen/Makefile: add analysis-coverity and analysis-eclair
2022-11-08 14:00 ` Luca Fancellu
@ 2022-11-08 15:49 ` Jan Beulich
2022-11-08 17:13 ` Luca Fancellu
0 siblings, 1 reply; 30+ messages in thread
From: Jan Beulich @ 2022-11-08 15:49 UTC (permalink / raw)
To: Luca Fancellu
Cc: Bertrand Marquis, Wei Chen, Andrew Cooper, George Dunlap,
Julien Grall, Stefano Stabellini, Wei Liu, xen-devel
On 08.11.2022 15:00, Luca Fancellu wrote:
>> On 8 Nov 2022, at 11:48, Jan Beulich <jbeulich@suse.com> wrote:
>> On 08.11.2022 11:59, Luca Fancellu wrote:
>>>> On 07.11.2022 11:47, Luca Fancellu wrote:
>>>>> @@ -757,6 +758,51 @@ cppcheck-version:
>>>>> $(objtree)/include/generated/compiler-def.h:
>>>>> $(Q)$(CC) -dM -E -o $@ - < /dev/null
>>>>>
>>>>> +JUSTIFICATION_FILES := $(XEN_ROOT)/docs/misra/safe.json \
>>>>> + $(XEN_ROOT)/docs/misra/false-positive-$$*.json
>>>>> +
>>>>> +# The following command is using grep to find all files that contains a comment
>>>>> +# containing "SAF-<anything>" on a single line.
>>>>> +# %.safparse will be the original files saved from the build system, these files
>>>>> +# will be restored at the end of the analysis step
>>>>> +PARSE_FILE_LIST := $(addsuffix .safparse,$(filter-out %.safparse,\
>>>>> +$(shell grep -ERl '^[[:blank:]]*\/\*[[:space:]]+SAF-.*\*\/$$' $(srctree))))
>>>>
>>>> Please indent such line continuations. And then isn't this going to risk
>>>> matching non-source files as well? Perhaps you want to restrict this to
>>>> *.c and *.h?
>>>
>>> Yes, how about this, it will filter out *.safparse files while keeping in only .h and .c:
>>>
>>> PARSE_FILE_LIST := $(addsuffix .safparse,$(filter %.c %.h,\
>>> $(shell grep -ERl '^[[:blank:]]*\/\*[[:space:]]+SAF-.*\*\/$$' $(srctree))))
>>
>> That's better, but still means touching all files by grep despite now
>> only a subset really looked for. If I was to use the new goals on a
>> more or less regular basis, I'd expect that this enumeration of files
>> doesn't read _much_ more stuff from disk than is actually necessary.
>
> Ok would it be ok?
>
> PARSE_FILE_LIST := $(addsuffix .safparse,$(shell grep -ERl --include=\*.h \
> --include=\*.c '^[[:blank:]]*\/\*[[:space:]]+SAF-.*\*\/$$' $(srctree)))
Hmm, not sure: --include isn't a standard option to grep, and we
generally try to be portable. Actually -R (or -r) isn't either. It
may still be okay that way if properly documented where the involved
goals will work and where not.
And then - why do you escape slashes in the ERE?
Talking of escaping - personally I find backslash escapes harder to
read / grok than quotation, so I'd like to recommend using quotes
around each of the two --include (if they remain in the first place)
instead of the \* construct.
>>>>> + done
>>>>> +
>>>>> +analysis-build-%: analysis-parse-tags-%
>>>>> + $(MAKE) O=$(abs_objtree) -f $(srctree)/Makefile build
>>>>
>>>> This rule doesn't use the stem, so I'm struggling to understand what
>>>> this is about.
>>>
>>> Yes, here my aim was to catch analysis-build-{eclair,coverity}, here I see that if the user has a typo
>>> the rule will run anyway, but it will be stopped by the dependency chain because at the end we have:
>>>
>>> JUSTIFICATION_FILES := $(XEN_ROOT)/docs/misra/safe.json \
>>> $(XEN_ROOT)/docs/misra/false-positive-$$*.json
>>>
>>> That will give an error because $(XEN_ROOT)/docs/misra/false-positive-<typo>.json does not exists.
>>>
>>> If you think it is not enough, what if I reduce the scope of the rule like this?
>>>
>>> _analysis-coverity _analysis-eclair: _analysis-%: analysis-build-%
>>
>> But then, without using the stem, how does it know whether to do an
>> Eclair or a Coverity run?
>
> Sorry I think I’m a bit lost here, the makefile is working on both analysis-coverity and analysis-eclair
> because the % is solving in coverity or eclair depending on which the makefile has in input, it is not complaining
> so I guess it works.
> Do you see something not working? If so, are you able to provide a piece of code for that to make me understand?
Well, my problem is that I don't see how the distinction is conveyed
without the stem being used. With what you say I understand I'm
overlooking something, so I'd appreciate some explanation or at least
a pointer.
>>> Or, if you are still worried about “analysis-build-%: analysis-parse-tags-%”, then I can do something
>>> like this:
>>>
>>> analysis-supported-coverity analysis-supported-eclair:
>>> @echo > /dev/null
>>>
>>> analysis-supported-%:
>>> @error Unsupported analysis tool @*
>>>
>>> analysis-build-%: analysis-parse-tags-% | analysis-supported-%
>>> $(MAKE) O=$(abs_objtree) -f $(srctree)/Makefile build
>>
>> If I'm not mistaken support for | doesn't exist in make 3.80 (the
>> minimum version we require to be used).
>
> IDK, we use order-only prerequisite already in the Makefile.
Hmm, yes, for $(objtree)/%.c.cppcheck: . Question is whether this was
simply overlooked before. As said above such may be okay for these
special goals, but this needs properly documenting then.
>>>>> +analysis-clean:
>>>>> +# Reverts the original file (-p preserves also timestamp)
>>>>> + $(Q)find $(srctree) -type f -name "*.safparse" -print | \
>>>>> + while IFS= read file; do \
>>>>> + cp -p "$${file}" "$${file%.safparse}"; \
>>>>> + rm -f "$${file}"; \
>>>>
>>>> Why not "mv"?
>>>>
>>>>> + done
>>>>> +
>>>>> +_analysis-%: analysis-build-%
>>>>> + $(Q)$(MAKE) O=$(abs_objtree) -f $(srctree)/Makefile analysis-clean
>>>>
>>>> Again no use of the stem, plus here I wonder if this may not lead to
>>>> people invoking "analysis-clean" without having said anything about
>>>> cleaning on their command line.
>>>
>>> In any case, the cleaning process is very safe and does not clean anything that was not dirty before,
>>> so in case of typos, it’s just like a nop.
>>
>> People may put transient files in their trees. Of course they need to be
>> aware that when they specify a "clean" target their files may be deleted.
>> But without any "clean" target specified nothing should be removed.
>
> *.safparse files are not supposed to be used freely by user in their tree, those
> files will be removed only if the user calls the “analysis-clean” target or if the
> analysis-coverity or analysis-eclair reaches the end (a process that creates *.safparse).
>
> There is no other way to trigger the “analysis-clean” unintentionally, so I’m not sure about
> the modification you would like to see there.
I guess I don't understand: You have _analysis-% as the target, which I'd
assume will handle _analysis-clean just as much as _analysis-abc. This may
be connected to my lack of understanding as expressed further up. Or maybe
I'm simply not understanding what the _analysis-% target is about in the
first place, because with the analysis-build-% dependency I don't see how
_analysis-clean would actually work (with the scope restriction you
suggested earlier a rule for analysis-build-clean would not be found
afaict).
Jan
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [RFC PATCH 1/4] xen/Makefile: add analysis-coverity and analysis-eclair
2022-11-08 15:49 ` Jan Beulich
@ 2022-11-08 17:13 ` Luca Fancellu
2022-11-09 8:31 ` Jan Beulich
0 siblings, 1 reply; 30+ messages in thread
From: Luca Fancellu @ 2022-11-08 17:13 UTC (permalink / raw)
To: Jan Beulich
Cc: Bertrand Marquis, Wei Chen, Andrew Cooper, George Dunlap,
Julien Grall, Stefano Stabellini, Wei Liu, xen-devel
> On 8 Nov 2022, at 15:49, Jan Beulich <jbeulich@suse.com> wrote:
>
> On 08.11.2022 15:00, Luca Fancellu wrote:
>>> On 8 Nov 2022, at 11:48, Jan Beulich <jbeulich@suse.com> wrote:
>>> On 08.11.2022 11:59, Luca Fancellu wrote:
>>>>> On 07.11.2022 11:47, Luca Fancellu wrote:
>>>>>> @@ -757,6 +758,51 @@ cppcheck-version:
>>>>>> $(objtree)/include/generated/compiler-def.h:
>>>>>> $(Q)$(CC) -dM -E -o $@ - < /dev/null
>>>>>>
>>>>>> +JUSTIFICATION_FILES := $(XEN_ROOT)/docs/misra/safe.json \
>>>>>> + $(XEN_ROOT)/docs/misra/false-positive-$$*.json
>>>>>> +
>>>>>> +# The following command is using grep to find all files that contains a comment
>>>>>> +# containing "SAF-<anything>" on a single line.
>>>>>> +# %.safparse will be the original files saved from the build system, these files
>>>>>> +# will be restored at the end of the analysis step
>>>>>> +PARSE_FILE_LIST := $(addsuffix .safparse,$(filter-out %.safparse,\
>>>>>> +$(shell grep -ERl '^[[:blank:]]*\/\*[[:space:]]+SAF-.*\*\/$$' $(srctree))))
>>>>>
>>>>> Please indent such line continuations. And then isn't this going to risk
>>>>> matching non-source files as well? Perhaps you want to restrict this to
>>>>> *.c and *.h?
>>>>
>>>> Yes, how about this, it will filter out *.safparse files while keeping in only .h and .c:
>>>>
>>>> PARSE_FILE_LIST := $(addsuffix .safparse,$(filter %.c %.h,\
>>>> $(shell grep -ERl '^[[:blank:]]*\/\*[[:space:]]+SAF-.*\*\/$$' $(srctree))))
>>>
>>> That's better, but still means touching all files by grep despite now
>>> only a subset really looked for. If I was to use the new goals on a
>>> more or less regular basis, I'd expect that this enumeration of files
>>> doesn't read _much_ more stuff from disk than is actually necessary.
>>
>> Ok would it be ok?
>>
>> PARSE_FILE_LIST := $(addsuffix .safparse,$(shell grep -ERl --include=\*.h \
>> --include=\*.c '^[[:blank:]]*\/\*[[:space:]]+SAF-.*\*\/$$' $(srctree)))
>
> Hmm, not sure: --include isn't a standard option to grep, and we
> generally try to be portable. Actually -R (or -r) isn't either. It
> may still be okay that way if properly documented where the involved
> goals will work and where not.
Is a comment before the line ok as documentation? To state that —include and
-R are not standard options so analysis-{coverity,eclair} will not work without a
grep that takes those parameters?
>
> And then - why do you escape slashes in the ERE?
>
> Talking of escaping - personally I find backslash escapes harder to
> read / grok than quotation, so I'd like to recommend using quotes
> around each of the two --include (if they remain in the first place)
> instead of the \* construct.
Ok I’ve removed the escape from the * and also from slashes:
PARSE_FILE_LIST := $(addsuffix .safparse,$(shell grep -ERl --include='*.h' \
--include='*.c' '^[[:blank:]]*/\*[[:space:]]+SAF-.*\*/$$' $(srctree)))
>
>>>>>> + done
>>>>>> +
>>>>>> +analysis-build-%: analysis-parse-tags-%
>>>>>> + $(MAKE) O=$(abs_objtree) -f $(srctree)/Makefile build
>>>>>
>>>>> This rule doesn't use the stem, so I'm struggling to understand what
>>>>> this is about.
>>>>
>>>> Yes, here my aim was to catch analysis-build-{eclair,coverity}, here I see that if the user has a typo
>>>> the rule will run anyway, but it will be stopped by the dependency chain because at the end we have:
>>>>
>>>> JUSTIFICATION_FILES := $(XEN_ROOT)/docs/misra/safe.json \
>>>> $(XEN_ROOT)/docs/misra/false-positive-$$*.json
>>>>
>>>> That will give an error because $(XEN_ROOT)/docs/misra/false-positive-<typo>.json does not exists.
>>>>
>>>> If you think it is not enough, what if I reduce the scope of the rule like this?
>>>>
>>>> _analysis-coverity _analysis-eclair: _analysis-%: analysis-build-%
>>>
>>> But then, without using the stem, how does it know whether to do an
>>> Eclair or a Coverity run?
>>
>> Sorry I think I’m a bit lost here, the makefile is working on both analysis-coverity and analysis-eclair
>> because the % is solving in coverity or eclair depending on which the makefile has in input, it is not complaining
>> so I guess it works.
>> Do you see something not working? If so, are you able to provide a piece of code for that to make me understand?
>
> Well, my problem is that I don't see how the distinction is conveyed
> without the stem being used. With what you say I understand I'm
> overlooking something, so I'd appreciate some explanation or at least
> a pointer.
Ok, I have that eclair and coverity shares the same commands to be executed by the build system,
so instead of duplicating the targets for coverity and eclair and their recipe, I’ve used the pattern rule
to have that these rules:
JUSTIFICATION_FILES := $(XEN_ROOT)/docs/misra/safe.json \
$(XEN_ROOT)/docs/misra/false-positive-$$*.json
[…]
.SECONDEXPANSION:
$(objtree)/%.sed: $(srctree)/tools/xenfusa-gen-tags.py $(JUSTIFICATION_FILES)
[…]
[…]
analysis-parse-tags-%: $(PARSE_FILE_LIST) $(objtree)/%.sed
[…]
analysis-build-%: analysis-parse-tags-%
$(MAKE) O=$(abs_objtree) -f $(srctree)/Makefile build
analysis-clean:
[…]
_analysis-%: analysis-build-%
$(Q)$(MAKE) O=$(abs_objtree) -f $(srctree)/Makefile analysis-clean
Matches the case where 'make analysis-coverity’ or ‘make analysis-eclair’ is called.
Now, please correct me if my assumption on the way make works are wrong, here my assumptions:
For example when ‘make analysis-coverity’ is called we have that this rule is the best match for the
called target:
_analysis-%:
So anything after _analysis- will be captured with % and this will be transferred to the dependency
of the target that is analysis-build-% -> analysis-build-coverity
Now analysis-build-coverity will be called, the best match is analysis-build-%, so again the dependency
which is analysis-parse-tags-%, will be translated to analysis-parse-tags-coverity.
Now analysis-parse-tags-coverity will be called, the best match is analysis-parse-tags-%, so the % will
Have the ‘coverity’ value and in the dependency we will have $(objtree)/%.sed -> $(objtree)/coverity.sed.
Looking for $(objtree)/coverity.sed the best match is $(objtree)/%.sed, which will have $(JUSTIFICATION_FILES)
and the python script in the dependency, here we will use the second expansion to solve
$(XEN_ROOT)/docs/misra/false-positive-$$*.json in $(XEN_ROOT)/docs/misra/false-positive-coverity.json
So now after analysis-parse-tags-coverity has ended its dependency it will start with its recipe, after it finishes,
the recipe of analysis-build-coverity will start and it will call make to actually build Xen.
After the build finishes, if the status is good, the analysis-build-coverity has finished and the _analysis-coverity
recipe can now run, it will call make with the analysis-clean target, restoring any <file>.{c,h}.safparse to <file>.{c,h}.
We will have the same with ‘make analysis-eclair’, if we do a mistake typing, like ‘make analysis-coveri’, we will
have:
make: Entering directory ‘/path/to/xen/xen'
make: *** No rule to make target 'analysis-coveri'. Stop.
make: Leaving directory '/path/to/xen/xen'
>
>>>> Or, if you are still worried about “analysis-build-%: analysis-parse-tags-%”, then I can do something
>>>> like this:
>>>>
>>>> analysis-supported-coverity analysis-supported-eclair:
>>>> @echo > /dev/null
>>>>
>>>> analysis-supported-%:
>>>> @error Unsupported analysis tool @*
>>>>
>>>> analysis-build-%: analysis-parse-tags-% | analysis-supported-%
>>>> $(MAKE) O=$(abs_objtree) -f $(srctree)/Makefile build
>>>
>>> If I'm not mistaken support for | doesn't exist in make 3.80 (the
>>> minimum version we require to be used).
>>
>> IDK, we use order-only prerequisite already in the Makefile.
>
> Hmm, yes, for $(objtree)/%.c.cppcheck: . Question is whether this was
> simply overlooked before. As said above such may be okay for these
> special goals, but this needs properly documenting then.
>
>>>>>> +analysis-clean:
>>>>>> +# Reverts the original file (-p preserves also timestamp)
>>>>>> + $(Q)find $(srctree) -type f -name "*.safparse" -print | \
>>>>>> + while IFS= read file; do \
>>>>>> + cp -p "$${file}" "$${file%.safparse}"; \
>>>>>> + rm -f "$${file}"; \
>>>>>
>>>>> Why not "mv"?
>>>>>
>>>>>> + done
>>>>>> +
>>>>>> +_analysis-%: analysis-build-%
>>>>>> + $(Q)$(MAKE) O=$(abs_objtree) -f $(srctree)/Makefile analysis-clean
>>>>>
>>>>> Again no use of the stem, plus here I wonder if this may not lead to
>>>>> people invoking "analysis-clean" without having said anything about
>>>>> cleaning on their command line.
>>>>
>>>> In any case, the cleaning process is very safe and does not clean anything that was not dirty before,
>>>> so in case of typos, it’s just like a nop.
>>>
>>> People may put transient files in their trees. Of course they need to be
>>> aware that when they specify a "clean" target their files may be deleted.
>>> But without any "clean" target specified nothing should be removed.
>>
>> *.safparse files are not supposed to be used freely by user in their tree, those
>> files will be removed only if the user calls the “analysis-clean” target or if the
>> analysis-coverity or analysis-eclair reaches the end (a process that creates *.safparse).
>>
>> There is no other way to trigger the “analysis-clean” unintentionally, so I’m not sure about
>> the modification you would like to see there.
>
> I guess I don't understand: You have _analysis-% as the target, which I'd
> assume will handle _analysis-clean just as much as _analysis-abc. This may
> be connected to my lack of understanding as expressed further up. Or maybe
> I'm simply not understanding what the _analysis-% target is about in the
> first place, because with the analysis-build-% dependency I don't see how
> _analysis-clean would actually work (with the scope restriction you
> suggested earlier a rule for analysis-build-clean would not be found
> afaict).
_analysis-clean will not work, neither _analysis-abc, because of what I wrote above.
analysis-clean instead is called from the recipe of _analysis-% if all its dependency are
built correctly, otherwise it’s the user that needs to call it directly by doing “make analysis-clean”.
>
> Jan
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [RFC PATCH 1/4] xen/Makefile: add analysis-coverity and analysis-eclair
2022-11-08 17:13 ` Luca Fancellu
@ 2022-11-09 8:31 ` Jan Beulich
2022-11-09 10:08 ` Luca Fancellu
0 siblings, 1 reply; 30+ messages in thread
From: Jan Beulich @ 2022-11-09 8:31 UTC (permalink / raw)
To: Luca Fancellu
Cc: Bertrand Marquis, Wei Chen, Andrew Cooper, George Dunlap,
Julien Grall, Stefano Stabellini, Wei Liu, xen-devel
On 08.11.2022 18:13, Luca Fancellu wrote:
>> On 8 Nov 2022, at 15:49, Jan Beulich <jbeulich@suse.com> wrote:
>> On 08.11.2022 15:00, Luca Fancellu wrote:
>>>> On 8 Nov 2022, at 11:48, Jan Beulich <jbeulich@suse.com> wrote:
>>>> On 08.11.2022 11:59, Luca Fancellu wrote:
>>>>>> On 07.11.2022 11:47, Luca Fancellu wrote:
>>>>>>> @@ -757,6 +758,51 @@ cppcheck-version:
>>>>>>> $(objtree)/include/generated/compiler-def.h:
>>>>>>> $(Q)$(CC) -dM -E -o $@ - < /dev/null
>>>>>>>
>>>>>>> +JUSTIFICATION_FILES := $(XEN_ROOT)/docs/misra/safe.json \
>>>>>>> + $(XEN_ROOT)/docs/misra/false-positive-$$*.json
>>>>>>> +
>>>>>>> +# The following command is using grep to find all files that contains a comment
>>>>>>> +# containing "SAF-<anything>" on a single line.
>>>>>>> +# %.safparse will be the original files saved from the build system, these files
>>>>>>> +# will be restored at the end of the analysis step
>>>>>>> +PARSE_FILE_LIST := $(addsuffix .safparse,$(filter-out %.safparse,\
>>>>>>> +$(shell grep -ERl '^[[:blank:]]*\/\*[[:space:]]+SAF-.*\*\/$$' $(srctree))))
>>>>>>
>>>>>> Please indent such line continuations. And then isn't this going to risk
>>>>>> matching non-source files as well? Perhaps you want to restrict this to
>>>>>> *.c and *.h?
>>>>>
>>>>> Yes, how about this, it will filter out *.safparse files while keeping in only .h and .c:
>>>>>
>>>>> PARSE_FILE_LIST := $(addsuffix .safparse,$(filter %.c %.h,\
>>>>> $(shell grep -ERl '^[[:blank:]]*\/\*[[:space:]]+SAF-.*\*\/$$' $(srctree))))
>>>>
>>>> That's better, but still means touching all files by grep despite now
>>>> only a subset really looked for. If I was to use the new goals on a
>>>> more or less regular basis, I'd expect that this enumeration of files
>>>> doesn't read _much_ more stuff from disk than is actually necessary.
>>>
>>> Ok would it be ok?
>>>
>>> PARSE_FILE_LIST := $(addsuffix .safparse,$(shell grep -ERl --include=\*.h \
>>> --include=\*.c '^[[:blank:]]*\/\*[[:space:]]+SAF-.*\*\/$$' $(srctree)))
>>
>> Hmm, not sure: --include isn't a standard option to grep, and we
>> generally try to be portable. Actually -R (or -r) isn't either. It
>> may still be okay that way if properly documented where the involved
>> goals will work and where not.
>
> Is a comment before the line ok as documentation? To state that —include and
> -R are not standard options so analysis-{coverity,eclair} will not work without a
> grep that takes those parameters?
A comment _might_ be okay. Is there no other documentation on how these
goals are to be used? The main question here is how impacting this might
be to the various environments we allow Xen to be built in: Would at
least modern versions of all Linux distros we care about allow using
these rules? What about non-Linux?
And could you at least bail when PARSE_FILE_LIST ends up empty, with a
clear error message augmenting the one grep would have issued?
>> And then - why do you escape slashes in the ERE?
>>
>> Talking of escaping - personally I find backslash escapes harder to
>> read / grok than quotation, so I'd like to recommend using quotes
>> around each of the two --include (if they remain in the first place)
>> instead of the \* construct.
>
> Ok I’ve removed the escape from the * and also from slashes:
>
> PARSE_FILE_LIST := $(addsuffix .safparse,$(shell grep -ERl --include='*.h' \
> --include='*.c' '^[[:blank:]]*/\*[[:space:]]+SAF-.*\*/$$' $(srctree)))
Good - seeing things more clearly now my next question is: Isn't
matching just "/* SAF-...*/" a little too lax? And is there really a
need to permit leading blanks?
>>>>>>> + done
>>>>>>> +
>>>>>>> +analysis-build-%: analysis-parse-tags-%
>>>>>>> + $(MAKE) O=$(abs_objtree) -f $(srctree)/Makefile build
>>>>>>
>>>>>> This rule doesn't use the stem, so I'm struggling to understand what
>>>>>> this is about.
>>>>>
>>>>> Yes, here my aim was to catch analysis-build-{eclair,coverity}, here I see that if the user has a typo
>>>>> the rule will run anyway, but it will be stopped by the dependency chain because at the end we have:
>>>>>
>>>>> JUSTIFICATION_FILES := $(XEN_ROOT)/docs/misra/safe.json \
>>>>> $(XEN_ROOT)/docs/misra/false-positive-$$*.json
>>>>>
>>>>> That will give an error because $(XEN_ROOT)/docs/misra/false-positive-<typo>.json does not exists.
>>>>>
>>>>> If you think it is not enough, what if I reduce the scope of the rule like this?
>>>>>
>>>>> _analysis-coverity _analysis-eclair: _analysis-%: analysis-build-%
>>>>
>>>> But then, without using the stem, how does it know whether to do an
>>>> Eclair or a Coverity run?
>>>
>>> Sorry I think I’m a bit lost here, the makefile is working on both analysis-coverity and analysis-eclair
>>> because the % is solving in coverity or eclair depending on which the makefile has in input, it is not complaining
>>> so I guess it works.
>>> Do you see something not working? If so, are you able to provide a piece of code for that to make me understand?
>>
>> Well, my problem is that I don't see how the distinction is conveyed
>> without the stem being used. With what you say I understand I'm
>> overlooking something, so I'd appreciate some explanation or at least
>> a pointer.
>
> Ok, I have that eclair and coverity shares the same commands to be executed by the build system,
> so instead of duplicating the targets for coverity and eclair and their recipe, I’ve used the pattern rule
> to have that these rules:
>
> JUSTIFICATION_FILES := $(XEN_ROOT)/docs/misra/safe.json \
> $(XEN_ROOT)/docs/misra/false-positive-$$*.json
>
> […]
>
> .SECONDEXPANSION:
> $(objtree)/%.sed: $(srctree)/tools/xenfusa-gen-tags.py $(JUSTIFICATION_FILES)
> […]
>
> […]
>
> analysis-parse-tags-%: $(PARSE_FILE_LIST) $(objtree)/%.sed
> […]
>
> analysis-build-%: analysis-parse-tags-%
> $(MAKE) O=$(abs_objtree) -f $(srctree)/Makefile build
>
> analysis-clean:
> […]
>
> _analysis-%: analysis-build-%
> $(Q)$(MAKE) O=$(abs_objtree) -f $(srctree)/Makefile analysis-clean
>
> Matches the case where 'make analysis-coverity’ or ‘make analysis-eclair’ is called.
>
> Now, please correct me if my assumption on the way make works are wrong, here my assumptions:
>
> For example when ‘make analysis-coverity’ is called we have that this rule is the best match for the
> called target:
>
> _analysis-%:
So my main oversight was your addition to main-targets, which makes the
connection with this underscore-prefixed goal.
As to you saying "best match" - I didn't think make had such a concept
when it comes to considering pattern rules. Aiui it is "first match", in
the order that rules were parsed from all involved makefiles.
> So anything after _analysis- will be captured with % and this will be transferred to the dependency
> of the target that is analysis-build-% -> analysis-build-coverity
>
> Now analysis-build-coverity will be called, the best match is analysis-build-%, so again the dependency
> which is analysis-parse-tags-%, will be translated to analysis-parse-tags-coverity.
>
> Now analysis-parse-tags-coverity will be called, the best match is analysis-parse-tags-%, so the % will
> Have the ‘coverity’ value and in the dependency we will have $(objtree)/%.sed -> $(objtree)/coverity.sed.
>
> Looking for $(objtree)/coverity.sed the best match is $(objtree)/%.sed, which will have $(JUSTIFICATION_FILES)
> and the python script in the dependency, here we will use the second expansion to solve
> $(XEN_ROOT)/docs/misra/false-positive-$$*.json in $(XEN_ROOT)/docs/misra/false-positive-coverity.json
>
> So now after analysis-parse-tags-coverity has ended its dependency it will start with its recipe, after it finishes,
> the recipe of analysis-build-coverity will start and it will call make to actually build Xen.
Okay, I see now - this building of Xen really _is_ independent of the
checker chosen. I'm not sure though whether it is a good idea to
integrate all this, including ...
> After the build finishes, if the status is good, the analysis-build-coverity has finished and the _analysis-coverity
> recipe can now run, it will call make with the analysis-clean target, restoring any <file>.{c,h}.safparse to <file>.{c,h}.
... the subsequent cleaning. The state of the _source_ tree after a
build failure would be different from that after a successful build.
Personally I consider this at best surprising.
I wonder whether instead there could be a shell(?) script driving a
sequence of make invocations, leaving the new make goals all be self-
contained. Such a script could revert the source tree to its original
state even upon build failure by default, with an option allowing to
suppress this behavior.
Jan
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [RFC PATCH 1/4] xen/Makefile: add analysis-coverity and analysis-eclair
2022-11-09 8:31 ` Jan Beulich
@ 2022-11-09 10:08 ` Luca Fancellu
2022-11-09 10:36 ` Jan Beulich
0 siblings, 1 reply; 30+ messages in thread
From: Luca Fancellu @ 2022-11-09 10:08 UTC (permalink / raw)
To: Jan Beulich
Cc: Bertrand Marquis, Wei Chen, Andrew Cooper, George Dunlap,
Julien Grall, Stefano Stabellini, Wei Liu, xen-devel
>>
>> On 07.11.2022 11:47, Luca Fancellu wrote:
>>> +Here is an example to add a new justification in false-positive-<tool>.json::
>>
>> With <tool> already present in the name, ...
>>
>>> +|{
>>> +| "version": "1.0",
>>> +| "content": [
>>> +| {
>>> +| "id": "SAF-0-false-positive-<tool>",
>>> +| "analyser": {
>>> +| "<tool>": "<proprietary-id>"
>>
>> ... can we avoid the redundancy here? Perhaps ...
>>
>>> +| },
>>> +| "tool-version": "<version>",
>>
>> ... it could be
>>
>> "analyser": {
>> "<version>": "<proprietary-id>"
>> },
About this, I’ve investigated a bit and I don’t think this is the right solution, it wouldn't make
much sense to have a schema where in one file the analyser dictionary key is the tool name
and in another it is a version (or range of versions).
However I can remove the analyser dictionary and use this schema for the false-positive, which is
more compact:
|{
| "version": "1.0",
| "content": [
| {
| "id": "SAF-0-false-positive-<tool>",
| “tool-proprietary-id”: "<proprietary-id>”,
| "tool-version": "<version>",
| "name": "R20.7 [...]",
| "text": "[...]"
| },
| {
| "id": "SAF-1-false-positive-<tool>",
| “tool-proprietary-id”: "",
| "tool-version": "",
| "name": "Sentinel",
| "text": "Next ID to be used"
| }
| ]
|}
This needs however a change in the initial design and more documentation on the different handlings
of the safe.json schema and the false-positive-<tool>.json schema. Is it worth?
> On 9 Nov 2022, at 08:31, Jan Beulich <jbeulich@suse.com> wrote:
>
> On 08.11.2022 18:13, Luca Fancellu wrote:
>>> On 8 Nov 2022, at 15:49, Jan Beulich <jbeulich@suse.com> wrote:
>>> On 08.11.2022 15:00, Luca Fancellu wrote:
>>>>> On 8 Nov 2022, at 11:48, Jan Beulich <jbeulich@suse.com> wrote:
>>>>> On 08.11.2022 11:59, Luca Fancellu wrote:
>>>>>>> On 07.11.2022 11:47, Luca Fancellu wrote:
>>>>>>>> @@ -757,6 +758,51 @@ cppcheck-version:
>>>>>>>> $(objtree)/include/generated/compiler-def.h:
>>>>>>>> $(Q)$(CC) -dM -E -o $@ - < /dev/null
>>>>>>>>
>>>>>>>> +JUSTIFICATION_FILES := $(XEN_ROOT)/docs/misra/safe.json \
>>>>>>>> + $(XEN_ROOT)/docs/misra/false-positive-$$*.json
>>>>>>>> +
>>>>>>>> +# The following command is using grep to find all files that contains a comment
>>>>>>>> +# containing "SAF-<anything>" on a single line.
>>>>>>>> +# %.safparse will be the original files saved from the build system, these files
>>>>>>>> +# will be restored at the end of the analysis step
>>>>>>>> +PARSE_FILE_LIST := $(addsuffix .safparse,$(filter-out %.safparse,\
>>>>>>>> +$(shell grep -ERl '^[[:blank:]]*\/\*[[:space:]]+SAF-.*\*\/$$' $(srctree))))
>>>>>>>
>>>>>>> Please indent such line continuations. And then isn't this going to risk
>>>>>>> matching non-source files as well? Perhaps you want to restrict this to
>>>>>>> *.c and *.h?
>>>>>>
>>>>>> Yes, how about this, it will filter out *.safparse files while keeping in only .h and .c:
>>>>>>
>>>>>> PARSE_FILE_LIST := $(addsuffix .safparse,$(filter %.c %.h,\
>>>>>> $(shell grep -ERl '^[[:blank:]]*\/\*[[:space:]]+SAF-.*\*\/$$' $(srctree))))
>>>>>
>>>>> That's better, but still means touching all files by grep despite now
>>>>> only a subset really looked for. If I was to use the new goals on a
>>>>> more or less regular basis, I'd expect that this enumeration of files
>>>>> doesn't read _much_ more stuff from disk than is actually necessary.
>>>>
>>>> Ok would it be ok?
>>>>
>>>> PARSE_FILE_LIST := $(addsuffix .safparse,$(shell grep -ERl --include=\*.h \
>>>> --include=\*.c '^[[:blank:]]*\/\*[[:space:]]+SAF-.*\*\/$$' $(srctree)))
>>>
>>> Hmm, not sure: --include isn't a standard option to grep, and we
>>> generally try to be portable. Actually -R (or -r) isn't either. It
>>> may still be okay that way if properly documented where the involved
>>> goals will work and where not.
>>
>> Is a comment before the line ok as documentation? To state that —include and
>> -R are not standard options so analysis-{coverity,eclair} will not work without a
>> grep that takes those parameters?
>
> A comment _might_ be okay. Is there no other documentation on how these
> goals are to be used? The main question here is how impacting this might
> be to the various environments we allow Xen to be built in: Would at
> least modern versions of all Linux distros we care about allow using
> these rules? What about non-Linux?
>
> And could you at least bail when PARSE_FILE_LIST ends up empty, with a
> clear error message augmenting the one grep would have issued?
An empty PARSE_FILE_LIST should not generate an error, it just means there are no
justifications, but I see it can be problematic in case grep does not work.
What about this? They should be standard options right?
PARSE_FILE_LIST := $(addsuffix .safparse,$(shell find $(srctree) -type f \
-name '*.c' -o -name '*.h' -exec \
grep -El '^[[:blank:]]*\/\*[[:space:]]+SAF-.*\*\/$$' {} + ))
>
>>> And then - why do you escape slashes in the ERE?
>>>
>>> Talking of escaping - personally I find backslash escapes harder to
>>> read / grok than quotation, so I'd like to recommend using quotes
>>> around each of the two --include (if they remain in the first place)
>>> instead of the \* construct.
>>
>> Ok I’ve removed the escape from the * and also from slashes:
>>
>> PARSE_FILE_LIST := $(addsuffix .safparse,$(shell grep -ERl --include='*.h' \
>> --include='*.c' '^[[:blank:]]*/\*[[:space:]]+SAF-.*\*/$$' $(srctree)))
>
> Good - seeing things more clearly now my next question is: Isn't
> matching just "/* SAF-...*/" a little too lax? And is there really a
> need to permit leading blanks?
I’m permitting blanks to allow spaces or tabs, zero or more times before the start of
the comment, I think it shall be like that.
About matching, maybe I can match also the number after SAF-, this should be enough,
[…] grep -El '^[[:blank:]]*\/\*[[:space:]]+SAF-[0-9]+.*\*\/$$’ […]
>
>>>>>>>> + done
>>>>>>>> +
>>>>>>>> +analysis-build-%: analysis-parse-tags-%
>>>>>>>> + $(MAKE) O=$(abs_objtree) -f $(srctree)/Makefile build
>>>>>>>
>>>>>>> This rule doesn't use the stem, so I'm struggling to understand what
>>>>>>> this is about.
>>>>>>
>>>>>> Yes, here my aim was to catch analysis-build-{eclair,coverity}, here I see that if the user has a typo
>>>>>> the rule will run anyway, but it will be stopped by the dependency chain because at the end we have:
>>>>>>
>>>>>> JUSTIFICATION_FILES := $(XEN_ROOT)/docs/misra/safe.json \
>>>>>> $(XEN_ROOT)/docs/misra/false-positive-$$*.json
>>>>>>
>>>>>> That will give an error because $(XEN_ROOT)/docs/misra/false-positive-<typo>.json does not exists.
>>>>>>
>>>>>> If you think it is not enough, what if I reduce the scope of the rule like this?
>>>>>>
>>>>>> _analysis-coverity _analysis-eclair: _analysis-%: analysis-build-%
>>>>>
>>>>> But then, without using the stem, how does it know whether to do an
>>>>> Eclair or a Coverity run?
>>>>
>>>> Sorry I think I’m a bit lost here, the makefile is working on both analysis-coverity and analysis-eclair
>>>> because the % is solving in coverity or eclair depending on which the makefile has in input, it is not complaining
>>>> so I guess it works.
>>>> Do you see something not working? If so, are you able to provide a piece of code for that to make me understand?
>>>
>>> Well, my problem is that I don't see how the distinction is conveyed
>>> without the stem being used. With what you say I understand I'm
>>> overlooking something, so I'd appreciate some explanation or at least
>>> a pointer.
>>
>> Ok, I have that eclair and coverity shares the same commands to be executed by the build system,
>> so instead of duplicating the targets for coverity and eclair and their recipe, I’ve used the pattern rule
>> to have that these rules:
>>
>> JUSTIFICATION_FILES := $(XEN_ROOT)/docs/misra/safe.json \
>> $(XEN_ROOT)/docs/misra/false-positive-$$*.json
>>
>> […]
>>
>> .SECONDEXPANSION:
>> $(objtree)/%.sed: $(srctree)/tools/xenfusa-gen-tags.py $(JUSTIFICATION_FILES)
>> […]
>>
>> […]
>>
>> analysis-parse-tags-%: $(PARSE_FILE_LIST) $(objtree)/%.sed
>> […]
>>
>> analysis-build-%: analysis-parse-tags-%
>> $(MAKE) O=$(abs_objtree) -f $(srctree)/Makefile build
>>
>> analysis-clean:
>> […]
>>
>> _analysis-%: analysis-build-%
>> $(Q)$(MAKE) O=$(abs_objtree) -f $(srctree)/Makefile analysis-clean
>>
>> Matches the case where 'make analysis-coverity’ or ‘make analysis-eclair’ is called.
>>
>> Now, please correct me if my assumption on the way make works are wrong, here my assumptions:
>>
>> For example when ‘make analysis-coverity’ is called we have that this rule is the best match for the
>> called target:
>>
>> _analysis-%:
>
> So my main oversight was your addition to main-targets, which makes the
> connection with this underscore-prefixed goal.
>
> As to you saying "best match" - I didn't think make had such a concept
> when it comes to considering pattern rules. Aiui it is "first match", in
> the order that rules were parsed from all involved makefiles.
Yes first match is the right term.
>
>> So anything after _analysis- will be captured with % and this will be transferred to the dependency
>> of the target that is analysis-build-% -> analysis-build-coverity
>>
>> Now analysis-build-coverity will be called, the best match is analysis-build-%, so again the dependency
>> which is analysis-parse-tags-%, will be translated to analysis-parse-tags-coverity.
>>
>> Now analysis-parse-tags-coverity will be called, the best match is analysis-parse-tags-%, so the % will
>> Have the ‘coverity’ value and in the dependency we will have $(objtree)/%.sed -> $(objtree)/coverity.sed.
>>
>> Looking for $(objtree)/coverity.sed the best match is $(objtree)/%.sed, which will have $(JUSTIFICATION_FILES)
>> and the python script in the dependency, here we will use the second expansion to solve
>> $(XEN_ROOT)/docs/misra/false-positive-$$*.json in $(XEN_ROOT)/docs/misra/false-positive-coverity.json
>>
>> So now after analysis-parse-tags-coverity has ended its dependency it will start with its recipe, after it finishes,
>> the recipe of analysis-build-coverity will start and it will call make to actually build Xen.
>
> Okay, I see now - this building of Xen really _is_ independent of the
> checker chosen. I'm not sure though whether it is a good idea to
> integrate all this, including ...
>
>> After the build finishes, if the status is good, the analysis-build-coverity has finished and the _analysis-coverity
>> recipe can now run, it will call make with the analysis-clean target, restoring any <file>.{c,h}.safparse to <file>.{c,h}.
>
> ... the subsequent cleaning. The state of the _source_ tree after a
> build failure would be different from that after a successful build.
> Personally I consider this at best surprising.
>
> I wonder whether instead there could be a shell(?) script driving a
> sequence of make invocations, leaving the new make goals all be self-
> contained. Such a script could revert the source tree to its original
> state even upon build failure by default, with an option allowing to
> suppress this behavior.
Instead of adding another tool, so another layer to the overall system, I would be more willing to add documentation
about this process, explaining how to use the analysis-* build targets, what to expect after a successful run and what
to expect after a failure.
What do you think?
Cheers,
Luca
>
> Jan
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [RFC PATCH 1/4] xen/Makefile: add analysis-coverity and analysis-eclair
2022-11-09 10:08 ` Luca Fancellu
@ 2022-11-09 10:36 ` Jan Beulich
2022-11-11 10:42 ` Luca Fancellu
0 siblings, 1 reply; 30+ messages in thread
From: Jan Beulich @ 2022-11-09 10:36 UTC (permalink / raw)
To: Luca Fancellu
Cc: Bertrand Marquis, Wei Chen, Andrew Cooper, George Dunlap,
Julien Grall, Stefano Stabellini, Wei Liu, xen-devel
On 09.11.2022 11:08, Luca Fancellu wrote:
>>> On 07.11.2022 11:47, Luca Fancellu wrote:
>>>> +Here is an example to add a new justification in false-positive-<tool>.json::
>>>
>>> With <tool> already present in the name, ...
>>>
>>>> +|{
>>>> +| "version": "1.0",
>>>> +| "content": [
>>>> +| {
>>>> +| "id": "SAF-0-false-positive-<tool>",
>>>> +| "analyser": {
>>>> +| "<tool>": "<proprietary-id>"
>>>
>>> ... can we avoid the redundancy here? Perhaps ...
>>>
>>>> +| },
>>>> +| "tool-version": "<version>",
>>>
>>> ... it could be
>>>
>>> "analyser": {
>>> "<version>": "<proprietary-id>"
>>> },
>
> About this, I’ve investigated a bit and I don’t think this is the right solution, it wouldn't make
> much sense to have a schema where in one file the analyser dictionary key is the tool name
> and in another it is a version (or range of versions).
>
> However I can remove the analyser dictionary and use this schema for the false-positive, which is
> more compact:
>
> |{
> | "version": "1.0",
> | "content": [
> | {
> | "id": "SAF-0-false-positive-<tool>",
> | “tool-proprietary-id”: "<proprietary-id>”,
> | "tool-version": "<version>",
> | "name": "R20.7 [...]",
> | "text": "[...]"
> | },
> | {
> | "id": "SAF-1-false-positive-<tool>",
> | “tool-proprietary-id”: "",
> | "tool-version": "",
> | "name": "Sentinel",
> | "text": "Next ID to be used"
> | }
> | ]
> |}
>
> This needs however a change in the initial design and more documentation on the different handlings
> of the safe.json schema and the false-positive-<tool>.json schema. Is it worth?
I think it is, but of others disagree, so be it.
>> On 9 Nov 2022, at 08:31, Jan Beulich <jbeulich@suse.com> wrote:
>>
>> On 08.11.2022 18:13, Luca Fancellu wrote:
>>>> On 8 Nov 2022, at 15:49, Jan Beulich <jbeulich@suse.com> wrote:
>>>> On 08.11.2022 15:00, Luca Fancellu wrote:
>>>>>> On 8 Nov 2022, at 11:48, Jan Beulich <jbeulich@suse.com> wrote:
>>>>>> On 08.11.2022 11:59, Luca Fancellu wrote:
>>>>>>>> On 07.11.2022 11:47, Luca Fancellu wrote:
>>>>>>>>> @@ -757,6 +758,51 @@ cppcheck-version:
>>>>>>>>> $(objtree)/include/generated/compiler-def.h:
>>>>>>>>> $(Q)$(CC) -dM -E -o $@ - < /dev/null
>>>>>>>>>
>>>>>>>>> +JUSTIFICATION_FILES := $(XEN_ROOT)/docs/misra/safe.json \
>>>>>>>>> + $(XEN_ROOT)/docs/misra/false-positive-$$*.json
>>>>>>>>> +
>>>>>>>>> +# The following command is using grep to find all files that contains a comment
>>>>>>>>> +# containing "SAF-<anything>" on a single line.
>>>>>>>>> +# %.safparse will be the original files saved from the build system, these files
>>>>>>>>> +# will be restored at the end of the analysis step
>>>>>>>>> +PARSE_FILE_LIST := $(addsuffix .safparse,$(filter-out %.safparse,\
>>>>>>>>> +$(shell grep -ERl '^[[:blank:]]*\/\*[[:space:]]+SAF-.*\*\/$$' $(srctree))))
>>>>>>>>
>>>>>>>> Please indent such line continuations. And then isn't this going to risk
>>>>>>>> matching non-source files as well? Perhaps you want to restrict this to
>>>>>>>> *.c and *.h?
>>>>>>>
>>>>>>> Yes, how about this, it will filter out *.safparse files while keeping in only .h and .c:
>>>>>>>
>>>>>>> PARSE_FILE_LIST := $(addsuffix .safparse,$(filter %.c %.h,\
>>>>>>> $(shell grep -ERl '^[[:blank:]]*\/\*[[:space:]]+SAF-.*\*\/$$' $(srctree))))
>>>>>>
>>>>>> That's better, but still means touching all files by grep despite now
>>>>>> only a subset really looked for. If I was to use the new goals on a
>>>>>> more or less regular basis, I'd expect that this enumeration of files
>>>>>> doesn't read _much_ more stuff from disk than is actually necessary.
>>>>>
>>>>> Ok would it be ok?
>>>>>
>>>>> PARSE_FILE_LIST := $(addsuffix .safparse,$(shell grep -ERl --include=\*.h \
>>>>> --include=\*.c '^[[:blank:]]*\/\*[[:space:]]+SAF-.*\*\/$$' $(srctree)))
>>>>
>>>> Hmm, not sure: --include isn't a standard option to grep, and we
>>>> generally try to be portable. Actually -R (or -r) isn't either. It
>>>> may still be okay that way if properly documented where the involved
>>>> goals will work and where not.
>>>
>>> Is a comment before the line ok as documentation? To state that —include and
>>> -R are not standard options so analysis-{coverity,eclair} will not work without a
>>> grep that takes those parameters?
>>
>> A comment _might_ be okay. Is there no other documentation on how these
>> goals are to be used? The main question here is how impacting this might
>> be to the various environments we allow Xen to be built in: Would at
>> least modern versions of all Linux distros we care about allow using
>> these rules? What about non-Linux?
>>
>> And could you at least bail when PARSE_FILE_LIST ends up empty, with a
>> clear error message augmenting the one grep would have issued?
>
> An empty PARSE_FILE_LIST should not generate an error, it just means there are no
> justifications, but I see it can be problematic in case grep does not work.
>
> What about this? They should be standard options right?
>
> PARSE_FILE_LIST := $(addsuffix .safparse,$(shell find $(srctree) -type f \
> -name '*.c' -o -name '*.h' -exec \
> grep -El '^[[:blank:]]*\/\*[[:space:]]+SAF-.*\*\/$$' {} + ))
Coming closer to being generally usable. You now have the problem of
potentially exceeding command line limits (iirc there were issues in
find and/or kernels), but I agree it looks standard-conforming now.
>>>> And then - why do you escape slashes in the ERE?
>>>>
>>>> Talking of escaping - personally I find backslash escapes harder to
>>>> read / grok than quotation, so I'd like to recommend using quotes
>>>> around each of the two --include (if they remain in the first place)
>>>> instead of the \* construct.
>>>
>>> Ok I’ve removed the escape from the * and also from slashes:
>>>
>>> PARSE_FILE_LIST := $(addsuffix .safparse,$(shell grep -ERl --include='*.h' \
>>> --include='*.c' '^[[:blank:]]*/\*[[:space:]]+SAF-.*\*/$$' $(srctree)))
>>
>> Good - seeing things more clearly now my next question is: Isn't
>> matching just "/* SAF-...*/" a little too lax? And is there really a
>> need to permit leading blanks?
>
> I’m permitting blanks to allow spaces or tabs, zero or more times before the start of
> the comment, I think it shall be like that.
Hmm, I withdraw my question realizing that you want these comments
indented the same as the line they relate to.
> About matching, maybe I can match also the number after SAF-, this should be enough,
>
> […] grep -El '^[[:blank:]]*\/\*[[:space:]]+SAF-[0-9]+.*\*\/$$’ […]
I'd like to suggest to go one tiny step further (and once again to
drop the escaping of slashes):
'^[[:blank:]]*/\*[[:space:]]+SAF-[0-9]+-.*\*/$$'
>>> Now analysis-build-coverity will be called, the best match is analysis-build-%, so again the dependency
>>> which is analysis-parse-tags-%, will be translated to analysis-parse-tags-coverity.
>>>
>>> Now analysis-parse-tags-coverity will be called, the best match is analysis-parse-tags-%, so the % will
>>> Have the ‘coverity’ value and in the dependency we will have $(objtree)/%.sed -> $(objtree)/coverity.sed.
>>>
>>> Looking for $(objtree)/coverity.sed the best match is $(objtree)/%.sed, which will have $(JUSTIFICATION_FILES)
>>> and the python script in the dependency, here we will use the second expansion to solve
>>> $(XEN_ROOT)/docs/misra/false-positive-$$*.json in $(XEN_ROOT)/docs/misra/false-positive-coverity.json
>>>
>>> So now after analysis-parse-tags-coverity has ended its dependency it will start with its recipe, after it finishes,
>>> the recipe of analysis-build-coverity will start and it will call make to actually build Xen.
>>
>> Okay, I see now - this building of Xen really _is_ independent of the
>> checker chosen. I'm not sure though whether it is a good idea to
>> integrate all this, including ...
>>
>>> After the build finishes, if the status is good, the analysis-build-coverity has finished and the _analysis-coverity
>>> recipe can now run, it will call make with the analysis-clean target, restoring any <file>.{c,h}.safparse to <file>.{c,h}.
>>
>> ... the subsequent cleaning. The state of the _source_ tree after a
>> build failure would be different from that after a successful build.
>> Personally I consider this at best surprising.
>>
>> I wonder whether instead there could be a shell(?) script driving a
>> sequence of make invocations, leaving the new make goals all be self-
>> contained. Such a script could revert the source tree to its original
>> state even upon build failure by default, with an option allowing to
>> suppress this behavior.
>
> Instead of adding another tool, so another layer to the overall system, I would be more willing to add documentation
> about this process, explaining how to use the analysis-* build targets, what to expect after a successful run and what
> to expect after a failure.
>
> What do you think?
Personally I'd prefer make goals to behave as such, with no surprises.
Jan
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [RFC PATCH 1/4] xen/Makefile: add analysis-coverity and analysis-eclair
2022-11-09 10:36 ` Jan Beulich
@ 2022-11-11 10:42 ` Luca Fancellu
2022-11-11 13:10 ` Jan Beulich
0 siblings, 1 reply; 30+ messages in thread
From: Luca Fancellu @ 2022-11-11 10:42 UTC (permalink / raw)
To: Jan Beulich
Cc: Bertrand Marquis, Wei Chen, Andrew Cooper, George Dunlap,
Julien Grall, Stefano Stabellini, Wei Liu, xen-devel
> On 9 Nov 2022, at 10:36, Jan Beulich <jbeulich@suse.com> wrote:
>
> On 09.11.2022 11:08, Luca Fancellu wrote:
>>>> On 07.11.2022 11:47, Luca Fancellu wrote:
>>>>> +Here is an example to add a new justification in false-positive-<tool>.json::
>>>>
>>>> With <tool> already present in the name, ...
>>>>
>>>>> +|{
>>>>> +| "version": "1.0",
>>>>> +| "content": [
>>>>> +| {
>>>>> +| "id": "SAF-0-false-positive-<tool>",
>>>>> +| "analyser": {
>>>>> +| "<tool>": "<proprietary-id>"
>>>>
>>>> ... can we avoid the redundancy here? Perhaps ...
>>>>
>>>>> +| },
>>>>> +| "tool-version": "<version>",
>>>>
>>>> ... it could be
>>>>
>>>> "analyser": {
>>>> "<version>": "<proprietary-id>"
>>>> },
>>
>> About this, I’ve investigated a bit and I don’t think this is the right solution, it wouldn't make
>> much sense to have a schema where in one file the analyser dictionary key is the tool name
>> and in another it is a version (or range of versions).
>>
>> However I can remove the analyser dictionary and use this schema for the false-positive, which is
>> more compact:
>>
>> |{
>> | "version": "1.0",
>> | "content": [
>> | {
>> | "id": "SAF-0-false-positive-<tool>",
>> | “tool-proprietary-id”: "<proprietary-id>”,
>> | "tool-version": "<version>",
>> | "name": "R20.7 [...]",
>> | "text": "[...]"
>> | },
>> | {
>> | "id": "SAF-1-false-positive-<tool>",
>> | “tool-proprietary-id”: "",
>> | "tool-version": "",
>> | "name": "Sentinel",
>> | "text": "Next ID to be used"
>> | }
>> | ]
>> |}
>>
>> This needs however a change in the initial design and more documentation on the different handlings
>> of the safe.json schema and the false-positive-<tool>.json schema. Is it worth?
>
> I think it is, but of others disagree, so be it.
So, since no one replied on that, I think everybody agrees that safe and false-positive can have a different schema,
I will update the python tool to handle that and I will update the make recipe consequently.
>>>>>
>>>>> Hmm, not sure: --include isn't a standard option to grep, and we
>>>>> generally try to be portable. Actually -R (or -r) isn't either. It
>>>>> may still be okay that way if properly documented where the involved
>>>>> goals will work and where not.
>>>>
>>>> Is a comment before the line ok as documentation? To state that —include and
>>>> -R are not standard options so analysis-{coverity,eclair} will not work without a
>>>> grep that takes those parameters?
>>>
>>> A comment _might_ be okay. Is there no other documentation on how these
>>> goals are to be used? The main question here is how impacting this might
>>> be to the various environments we allow Xen to be built in: Would at
>>> least modern versions of all Linux distros we care about allow using
>>> these rules? What about non-Linux?
>>>
>>> And could you at least bail when PARSE_FILE_LIST ends up empty, with a
>>> clear error message augmenting the one grep would have issued?
>>
>> An empty PARSE_FILE_LIST should not generate an error, it just means there are no
>> justifications, but I see it can be problematic in case grep does not work.
>>
>> What about this? They should be standard options right?
>>
>> PARSE_FILE_LIST := $(addsuffix .safparse,$(shell find $(srctree) -type f \
>> -name '*.c' -o -name '*.h' -exec \
>> grep -El '^[[:blank:]]*\/\*[[:space:]]+SAF-.*\*\/$$' {} + ))
>
> Coming closer to being generally usable. You now have the problem of
> potentially exceeding command line limits (iirc there were issues in
> find and/or kernels), but I agree it looks standard-conforming now.
>
>>>>> And then - why do you escape slashes in the ERE?
>>>>>
>>>>> Talking of escaping - personally I find backslash escapes harder to
>>>>> read / grok than quotation, so I'd like to recommend using quotes
>>>>> around each of the two --include (if they remain in the first place)
>>>>> instead of the \* construct.
>>>>
>>>> Ok I’ve removed the escape from the * and also from slashes:
>>>>
>>>> PARSE_FILE_LIST := $(addsuffix .safparse,$(shell grep -ERl --include='*.h' \
>>>> --include='*.c' '^[[:blank:]]*/\*[[:space:]]+SAF-.*\*/$$' $(srctree)))
>>>
>>> Good - seeing things more clearly now my next question is: Isn't
>>> matching just "/* SAF-...*/" a little too lax? And is there really a
>>> need to permit leading blanks?
>>
>> I’m permitting blanks to allow spaces or tabs, zero or more times before the start of
>> the comment, I think it shall be like that.
>
> Hmm, I withdraw my question realizing that you want these comments
> indented the same as the line they relate to.
>
>> About matching, maybe I can match also the number after SAF-, this should be enough,
>>
>> […] grep -El '^[[:blank:]]*\/\*[[:space:]]+SAF-[0-9]+.*\*\/$$’ […]
>
> I'd like to suggest to go one tiny step further (and once again to
> drop the escaping of slashes):
>
> '^[[:blank:]]*/\*[[:space:]]+SAF-[0-9]+-.*\*/$$'
I agree, I will use this one that is safer and includes your suggestions:
PARSE_FILE_LIST := $(addsuffix .safparse,$(shell find $(srctree) -type f \
-name '*.c' -o -name '*.h' -exec \
grep -El '^[[:blank:]]*/\*[[:space:]]+SAF-[0-9]+-.*\*/$$' {} \; ))
>
>>>> Now analysis-build-coverity will be called, the best match is analysis-build-%, so again the dependency
>>>> which is analysis-parse-tags-%, will be translated to analysis-parse-tags-coverity.
>>>>
>>>> Now analysis-parse-tags-coverity will be called, the best match is analysis-parse-tags-%, so the % will
>>>> Have the ‘coverity’ value and in the dependency we will have $(objtree)/%.sed -> $(objtree)/coverity.sed.
>>>>
>>>> Looking for $(objtree)/coverity.sed the best match is $(objtree)/%.sed, which will have $(JUSTIFICATION_FILES)
>>>> and the python script in the dependency, here we will use the second expansion to solve
>>>> $(XEN_ROOT)/docs/misra/false-positive-$$*.json in $(XEN_ROOT)/docs/misra/false-positive-coverity.json
>>>>
>>>> So now after analysis-parse-tags-coverity has ended its dependency it will start with its recipe, after it finishes,
>>>> the recipe of analysis-build-coverity will start and it will call make to actually build Xen.
>>>
>>> Okay, I see now - this building of Xen really _is_ independent of the
>>> checker chosen. I'm not sure though whether it is a good idea to
>>> integrate all this, including ...
>>>
>>>> After the build finishes, if the status is good, the analysis-build-coverity has finished and the _analysis-coverity
>>>> recipe can now run, it will call make with the analysis-clean target, restoring any <file>.{c,h}.safparse to <file>.{c,h}.
>>>
>>> ... the subsequent cleaning. The state of the _source_ tree after a
>>> build failure would be different from that after a successful build.
>>> Personally I consider this at best surprising.
>>>
>>> I wonder whether instead there could be a shell(?) script driving a
>>> sequence of make invocations, leaving the new make goals all be self-
>>> contained. Such a script could revert the source tree to its original
>>> state even upon build failure by default, with an option allowing to
>>> suppress this behavior.
>>
>> Instead of adding another tool, so another layer to the overall system, I would be more willing to add documentation
>> about this process, explaining how to use the analysis-* build targets, what to expect after a successful run and what
>> to expect after a failure.
>>
>> What do you think?
>
> Personally I'd prefer make goals to behave as such, with no surprises.
The analysis-* goal requires a build step, otherwise no analysis can be performed by the analysis tools, so I hope we agree
we need to integrate that step as a dependency of the analysis-*.
I understand that the analysis-clean might be a “surprise” if not well documented, this comes from the need to substitute the
tags in the tree (to keep the real path in the report log) and to revert them back at the end of the analysis.
So, such script should just mask to the user the analysis-clean invocation in case of errors (with an option to don’t do that)?
>
> Jan
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [RFC PATCH 1/4] xen/Makefile: add analysis-coverity and analysis-eclair
2022-11-11 10:42 ` Luca Fancellu
@ 2022-11-11 13:10 ` Jan Beulich
2022-11-11 20:52 ` Stefano Stabellini
0 siblings, 1 reply; 30+ messages in thread
From: Jan Beulich @ 2022-11-11 13:10 UTC (permalink / raw)
To: Luca Fancellu
Cc: Bertrand Marquis, Wei Chen, Andrew Cooper, George Dunlap,
Julien Grall, Stefano Stabellini, Wei Liu, xen-devel,
Anthony Perard
On 11.11.2022 11:42, Luca Fancellu wrote:
>> On 9 Nov 2022, at 10:36, Jan Beulich <jbeulich@suse.com> wrote:
>> On 09.11.2022 11:08, Luca Fancellu wrote:
>>>>> On 07.11.2022 11:47, Luca Fancellu wrote:
>>>>> Now analysis-build-coverity will be called, the best match is analysis-build-%, so again the dependency
>>>>> which is analysis-parse-tags-%, will be translated to analysis-parse-tags-coverity.
>>>>>
>>>>> Now analysis-parse-tags-coverity will be called, the best match is analysis-parse-tags-%, so the % will
>>>>> Have the ‘coverity’ value and in the dependency we will have $(objtree)/%.sed -> $(objtree)/coverity.sed.
>>>>>
>>>>> Looking for $(objtree)/coverity.sed the best match is $(objtree)/%.sed, which will have $(JUSTIFICATION_FILES)
>>>>> and the python script in the dependency, here we will use the second expansion to solve
>>>>> $(XEN_ROOT)/docs/misra/false-positive-$$*.json in $(XEN_ROOT)/docs/misra/false-positive-coverity.json
>>>>>
>>>>> So now after analysis-parse-tags-coverity has ended its dependency it will start with its recipe, after it finishes,
>>>>> the recipe of analysis-build-coverity will start and it will call make to actually build Xen.
>>>>
>>>> Okay, I see now - this building of Xen really _is_ independent of the
>>>> checker chosen. I'm not sure though whether it is a good idea to
>>>> integrate all this, including ...
>>>>
>>>>> After the build finishes, if the status is good, the analysis-build-coverity has finished and the _analysis-coverity
>>>>> recipe can now run, it will call make with the analysis-clean target, restoring any <file>.{c,h}.safparse to <file>.{c,h}.
>>>>
>>>> ... the subsequent cleaning. The state of the _source_ tree after a
>>>> build failure would be different from that after a successful build.
>>>> Personally I consider this at best surprising.
>>>>
>>>> I wonder whether instead there could be a shell(?) script driving a
>>>> sequence of make invocations, leaving the new make goals all be self-
>>>> contained. Such a script could revert the source tree to its original
>>>> state even upon build failure by default, with an option allowing to
>>>> suppress this behavior.
>>>
>>> Instead of adding another tool, so another layer to the overall system, I would be more willing to add documentation
>>> about this process, explaining how to use the analysis-* build targets, what to expect after a successful run and what
>>> to expect after a failure.
>>>
>>> What do you think?
>>
>> Personally I'd prefer make goals to behave as such, with no surprises.
>
> The analysis-* goal requires a build step, otherwise no analysis can be performed by the analysis tools, so I hope we agree
> we need to integrate that step as a dependency of the analysis-*.
No, I'm afraid we don't agree. But like said for another piece we didn't
initially agree on - if others think what you propose is fine, so be it.
I'm specifically adding Anthony to Cc, as he's been working on make rules
the most of all of us in the recent past.
> I understand that the analysis-clean might be a “surprise” if not well documented, this comes from the need to substitute the
> tags in the tree (to keep the real path in the report log) and to revert them back at the end of the analysis.
>
> So, such script should just mask to the user the analysis-clean invocation in case of errors (with an option to don’t do that)?
Hmm, here you're saying "such script", which looks to not fit with the
earlier part of your reply above. (Just in case that's what I was to read
out of this: I wouldn't see value in a script which existed _solely_ to
make the cleaning conditional.)
Did you consider the alternative approach of copying the tree, altering
it (while or after copying), running the build there, pulling out the
result files, and delete the entire copy? Such a model would likely get
away without introducing surprising make rules.
Jan
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [RFC PATCH 1/4] xen/Makefile: add analysis-coverity and analysis-eclair
2022-11-11 13:10 ` Jan Beulich
@ 2022-11-11 20:52 ` Stefano Stabellini
2022-11-14 7:30 ` Jan Beulich
0 siblings, 1 reply; 30+ messages in thread
From: Stefano Stabellini @ 2022-11-11 20:52 UTC (permalink / raw)
To: Jan Beulich
Cc: Luca Fancellu, Bertrand Marquis, Wei Chen, Andrew Cooper,
George Dunlap, Julien Grall, Stefano Stabellini, Wei Liu,
xen-devel, Anthony Perard
[-- Attachment #1: Type: text/plain, Size: 4717 bytes --]
On Fri, 11 Nov 2022, Jan Beulich wrote:
> On 11.11.2022 11:42, Luca Fancellu wrote:
> >> On 9 Nov 2022, at 10:36, Jan Beulich <jbeulich@suse.com> wrote:
> >> On 09.11.2022 11:08, Luca Fancellu wrote:
> >>>>> On 07.11.2022 11:47, Luca Fancellu wrote:
> >>>>> Now analysis-build-coverity will be called, the best match is analysis-build-%, so again the dependency
> >>>>> which is analysis-parse-tags-%, will be translated to analysis-parse-tags-coverity.
> >>>>>
> >>>>> Now analysis-parse-tags-coverity will be called, the best match is analysis-parse-tags-%, so the % will
> >>>>> Have the ‘coverity’ value and in the dependency we will have $(objtree)/%.sed -> $(objtree)/coverity.sed.
> >>>>>
> >>>>> Looking for $(objtree)/coverity.sed the best match is $(objtree)/%.sed, which will have $(JUSTIFICATION_FILES)
> >>>>> and the python script in the dependency, here we will use the second expansion to solve
> >>>>> $(XEN_ROOT)/docs/misra/false-positive-$$*.json in $(XEN_ROOT)/docs/misra/false-positive-coverity.json
> >>>>>
> >>>>> So now after analysis-parse-tags-coverity has ended its dependency it will start with its recipe, after it finishes,
> >>>>> the recipe of analysis-build-coverity will start and it will call make to actually build Xen.
> >>>>
> >>>> Okay, I see now - this building of Xen really _is_ independent of the
> >>>> checker chosen. I'm not sure though whether it is a good idea to
> >>>> integrate all this, including ...
> >>>>
> >>>>> After the build finishes, if the status is good, the analysis-build-coverity has finished and the _analysis-coverity
> >>>>> recipe can now run, it will call make with the analysis-clean target, restoring any <file>.{c,h}.safparse to <file>.{c,h}.
> >>>>
> >>>> ... the subsequent cleaning. The state of the _source_ tree after a
> >>>> build failure would be different from that after a successful build.
> >>>> Personally I consider this at best surprising.
> >>>>
> >>>> I wonder whether instead there could be a shell(?) script driving a
> >>>> sequence of make invocations, leaving the new make goals all be self-
> >>>> contained. Such a script could revert the source tree to its original
> >>>> state even upon build failure by default, with an option allowing to
> >>>> suppress this behavior.
> >>>
> >>> Instead of adding another tool, so another layer to the overall system, I would be more willing to add documentation
> >>> about this process, explaining how to use the analysis-* build targets, what to expect after a successful run and what
> >>> to expect after a failure.
> >>>
> >>> What do you think?
> >>
> >> Personally I'd prefer make goals to behave as such, with no surprises.
> >
> > The analysis-* goal requires a build step, otherwise no analysis can be performed by the analysis tools, so I hope we agree
> > we need to integrate that step as a dependency of the analysis-*.
>
> No, I'm afraid we don't agree. But like said for another piece we didn't
> initially agree on - if others think what you propose is fine, so be it.
> I'm specifically adding Anthony to Cc, as he's been working on make rules
> the most of all of us in the recent past.
>
> > I understand that the analysis-clean might be a “surprise” if not well documented, this comes from the need to substitute the
> > tags in the tree (to keep the real path in the report log) and to revert them back at the end of the analysis.
> >
> > So, such script should just mask to the user the analysis-clean invocation in case of errors (with an option to don’t do that)?
>
> Hmm, here you're saying "such script", which looks to not fit with the
> earlier part of your reply above. (Just in case that's what I was to read
> out of this: I wouldn't see value in a script which existed _solely_ to
> make the cleaning conditional.)
>
> Did you consider the alternative approach of copying the tree, altering
> it (while or after copying), running the build there, pulling out the
> result files, and delete the entire copy? Such a model would likely get
> away without introducing surprising make rules.
Another, maybe simpler idea: what if the build step is not a dependency
of the analysis-* goals?
Basically, the user is supposed to:
1) call analysis-parse-tags-*
2) build Xen (in any way they like)
3) call analysis-clean
Making steps 1-3 into a single step is slightly more convenient for the
user but the downside is that dealing with build errors becomes
problematic.
On the other hand, if we let the user call steps 1-3 by hand
individually, it is slightly less convenient for the user but they can
more easily deal with any build error and sophisticated build
configurations.
This is one of those cases where I think "less is more".
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [RFC PATCH 1/4] xen/Makefile: add analysis-coverity and analysis-eclair
2022-11-11 20:52 ` Stefano Stabellini
@ 2022-11-14 7:30 ` Jan Beulich
2022-11-14 12:30 ` Luca Fancellu
0 siblings, 1 reply; 30+ messages in thread
From: Jan Beulich @ 2022-11-14 7:30 UTC (permalink / raw)
To: Stefano Stabellini
Cc: Luca Fancellu, Bertrand Marquis, Wei Chen, Andrew Cooper,
George Dunlap, Julien Grall, Wei Liu, xen-devel, Anthony Perard
On 11.11.2022 21:52, Stefano Stabellini wrote:
> On Fri, 11 Nov 2022, Jan Beulich wrote:
>> On 11.11.2022 11:42, Luca Fancellu wrote:
>>>> On 9 Nov 2022, at 10:36, Jan Beulich <jbeulich@suse.com> wrote:
>>>> On 09.11.2022 11:08, Luca Fancellu wrote:
>>>>>>> On 07.11.2022 11:47, Luca Fancellu wrote:
>>>>>>> Now analysis-build-coverity will be called, the best match is analysis-build-%, so again the dependency
>>>>>>> which is analysis-parse-tags-%, will be translated to analysis-parse-tags-coverity.
>>>>>>>
>>>>>>> Now analysis-parse-tags-coverity will be called, the best match is analysis-parse-tags-%, so the % will
>>>>>>> Have the ‘coverity’ value and in the dependency we will have $(objtree)/%.sed -> $(objtree)/coverity.sed.
>>>>>>>
>>>>>>> Looking for $(objtree)/coverity.sed the best match is $(objtree)/%.sed, which will have $(JUSTIFICATION_FILES)
>>>>>>> and the python script in the dependency, here we will use the second expansion to solve
>>>>>>> $(XEN_ROOT)/docs/misra/false-positive-$$*.json in $(XEN_ROOT)/docs/misra/false-positive-coverity.json
>>>>>>>
>>>>>>> So now after analysis-parse-tags-coverity has ended its dependency it will start with its recipe, after it finishes,
>>>>>>> the recipe of analysis-build-coverity will start and it will call make to actually build Xen.
>>>>>>
>>>>>> Okay, I see now - this building of Xen really _is_ independent of the
>>>>>> checker chosen. I'm not sure though whether it is a good idea to
>>>>>> integrate all this, including ...
>>>>>>
>>>>>>> After the build finishes, if the status is good, the analysis-build-coverity has finished and the _analysis-coverity
>>>>>>> recipe can now run, it will call make with the analysis-clean target, restoring any <file>.{c,h}.safparse to <file>.{c,h}.
>>>>>>
>>>>>> ... the subsequent cleaning. The state of the _source_ tree after a
>>>>>> build failure would be different from that after a successful build.
>>>>>> Personally I consider this at best surprising.
>>>>>>
>>>>>> I wonder whether instead there could be a shell(?) script driving a
>>>>>> sequence of make invocations, leaving the new make goals all be self-
>>>>>> contained. Such a script could revert the source tree to its original
>>>>>> state even upon build failure by default, with an option allowing to
>>>>>> suppress this behavior.
>>>>>
>>>>> Instead of adding another tool, so another layer to the overall system, I would be more willing to add documentation
>>>>> about this process, explaining how to use the analysis-* build targets, what to expect after a successful run and what
>>>>> to expect after a failure.
>>>>>
>>>>> What do you think?
>>>>
>>>> Personally I'd prefer make goals to behave as such, with no surprises.
>>>
>>> The analysis-* goal requires a build step, otherwise no analysis can be performed by the analysis tools, so I hope we agree
>>> we need to integrate that step as a dependency of the analysis-*.
>>
>> No, I'm afraid we don't agree. But like said for another piece we didn't
>> initially agree on - if others think what you propose is fine, so be it.
>> I'm specifically adding Anthony to Cc, as he's been working on make rules
>> the most of all of us in the recent past.
>>
>>> I understand that the analysis-clean might be a “surprise” if not well documented, this comes from the need to substitute the
>>> tags in the tree (to keep the real path in the report log) and to revert them back at the end of the analysis.
>>>
>>> So, such script should just mask to the user the analysis-clean invocation in case of errors (with an option to don’t do that)?
>>
>> Hmm, here you're saying "such script", which looks to not fit with the
>> earlier part of your reply above. (Just in case that's what I was to read
>> out of this: I wouldn't see value in a script which existed _solely_ to
>> make the cleaning conditional.)
>>
>> Did you consider the alternative approach of copying the tree, altering
>> it (while or after copying), running the build there, pulling out the
>> result files, and delete the entire copy? Such a model would likely get
>> away without introducing surprising make rules.
>
> Another, maybe simpler idea: what if the build step is not a dependency
> of the analysis-* goals?
>
> Basically, the user is supposed to:
>
> 1) call analysis-parse-tags-*
> 2) build Xen (in any way they like)
> 3) call analysis-clean
Well, that's exactly what I've been proposing, with the (optional)
addition of a small (shell) script doing all of the three for ...
> Making steps 1-3 into a single step is slightly more convenient for the
> user but the downside is that dealing with build errors becomes
> problematic.
>
> On the other hand, if we let the user call steps 1-3 by hand
> individually, it is slightly less convenient for the user but they can
> more easily deal with any build error and sophisticated build
> configurations.
... convenience.
Jan
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [RFC PATCH 1/4] xen/Makefile: add analysis-coverity and analysis-eclair
2022-11-14 7:30 ` Jan Beulich
@ 2022-11-14 12:30 ` Luca Fancellu
2022-11-14 16:05 ` Jan Beulich
2022-11-14 17:16 ` Anthony PERARD
0 siblings, 2 replies; 30+ messages in thread
From: Luca Fancellu @ 2022-11-14 12:30 UTC (permalink / raw)
To: Jan Beulich
Cc: Stefano Stabellini, Bertrand Marquis, Wei Chen, Andrew Cooper,
George Dunlap, Julien Grall, Wei Liu, xen-devel, Anthony Perard
> On 14 Nov 2022, at 07:30, Jan Beulich <jbeulich@suse.com> wrote:
>
> On 11.11.2022 21:52, Stefano Stabellini wrote:
>> On Fri, 11 Nov 2022, Jan Beulich wrote:
>>> On 11.11.2022 11:42, Luca Fancellu wrote:
>>>>> On 9 Nov 2022, at 10:36, Jan Beulich <jbeulich@suse.com> wrote:
>>>>> On 09.11.2022 11:08, Luca Fancellu wrote:
>>>>>>>> On 07.11.2022 11:47, Luca Fancellu wrote:
>>>>>>>> Now analysis-build-coverity will be called, the best match is analysis-build-%, so again the dependency
>>>>>>>> which is analysis-parse-tags-%, will be translated to analysis-parse-tags-coverity.
>>>>>>>>
>>>>>>>> Now analysis-parse-tags-coverity will be called, the best match is analysis-parse-tags-%, so the % will
>>>>>>>> Have the ‘coverity’ value and in the dependency we will have $(objtree)/%.sed -> $(objtree)/coverity.sed.
>>>>>>>>
>>>>>>>> Looking for $(objtree)/coverity.sed the best match is $(objtree)/%.sed, which will have $(JUSTIFICATION_FILES)
>>>>>>>> and the python script in the dependency, here we will use the second expansion to solve
>>>>>>>> $(XEN_ROOT)/docs/misra/false-positive-$$*.json in $(XEN_ROOT)/docs/misra/false-positive-coverity.json
>>>>>>>>
>>>>>>>> So now after analysis-parse-tags-coverity has ended its dependency it will start with its recipe, after it finishes,
>>>>>>>> the recipe of analysis-build-coverity will start and it will call make to actually build Xen.
>>>>>>>
>>>>>>> Okay, I see now - this building of Xen really _is_ independent of the
>>>>>>> checker chosen. I'm not sure though whether it is a good idea to
>>>>>>> integrate all this, including ...
>>>>>>>
>>>>>>>> After the build finishes, if the status is good, the analysis-build-coverity has finished and the _analysis-coverity
>>>>>>>> recipe can now run, it will call make with the analysis-clean target, restoring any <file>.{c,h}.safparse to <file>.{c,h}.
>>>>>>>
>>>>>>> ... the subsequent cleaning. The state of the _source_ tree after a
>>>>>>> build failure would be different from that after a successful build.
>>>>>>> Personally I consider this at best surprising.
>>>>>>>
>>>>>>> I wonder whether instead there could be a shell(?) script driving a
>>>>>>> sequence of make invocations, leaving the new make goals all be self-
>>>>>>> contained. Such a script could revert the source tree to its original
>>>>>>> state even upon build failure by default, with an option allowing to
>>>>>>> suppress this behavior.
>>>>>>
>>>>>> Instead of adding another tool, so another layer to the overall system, I would be more willing to add documentation
>>>>>> about this process, explaining how to use the analysis-* build targets, what to expect after a successful run and what
>>>>>> to expect after a failure.
>>>>>>
>>>>>> What do you think?
>>>>>
>>>>> Personally I'd prefer make goals to behave as such, with no surprises.
>>>>
>>>> The analysis-* goal requires a build step, otherwise no analysis can be performed by the analysis tools, so I hope we agree
>>>> we need to integrate that step as a dependency of the analysis-*.
>>>
>>> No, I'm afraid we don't agree. But like said for another piece we didn't
>>> initially agree on - if others think what you propose is fine, so be it.
>>> I'm specifically adding Anthony to Cc, as he's been working on make rules
>>> the most of all of us in the recent past.
>>>
>>>> I understand that the analysis-clean might be a “surprise” if not well documented, this comes from the need to substitute the
>>>> tags in the tree (to keep the real path in the report log) and to revert them back at the end of the analysis.
>>>>
>>>> So, such script should just mask to the user the analysis-clean invocation in case of errors (with an option to don’t do that)?
>>>
>>> Hmm, here you're saying "such script", which looks to not fit with the
>>> earlier part of your reply above. (Just in case that's what I was to read
>>> out of this: I wouldn't see value in a script which existed _solely_ to
>>> make the cleaning conditional.)
>>>
>>> Did you consider the alternative approach of copying the tree, altering
>>> it (while or after copying), running the build there, pulling out the
>>> result files, and delete the entire copy? Such a model would likely get
>>> away without introducing surprising make rules.
This approach does not work because the report will contain a path that is different from the source path and
some web based tools won’t be able to track back the origin of the finding.
e.g. /path/to/xen/arch/arm/<file> is the original file, we run the analysis on /path/to2/xen/arch/arm/<file>,
the finding is in /path/to2/xen/arch/arm/<file> but the source repository contains only /path/to/xen/arch/arm/<file>
>>
>> Another, maybe simpler idea: what if the build step is not a dependency
>> of the analysis-* goals?
>>
>> Basically, the user is supposed to:
>>
>> 1) call analysis-parse-tags-*
>> 2) build Xen (in any way they like)
>> 3) call analysis-clean
>
> Well, that's exactly what I've been proposing, with the (optional)
> addition of a small (shell) script doing all of the three for ...
>
>> Making steps 1-3 into a single step is slightly more convenient for the
>> user but the downside is that dealing with build errors becomes
>> problematic.
>>
>> On the other hand, if we let the user call steps 1-3 by hand
>> individually, it is slightly less convenient for the user but they can
>> more easily deal with any build error and sophisticated build
>> configurations.
>
> ... convenience.
For coverity and eclair, it makes sense, these tools doesn’t require much effort to be integrated,
they are built to intercept files, compilers, environment variables during the make run in a
transparent way.
So the workflow is:
1) call analysis-parse-tags-*
2) build Xen (in any way they like)
3) call analysis-clean
If we think about cppcheck however, here the story changes, as it requires all these information
to be given as inputs, we have to do all the work the commercial tools do under the hood.
The cppcheck workflow instead is:
1) call analysis-parse-tags-cppcheck
2) generate cppcheck suppression list
3) build Xen (and run cppcheck on built source files)
4) collect and generate report
5) call analysis-clean
So let’s think about detaching the build stage from the previous stages, I think it is not very convenient
for the user, as during cppcheck analysis we build $(objtree)/include/generated/compiler-def.h, we build
$(objtree)/suppression-list.txt, so the user needs to build Xen where those files are created
(in-tree or out-of-tree) otherwise the analysis won’t work and that’s the first user requirement (stage #3).
The most critical input to cppcheck is Xen’s $(CC), it comes from the build system in this serie, the user would
need to pass the correct one to cppcheck wrapper, together with cppcheck flags, and pass to Xen build stage #3
the wrapper as CC, second user requirement.
After the analysis, the user needs to run some scripts to put together the cppcheck report fragments
after its analysis, this step requires also the knowledge of were Xen is built, in-tree or out-of-tree, so
here the third user requirement (similar to the first one, but the stage is #4).
In the end, we can see the user would not be able to call individually the targets if it is not mastering
the system, it’s too complex to have something working, we could create a script to handle these requirements,
but it would be complex as it would do the job of the make system, plus it needs to forward additional make arguments
to it as well (CROSS_COMPILE, XEN_TARGET_ARCH, in-tree or Out-of-tree build, ... for example).
In this thread the message is that in case of errors, there will be some artifacts (<file>.safparse, modified <file>)
and this is unexpected or surprising, but we are going to add a lot of complexity to handle something that needs
just documentation (in my opinion).
If the community don’t agree that documentation is enough, a solution could be to provide a script that in case of
errors, calls automatically the analysis-clean target, analysis-<tool> will call also the build step in this case,
here some pseudocode:
#!/bin/bash
set -e
trap [call analysis-clean] EXIT
[call analysis-<tool>]
This script needs however all the make arguments that we would have passed to make instead:
./script.sh --tool=<tool> [--dont-clean-on-err] -- CROSS_COMPILE=“[...]“ XEN_TARGET_ARCH=“[...]” [others...]
>
> Jan
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [RFC PATCH 1/4] xen/Makefile: add analysis-coverity and analysis-eclair
2022-11-14 12:30 ` Luca Fancellu
@ 2022-11-14 16:05 ` Jan Beulich
2022-11-14 17:16 ` Anthony PERARD
1 sibling, 0 replies; 30+ messages in thread
From: Jan Beulich @ 2022-11-14 16:05 UTC (permalink / raw)
To: Luca Fancellu
Cc: Stefano Stabellini, Bertrand Marquis, Wei Chen, Andrew Cooper,
George Dunlap, Julien Grall, Wei Liu, xen-devel, Anthony Perard
On 14.11.2022 13:30, Luca Fancellu wrote:
>> On 14 Nov 2022, at 07:30, Jan Beulich <jbeulich@suse.com> wrote:
>> On 11.11.2022 21:52, Stefano Stabellini wrote:
>>> On Fri, 11 Nov 2022, Jan Beulich wrote:
>>>> Did you consider the alternative approach of copying the tree, altering
>>>> it (while or after copying), running the build there, pulling out the
>>>> result files, and delete the entire copy? Such a model would likely get
>>>> away without introducing surprising make rules.
>
> This approach does not work because the report will contain a path that is different from the source path and
> some web based tools won’t be able to track back the origin of the finding.
>
> e.g. /path/to/xen/arch/arm/<file> is the original file, we run the analysis on /path/to2/xen/arch/arm/<file>,
> the finding is in /path/to2/xen/arch/arm/<file> but the source repository contains only /path/to/xen/arch/arm/<file>
Simply run "sed" over the result?
>>> Another, maybe simpler idea: what if the build step is not a dependency
>>> of the analysis-* goals?
>>>
>>> Basically, the user is supposed to:
>>>
>>> 1) call analysis-parse-tags-*
>>> 2) build Xen (in any way they like)
>>> 3) call analysis-clean
>>
>> Well, that's exactly what I've been proposing, with the (optional)
>> addition of a small (shell) script doing all of the three for ...
>>
>>> Making steps 1-3 into a single step is slightly more convenient for the
>>> user but the downside is that dealing with build errors becomes
>>> problematic.
>>>
>>> On the other hand, if we let the user call steps 1-3 by hand
>>> individually, it is slightly less convenient for the user but they can
>>> more easily deal with any build error and sophisticated build
>>> configurations.
>>
>> ... convenience.
>
> For coverity and eclair, it makes sense, these tools doesn’t require much effort to be integrated,
> they are built to intercept files, compilers, environment variables during the make run in a
> transparent way.
>
> So the workflow is:
>
> 1) call analysis-parse-tags-*
> 2) build Xen (in any way they like)
> 3) call analysis-clean
>
>
> If we think about cppcheck however, here the story changes, as it requires all these information
> to be given as inputs, we have to do all the work the commercial tools do under the hood.
>
> The cppcheck workflow instead is:
>
> 1) call analysis-parse-tags-cppcheck
> 2) generate cppcheck suppression list
> 3) build Xen (and run cppcheck on built source files)
> 4) collect and generate report
> 5) call analysis-clean
Which merely makes for a more involved (shell) script.
> So let’s think about detaching the build stage from the previous stages, I think it is not very convenient
> for the user, as during cppcheck analysis we build $(objtree)/include/generated/compiler-def.h, we build
> $(objtree)/suppression-list.txt, so the user needs to build Xen where those files are created
> (in-tree or out-of-tree) otherwise the analysis won’t work and that’s the first user requirement (stage #3).
>
> The most critical input to cppcheck is Xen’s $(CC), it comes from the build system in this serie, the user would
> need to pass the correct one to cppcheck wrapper, together with cppcheck flags, and pass to Xen build stage #3
> the wrapper as CC, second user requirement.
>
> After the analysis, the user needs to run some scripts to put together the cppcheck report fragments
> after its analysis, this step requires also the knowledge of were Xen is built, in-tree or out-of-tree, so
> here the third user requirement (similar to the first one, but the stage is #4).
>
> In the end, we can see the user would not be able to call individually the targets if it is not mastering
> the system, it’s too complex to have something working, we could create a script to handle these requirements,
> but it would be complex as it would do the job of the make system, plus it needs to forward additional make arguments
> to it as well (CROSS_COMPILE, XEN_TARGET_ARCH, in-tree or Out-of-tree build, ... for example).
>
> In this thread the message is that in case of errors, there will be some artifacts (<file>.safparse, modified <file>)
> and this is unexpected or surprising, but we are going to add a lot of complexity to handle something that needs
> just documentation (in my opinion).
>
> If the community don’t agree that documentation is enough, a solution could be to provide a script that in case of
> errors, calls automatically the analysis-clean target, analysis-<tool> will call also the build step in this case,
> here some pseudocode:
>
> #!/bin/bash
> set -e
>
> trap [call analysis-clean] EXIT
>
> [call analysis-<tool>]
>
>
> This script needs however all the make arguments that we would have passed to make instead:
>
> ./script.sh --tool=<tool> [--dont-clean-on-err] -- CROSS_COMPILE=“[...]“ XEN_TARGET_ARCH=“[...]” [others...]
Well, of course the suggested script would need to be passed overrides you'd
otherwise pass with "make build" or alike.
Jan
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [RFC PATCH 1/4] xen/Makefile: add analysis-coverity and analysis-eclair
2022-11-14 12:30 ` Luca Fancellu
2022-11-14 16:05 ` Jan Beulich
@ 2022-11-14 17:16 ` Anthony PERARD
1 sibling, 0 replies; 30+ messages in thread
From: Anthony PERARD @ 2022-11-14 17:16 UTC (permalink / raw)
To: Luca Fancellu
Cc: Jan Beulich, Stefano Stabellini, Bertrand Marquis, Wei Chen,
Andrew Cooper, George Dunlap, Julien Grall, Wei Liu, xen-devel
On Mon, Nov 14, 2022 at 12:30:39PM +0000, Luca Fancellu wrote:
> The cppcheck workflow instead is:
>
> 1) call analysis-parse-tags-cppcheck
> 2) generate cppcheck suppression list
> 3) build Xen (and run cppcheck on built source files)
> 4) collect and generate report
> 5) call analysis-clean
>
> So let’s think about detaching the build stage from the previous stages, I think it is not very convenient
> for the user, as during cppcheck analysis we build $(objtree)/include/generated/compiler-def.h, we build
> $(objtree)/suppression-list.txt, so the user needs to build Xen where those files are created
> (in-tree or out-of-tree) otherwise the analysis won’t work and that’s the first user requirement (stage #3).
>
> The most critical input to cppcheck is Xen’s $(CC), it comes from the build system in this serie, the user would
> need to pass the correct one to cppcheck wrapper, together with cppcheck flags, and pass to Xen build stage #3
> the wrapper as CC, second user requirement.
You could add something like that to Makefile:
export-variables:
@echo "CC='$(CC)'"
And if "the user" is a shell script, it could easily figure out what $CC
is, without having to duplicate the Makefile's logic for it.
> After the analysis, the user needs to run some scripts to put together the cppcheck report fragments
> after its analysis, this step requires also the knowledge of were Xen is built, in-tree or out-of-tree, so
> here the third user requirement (similar to the first one, but the stage is #4).
Don't support out-of-tree, that would make things easier. I don't see
how that would work anyway with the needed temporary changes to the
source code.
> In the end, we can see the user would not be able to call individually the targets if it is not mastering
> the system, it’s too complex to have something working, we could create a script to handle these requirements,
> but it would be complex as it would do the job of the make system, plus it needs to forward additional make arguments
> to it as well (CROSS_COMPILE, XEN_TARGET_ARCH, in-tree or Out-of-tree build, ... for example).
Well, instead of running `make X XEN_TARGET_ARCH=x86`, a script would be
run as `./script XEN_TARGET_ARCH=x86`, so not much change.
Then the script can easily run `make "$@"`.
Cheers,
--
Anthony PERARD
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [RFC PATCH 1/4] xen/Makefile: add analysis-coverity and analysis-eclair
2022-11-07 10:47 ` [RFC PATCH 1/4] xen/Makefile: add analysis-coverity and analysis-eclair Luca Fancellu
2022-11-07 16:35 ` Jan Beulich
@ 2022-11-14 16:25 ` Anthony PERARD
2022-11-25 8:50 ` Luca Fancellu
1 sibling, 1 reply; 30+ messages in thread
From: Anthony PERARD @ 2022-11-14 16:25 UTC (permalink / raw)
To: Luca Fancellu
Cc: xen-devel, bertrand.marquis, wei.chen, Andrew Cooper,
George Dunlap, Jan Beulich, Julien Grall, Stefano Stabellini,
Wei Liu
On Mon, Nov 07, 2022 at 10:47:36AM +0000, Luca Fancellu wrote:
> xen/Makefile | 50 ++++++-
Hi Luca,
Could you write a shell script which would probably be easier to
read/modify than this rather complicated looking set of Makefile rules?
As I see it, a potential `analysis` shell script would have a single
interaction with make, it would just have to run `make build
CC=cppcheck-gcc` or other.
Because I don't see how make is useful in this case. Or maybe you could
explain how writing this in make help?
Also non of this would work with out-of-tree builds, as you shouldn't
make modification to the source tree.
Cheers,
--
Anthony PERARD
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [RFC PATCH 1/4] xen/Makefile: add analysis-coverity and analysis-eclair
2022-11-14 16:25 ` Anthony PERARD
@ 2022-11-25 8:50 ` Luca Fancellu
0 siblings, 0 replies; 30+ messages in thread
From: Luca Fancellu @ 2022-11-25 8:50 UTC (permalink / raw)
To: Anthony PERARD
Cc: Xen-devel, Bertrand Marquis, Wei Chen, Andrew Cooper,
George Dunlap, Jan Beulich, Julien Grall, Stefano Stabellini,
Wei Liu
> On 14 Nov 2022, at 16:25, Anthony PERARD <anthony.perard@citrix.com> wrote:
>
> On Mon, Nov 07, 2022 at 10:47:36AM +0000, Luca Fancellu wrote:
>> xen/Makefile | 50 ++++++-
>
> Hi Luca,
Hi,
>
> Could you write a shell script which would probably be easier to
> read/modify than this rather complicated looking set of Makefile rules?
I admit the rules are a bit complicated
>
> As I see it, a potential `analysis` shell script would have a single
> interaction with make, it would just have to run `make build
> CC=cppcheck-gcc` or other.
>
> Because I don't see how make is useful in this case. Or maybe you could
> explain how writing this in make help?
> Also non of this would work with out-of-tree builds, as you shouldn't
> make modification to the source tree.
They both are good points, I will rewrite the rules as a script.
>
> Cheers,
>
> --
> Anthony PERARD
^ permalink raw reply [flat|nested] 30+ messages in thread
* [RFC PATCH 2/4] xen/Makefile: add analysis-cppcheck and analysis-cppcheck-html
2022-11-07 10:47 [RFC PATCH 0/4] Static analyser finding deviation Luca Fancellu
2022-11-07 10:47 ` [RFC PATCH 1/4] xen/Makefile: add analysis-coverity and analysis-eclair Luca Fancellu
@ 2022-11-07 10:47 ` Luca Fancellu
2022-11-07 10:47 ` [RFC PATCH 3/4] tools/misra: fix skipped rule numbers Luca Fancellu
2022-11-07 10:47 ` [RFC PATCH 4/4] xen: Justify linker script defined symbols in include/xen/kernel.h Luca Fancellu
3 siblings, 0 replies; 30+ messages in thread
From: Luca Fancellu @ 2022-11-07 10:47 UTC (permalink / raw)
To: xen-devel
Cc: bertrand.marquis, wei.chen, Andrew Cooper, George Dunlap,
Jan Beulich, Julien Grall, Stefano Stabellini, Wei Liu
Change cppcheck invocation method by substituting the Makefile
targets cppcheck{-html} with analysis-cppcheck{-html}.
Now cppcheck will build Xen while analysing the source files, and it
will produce a text output when called with analysis-cppcheck and
an additional html output when called with analysis-cppcheck-html.
With this patch cppcheck will benefit of platform configuration files
that will help it to understand the target of the compilation and
improve the analysis.
To do so:
- modify cppcheck calls in Makefile and add files to clean and
distclean.
- add platform configuration files for cppcheck.
- add scripts to generate text and html output.
- add cppcheck-cc.sh script that is a wrapper for cppcheck and it's
used as Xen compiler, it will intercept all flags given from the
make build system and it will execute cppcheck on the compiled
file together with the file compilation.
- add a script that generates a suppression list for cppcheck to
overcome a problem where cppcheck is not suppressing findings
in the headers using in-code comment. The system uses the headers
in-code comment to produce the list, so it's transparent to the
developer and both c files and header can benefit from in-code
comment suppression.
- guarded hypercall-defs.c with CPPCHECK define because cppcheck
gets confused as the file does not contain c code.
- add false-positive-cppcheck.json file
- update documentation.
- update .gitignore
Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>
---
.gitignore | 8 +-
docs/misra/cppcheck.txt | 47 +++--
docs/misra/documenting-violations.rst | 7 +-
docs/misra/false-positive-cppcheck.json | 12 ++
xen/Makefile | 143 ++++++++-----
xen/include/hypercall-defs.c | 9 +
xen/tools/cppcheck-build-suppr-list.sh | 81 +++++++
xen/tools/cppcheck-cc.sh | 223 ++++++++++++++++++++
xen/tools/cppcheck-html-prepare.sh | 110 ++++++++++
xen/tools/cppcheck-plat/arm32-wchar_t4.xml | 17 ++
xen/tools/cppcheck-plat/arm64-wchar_t2.xml | 17 ++
xen/tools/cppcheck-plat/arm64-wchar_t4.xml | 17 ++
xen/tools/cppcheck-plat/x86_64-wchar_t2.xml | 17 ++
xen/tools/cppcheck-plat/x86_64-wchar_t4.xml | 17 ++
xen/tools/cppcheck-txt-prepare.sh | 74 +++++++
15 files changed, 717 insertions(+), 82 deletions(-)
create mode 100644 docs/misra/false-positive-cppcheck.json
create mode 100755 xen/tools/cppcheck-build-suppr-list.sh
create mode 100755 xen/tools/cppcheck-cc.sh
create mode 100755 xen/tools/cppcheck-html-prepare.sh
create mode 100644 xen/tools/cppcheck-plat/arm32-wchar_t4.xml
create mode 100644 xen/tools/cppcheck-plat/arm64-wchar_t2.xml
create mode 100644 xen/tools/cppcheck-plat/arm64-wchar_t4.xml
create mode 100644 xen/tools/cppcheck-plat/x86_64-wchar_t2.xml
create mode 100644 xen/tools/cppcheck-plat/x86_64-wchar_t4.xml
create mode 100755 xen/tools/cppcheck-txt-prepare.sh
diff --git a/.gitignore b/.gitignore
index b48e1e20c4fc..abe47bfda9d2 100644
--- a/.gitignore
+++ b/.gitignore
@@ -7,9 +7,11 @@
*.o
*.d
*.d2
-*.c.cppcheck
+*.cppcheck
+*.cppcheck.txt
*.opic
*.a
+*.c.json
*.safparse
*.so
*.so.[0-9]*
@@ -283,9 +285,11 @@ xen/arch/*/efi/efi.h
xen/arch/*/efi/pe.c
xen/arch/*/efi/runtime.c
xen/arch/*/include/asm/asm-offsets.h
+xen/build-dir-cppcheck
xen/common/config_data.S
xen/common/config.gz
xen/cppcheck-htmlreport/
+xen/cppcheck-report/
xen/cppcheck-misra.*
xen/include/headers*.chk
xen/include/compat/*
@@ -317,7 +321,7 @@ tools/flask/policy/policy.conf
tools/flask/policy/xenpolicy-*
xen/*.sed
xen/xen
-xen/xen-cppcheck.xml
+xen/suppression-list.txt
xen/xen-syms
xen/xen-syms.map
xen/xen.*
diff --git a/docs/misra/cppcheck.txt b/docs/misra/cppcheck.txt
index 25d8c3050b72..c59ad03dc7e9 100644
--- a/docs/misra/cppcheck.txt
+++ b/docs/misra/cppcheck.txt
@@ -38,27 +38,32 @@ Dependencies are listed in the readme.md of the project repository.
Use cppcheck to analyse Xen
===========================
-Using cppcheck integration is very simple, it requires few steps:
-
- 1) Compile Xen
- 2) call the cppcheck make target to generate a report in xml format:
- make CPPCHECK_MISRA=y cppcheck
- 3) call the cppcheck-html make target to generate a report in xml and html
- format:
- make CPPCHECK_MISRA=y cppcheck-html
-
- In case the cppcheck binaries are not in the PATH, CPPCHECK and
- CPPCHECK_HTMLREPORT variables can be overridden with the full path to the
- binaries:
-
- make -C xen \
- CPPCHECK=/path/to/cppcheck \
- CPPCHECK_HTMLREPORT=/path/to/cppcheck-htmlreport \
- CPPCHECK_MISRA=y \
- cppcheck-html
-
-The output is by default in a folder named cppcheck-htmlreport, but the name
-can be changed by passing it in the CPPCHECK_HTMLREPORT_OUTDIR variable.
+Using cppcheck integration is very simple, it requires one of the following
+steps, depending on whether the user wants a text report or also an html report.
+The CPPCHECK_MISRA=y variable in the examples instructs cppcheck to analyse for
+MISRA compliance, but when not specified, the report will contain just the
+static analysis.
+
+ * call the analysis-cppcheck make target to generate a report in text format:
+ make CPPCHECK_MISRA=y analysis-cppcheck
+ * call the analysis-cppcheck-html make target to generate a report in text and
+ html format:
+ make CPPCHECK_MISRA=y analysis-cppcheck-html
+
+In case the cppcheck binaries are not in the PATH, CPPCHECK and
+CPPCHECK_HTMLREPORT variables can be overridden with the full path to the
+binaries:
+
+make -C xen \
+ CPPCHECK=/path/to/cppcheck \
+ CPPCHECK_HTMLREPORT=/path/to/cppcheck-htmlreport \
+ CPPCHECK_MISRA=y \
+ analysis-cppcheck-html
+
+The text report is by default in a folder named cppcheck-report, but the name
+can be changed by passing it in the CPPCHECK_REPORT_OUTDIR variable.
+The html report is by default in a folder named cppcheck-htmlreport, but the
+name can be changed by passing it in the CPPCHECK_HTMLREPORT_OUTDIR variable.
[1] https://sourceforge.net/p/cppcheck/discussion/general/thread/bfc3ab6c41/?limit=25
diff --git a/docs/misra/documenting-violations.rst b/docs/misra/documenting-violations.rst
index 3430abfaa177..f4f54a77d2a2 100644
--- a/docs/misra/documenting-violations.rst
+++ b/docs/misra/documenting-violations.rst
@@ -50,6 +50,7 @@ Here is an example to add a new justification in safe.json::
| {
| "id": "SAF-0-safe",
| "analyser": {
+| "cppcheck": "misra-c2012-20.7",
| "coverity": "misra_c_2012_rule_20_7_violation",
| "eclair": "MC3R1.R20.7"
| },
@@ -100,9 +101,9 @@ Here an explanation of the field inside an object of the "content" array:
It tells the tool to substitute a Xen in-code comment having this structure:
/* SAF-0-safe [...] \*/
- analyser: it is an object containing pair of key-value strings, the key is
- the analyser, so it can be coverity or eclair, the value is the proprietary
- id corresponding on the finding, for example when coverity is used as
- analyser, the tool will translate the Xen in-code coment in this way:
+ the analyser, so it can be cppcheck, coverity or eclair, the value is the
+ proprietary id corresponding on the finding, for example when coverity is
+ used as analyser, the tool will translate the Xen in-code coment in this way:
/* SAF-0-safe [...] \*/ -> /* coverity[misra_c_2012_rule_20_7_violation] \*/
if the object doesn't have a key-value, then the corresponding in-code
comment won't be translated.
diff --git a/docs/misra/false-positive-cppcheck.json b/docs/misra/false-positive-cppcheck.json
new file mode 100644
index 000000000000..0d8a8059d9cd
--- /dev/null
+++ b/docs/misra/false-positive-cppcheck.json
@@ -0,0 +1,12 @@
+{
+ "version": "1.0",
+ "content": [
+ {
+ "id": "SAF-0-false-positive-cppcheck",
+ "analyser": {},
+ "tool-version": "",
+ "name": "Sentinel",
+ "text": "Next ID to be used"
+ }
+ ]
+}
diff --git a/xen/Makefile b/xen/Makefile
index 3b8d1acd1697..e8a275e6d8a9 100644
--- a/xen/Makefile
+++ b/xen/Makefile
@@ -457,8 +457,8 @@ endif # need-config
__all: build
-main-targets := build install uninstall clean distclean MAP cppcheck \
- cppcheck-html analysis-coverity analysis-eclair
+main-targets := build install uninstall clean distclean MAP analysis-cppcheck \
+ analysis-cppcheck-html analysis-coverity analysis-eclair
.PHONY: $(main-targets)
ifneq ($(XEN_TARGET_ARCH),x86_32)
$(main-targets): %: _% ;
@@ -567,18 +567,20 @@ _clean:
$(Q)$(MAKE) $(clean)=tools/kconfig
find . \( -name "*.o" -o -name ".*.d" -o -name ".*.d2" \
-o -name ".*.o.tmp" -o -name "*~" -o -name "core" \
- -o -name '*.lex.c' -o -name '*.tab.[ch]' -o -name '*.c.cppcheck' \
- -o -name "*.gcno" -o -name ".*.cmd" -o -name "lib.a" \) -exec rm -f {} \;
+ -o -name '*.lex.c' -o -name '*.tab.[ch]' -o -name '*.cppcheck' \
+ -o -name '*.cppcheck.txt' -o -name "*.gcno" -o -name ".*.cmd" \
+ -o -name "lib.a" -o -name '*.c.json' \) -exec rm -f {} \;
rm -f include/asm $(TARGET) $(TARGET).gz $(TARGET)-syms $(TARGET)-syms.map
rm -f $(TARGET).efi $(TARGET).efi.map $(TARGET).efi.stripped
rm -f asm-offsets.s arch/*/include/asm/asm-offsets.h
rm -f .banner .allconfig.tmp include/xen/compile.h
- rm -f cppcheck-misra.* xen-cppcheck.xml *.sed
+ rm -f cppcheck-misra.* xen-cppcheck.xml suppression-list.txt *.sed
+ rm -rf $(CPPCHECK_BUILD_DIR)
.PHONY: _distclean
_distclean: clean
rm -f tags TAGS cscope.files cscope.in.out cscope.out cscope.po.out GTAGS GPATH GRTAGS GSYMS .config source
- rm -rf $(CPPCHECK_HTMLREPORT_OUTDIR)
+ rm -rf $(CPPCHECK_HTMLREPORT_OUTDIR) $(CPPCHECK_REPORT_OUTDIR)
$(TARGET).gz: $(TARGET)
gzip -n -f -9 < $< > $@.new
@@ -663,73 +665,50 @@ CPPCHECK ?= cppcheck
# On recent distribution, this is available in the standard path.
CPPCHECK_HTMLREPORT ?= cppcheck-htmlreport
-# By default we generate the report in cppcheck-htmlreport directory in the
+# By default we generate the html report in cppcheck-htmlreport directory in the
# build directory. This can be changed by giving a directory in this variable.
CPPCHECK_HTMLREPORT_OUTDIR ?= cppcheck-htmlreport
+# By default we generate the text report in cppcheck-report directory in the
+# build directory. This can be changed by giving a directory in this variable.
+CPPCHECK_REPORT_OUTDIR ?= cppcheck-report
+
# By default we do not check misra rules, to enable pass "CPPCHECK_MISRA=y" to
# make command line.
CPPCHECK_MISRA ?= n
+CPPCHECK_BUILD_DIR := $(objtree)/build-dir-cppcheck
+
# Compile flags to pass to cppcheck:
-# - include directories and defines Xen Makefile is passing (from CFLAGS)
# - include config.h as this is passed directly to the compiler.
-# - define CPPCHECK as we use to disable or enable some specific part of the
+# - define CPPCHECK as we use it to disable or enable some specific part of the
# code to solve some cppcheck issues.
# - explicitely enable some cppcheck checks as we do not want to use "all"
# which includes unusedFunction which gives wrong positives as we check file
# per file.
+# - Explicitly suppress warnings on compiler-def.h because cppcheck throws an
+# unmatchedSuppression due to the rule we put in suppression-list.txt to skip
+# every finding in the file.
#
# Compiler defines are in compiler-def.h which is included in config.h
#
-CPPCHECKFLAGS := -DCPPCHECK --max-ctu-depth=10 \
- --enable=style,information,missingInclude \
- --include=$(srctree)/include/xen/config.h \
- -I $(srctree)/xsm/flask/include \
- -I $(srctree)/include/xen/libfdt \
- $(filter -D% -I%,$(CFLAGS))
-
-# We need to find all C files (as we are not checking assembly files) so
-# we find all generated .o files which have a .c corresponding file.
-CPPCHECKFILES := $(wildcard $(patsubst $(objtree)/%.o,$(srctree)/%.c, \
- $(filter-out $(objtree)/tools/%, \
- $(shell find $(objtree) -name "*.o"))))
-
-# Headers and files required to run cppcheck on a file
-CPPCHECKDEPS := $(objtree)/include/generated/autoconf.h \
- $(objtree)/include/generated/compiler-def.h
+CPPCHECK_FLAGS := --cppcheck-build-dir=$(CPPCHECK_BUILD_DIR) \
+ --max-ctu-depth=10 \
+ --enable=style,information,missingInclude \
+ --template='{file}({line},{column}):{id}:{severity}:{message}' \
+ --relative-paths=$(srctree) \
+ --inline-suppr \
+ --suppressions-list=$(objtree)/suppression-list.txt \
+ --suppress='unmatchedSuppression:*generated/compiler-def.h' \
+ --include=$(srctree)/include/xen/config.h \
+ -DCPPCHECK
ifeq ($(CPPCHECK_MISRA),y)
- CPPCHECKFLAGS += --addon=cppcheck-misra.json
- CPPCHECKDEPS += cppcheck-misra.json
-endif
-
-quiet_cmd_cppcheck_xml = CPPCHECK $(patsubst $(srctree)/%,%,$<)
-cmd_cppcheck_xml = $(CPPCHECK) -v -q --xml $(CPPCHECKFLAGS) \
- --output-file=$@ $<
-
-quiet_cmd_merge_cppcheck_reports = CPPCHECK-MERGE $@
-cmd_merge_cppcheck_reports = $(PYTHON) $(srctree)/tools/merge_cppcheck_reports.py $^ $@
-
-quiet_cmd_cppcheck_html = CPPCHECK-HTML $<
-cmd_cppcheck_html = $(CPPCHECK_HTMLREPORT) --file=$< --source-dir=$(srctree) \
- --report-dir=$(CPPCHECK_HTMLREPORT_OUTDIR) --title=Xen
-
-PHONY += _cppcheck _cppcheck-html cppcheck-version
-
-_cppcheck-html: xen-cppcheck.xml
- $(call if_changed,cppcheck_html)
-
-_cppcheck: xen-cppcheck.xml
-
-xen-cppcheck.xml: $(patsubst $(srctree)/%.c,$(objtree)/%.c.cppcheck,$(CPPCHECKFILES))
-ifeq ($(CPPCHECKFILES),)
- $(error Please build Xen before running cppcheck)
+CPPCHECK_FLAGS += --addon=cppcheck-misra.json
+CPPCHECK_BUILD_EXTRA_DEPS += cppcheck-misra.json
endif
- $(call if_changed,merge_cppcheck_reports)
-$(objtree)/%.c.cppcheck: $(srctree)/%.c $(CPPCHECKDEPS) | cppcheck-version
- $(call if_changed,cppcheck_xml)
+PHONY += cppcheck-version
cppcheck-version:
$(Q)if ! which $(CPPCHECK) > /dev/null 2>&1; then \
@@ -761,6 +740,11 @@ $(objtree)/include/generated/compiler-def.h:
JUSTIFICATION_FILES := $(XEN_ROOT)/docs/misra/safe.json \
$(XEN_ROOT)/docs/misra/false-positive-$$*.json
+CPPCHECK_CC_FLAGS := --compiler=$(CC) \
+ --cppcheck-cmd=$(CPPCHECK) $(CPPCHECK_FLAGS) \
+ --cppcheck-plat=$(srctree)/tools/cppcheck-plat \
+ --ignore-path=tools/
+
# The following command is using grep to find all files that contains a comment
# containing "SAF-<anything>" on a single line.
# %.safparse will be the original files saved from the build system, these files
@@ -789,8 +773,49 @@ analysis-parse-tags-%: $(PARSE_FILE_LIST) $(objtree)/%.sed
sed -i -f "$(objtree)/$*.sed" "$${file}"; \
done
-analysis-build-%: analysis-parse-tags-%
- $(MAKE) O=$(abs_objtree) -f $(srctree)/Makefile build
+.SECONDEXPANSION:
+analysis-build-%: $$(ANALYSIS_BUILD_DEPS)
+ $(MAKE) O=$(abs_objtree) $(ANALYSIS_EXTRA_MAKE) -f $(srctree)/Makefile build
+
+$(CPPCHECK_BUILD_DIR) $(CPPCHECK_REPORT_OUTDIR) $(CPPCHECK_HTMLREPORT_OUTDIR):
+ $(Q)mkdir -p $@
+
+$(objtree)/suppression-list.txt: analysis-parse-tags-cppcheck
+ $(Q)$(srctree)/tools/cppcheck-build-suppr-list.sh --out-list=$@ \
+ --source-dir=$(abs_srctree)
+# Add this rule to skip every finding in the autogenerated header for cppcheck
+ $(Q)echo "*:*generated/compiler-def.h" >> $@
+
+build-cppcheck: ANALYSIS_EXTRA_MAKE += CC="$(srctree)/tools/cppcheck-cc.sh \
+ $(CPPCHECK_CC_FLAGS) $(CPPCHECK_CC_EXTRA_FLAGS) --"
+build-cppcheck: ANALYSIS_BUILD_DEPS = $(objtree)/suppression-list.txt \
+ $(CPPCHECK_BUILD_EXTRA_DEPS) $(CPPCHECK_BUILD_DIR) \
+ $(objtree)/include/generated/compiler-def.h
+
+build-cppcheck: analysis-build-cppcheck | cppcheck-version
+
+run-cppcheck: build-cppcheck $(CPPCHECK_REPORT_OUTDIR)
+ $(Q)$(srctree)/tools/cppcheck-txt-prepare.sh --frag-ext=.cppcheck.txt \
+ --rel-path=$(abs_srctree)/ \
+ --outfile=$(CPPCHECK_REPORT_OUTDIR)/xen-cppcheck.txt
+
+run-cppcheck-html: CPPCHECK_CC_EXTRA_FLAGS += --cppcheck-html
+
+run-cppcheck-html: run-cppcheck $(CPPCHECK_HTMLREPORT_OUTDIR)
+ $(Q)$(srctree)/tools/cppcheck-html-prepare.sh --frag-ext=.cppcheck \
+ --merge-tool=$(abs_srctree)/tools/merge_cppcheck_reports.py \
+ --source-dir=$(srctree) \
+ --outfile=$(CPPCHECK_HTMLREPORT_OUTDIR)/xen-cppcheck.xml
+ $(CPPCHECK_HTMLREPORT) \
+ --file=$(CPPCHECK_HTMLREPORT_OUTDIR)/xen-cppcheck.xml \
+ --source-dir=$(srctree) \
+ --report-dir=$(CPPCHECK_HTMLREPORT_OUTDIR)/html --title=Xen
+# Strip full build path from html report
+ $(Q)find $(CPPCHECK_HTMLREPORT_OUTDIR)/html -type f -name '*.html' \
+ -exec sed -i -re 's|$(abs_objtree)/||g' {} +
+# Strip full source path from html report
+ $(Q)find $(CPPCHECK_HTMLREPORT_OUTDIR)/html -type f -name '*.html' \
+ -exec sed -i -re 's|$(abs_srctree)/||g' {} +
analysis-clean:
# Reverts the original file (-p preserves also timestamp)
@@ -800,7 +825,13 @@ analysis-clean:
rm -f "$${file}"; \
done
-_analysis-%: analysis-build-%
+_analysis-coverity: ANALYSIS_BUILD_DEPS = analysis-parse-tags-coverity
+_analysis-eclair: ANALYSIS_BUILD_DEPS = analysis-parse-tags-eclair
+
+_analysis-coverity _analysis-eclair: _analysis-%: analysis-build-%
+ $(Q)$(MAKE) O=$(abs_objtree) -f $(srctree)/Makefile analysis-clean
+
+_analysis-cppcheck _analysis-cppcheck-html: _analysis-cppcheck%: run-cppcheck%
$(Q)$(MAKE) O=$(abs_objtree) -f $(srctree)/Makefile analysis-clean
endif #config-build
diff --git a/xen/include/hypercall-defs.c b/xen/include/hypercall-defs.c
index 60cbeb18e4da..d9fbcffa9567 100644
--- a/xen/include/hypercall-defs.c
+++ b/xen/include/hypercall-defs.c
@@ -60,6 +60,13 @@
* are possible.
*/
+/*
+ * Cppcheck thinks this file needs to be analysed because it is preprocessed by
+ * the compiler, but it gets confused because this file does not contains C
+ * code. Hence protect the code when CPPCHECK is used.
+ */
+#ifndef CPPCHECK
+
#ifdef CONFIG_HVM
#define PREFIX_hvm hvm
#else
@@ -283,3 +290,5 @@ mca do do - - -
#ifndef CONFIG_PV_SHIM_EXCLUSIVE
paging_domctl_cont do do do do -
#endif
+
+#endif /* !CPPCHECK */
diff --git a/xen/tools/cppcheck-build-suppr-list.sh b/xen/tools/cppcheck-build-suppr-list.sh
new file mode 100755
index 000000000000..637ec3ce8d70
--- /dev/null
+++ b/xen/tools/cppcheck-build-suppr-list.sh
@@ -0,0 +1,81 @@
+#!/usr/bin/env bash
+
+set -e
+
+function help() {
+ cat <<EOF
+Usage: ${0} [OPTION] ...
+
+This script builds a suppression list file for cppcheck, it checks which header
+files has in-code comment like /* cppcheck-suppress ... */ and adds a
+suppression rule to the list like this:
+
+[error id]:[filename]:[line]
+
+This list is created to overcome a problem where in-code suppression comment are
+not used when they are in header files, c files in-code comments works as
+expected.
+
+Options:
+ --out-list= Use this compiler for the build
+ --source-dir= The base source dir where to find the headers
+ -h, --help Print this help
+EOF
+}
+
+OUT_LIST=""
+SOURCE_DIR=""
+
+for OPTION in "$@"
+do
+ case ${OPTION} in
+ -h|--help)
+ help
+ exit 0
+ ;;
+ --out-list=*)
+ OUT_LIST="$(eval echo "${OPTION#*=}")"
+ ;;
+ --source-dir=*)
+ SOURCE_DIR="$(eval echo "${OPTION#*=}")"
+ ;;
+ *)
+ echo "Invalid option ${OPTION}"
+ exit 1
+ ;;
+ esac
+done
+
+rm -f "${OUT_LIST}"
+touch "${OUT_LIST}"
+
+while IFS= read -r entry; do
+ file=$(realpath "$(echo "${entry}" | cut -d':' -f1)")
+ line=$(echo "${entry}" | cut -d':' -f2)
+ cppcheck_id=$(echo "${entry}" | cut -d':' -f3 | \
+ sed -nre 's,\/\* cppcheck-suppress\[(.*)\] \*\/,\1,p')
+ # Expected code line is maximum 19 lines from the in-code comment
+ bound=20
+
+ # Adjust line number to be the next non-empty line that don't contains a
+ # comment
+ while [ ${bound} -gt 0 ]; do
+ line=$(( line + 1 ))
+ line_content=$(sed "${line}q;d" "${file}")
+ if [ ! "${line_content}" = "" ] && [[ ! ${line_content} == *"/*"* ]]
+ then
+ break
+ fi
+ bound=$(( bound - 1 ))
+ done
+
+ if [ ${bound} -eq 0 ]
+ then
+ echo "Please check if finding at '${entry}' is valid."
+ exit 1
+ fi
+
+ echo "${cppcheck_id}:${file}:${line}" >> "${OUT_LIST}"
+
+done < <(grep -ERn '^[[:blank:]]*\/\*[[:space:]]+cppcheck-suppress.*\*\/$' \
+ "${SOURCE_DIR}" | grep -E '.*\.h')
diff --git a/xen/tools/cppcheck-cc.sh b/xen/tools/cppcheck-cc.sh
new file mode 100755
index 000000000000..45a9f749f8c7
--- /dev/null
+++ b/xen/tools/cppcheck-cc.sh
@@ -0,0 +1,223 @@
+#!/usr/bin/env bash
+
+set -e
+
+function help() {
+ cat <<EOF
+Usage: ${0} [OPTION] ... -- <compiler arguments>
+
+This script is a wrapper for cppcheck that enables it to analyse the files that
+are the target for the build, it is used in place of a selected compiler and the
+make process will run it on every file that needs to be built.
+All the arguments passed to the original compiler are forwarded to it without
+modification, furthermore, they are used to improve the cppcheck analysis.
+
+Options:
+ --compiler= Use this compiler for the build
+ --cppcheck-cmd= Command line for the cppcheck analysis.
+ --cppcheck-html Prepare for cppcheck HTML output
+ --cppcheck-plat= Path to the cppcheck platform folder
+ --ignore-path= This script won't run cppcheck on the files having this
+ path, the compiler will run anyway on them. This argument
+ can be specified multiple times.
+ -h, --help Print this help
+EOF
+}
+
+CC_FILE=""
+COMPILER=""
+CPPCHECK_HTML="n"
+CPPCHECK_PLAT_PATH=""
+CPPCHECK_TOOL=""
+CPPCHECK_TOOL_ARGS=""
+FORWARD_FLAGS=""
+IGNORE_PATH="n"
+IGNORE_PATH_LIST=""
+JDB_FILE=""
+OBJTREE_PATH=""
+
+# Variable used for arg parsing
+forward_to_cc="n"
+sm_tool_args="n"
+obj_arg_content="n"
+
+for OPTION in "$@"
+do
+ if [ "${forward_to_cc}" = "y" ]; then
+ if [[ ${OPTION} == *.c ]]
+ then
+ CC_FILE="${OPTION}"
+ elif [ "${OPTION}" = "-o" ]
+ then
+ # After -o there is the path to the obj file, flag it
+ obj_arg_content="y"
+ elif [ "${obj_arg_content}" = "y" ]
+ then
+ # This must be the path to the obj file, turn off flag and save path
+ OBJTREE_PATH="$(dirname "${OPTION}")"
+ obj_arg_content="n"
+ fi
+ # Forward any argument to the compiler
+ FORWARD_FLAGS="${FORWARD_FLAGS} ${OPTION}"
+ continue
+ fi
+ case ${OPTION} in
+ -h|--help)
+ help
+ exit 0
+ ;;
+ --compiler=*)
+ COMPILER="$(eval echo "${OPTION#*=}")"
+ sm_tool_args="n"
+ ;;
+ --cppcheck-cmd=*)
+ CPPCHECK_TOOL="$(eval echo "${OPTION#*=}")"
+ sm_tool_args="y"
+ ;;
+ --cppcheck-html)
+ CPPCHECK_HTML="y"
+ sm_tool_args="n"
+ ;;
+ --cppcheck-plat=*)
+ CPPCHECK_PLAT_PATH="$(eval echo "${OPTION#*=}")"
+ sm_tool_args="n"
+ ;;
+ --ignore-path=*)
+ IGNORE_PATH_LIST="${IGNORE_PATH_LIST} $(eval echo "${OPTION#*=}")"
+ sm_tool_args="n"
+ ;;
+ --)
+ forward_to_cc="y"
+ sm_tool_args="n"
+ ;;
+ *)
+ if [ "${sm_tool_args}" = "y" ]; then
+ CPPCHECK_TOOL_ARGS="${CPPCHECK_TOOL_ARGS} ${OPTION}"
+ else
+ echo "Invalid option ${OPTION}"
+ exit 1
+ fi
+ ;;
+ esac
+done
+
+if [ "${COMPILER}" = "" ]
+then
+ echo "--compiler arg is mandatory."
+ exit 1
+fi
+
+function print_file() {
+ local text="${1}"
+ local init_file="${2}"
+
+ if [ "${init_file}" = "y" ]
+ then
+ echo -e -n "${text}" > "${JDB_FILE}"
+ else
+ echo -e -n "${text}" >> "${JDB_FILE}"
+ fi
+}
+
+function create_jcd() {
+ local line="${1}"
+ local arg_num=0
+ local same_line=0
+
+ print_file "[\n" "y"
+ print_file " {\n"
+ print_file " \"arguments\": [\n"
+
+ for arg in ${line}; do
+ # This code prevents to put comma in the last element of the list or on
+ # sequential lines that are going to be merged
+ [ "${arg_num}" -ne 0 ] && [ "${same_line}" -eq 0 ] && print_file ",\n"
+ if [ "${same_line}" -ne 0 ]
+ then
+ print_file "${arg}\""
+ same_line=0
+ elif [ "${arg}" = "-iquote" ] || [ "${arg}" = "-I" ]
+ then
+ # cppcheck doesn't understand -iquote, substitute with -I
+ print_file " \"-I"
+ same_line=1
+ else
+ print_file " \"${arg}\""
+ fi
+ arg_num=$(( arg_num + 1 ))
+ done
+ print_file "\n"
+ print_file " ],\n"
+ print_file " \"directory\": \"$(pwd -P)\",\n"
+ print_file " \"file\": \"${CC_FILE}\"\n"
+ print_file " }\n"
+ print_file "]\n"
+}
+
+
+# Execute compiler with forwarded flags
+# Shellcheck complains about missing quotes on FORWARD_FLAGS, but they can't be
+# used here
+# shellcheck disable=SC2086
+${COMPILER} ${FORWARD_FLAGS}
+
+if [ -n "${CC_FILE}" ];
+then
+ for path in ${IGNORE_PATH_LIST}
+ do
+ if [[ ${CC_FILE} == *${path}* ]]
+ then
+ IGNORE_PATH="y"
+ echo "${0}: ${CC_FILE} ignored by --ignore-path matching *${path}*"
+ fi
+ done
+ if [ "${IGNORE_PATH}" = "n" ]
+ then
+ JDB_FILE="${OBJTREE_PATH}/$(basename "${CC_FILE}".json)"
+
+ # Prepare the Json Compilation Database for the file
+ create_jcd "${COMPILER} ${FORWARD_FLAGS}"
+
+ out_file="${OBJTREE_PATH}/$(basename "${CC_FILE%.c}".cppcheck.txt)"
+
+ # Check wchar size
+ wchar_plat_suffix="t4"
+ # sed prints the last occurence of -f(no-)short-wchar which is the one
+ # applied to the file by the compiler
+ wchar_option=$(echo "${FORWARD_FLAGS}" | \
+ sed -nre 's,.*(-f(no-)?short-wchar).*,\1,p')
+ if [ "${wchar_option}" = "-fshort-wchar" ]
+ then
+ wchar_plat_suffix="t2"
+ fi
+
+ # Select the right target platform, ARCH is generated from Xen Makefile
+ platform="${CPPCHECK_PLAT_PATH}/${ARCH}-wchar_${wchar_plat_suffix}.xml"
+ if [ ! -f "${platform}" ]
+ then
+ echo "${platform} not found!"
+ exit 1
+ fi
+
+ # Shellcheck complains about missing quotes on CPPCHECK_TOOL_ARGS, but
+ # they can't be used here
+ # shellcheck disable=SC2086
+ ${CPPCHECK_TOOL} ${CPPCHECK_TOOL_ARGS} \
+ --project="${JDB_FILE}" \
+ --output-file="${out_file}" \
+ --platform=${platform}
+
+ if [ "${CPPCHECK_HTML}" = "y" ]
+ then
+ # Shellcheck complains about missing quotes on CPPCHECK_TOOL_ARGS,
+ # but they can't be used here
+ # shellcheck disable=SC2086
+ ${CPPCHECK_TOOL} ${CPPCHECK_TOOL_ARGS} \
+ --project="${JDB_FILE}" \
+ --output-file="${out_file%.txt}" \
+ --platform=${platform} \
+ -q \
+ --xml
+ fi
+ fi
+fi
diff --git a/xen/tools/cppcheck-html-prepare.sh b/xen/tools/cppcheck-html-prepare.sh
new file mode 100755
index 000000000000..c889dcc3582c
--- /dev/null
+++ b/xen/tools/cppcheck-html-prepare.sh
@@ -0,0 +1,110 @@
+#!/usr/bin/env bash
+
+set -e
+
+function help() {
+ cat <<EOF
+Usage: ${0} [OPTION] ...
+
+This script collects all the cppcheck xml report fragment to create one report.
+
+Options:
+ --frag-ext= Extension of the report fragment to be collected
+ --merge-tool= Tool used to merge the xml fragments
+ --outfile= Path and filename for the text report to be generated
+ -h, --help Print this help
+EOF
+}
+
+FRAG_EXT=""
+MERGE_TOOL=""
+OUTFILE=""
+SRCTREE=""
+
+for OPTION in "$@"
+do
+ case ${OPTION} in
+ -h|--help)
+ help
+ exit 0
+ ;;
+ --frag-ext=*)
+ FRAG_EXT="$(eval echo "${OPTION#*=}")"
+ ;;
+ --merge-tool=*)
+ MERGE_TOOL="$(eval echo "${OPTION#*=}")"
+ ;;
+ --outfile=*)
+ OUTFILE="$(eval echo "${OPTION#*=}")"
+ ;;
+ --source-dir=*)
+ SRCTREE="$(eval echo "${OPTION#*=}")"
+ ;;
+ *)
+ echo "Invalid option ${OPTION}"
+ exit 1
+ ;;
+ esac
+done
+
+if [ "${FRAG_EXT}" = "" ]
+then
+ echo "Please provide --frag-ext argument"
+ exit 1
+fi
+
+if [ "${MERGE_TOOL}" = "" ]
+then
+ echo "Please provide --merge-tool argument"
+ exit 1
+fi
+
+if [ "${OUTFILE}" = "" ]
+then
+ echo "Please provide --outfile argument"
+ exit 1
+fi
+
+if [ "${SRCTREE}" = "" ]
+then
+ echo "Please provide --source-dir argument"
+ exit 1
+fi
+
+rm -f "${OUTFILE}"
+
+XML_PARAM_LIST=""
+FILE_LIST=$(find . -name "*${FRAG_EXT}" -print)
+
+for file in ${FILE_LIST}
+do
+ XML_PARAM_LIST="${XML_PARAM_LIST} ${file}"
+
+done
+
+if [ -n "${XML_PARAM_LIST}" ]
+then
+ # Shellcheck complains about using quotes on XML_PARAM_LIST, but it can't
+ # work with them, disable this finding
+ # shellcheck disable=SC2086
+ ${MERGE_TOOL} ${XML_PARAM_LIST} "${OUTFILE}"
+fi
+
+# Retrieve all path to .h and .c from the xml file
+PATHLIST_IN_FILE=$(sed -nre 's/^.*"(.*.[ch])".*$/\1/p' "${OUTFILE}")
+
+# Some path are relative to the source tree but some others are generated
+# in the obj tree, for cppcheck when using cppcheck-htmlreport we can pass
+# only one source tree where the files will be fetched if relative path are
+# found. So for every path that does not exists in src tree, we guess it
+# comes from obj tree and we put explicit absolute path to it
+for path in ${PATHLIST_IN_FILE}
+do
+ # If path is not absolute and it doesn't exists in srctree, but it exists in
+ # objtree, then modify the path to use obj tree
+ if [[ ! ${path} == /* ]] && [ ! -f "${SRCTREE}/${path}" ] \
+ && [ -f "$(pwd -P)/${path}" ]
+ then
+ sed -i -re "s|\"(${path})\"|\"$(pwd -P)/\1\"|g" "${OUTFILE}"
+ fi
+done
diff --git a/xen/tools/cppcheck-plat/arm32-wchar_t4.xml b/xen/tools/cppcheck-plat/arm32-wchar_t4.xml
new file mode 100644
index 000000000000..3aefa7ba5c98
--- /dev/null
+++ b/xen/tools/cppcheck-plat/arm32-wchar_t4.xml
@@ -0,0 +1,17 @@
+<?xml version="1.0"?>
+<platform>
+ <char_bit>8</char_bit>
+ <default-sign>unsigned</default-sign>
+ <sizeof>
+ <short>2</short>
+ <int>4</int>
+ <long>4</long>
+ <long-long>8</long-long>
+ <float>4</float>
+ <double>8</double>
+ <long-double>8</long-double>
+ <pointer>4</pointer>
+ <size_t>4</size_t>
+ <wchar_t>4</wchar_t>
+ </sizeof>
+</platform>
diff --git a/xen/tools/cppcheck-plat/arm64-wchar_t2.xml b/xen/tools/cppcheck-plat/arm64-wchar_t2.xml
new file mode 100644
index 000000000000..e345b934a986
--- /dev/null
+++ b/xen/tools/cppcheck-plat/arm64-wchar_t2.xml
@@ -0,0 +1,17 @@
+<?xml version="1.0"?>
+<platform>
+ <char_bit>8</char_bit>
+ <default-sign>unsigned</default-sign>
+ <sizeof>
+ <short>2</short>
+ <int>4</int>
+ <long>8</long>
+ <long-long>8</long-long>
+ <float>4</float>
+ <double>8</double>
+ <long-double>16</long-double>
+ <pointer>8</pointer>
+ <size_t>4</size_t>
+ <wchar_t>2</wchar_t>
+ </sizeof>
+</platform>
diff --git a/xen/tools/cppcheck-plat/arm64-wchar_t4.xml b/xen/tools/cppcheck-plat/arm64-wchar_t4.xml
new file mode 100644
index 000000000000..952b3640c91d
--- /dev/null
+++ b/xen/tools/cppcheck-plat/arm64-wchar_t4.xml
@@ -0,0 +1,17 @@
+<?xml version="1.0"?>
+<platform>
+ <char_bit>8</char_bit>
+ <default-sign>unsigned</default-sign>
+ <sizeof>
+ <short>2</short>
+ <int>4</int>
+ <long>8</long>
+ <long-long>8</long-long>
+ <float>4</float>
+ <double>8</double>
+ <long-double>16</long-double>
+ <pointer>8</pointer>
+ <size_t>4</size_t>
+ <wchar_t>4</wchar_t>
+ </sizeof>
+</platform>
diff --git a/xen/tools/cppcheck-plat/x86_64-wchar_t2.xml b/xen/tools/cppcheck-plat/x86_64-wchar_t2.xml
new file mode 100644
index 000000000000..b2dc2fb2cc50
--- /dev/null
+++ b/xen/tools/cppcheck-plat/x86_64-wchar_t2.xml
@@ -0,0 +1,17 @@
+<?xml version="1.0"?>
+<platform>
+ <char_bit>8</char_bit>
+ <default-sign>unsigned</default-sign>
+ <sizeof>
+ <short>2</short>
+ <int>4</int>
+ <long>8</long>
+ <long-long>8</long-long>
+ <float>4</float>
+ <double>8</double>
+ <long-double>16</long-double>
+ <pointer>8</pointer>
+ <size_t>8</size_t>
+ <wchar_t>2</wchar_t>
+ </sizeof>
+</platform>
diff --git a/xen/tools/cppcheck-plat/x86_64-wchar_t4.xml b/xen/tools/cppcheck-plat/x86_64-wchar_t4.xml
new file mode 100644
index 000000000000..21d97b611505
--- /dev/null
+++ b/xen/tools/cppcheck-plat/x86_64-wchar_t4.xml
@@ -0,0 +1,17 @@
+<?xml version="1.0"?>
+<platform>
+ <char_bit>8</char_bit>
+ <default-sign>unsigned</default-sign>
+ <sizeof>
+ <short>2</short>
+ <int>4</int>
+ <long>8</long>
+ <long-long>8</long-long>
+ <float>4</float>
+ <double>8</double>
+ <long-double>16</long-double>
+ <pointer>8</pointer>
+ <size_t>8</size_t>
+ <wchar_t>4</wchar_t>
+ </sizeof>
+</platform>
diff --git a/xen/tools/cppcheck-txt-prepare.sh b/xen/tools/cppcheck-txt-prepare.sh
new file mode 100755
index 000000000000..a3fbaa150111
--- /dev/null
+++ b/xen/tools/cppcheck-txt-prepare.sh
@@ -0,0 +1,74 @@
+#!/usr/bin/env bash
+
+function help() {
+ cat <<EOF
+Usage: ${0} [OPTION] ...
+
+This script collects all the cppcheck text report fragment to create one report.
+
+Options:
+ --frag-ext= Extension of the report fragment to be collected
+ --outfile= Path and filename for the text report to be generated
+ --rel-path= Path to be removed from the path of the file in the report
+ -h, --help Print this help
+EOF
+}
+
+FRAG_EXT=""
+OUTFILE=""
+RELPATH=""
+
+for OPTION in "$@"
+do
+ case ${OPTION} in
+ -h|--help)
+ help
+ exit 0
+ ;;
+ --frag-ext=*)
+ FRAG_EXT="$(eval echo "${OPTION#*=}")"
+ ;;
+ --outfile=*)
+ OUTFILE="$(eval echo "${OPTION#*=}")"
+ ;;
+ --rel-path=*)
+ RELPATH="$(eval echo "${OPTION#*=}")"
+ ;;
+ *)
+ echo "Invalid option ${OPTION}"
+ exit 1
+ ;;
+ esac
+done
+
+if [ "${FRAG_EXT}" = "" ]
+then
+ echo "Please provide --frag-ext argument"
+ exit 1
+fi
+
+if [ "${OUTFILE}" = "" ]
+then
+ echo "Please provide --outfile argument"
+ exit 1
+fi
+
+rm -f "${OUTFILE}"
+
+FILE_LIST=$(find . -name "*${FRAG_EXT}" -print)
+
+for file in ${FILE_LIST}
+do
+ cat "${file}" >> "${OUTFILE}"
+done
+
+# Remove duplicates, some awk implementation doesn't have inplace change mode
+mv "${OUTFILE}" "${OUTFILE}_tmp"
+awk '/^\s*?$/||!seen[$0]++' "${OUTFILE}_tmp" > "${OUTFILE}"
+rm "${OUTFILE}_tmp"
+
+if [ -n "${RELPATH}" ]
+then
+ # Strip path
+ sed -i -re "s|${RELPATH}||g" "${OUTFILE}"
+fi
--
2.17.1
^ permalink raw reply related [flat|nested] 30+ messages in thread* [RFC PATCH 3/4] tools/misra: fix skipped rule numbers
2022-11-07 10:47 [RFC PATCH 0/4] Static analyser finding deviation Luca Fancellu
2022-11-07 10:47 ` [RFC PATCH 1/4] xen/Makefile: add analysis-coverity and analysis-eclair Luca Fancellu
2022-11-07 10:47 ` [RFC PATCH 2/4] xen/Makefile: add analysis-cppcheck and analysis-cppcheck-html Luca Fancellu
@ 2022-11-07 10:47 ` Luca Fancellu
2022-11-07 10:47 ` [RFC PATCH 4/4] xen: Justify linker script defined symbols in include/xen/kernel.h Luca Fancellu
3 siblings, 0 replies; 30+ messages in thread
From: Luca Fancellu @ 2022-11-07 10:47 UTC (permalink / raw)
To: xen-devel
Cc: bertrand.marquis, wei.chen, Andrew Cooper, George Dunlap,
Jan Beulich, Julien Grall, Stefano Stabellini, Wei Liu
Currently the script convert_misra_doc.py is using a loop through
range(1,22) to enumerate rules that needs to be skipped, however
range function does not include the stop counter in the enumeration
ending up into list rules until 21.21 instead of including rule 22.
Fix the issue using a dictionary that list the rules in misra c2012.
Fixes: 57caa5375321 ("xen: Add MISRA support to cppcheck make rule")
Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>
---
xen/tools/convert_misra_doc.py | 32 ++++++++++++++++++++++++++++++--
1 file changed, 30 insertions(+), 2 deletions(-)
diff --git a/xen/tools/convert_misra_doc.py b/xen/tools/convert_misra_doc.py
index caa4487f645f..13074d8a2e91 100755
--- a/xen/tools/convert_misra_doc.py
+++ b/xen/tools/convert_misra_doc.py
@@ -14,6 +14,34 @@ Usage:
import sys, getopt, re
+# MISRA rule are identified by two numbers, e.g. Rule 1.2, the main rule number
+# and a sub-number. This dictionary contains the number of the MISRA rule as key
+# and the maximum sub-number for that rule as value.
+misra_c2012_rules = {
+ 1:4,
+ 2:7,
+ 3:2,
+ 4:2,
+ 5:9,
+ 6:2,
+ 7:4,
+ 8:14,
+ 9:5,
+ 10:8,
+ 11:9,
+ 12:5,
+ 13:6,
+ 14:4,
+ 15:7,
+ 16:7,
+ 17:8,
+ 18:8,
+ 19:2,
+ 20:14,
+ 21:21,
+ 22:10
+}
+
def main(argv):
infile = ''
outfile = ''
@@ -142,8 +170,8 @@ def main(argv):
skip_list = []
# Search for missing rules and add a dummy text with the rule number
- for i in list(range(1,22)):
- for j in list(range(1,22)):
+ for i in misra_c2012_rules:
+ for j in list(range(1,misra_c2012_rules[i]+1)):
if str(i) + '.' + str(j) not in rule_list:
outstr.write('Rule ' + str(i) + '.' + str(j) + '\n')
outstr.write('No description for rule ' + str(i) + '.' + str(j)
--
2.17.1
^ permalink raw reply related [flat|nested] 30+ messages in thread* [RFC PATCH 4/4] xen: Justify linker script defined symbols in include/xen/kernel.h
2022-11-07 10:47 [RFC PATCH 0/4] Static analyser finding deviation Luca Fancellu
` (2 preceding siblings ...)
2022-11-07 10:47 ` [RFC PATCH 3/4] tools/misra: fix skipped rule numbers Luca Fancellu
@ 2022-11-07 10:47 ` Luca Fancellu
2022-11-07 11:49 ` Jan Beulich
3 siblings, 1 reply; 30+ messages in thread
From: Luca Fancellu @ 2022-11-07 10:47 UTC (permalink / raw)
To: xen-devel
Cc: bertrand.marquis, wei.chen, Andrew Cooper, George Dunlap,
Jan Beulich, Julien Grall, Stefano Stabellini, Wei Liu
Eclair and Coverity found violation of the MISRA rule 8.6 for the
symbols _start, _end, start, _stext, _etext, _srodata, _erodata,
_sinittext, _einittext which are declared in
xen/include/xen/kernel.h.
All those symbols are defined by the liker script so we can deviate
from the rule 8.6 for these cases.
Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>
---
docs/misra/safe.json | 9 +++++++++
xen/include/xen/kernel.h | 4 ++++
2 files changed, 13 insertions(+)
diff --git a/docs/misra/safe.json b/docs/misra/safe.json
index e079d3038120..e3c8a1d8eb36 100644
--- a/docs/misra/safe.json
+++ b/docs/misra/safe.json
@@ -3,6 +3,15 @@
"content": [
{
"id": "SAF-0-safe",
+ "analyser": {
+ "eclair": "MC3R1.R8.6",
+ "coverity": "misra_c_2012_rule_8_6_violation"
+ },
+ "name": "Rule 8.6: linker script defined symbols",
+ "text": "It is safe to declare this symbol because it is defined in the linker script."
+ },
+ {
+ "id": "SAF-1-safe",
"analyser": {},
"name": "Sentinel",
"text": "Next ID to be used"
diff --git a/xen/include/xen/kernel.h b/xen/include/xen/kernel.h
index 8cd142032d3b..efcd24b355d6 100644
--- a/xen/include/xen/kernel.h
+++ b/xen/include/xen/kernel.h
@@ -65,24 +65,28 @@
1; \
})
+/* SAF-0-safe R8.6 linker script defined symbols */
extern char _start[], _end[], start[];
#define is_kernel(p) ({ \
char *__p = (char *)(unsigned long)(p); \
(__p >= _start) && (__p < _end); \
})
+/* SAF-0-safe R8.6 linker script defined symbols */
extern char _stext[], _etext[];
#define is_kernel_text(p) ({ \
char *__p = (char *)(unsigned long)(p); \
(__p >= _stext) && (__p < _etext); \
})
+/* SAF-0-safe R8.6 linker script defined symbols */
extern const char _srodata[], _erodata[];
#define is_kernel_rodata(p) ({ \
const char *__p = (const char *)(unsigned long)(p); \
(__p >= _srodata) && (__p < _erodata); \
})
+/* SAF-0-safe R8.6 linker script defined symbols */
extern char _sinittext[], _einittext[];
#define is_kernel_inittext(p) ({ \
char *__p = (char *)(unsigned long)(p); \
--
2.17.1
^ permalink raw reply related [flat|nested] 30+ messages in thread* Re: [RFC PATCH 4/4] xen: Justify linker script defined symbols in include/xen/kernel.h
2022-11-07 10:47 ` [RFC PATCH 4/4] xen: Justify linker script defined symbols in include/xen/kernel.h Luca Fancellu
@ 2022-11-07 11:49 ` Jan Beulich
2022-11-07 11:53 ` Luca Fancellu
0 siblings, 1 reply; 30+ messages in thread
From: Jan Beulich @ 2022-11-07 11:49 UTC (permalink / raw)
To: Luca Fancellu
Cc: bertrand.marquis, wei.chen, Andrew Cooper, George Dunlap,
Julien Grall, Stefano Stabellini, Wei Liu, xen-devel
On 07.11.2022 11:47, Luca Fancellu wrote:
> --- a/xen/include/xen/kernel.h
> +++ b/xen/include/xen/kernel.h
> @@ -65,24 +65,28 @@
> 1; \
> })
>
> +/* SAF-0-safe R8.6 linker script defined symbols */
> extern char _start[], _end[], start[];
> #define is_kernel(p) ({ \
> char *__p = (char *)(unsigned long)(p); \
> (__p >= _start) && (__p < _end); \
> })
>
> +/* SAF-0-safe R8.6 linker script defined symbols */
> extern char _stext[], _etext[];
> #define is_kernel_text(p) ({ \
> char *__p = (char *)(unsigned long)(p); \
> (__p >= _stext) && (__p < _etext); \
> })
>
> +/* SAF-0-safe R8.6 linker script defined symbols */
> extern const char _srodata[], _erodata[];
> #define is_kernel_rodata(p) ({ \
> const char *__p = (const char *)(unsigned long)(p); \
> (__p >= _srodata) && (__p < _erodata); \
> })
>
> +/* SAF-0-safe R8.6 linker script defined symbols */
> extern char _sinittext[], _einittext[];
> #define is_kernel_inittext(p) ({ \
> char *__p = (char *)(unsigned long)(p); \
Why the "R8.6" everywhere here? Didn't we agree that the in-code
comments should be tool-agnostic?
Jan
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [RFC PATCH 4/4] xen: Justify linker script defined symbols in include/xen/kernel.h
2022-11-07 11:49 ` Jan Beulich
@ 2022-11-07 11:53 ` Luca Fancellu
2022-11-07 12:56 ` Jan Beulich
0 siblings, 1 reply; 30+ messages in thread
From: Luca Fancellu @ 2022-11-07 11:53 UTC (permalink / raw)
To: Jan Beulich
Cc: Bertrand Marquis, Wei Chen, Andrew Cooper, George Dunlap,
Julien Grall, Stefano Stabellini, Wei Liu, xen-devel
> On 7 Nov 2022, at 11:49, Jan Beulich <jbeulich@suse.com> wrote:
>
> On 07.11.2022 11:47, Luca Fancellu wrote:
>> --- a/xen/include/xen/kernel.h
>> +++ b/xen/include/xen/kernel.h
>> @@ -65,24 +65,28 @@
>> 1; \
>> })
>>
>> +/* SAF-0-safe R8.6 linker script defined symbols */
>> extern char _start[], _end[], start[];
>> #define is_kernel(p) ({ \
>> char *__p = (char *)(unsigned long)(p); \
>> (__p >= _start) && (__p < _end); \
>> })
>>
>> +/* SAF-0-safe R8.6 linker script defined symbols */
>> extern char _stext[], _etext[];
>> #define is_kernel_text(p) ({ \
>> char *__p = (char *)(unsigned long)(p); \
>> (__p >= _stext) && (__p < _etext); \
>> })
>>
>> +/* SAF-0-safe R8.6 linker script defined symbols */
>> extern const char _srodata[], _erodata[];
>> #define is_kernel_rodata(p) ({ \
>> const char *__p = (const char *)(unsigned long)(p); \
>> (__p >= _srodata) && (__p < _erodata); \
>> })
>>
>> +/* SAF-0-safe R8.6 linker script defined symbols */
>> extern char _sinittext[], _einittext[];
>> #define is_kernel_inittext(p) ({ \
>> char *__p = (char *)(unsigned long)(p); \
>
Hi Jan,
> Why the "R8.6" everywhere here? Didn't we agree that the in-code
> comments should be tool-agnostic?
The R8.6 is not tool specific, it is to give the quick hint that we are deviating
from MISRA Rule 8.6.
>
> Jan
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [RFC PATCH 4/4] xen: Justify linker script defined symbols in include/xen/kernel.h
2022-11-07 11:53 ` Luca Fancellu
@ 2022-11-07 12:56 ` Jan Beulich
2022-11-07 19:06 ` Julien Grall
0 siblings, 1 reply; 30+ messages in thread
From: Jan Beulich @ 2022-11-07 12:56 UTC (permalink / raw)
To: Luca Fancellu
Cc: Bertrand Marquis, Wei Chen, Andrew Cooper, George Dunlap,
Julien Grall, Stefano Stabellini, Wei Liu, xen-devel
On 07.11.2022 12:53, Luca Fancellu wrote:
>> On 7 Nov 2022, at 11:49, Jan Beulich <jbeulich@suse.com> wrote:
>> On 07.11.2022 11:47, Luca Fancellu wrote:
>>> --- a/xen/include/xen/kernel.h
>>> +++ b/xen/include/xen/kernel.h
>>> @@ -65,24 +65,28 @@
>>> 1; \
>>> })
>>>
>>> +/* SAF-0-safe R8.6 linker script defined symbols */
>>> extern char _start[], _end[], start[];
>>> #define is_kernel(p) ({ \
>>> char *__p = (char *)(unsigned long)(p); \
>>> (__p >= _start) && (__p < _end); \
>>> })
>>>
>>> +/* SAF-0-safe R8.6 linker script defined symbols */
>>> extern char _stext[], _etext[];
>>> #define is_kernel_text(p) ({ \
>>> char *__p = (char *)(unsigned long)(p); \
>>> (__p >= _stext) && (__p < _etext); \
>>> })
>>>
>>> +/* SAF-0-safe R8.6 linker script defined symbols */
>>> extern const char _srodata[], _erodata[];
>>> #define is_kernel_rodata(p) ({ \
>>> const char *__p = (const char *)(unsigned long)(p); \
>>> (__p >= _srodata) && (__p < _erodata); \
>>> })
>>>
>>> +/* SAF-0-safe R8.6 linker script defined symbols */
>>> extern char _sinittext[], _einittext[];
>>> #define is_kernel_inittext(p) ({ \
>>> char *__p = (char *)(unsigned long)(p); \
>>
>> Why the "R8.6" everywhere here? Didn't we agree that the in-code
>> comments should be tool-agnostic?
>
> The R8.6 is not tool specific, it is to give the quick hint that we are deviating
> from MISRA Rule 8.6.
Well, yes, "tool" was wrong for me to write. Imo references to a specific
spec should equally be avoided in in-code comments, as other specs may
turn up.
Jan
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [RFC PATCH 4/4] xen: Justify linker script defined symbols in include/xen/kernel.h
2022-11-07 12:56 ` Jan Beulich
@ 2022-11-07 19:06 ` Julien Grall
2022-11-08 11:00 ` Luca Fancellu
0 siblings, 1 reply; 30+ messages in thread
From: Julien Grall @ 2022-11-07 19:06 UTC (permalink / raw)
To: Jan Beulich, Luca Fancellu
Cc: Bertrand Marquis, Wei Chen, Andrew Cooper, George Dunlap,
Stefano Stabellini, Wei Liu, xen-devel
On 07/11/2022 12:56, Jan Beulich wrote:
> On 07.11.2022 12:53, Luca Fancellu wrote:
>>> On 7 Nov 2022, at 11:49, Jan Beulich <jbeulich@suse.com> wrote:
>>> On 07.11.2022 11:47, Luca Fancellu wrote:
>>>> --- a/xen/include/xen/kernel.h
>>>> +++ b/xen/include/xen/kernel.h
>>>> @@ -65,24 +65,28 @@
>>>> 1; \
>>>> })
>>>>
>>>> +/* SAF-0-safe R8.6 linker script defined symbols */
>>>> extern char _start[], _end[], start[];
>>>> #define is_kernel(p) ({ \
>>>> char *__p = (char *)(unsigned long)(p); \
>>>> (__p >= _start) && (__p < _end); \
>>>> })
>>>>
>>>> +/* SAF-0-safe R8.6 linker script defined symbols */
>>>> extern char _stext[], _etext[];
>>>> #define is_kernel_text(p) ({ \
>>>> char *__p = (char *)(unsigned long)(p); \
>>>> (__p >= _stext) && (__p < _etext); \
>>>> })
>>>>
>>>> +/* SAF-0-safe R8.6 linker script defined symbols */
>>>> extern const char _srodata[], _erodata[];
>>>> #define is_kernel_rodata(p) ({ \
>>>> const char *__p = (const char *)(unsigned long)(p); \
>>>> (__p >= _srodata) && (__p < _erodata); \
>>>> })
>>>>
>>>> +/* SAF-0-safe R8.6 linker script defined symbols */
>>>> extern char _sinittext[], _einittext[];
>>>> #define is_kernel_inittext(p) ({ \
>>>> char *__p = (char *)(unsigned long)(p); \
>>>
>>> Why the "R8.6" everywhere here? Didn't we agree that the in-code
>>> comments should be tool-agnostic?
>>
>> The R8.6 is not tool specific, it is to give the quick hint that we are deviating
>> from MISRA Rule 8.6.
>
> Well, yes, "tool" was wrong for me to write. Imo references to a specific
> spec should equally be avoided in in-code comments, as other specs may
> turn up.
+1. The comment duplication is not great and sometimes even a short
explanation it may not fit in 80 characters (AFAICT the justification
should be a one line comment).
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [RFC PATCH 4/4] xen: Justify linker script defined symbols in include/xen/kernel.h
2022-11-07 19:06 ` Julien Grall
@ 2022-11-08 11:00 ` Luca Fancellu
2022-11-08 11:32 ` Julien Grall
0 siblings, 1 reply; 30+ messages in thread
From: Luca Fancellu @ 2022-11-08 11:00 UTC (permalink / raw)
To: Julien Grall
Cc: Jan Beulich, Bertrand Marquis, Wei Chen, Andrew Cooper,
George Dunlap, Stefano Stabellini, Wei Liu, xen-devel
> On 7 Nov 2022, at 19:06, Julien Grall <julien@xen.org> wrote:
>
>
>
> On 07/11/2022 12:56, Jan Beulich wrote:
>> On 07.11.2022 12:53, Luca Fancellu wrote:
>>>> On 7 Nov 2022, at 11:49, Jan Beulich <jbeulich@suse.com> wrote:
>>>> On 07.11.2022 11:47, Luca Fancellu wrote:
>>>>> --- a/xen/include/xen/kernel.h
>>>>> +++ b/xen/include/xen/kernel.h
>>>>> @@ -65,24 +65,28 @@
>>>>> 1; \
>>>>> })
>>>>>
>>>>> +/* SAF-0-safe R8.6 linker script defined symbols */
>>>>> extern char _start[], _end[], start[];
>>>>> #define is_kernel(p) ({ \
>>>>> char *__p = (char *)(unsigned long)(p); \
>>>>> (__p >= _start) && (__p < _end); \
>>>>> })
>>>>>
>>>>> +/* SAF-0-safe R8.6 linker script defined symbols */
>>>>> extern char _stext[], _etext[];
>>>>> #define is_kernel_text(p) ({ \
>>>>> char *__p = (char *)(unsigned long)(p); \
>>>>> (__p >= _stext) && (__p < _etext); \
>>>>> })
>>>>>
>>>>> +/* SAF-0-safe R8.6 linker script defined symbols */
>>>>> extern const char _srodata[], _erodata[];
>>>>> #define is_kernel_rodata(p) ({ \
>>>>> const char *__p = (const char *)(unsigned long)(p); \
>>>>> (__p >= _srodata) && (__p < _erodata); \
>>>>> })
>>>>>
>>>>> +/* SAF-0-safe R8.6 linker script defined symbols */
>>>>> extern char _sinittext[], _einittext[];
>>>>> #define is_kernel_inittext(p) ({ \
>>>>> char *__p = (char *)(unsigned long)(p); \
>>>>
>>>> Why the "R8.6" everywhere here? Didn't we agree that the in-code
>>>> comments should be tool-agnostic?
>>>
>>> The R8.6 is not tool specific, it is to give the quick hint that we are deviating
>>> from MISRA Rule 8.6.
>> Well, yes, "tool" was wrong for me to write. Imo references to a specific
>> spec should equally be avoided in in-code comments, as other specs may
>> turn up.
>
> +1. The comment duplication is not great and sometimes even a short explanation it may not fit in 80 characters (AFAICT the justification should be a one line comment).
>
Ok we can remove the R8.6 from the comments, is the remaining part ok?
> Cheers,
>
> --
> Julien Grall
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [RFC PATCH 4/4] xen: Justify linker script defined symbols in include/xen/kernel.h
2022-11-08 11:00 ` Luca Fancellu
@ 2022-11-08 11:32 ` Julien Grall
2022-11-08 11:55 ` Luca Fancellu
0 siblings, 1 reply; 30+ messages in thread
From: Julien Grall @ 2022-11-08 11:32 UTC (permalink / raw)
To: Luca Fancellu
Cc: Jan Beulich, Bertrand Marquis, Wei Chen, Andrew Cooper,
George Dunlap, Stefano Stabellini, Wei Liu, xen-devel
Hi Luca,
On 08/11/2022 11:00, Luca Fancellu wrote:
>
>
>> On 7 Nov 2022, at 19:06, Julien Grall <julien@xen.org> wrote:
>>
>>
>>
>> On 07/11/2022 12:56, Jan Beulich wrote:
>>> On 07.11.2022 12:53, Luca Fancellu wrote:
>>>>> On 7 Nov 2022, at 11:49, Jan Beulich <jbeulich@suse.com> wrote:
>>>>> On 07.11.2022 11:47, Luca Fancellu wrote:
>>>>>> --- a/xen/include/xen/kernel.h
>>>>>> +++ b/xen/include/xen/kernel.h
>>>>>> @@ -65,24 +65,28 @@
>>>>>> 1; \
>>>>>> })
>>>>>>
>>>>>> +/* SAF-0-safe R8.6 linker script defined symbols */
>>>>>> extern char _start[], _end[], start[];
>>>>>> #define is_kernel(p) ({ \
>>>>>> char *__p = (char *)(unsigned long)(p); \
>>>>>> (__p >= _start) && (__p < _end); \
>>>>>> })
>>>>>>
>>>>>> +/* SAF-0-safe R8.6 linker script defined symbols */
>>>>>> extern char _stext[], _etext[];
>>>>>> #define is_kernel_text(p) ({ \
>>>>>> char *__p = (char *)(unsigned long)(p); \
>>>>>> (__p >= _stext) && (__p < _etext); \
>>>>>> })
>>>>>>
>>>>>> +/* SAF-0-safe R8.6 linker script defined symbols */
>>>>>> extern const char _srodata[], _erodata[];
>>>>>> #define is_kernel_rodata(p) ({ \
>>>>>> const char *__p = (const char *)(unsigned long)(p); \
>>>>>> (__p >= _srodata) && (__p < _erodata); \
>>>>>> })
>>>>>>
>>>>>> +/* SAF-0-safe R8.6 linker script defined symbols */
>>>>>> extern char _sinittext[], _einittext[];
>>>>>> #define is_kernel_inittext(p) ({ \
>>>>>> char *__p = (char *)(unsigned long)(p); \
>>>>>
>>>>> Why the "R8.6" everywhere here? Didn't we agree that the in-code
>>>>> comments should be tool-agnostic?
>>>>
>>>> The R8.6 is not tool specific, it is to give the quick hint that we are deviating
>>>> from MISRA Rule 8.6.
>>> Well, yes, "tool" was wrong for me to write. Imo references to a specific
>>> spec should equally be avoided in in-code comments, as other specs may
>>> turn up.
>>
>> +1. The comment duplication is not great and sometimes even a short explanation it may not fit in 80 characters (AFAICT the justification should be a one line comment).
>>
>
> Ok we can remove the R8.6 from the comments, is the remaining part ok?
I am afraid no. The comment should only be /* SAF-0-safe */.
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [RFC PATCH 4/4] xen: Justify linker script defined symbols in include/xen/kernel.h
2022-11-08 11:32 ` Julien Grall
@ 2022-11-08 11:55 ` Luca Fancellu
0 siblings, 0 replies; 30+ messages in thread
From: Luca Fancellu @ 2022-11-08 11:55 UTC (permalink / raw)
To: Julien Grall
Cc: Jan Beulich, Bertrand Marquis, Wei Chen, Andrew Cooper,
George Dunlap, Stefano Stabellini, Wei Liu, xen-devel
> On 8 Nov 2022, at 11:32, Julien Grall <julien@xen.org> wrote:
>
> Hi Luca,
>
> On 08/11/2022 11:00, Luca Fancellu wrote:
>>> On 7 Nov 2022, at 19:06, Julien Grall <julien@xen.org> wrote:
>>>
>>>
>>>
>>> On 07/11/2022 12:56, Jan Beulich wrote:
>>>> On 07.11.2022 12:53, Luca Fancellu wrote:
>>>>>> On 7 Nov 2022, at 11:49, Jan Beulich <jbeulich@suse.com> wrote:
>>>>>> On 07.11.2022 11:47, Luca Fancellu wrote:
>>>>>>> --- a/xen/include/xen/kernel.h
>>>>>>> +++ b/xen/include/xen/kernel.h
>>>>>>> @@ -65,24 +65,28 @@
>>>>>>> 1; \
>>>>>>> })
>>>>>>>
>>>>>>> +/* SAF-0-safe R8.6 linker script defined symbols */
>>>>>>> extern char _start[], _end[], start[];
>>>>>>> #define is_kernel(p) ({ \
>>>>>>> char *__p = (char *)(unsigned long)(p); \
>>>>>>> (__p >= _start) && (__p < _end); \
>>>>>>> })
>>>>>>>
>>>>>>> +/* SAF-0-safe R8.6 linker script defined symbols */
>>>>>>> extern char _stext[], _etext[];
>>>>>>> #define is_kernel_text(p) ({ \
>>>>>>> char *__p = (char *)(unsigned long)(p); \
>>>>>>> (__p >= _stext) && (__p < _etext); \
>>>>>>> })
>>>>>>>
>>>>>>> +/* SAF-0-safe R8.6 linker script defined symbols */
>>>>>>> extern const char _srodata[], _erodata[];
>>>>>>> #define is_kernel_rodata(p) ({ \
>>>>>>> const char *__p = (const char *)(unsigned long)(p); \
>>>>>>> (__p >= _srodata) && (__p < _erodata); \
>>>>>>> })
>>>>>>>
>>>>>>> +/* SAF-0-safe R8.6 linker script defined symbols */
>>>>>>> extern char _sinittext[], _einittext[];
>>>>>>> #define is_kernel_inittext(p) ({ \
>>>>>>> char *__p = (char *)(unsigned long)(p); \
>>>>>>
>>>>>> Why the "R8.6" everywhere here? Didn't we agree that the in-code
>>>>>> comments should be tool-agnostic?
>>>>>
>>>>> The R8.6 is not tool specific, it is to give the quick hint that we are deviating
>>>>> from MISRA Rule 8.6.
>>>> Well, yes, "tool" was wrong for me to write. Imo references to a specific
>>>> spec should equally be avoided in in-code comments, as other specs may
>>>> turn up.
>>>
>>> +1. The comment duplication is not great and sometimes even a short explanation it may not fit in 80 characters (AFAICT the justification should be a one line comment).
>>>
>> Ok we can remove the R8.6 from the comments, is the remaining part ok?
>
> I am afraid no. The comment should only be /* SAF-0-safe */.
Ok I will go for that in the next serie
>
> Cheers,
>
> --
> Julien Grall
^ permalink raw reply [flat|nested] 30+ messages in thread