Re: [PATCH COMMITTED] src: Fix NDEBUG compilation: avoid side-effects in assert

[Eric Blake]

On 2/27/20 1:13 AM, Dan Čermák wrote:
> Hi list,
>
> on a related note: how about re-defining `assert` to `(void)(cond)` for
> NDEBUG builds, so that we don't get a plethora of unused variable
> warnings now?

No, because then it's harder to copy-and-paste code from poke into other 
projects that DO use assert as defined by C.  Really, if it is a 
standard function, you should NOT abuse the semantics that people expect 
from the standard.  If we want to define a new macro, such as assure(), 
then our new macro has the freedom to have the semantics we want.

> Also, are NDEBUG builds actually a good idea? I recall that when fuzzing
> poke a lot of "crashes" that I found were just failed assertions. So
> maybe we should refrain from supporting NDEBUG builds for now?

I didn't actually test how far an NDEBUG build would get, only that I 
got rid of a lot of side effects (git grep 'assert.*(.*(' where the 
inner ( was non-trivial), because it is just bad coding practice to rely 
on side effects in assert.

> > +++ b/src/ios.c
> > @@ -220,12 +220,14 @@ void
> >   ios_close (ios io)
> >   {
> >     struct ios *tmp;
> > +  int r;
> >
> >     /* XXX: if not saved, ask before closing.  */
> >
> >     /* Close the device operated by the IO space.
> >        XXX: handle errors.  */
> > -  assert (io->dev_if->close (io->dev));
> > +  r = io->dev_if->close (io->dev);
> > +  assert (r);

Another way to do this that does not use temporary variables that we'd 
have to ifdef out under NDEBUG would be:

if (! io->dev_if->close (io->dev))
  assert (0);

I can make that switch if you'd like (and undo the addition of all my 
temporaries).


--
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org