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

[Luca Fancellu <Luca.Fancellu@xxxxxxx>]

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