[systemd-devel] [PATCH 2/2] macro: allow assert_se() assertions to also be optimized out when NDEBUG is set

[Djalal Harouni]

On Fri, Mar 27, 2015 at 04:16:55PM +0100, Tom Gundersen wrote:
> On Fri, Mar 27, 2015 at 2:04 PM, Djalal Harouni <tixxdz at opendz.org> wrote:
> > Hi Shawn,
> >
> > On Thu, Mar 26, 2015 at 11:21:54PM -0700, Shawn Landden wrote:
> >> On Thu, Mar 26, 2015 at 5:47 PM, Djalal Harouni <tixxdz at opendz.org> wrote:
> >> > On Fri, Mar 27, 2015 at 12:30:53AM +0100, Tom Gundersen wrote:
> >> >> On Thu, Mar 26, 2015 at 9:19 AM, Lennart Poettering
> >> >> <lennart at poettering.net> wrote:
> >> >> > On Tue, 24.03.15 11:11, Shawn Landden (shawn at churchofgit.com) wrote:
> >> >> >
> >> >> >> Will result in slightly smaller binaries, and cuts out the branch, even if
> >> >> >> the expression is still executed.
> >> >> >
> >> >> > I am sorry, but the whole point of assert_se() is that it has side
> >> >> > effects. That's why it is called that way (the "se" suffix stands for
> >> >> > "side effects"), and is different from assert(). You are supposed to
> >> >> > use it whenever the code enclosed in it shall not be suppressed if
> >> >> > NDEBUG is defined. This patch of yours breaks that whole logic!
> >> >>
> >> >> Hm, am I reading the patch wrong? It looks good to me. It is simply
> >> >> dropping the branching and logging in the NDEBUG case, but the
> >> >> expression itself is still evaluated.
> >> > Yep but it seems that abort() will never be called,
> >> > log_assert_failed() => abort()
> >> >
> >> > And the logging mechanism is also one of those side effects. IMO unless
> >> > there are real valid reasons for any change to these macors... changing
> >> > anything here will just bring more bugs that we may never notice.
> >> >
> >> Those are the side effects of assert(), the side effects of the
> >> expression are still evaluated.
> > Yes but there are also the following points:
> > * assert() is simple, assert_se() is more complex and it may modify other
> > global sate. I don't think that we can break down side effects of
> > assert_se() into:
> >  1) side effects of assert()
> >  2) side effects of other expressions.
> >
> > And then remove that part 1)
> >
> >
> > So given the fact that asser_se() do not depend on NDEBUG, did you
> > consider the following:
> >
> > * You don't have control over what the expression do, it may just call
> >   abort() or exit() in case of fatal errors, so even if you remove the
> >   explicit abort() call, expressions may just call it.
> 
> If expr() aborts or exits, then it really does not matter whether we
> abort() explicitly or not, so I don't see what this has to do with
> this patch. Sure, even with NDEBUG the program may abort if that is
> called explicitly...
Hmm If expr() aborts or do some other complex things on its own then it
should not be used with assert(), right ? that comment was in regard of
this.

Since it aborts this is a side effect, and perhaps followup instructions
depend on the state if it is correct or not otherwise things will break
later. If you use assert() here and if NDEBUG is set, assert will just be
a nop and everything should continue to work.

The patch seems simple but it is introducing *new* semantics for
assert_se() if NDEBUG is set that current callers do not expect.


> > * Some expressions were perhaps depending on the logging mechanism which
> >   is part of the side effect. Callers to assert_se() are perhaps using
> >   it for a reason, are you sure that you are not introducing a
> >   regression here ?! it will be difficult to debug the error if we don't
> >   have logs.
> 
> As far as I can tell all the relevant logging functions are pure. They
> should not interact with global state, but maybe I'm missing
> something?
Hmm, we are opening, closing and logging to file descriptors and virtual
logging devices...

And callers expect to find a log message why the function failed.

> > * Current expression may modify/interact with a global state which may
> >   cause a fatal error, and if the caller wants to know if that failed,
> >   then abort(), your patch just introduced a regression, without the
> >   explicit abort(), all callers now have to call abort().
> 
> Yeah, this is the point I think. I still think the patch makes sense
> though, it really don't see why there should be a distinction between
> assert_se() and assert() when it comes to logging and aborting.
Yes, perhaps if this was the case from the start! but currently
assert_se() depends on the aborting mechanism which is not the case of
assert().


> If assert_se() fails it indicates we may have messed up the global
> state, and if assert() fails it indicates that the global state may be
> messed up (by someone else). Either way the global state is
> potentially messed up and not logging/aborting seems as (un)safe in
> both situations.
Indeed, yes.


> Another way of seeing it, intuitively I don't see why we should
> distinguish between
> 
> assert_se(foo() >= 0);
> 
> and
> 
> r = foo();
> assert(r >= 0); => nop
Yes sure, but since assert_se() did not interact with NDEBUG, to do this
you have to modify _all_ callers, the second assert() will be a nop.

Thanks!

-- 
Djalal Harouni
http://opendz.org