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

[Luca Fancellu <Luca.Fancellu@xxxxxxx>]

> On 14 Nov 2022, at 07:30, Jan Beulich <jbeulich@xxxxxxxx> wrote:
> 
> On 11.11.2022 21:52, Stefano Stabellini wrote:
>> On Fri, 11 Nov 2022, Jan Beulich wrote:
>>> On 11.11.2022 11:42, Luca Fancellu wrote:
>>>>> On 9 Nov 2022, at 10:36, Jan Beulich <jbeulich@xxxxxxxx> wrote:
>>>>> On 09.11.2022 11:08, Luca Fancellu wrote:
>>>>>>>> On 07.11.2022 11:47, Luca Fancellu wrote:
>>>>>>>> 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.
>>>>>> 
>>>>>> Instead of adding another tool, so another layer to the overall system, 
>>>>>> I would be more willing to add documentation
>>>>>> about this process, explaining how to use the analysis-* build targets, 
>>>>>> what to expect after a successful run and what
>>>>>> to expect after a failure.
>>>>>> 
>>>>>> What do you think?
>>>>> 
>>>>> Personally I'd prefer make goals to behave as such, with no surprises.
>>>> 
>>>> The analysis-* goal requires a build step, otherwise no analysis can be 
>>>> performed by the analysis tools, so I hope we agree
>>>> we need to integrate that step as a dependency of the analysis-*.
>>> 
>>> No, I'm afraid we don't agree. But like said for another piece we didn't
>>> initially agree on - if others think what you propose is fine, so be it.
>>> I'm specifically adding Anthony to Cc, as he's been working on make rules
>>> the most of all of us in the recent past.
>>> 
>>>> I understand that the analysis-clean might be a “surprise” if not well 
>>>> documented, this comes from the need to substitute the
>>>> tags in the tree (to keep the real path in the report log) and to revert 
>>>> them back at the end of the analysis.
>>>> 
>>>> So, such script should just mask to the user the analysis-clean invocation 
>>>> in case of errors (with an option to don’t do that)?
>>> 
>>> Hmm, here you're saying "such script", which looks to not fit with the
>>> earlier part of your reply above. (Just in case that's what I was to read
>>> out of this: I wouldn't see value in a script which existed _solely_ to
>>> make the cleaning conditional.)
>>> 
>>> Did you consider the alternative approach of copying the tree, altering
>>> it (while or after copying), running the build there, pulling out the
>>> result files, and delete the entire copy? Such a model would likely get
>>> away without introducing surprising make rules.

This approach does not work because the report will contain a path that is 
different from the source path and
some web based tools won’t be able to track back the origin of the finding.

e.g. /path/to/xen/arch/arm/<file> is the original file, we run the analysis on 
/path/to2/xen/arch/arm/<file>,
the finding is in /path/to2/xen/arch/arm/<file> but the source repository 
contains only /path/to/xen/arch/arm/<file>

>> 
>> Another, maybe simpler idea: what if the build step is not a dependency
>> of the analysis-* goals?
>> 
>> Basically, the user is supposed to:
>> 
>> 1) call analysis-parse-tags-*
>> 2) build Xen (in any way they like)
>> 3) call analysis-clean
> 
> Well, that's exactly what I've been proposing, with the (optional)
> addition of a small (shell) script doing all of the three for ...
> 
>> Making steps 1-3 into a single step is slightly more convenient for the
>> user but the downside is that dealing with build errors becomes
>> problematic.
>> 
>> On the other hand, if we let the user call steps 1-3 by hand
>> individually, it is slightly less convenient for the user but they can
>> more easily deal with any build error and sophisticated build
>> configurations.
> 
> ... convenience.

For coverity and eclair, it makes sense, these tools doesn’t require much 
effort to be integrated,
they are built to intercept files, compilers, environment variables during the 
make run in a
transparent way.

So the workflow is:

1) call analysis-parse-tags-*
2) build Xen (in any way they like)
3) call analysis-clean


If we think about cppcheck however, here the story changes, as it requires all 
these information
to be given as inputs, we have to do all the work the commercial tools do under 
the hood.

The cppcheck workflow instead is:

1) call analysis-parse-tags-cppcheck
2) generate cppcheck suppression list
3) build Xen (and run cppcheck on built source files)
4) collect and generate report
5) call analysis-clean

So let’s think about detaching the build stage from the previous stages, I 
think it is not very convenient
for the user, as during cppcheck analysis we build 
$(objtree)/include/generated/compiler-def.h, we build 
$(objtree)/suppression-list.txt, so the user needs to build Xen where those 
files are created
(in-tree or out-of-tree) otherwise the analysis won’t work and that’s the first 
user requirement (stage #3).

The most critical input to cppcheck is Xen’s $(CC), it comes from the build 
system in this serie, the user would
need to pass the correct one to cppcheck wrapper, together with cppcheck flags, 
and pass to Xen build stage #3
the wrapper as CC, second user requirement.

After the analysis, the user needs to run some scripts to put together the 
cppcheck report fragments
after its analysis, this step requires also the knowledge of were Xen is built, 
in-tree or out-of-tree, so
here the third user requirement (similar to the first one, but the stage is #4).

In the end, we can see the user would not be able to call individually the 
targets if it is not mastering
the system, it’s too complex to have something working, we could create a 
script to handle these requirements,
but it would be complex as it would do the job of the make system, plus it 
needs to forward additional make arguments
to it as well (CROSS_COMPILE, XEN_TARGET_ARCH, in-tree or Out-of-tree build, 
... for example).

In this thread the message is that in case of errors, there will be some 
artifacts (<file>.safparse, modified <file>)
and this is unexpected or surprising, but we are going to add a lot of 
complexity to handle something that needs
just documentation (in my opinion).

If the community don’t agree that documentation is enough, a solution could be 
to provide a script that in case of
errors, calls automatically the analysis-clean target, analysis-<tool> will 
call also the build step in this case,
here some pseudocode:

        #!/bin/bash
        set -e

        trap [call analysis-clean] EXIT

        [call analysis-<tool>]


This script needs however all the make arguments that we would have passed to 
make instead:

./script.sh --tool=<tool> [--dont-clean-on-err] -- CROSS_COMPILE=“[...]“ 
XEN_TARGET_ARCH=“[...]” [others...]




> 
> Jan