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

[Shawn Landden]

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.
>
>> 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
> _______________________________________________
> systemd-devel mailing list
> systemd-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/systemd-devel



-- 
Shawn Landden