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


> So in the NDEBUG case "assert_se(foo())" becomes equivalent to
> "foo()", which I guess makes sense (though I doubt it makes much of a
> difference).
> 
> -t
> 
> >> ---
> >>  src/shared/macro.h | 12 ++++++------
> >>  1 file changed, 6 insertions(+), 6 deletions(-)
> >>
> >> diff --git a/src/shared/macro.h b/src/shared/macro.h
> >> index 7f89951..02219ea 100644
> >> --- a/src/shared/macro.h
> >> +++ b/src/shared/macro.h
> >> @@ -212,17 +212,17 @@ static inline unsigned long ALIGN_POWER2(unsigned long u) {
> >>                  (__x / __y + !!(__x % __y));                            \
> >>          })
> >>
> >> -#define assert_se(expr)                                                 \
> >> -        do {                                                            \
> >> -                if (_unlikely_(!(expr)))                                \
> >> -                        log_assert_failed(#expr, __FILE__, __LINE__, __PRETTY_FUNCTION__); \
> >> -        } while (false)                                                 \
> >> -
> >>  /* We override the glibc assert() here. */
> >>  #undef assert
> >>  #ifdef NDEBUG
> >> +#define assert_se(expr) do {expr} while(false)
> >>  #define assert(expr) do {} while(false)
> >>  #else
> >> +#define assert_se(expr)                                                 \
> >> +        do {                                                            \
> >> +                if (_unlikely_(!(expr)))                                \
> >> +                        log_assert_failed(#expr, __FILE__, __LINE__, __PRETTY_FUNCTION__); \
> >> +        } while (false)
> >>  #define assert(expr) assert_se(expr)
> >>  #endif
> >>
> >> --
> >> 2.2.1.209.g41e5f3a
> >>
> >> _______________________________________________
> >> systemd-devel mailing list
> >> systemd-devel at lists.freedesktop.org
> >> http://lists.freedesktop.org/mailman/listinfo/systemd-devel
> >
> >
> > Lennart
> >
> > --
> > Lennart Poettering, Red Hat
> > _______________________________________________
> > systemd-devel mailing list
> > systemd-devel at lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/systemd-devel
> _______________________________________________
> systemd-devel mailing list
> systemd-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/systemd-devel

-- 
Djalal Harouni
http://opendz.org