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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 08.11.2022 18:13, Luca Fancellu wrote:
>> On 8 Nov 2022, at 15:49, Jan Beulich <jbeulich@xxxxxxxx> wrote:
>> 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.
> 
> Is a comment before the line ok as documentation? To state that —include and
> -R are not standard options so analysis-{coverity,eclair} will not work 
> without a
> grep that takes those parameters?

A comment _might_ be okay. Is there no other documentation on how these
goals are to be used? The main question here is how impacting this might
be to the various environments we allow Xen to be built in: Would at
least modern versions of all Linux distros we care about allow using
these rules? What about non-Linux?

And could you at least bail when PARSE_FILE_LIST ends up empty, with a
clear error message augmenting the one grep would have issued?

>> 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.
> 
> Ok I’ve removed the escape from the * and also from slashes:
> 
> PARSE_FILE_LIST := $(addsuffix .safparse,$(shell grep -ERl --include='*.h' \
>     --include='*.c' '^[[:blank:]]*/\*[[:space:]]+SAF-.*\*/$$' $(srctree)))

Good - seeing things more clearly now my next question is: Isn't
matching just "/* SAF-...*/" a little too lax? And is there really a
need to permit leading blanks?

>>>>>>> +       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.
> 
> Ok, I have that eclair and coverity shares the same commands to be executed 
> by the build system,
> so instead of duplicating the targets for coverity and eclair and their 
> recipe, I’ve used the pattern rule
> to have that these rules:
> 
> JUSTIFICATION_FILES := $(XEN_ROOT)/docs/misra/safe.json \
>                        $(XEN_ROOT)/docs/misra/false-positive-$$*.json
> 
> […]
> 
> .SECONDEXPANSION:
> $(objtree)/%.sed: $(srctree)/tools/xenfusa-gen-tags.py $(JUSTIFICATION_FILES)
>     […]
> 
> […]
> 
> analysis-parse-tags-%: $(PARSE_FILE_LIST) $(objtree)/%.sed
>     […]
> 
> analysis-build-%: analysis-parse-tags-%
>     $(MAKE) O=$(abs_objtree) -f $(srctree)/Makefile build
> 
> analysis-clean:
>    […]
> 
> _analysis-%: analysis-build-%
>     $(Q)$(MAKE) O=$(abs_objtree) -f $(srctree)/Makefile analysis-clean
> 
> Matches the case where 'make analysis-coverity’ or ‘make analysis-eclair’ is 
> called.
> 
> Now, please correct me if my assumption on the way make works are wrong, here 
> my assumptions:
> 
> For example when ‘make analysis-coverity’ is called we have that this rule is 
> the best match for the
> called target:
> 
> _analysis-%:

So my main oversight was your addition to main-targets, which makes the
connection with this underscore-prefixed goal.

As to you saying "best match" - I didn't think make had such a concept
when it comes to considering pattern rules. Aiui it is "first match", in
the order that rules were parsed from all involved makefiles.

> So anything after _analysis- will be captured with % and this will be 
> transferred to the dependency
> of the target that is analysis-build-% -> analysis-build-coverity
> 
> Now analysis-build-coverity will be called, the best match is 
> analysis-build-%, so again the dependency
> which is analysis-parse-tags-%, will be translated to 
> analysis-parse-tags-coverity.
> 
> Now analysis-parse-tags-coverity will be called, the best match is 
> analysis-parse-tags-%, so the % will
> Have the ‘coverity’ value and in the dependency we will have $(objtree)/%.sed 
> -> $(objtree)/coverity.sed.
> 
> Looking for $(objtree)/coverity.sed the best match is $(objtree)/%.sed, which 
> will have $(JUSTIFICATION_FILES)
> and the python script in the dependency, here we will use the second 
> expansion to solve
> $(XEN_ROOT)/docs/misra/false-positive-$$*.json in 
> $(XEN_ROOT)/docs/misra/false-positive-coverity.json
> 
> So now after analysis-parse-tags-coverity has ended its dependency it will 
> start with its recipe, after it finishes,
> the recipe of analysis-build-coverity will start and it will call make to 
> actually build Xen.

Okay, I see now - this building of Xen really _is_ independent of the
checker chosen. I'm not sure though whether it is a good idea to
integrate all this, including ...

> After the build finishes, if the status is good, the analysis-build-coverity 
> has finished and the _analysis-coverity
> recipe can now run, it will call make with the analysis-clean target, 
> restoring any <file>.{c,h}.safparse to <file>.{c,h}.

... the subsequent cleaning. The state of the _source_ tree after a
build failure would be different from that after a successful build.
Personally I consider this at best surprising.

I wonder whether instead there could be a shell(?) script driving a
sequence of make invocations, leaving the new make goals all be self-
contained. Such a script could revert the source tree to its original
state even upon build failure by default, with an option allowing to
suppress this behavior.

Jan