QEMU tests, Coverity, and g_test_set_nonfatal_assertions()

QEMU tests, Coverity, and g_test_set_nonfatal_assertions()

[Peter Maydell]

Currently we generally assume that assertions are always present
and always fatal, and we tell Coverity this by putting this into
our model file:

void g_assertion_message_expr(const char     *domain,
                              const char     *file,
                              int             line,
                              const char     *func,
                              const char     *expr)
{
    __coverity_panic__();
}

However, this doesn't work for the tests, which use a variety
of other assertion macros like g_assert_cmpstr(), g_assert_true(),
etc, because those glib macros turn into calls to other functions:
 g_assertion_message
 g_assertion_message_cmpstr
 g_assertion_message_cmpnum
 g_assertion_message_error
which we don't model.

So, we could just add models of those four functions. However,
in some of our tests we call g_test_set_nonfatal_assertions()
(which does what it says on the tin for the g_assert_something
macros, but *not* for plain g_assert() or g_assert_not_reached()).
The rationale here is that non-fatal assertions for code in the
glib test framework allow failures to be reported cleanly as
"this test failed", whereas an abort will give less useful
information, eg it doesn't report the test failure name and
it doesn't proceed to do further tests in the same test binary:

**
ERROR:../../../tests/qtest/npcm7xx_rng-test.c:256:test_first_byte_runs:
assertion failed (calc_runs_p(buf.l, sizeof(buf) * BITS_PER_BYTE) >
0.01): (0.00204666737 > 0.01)
Bail out! ERROR:../../../tests/qtest/npcm7xx_rng-test.c:256:test_first_byte_runs:
assertion failed (calc_runs_p(buf.l, sizeof(buf) * BITS_PER_BYTE) >
0.01): (0.00204666737 > 0.01)
Aborted

vs

**
ERROR:../../../tests/qtest/npcm7xx_rng-test.c:232:test_first_byte_monobit:
assertion failed (calc_monobit_p(buf, sizeof(buf)) > 0.01):
(4.78548397e-05 > 0.01)
# ERROR:../../../tests/qtest/npcm7xx_rng-test.c:232:test_first_byte_monobit:
assertion failed (calc_monobit_p(buf, sizeof(buf)) > 0.01):
(4.78548397e-05 > 0.01)
not ok 5 /arm/npcm7xx_rng/first_byte/monobit
ok 6 /arm/npcm7xx_rng/first_byte/runs

Of course test code that opts into this needs to be prepared for
its g_assert_something() to return even on failure. But if we make
our Coverity model treat all these kinds of g_assert_something as
fatal-on-failure then we won't be able to use Coverity to identify
places in these tests that would accidentally crash on failure.

In summary, we have a few options:

(1) Expand "assertions always fatal" to test code, and add "panics"
models of the g_assertion_message* functions. Remove all the calls
to g_test_set_nonfatal_assertions().

(2) Aim to expand the ability to use g_test_set_nonfatal_assertions()
to other tests than the handful that currently use it (perhaps by
providing a standard place where it gets called for all tests, though
there isn't currently an obvious place to do that). Treat Coverity
issues in our test code which flag up "this would crash if the
assertion fired but execution continued" as bugs to be fixed (though
not very high-priority ones, obviously).

(3) Something else ?

I think I vaguely favour 2, though it is of course more work...
In any case, we need to make a decision so we can decide whether
the pile of coverity issues should be either dismissed as intentional
or gradually worked through and fixed.

thanks
-- PMM

Re: QEMU tests, Coverity, and g_test_set_nonfatal_assertions()

[Richard Henderson]

On 5/3/21 9:49 AM, Peter Maydell wrote:
> (1) Expand "assertions always fatal" to test code, and add "panics"
> models of the g_assertion_message* functions. Remove all the calls
> to g_test_set_nonfatal_assertions().

I vaguely prefer this.  To me, "assert" means can't continue.

If we want tests that report and continue, we should use something else. 
Though of course could mean quite a bit of churn.


r~


> 
> (2) Aim to expand the ability to use g_test_set_nonfatal_assertions()
> to other tests than the handful that currently use it (perhaps by
> providing a standard place where it gets called for all tests, though
> there isn't currently an obvious place to do that). Treat Coverity
> issues in our test code which flag up "this would crash if the
> assertion fired but execution continued" as bugs to be fixed (though
> not very high-priority ones, obviously).
> 
> (3) Something else ?
> 
> I think I vaguely favour 2, though it is of course more work...
> In any case, we need to make a decision so we can decide whether
> the pile of coverity issues should be either dismissed as intentional
> or gradually worked through and fixed.
> 
> thanks
> -- PMM
> 

Re: QEMU tests, Coverity, and g_test_set_nonfatal_assertions()

[Peter Maydell]

On Mon, 3 May 2021 at 18:15, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> On 5/3/21 9:49 AM, Peter Maydell wrote:
> > (1) Expand "assertions always fatal" to test code, and add "panics"
> > models of the g_assertion_message* functions. Remove all the calls
> > to g_test_set_nonfatal_assertions().
>
> I vaguely prefer this.  To me, "assert" means can't continue.
>
> If we want tests that report and continue, we should use something else.
> Though of course could mean quite a bit of churn.

The thing is that this is fighting against the glib test
framework's intent, which is that one uses g_assert_cmpstr() etc
(but not plain g_assert()) to do the "report and continue" part.
We could wrap them all in QEMU-specific versions that rename
them to something other than g_assert_* but I'm not sure that
that's a great idea.

thanks
-- PMM

Re: QEMU tests, Coverity, and g_test_set_nonfatal_assertions()

[Markus Armbruster]

Peter Maydell <peter.maydell@linaro.org> writes:

[...]

> In summary, we have a few options:
>
> (1) Expand "assertions always fatal" to test code, and add "panics"
> models of the g_assertion_message* functions. Remove all the calls
> to g_test_set_nonfatal_assertions().
>
> (2) Aim to expand the ability to use g_test_set_nonfatal_assertions()
> to other tests than the handful that currently use it (perhaps by
> providing a standard place where it gets called for all tests, though
> there isn't currently an obvious place to do that). Treat Coverity
> issues in our test code which flag up "this would crash if the
> assertion fired but execution continued" as bugs to be fixed (though
> not very high-priority ones, obviously).

Further discussed under Richard's reply.

> (3) Something else ?

We could try to model what the GLib functions do:
g_test_set_nonfatal_assertions() sets a global flag, the g_assert_FOO()
other than g_assert_not_reached() check it.  We'll then find out whether
Coverity's analysis is strong enough to propagate the value passed to
g_test_set_nonfatal_assertions() to its uses.

[...]