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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 08.11.2022 15:00, Luca Fancellu wrote:
>> 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:
>>>>> @@ -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