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

[Jan Beulich <jbeulich@xxxxxxxx>]

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