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

[Luca Fancellu <Luca.Fancellu@xxxxxxx>]

> 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?

> 
> 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)))

> 
>>>>>> +        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 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.

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}.

We will have the same with ‘make analysis-eclair’, if we do a mistake typing, 
like ‘make analysis-coveri’, we will
have:

make: Entering directory ‘/path/to/xen/xen'
make: *** No rule to make target 'analysis-coveri'.  Stop.
make: Leaving directory '/path/to/xen/xen'



> 
>>>> 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).

_analysis-clean will not work, neither _analysis-abc, because of what I wrote 
above.
analysis-clean instead is called from the recipe of _analysis-% if all its 
dependency are
built correctly, otherwise it’s the user that needs to call it directly by 
doing “make analysis-clean”.



> 
> Jan