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

[John Ferlan]

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

John