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

[Luca Fancellu <Luca.Fancellu@xxxxxxx>]

> On 8 Nov 2022, at 11:48, Jan Beulich <jbeulich@xxxxxxxx> 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