Re: [RFC PATCH 1/4] xen/Makefile: add analysis-coverity and analysis-eclair

[Jan Beulich <jbeulich@xxxxxxxx>]

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