[libvirt PATCH 1/2] src: rework static analysis detection

Fri Nov 20 16:05:05 UTC 2020

On Fri, Nov 20, 2020 at 10:31:54AM -0500, John Ferlan wrote:
> 
> 
> On 11/20/20 8:39 AM, Pavel Hrdina wrote:
> > On Fri, Nov 20, 2020 at 06:57:10AM -0500, John Ferlan wrote:
> >>
> >>
> >> On 11/16/20 10:36 AM, Pavel Hrdina wrote:
> >>> Inspired by QEMU code.
> >>>
> >>> Signed-off-by: Pavel Hrdina <phrdina at redhat.com>
> >>> ---
> >>>  meson.build    | 14 --------------
> >>>  src/internal.h |  4 ++++
> >>>  2 files changed, 4 insertions(+), 14 deletions(-)
> >>>
> >>> diff --git a/meson.build b/meson.build
> >>> index cecaad199d..dbbc9632f1 100644
> >>> --- a/meson.build
> >>> +++ b/meson.build
> >>> @@ -142,20 +142,6 @@ if get_option('test_coverage')
> >>>  endif
> >>>  
> >>>  
> >>> -# Detect when running under the clang static analyzer's scan-build driver
> >>> -# or Coverity-prevent's cov-build. Define STATIC_ANALYSIS accordingly.
> >>> -
> >>> -rc = run_command(
> >>> -  'sh', '-c',
> >>> -  'test -n "${CCC_ANALYZER_HTML}"' +
> >>> -    ' -o -n "${CCC_ANALYZER_ANALYSIS+set}"' +
> >>> -    ' -o -n "$COVERITY_BUILD_COMMAND$COVERITY_LD_PRELOAD"',
> >>> -)
> >>> -if rc.returncode() == 0
> >>> -  conf.set('STATIC_ANALYSIS', 1)
> >>> -endif
> >>> -
> >>> -
> >>>  # Add RPATH information when building for a non-standard prefix, or
> >>>  # when explicitly requested to do so
> >>>  
> >>> diff --git a/src/internal.h b/src/internal.h
> >>> index d167e56b48..5226667d3d 100644
> >>> --- a/src/internal.h
> >>> +++ b/src/internal.h
> >>> @@ -29,6 +29,10 @@
> >>>  #include <stdlib.h>
> >>>  #include "glibcompat.h"
> >>>  
> >>> +#if defined __clang_analyzer__ || defined __COVERITY__
> >>
> >> ^^ Bah humbug ... what defines __COVERITY__ then?
> >>
> >> In my Coverity environment, nothing... OK, sure I can add it, no problem
> >> but something in QEMU's coverity build environment must do that as it's
> >> not predefined as far as I can tell.
> > 
> > Hi John,
> > 
> > There is no need to add it anywhere. When building something using
> > coverity it is indeed not defined so GCC will ignore any code guarded
> > with __COVERITY__.
> > 
> > The __COVERITY__ is defined by cov-emit binary which is executed by
> > cov-build internally which creates the files that are later analyzed.
> > 
> > This is directly from cov-emit --help:
> > 
> >     Description
> > 
> >     The cov-emit command parses a source file and outputs it into a directory
> >     (emit repository) that can later be analyzed with cov-analyze. The
> >     cov-emit command is typically called by cov-translate, which is in turn
> >     typically called by cov-build (cov-emit is a low-level command and is not
> >     normally called directly). The cov-emit command defines the __COVERITY__
> >     preprocessor symbol.
> > 
> > Pavel
> > 
> 
> Well it didn't seem to get defined in my case; otherwise, I wouldn't
> have noted it here. ;-) Of course more research ends up figuring this
> out <read on if truly interested>.
> 
> Prior to this change, STATIC_ANALYSIS was defined in my <build-dir>/
> meson-config.h file because COVERITY_LD_PRELOAD is defined. After this
> change it is not defined there.
> 
> After this change, I got a build error because I have some extra
> sa_assert's in my local sources to avoid various Coverity warnings. One
> of those is in qemu_agent.c in the qemuAgentCommandFull where I had to add:
> 
> +    if (ret == 0)
> +        sa_assert(*reply);
> 
> just prior to the return. This resulted in the build error:
> 
> ../src/qemu/qemu_agent.c: In function ‘qemuAgentCommandFull’:
> ../src/qemu/qemu_agent.c:1137:26: error: suggest braces around empty
> body in an ‘if’ statement [-Werror=empty-body]
>  1137 |         sa_assert(*reply);
>       |                          ^
> cc1: all warnings being treated as errors
> [10/305] Compiling C object
> src/qemu/l..._driver_qemu_impl.a.p/qemu_hotplug.c.o
> ninja: build stopped: subcommand failed.
> 
> which implied to me that sa_assert wasn't defined as a result of a
> coding error if sa_assert wasn't defined.
> 
> FWIW:
> This was done to avoid callers complaining because *reply was set to
> NULL before a goto cleanup; and before being set to msg.rxObject.
> Coverity has a "hard time" deciding when one variable (in this case
> @ret) controls two conditions (in this case @*reply would be filled in).
> So in the caller it runs a pass where the return value would 0 and
> *reply = NULL - it's an impossible condition when you read the code.
> Hence I added the sa_assert, which interestingly enough in this case
> tripped because of missing { ... }, but that ended up being showing me
> that STATIC_ANALYSIS was not defined in my build env any more.
> 
> 
> What got a bit more confusing to me on this though is that if I fix my
> local patch to have the { }, then I don't get any Coverity warnings like
> I was getting when the sa_assert wasn't there. So that perhaps implies
> there's a pass running with *and* without STATIC_ANALYSIS being defined.
> If I look at the Coverity created build-log.txt I see two passes being made:
> 
> ...
> [1182705] EXECUTING: /bin/sh -c "gcc ... qemu_agent.c"
> ...
> [1182707] COMPILING: /opt/cov-analysis-linux64-2020.09/bin/cov-translate
> gcc ... qemu_agent.c
> ...
> 
> So I believe I have my answer <sigh>...

Well, that's exactly what I have wrote in my previous reply:

> There is no need to add it anywhere. When building something using
> coverity it is indeed not defined so GCC will ignore any code guarded
> with __COVERITY__.

The compilation using GCC doesn't have __COVERITY__ so it doesn't have
STATIC_ANALYSIS as well. And that is correct, there is no need to have
it defined at this point.

Before this change it was mandatory to run "meson build" using
"cov-build" as well in order to get the COVERITY_LD_PRELOAD defined to
detect running under coverity. After this change this is no longer
necessary.

I understand that this change probably complicates all the workarounds
and hacks that you have locally to silence coverity, but overall it
simplifies the static analysis detection and speeds up the coverity
build process as running "meson build" using "cov-build" is slow.

Pavel
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20201120/960ebcd6/attachment-0001.sig>