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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 14.11.2022 13:30, Luca Fancellu wrote:
>> 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:
>>>> 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>

Simply run "sed" over the result?

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

Which merely makes for a more involved (shell) script.

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

Well, of course the suggested script would need to be passed overrides you'd
otherwise pass with "make build" or alike.

Jan