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

[Shawn Landden]

On Tue, Mar 31, 2015 at 8:38 AM, Djalal Harouni <tixxdz at opendz.org> wrote:
> On Mon, Mar 30, 2015 at 07:32:35PM -0700, Shawn Landden wrote:
>> On Mon, Mar 30, 2015 at 5:04 PM, Djalal Harouni <tixxdz at opendz.org> wrote:
>> > On Fri, Mar 27, 2015 at 09:51:26AM -0700, Shawn Landden wrote:
>> >> On Fri, Mar 27, 2015 at 8:16 AM, Tom Gundersen <teg at jklm.no> wrote:
>> > [...]
>> >> >> * 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.
>> >> >
>> >> > 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.
>> >> >
>> >> > 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);
>> >> Yes. The use case is that embedded people don't want all the strings
>> >> that the logging adds to their binaries when these are ASSERTS, they
>> >> are only expected to catch programming errors. It is not to make it
>> >> faster, I think that is negligible.
>> > Hmm embedded cases are real, I had to deal with some in the past. But
>> > not sure here since I didn't see any numbers before/after stripping, but
>> > perhaps you can start by updating the callers and their semantics if you
>> > think that the code there is robust and it's worth it... ?
>> How about we leave the abort() and just drop the logging with NDEBUG set?
> Not sure if this is the best solution, you will just hide the path of
> code where we failed! it will not be easy to debug later.
>
> As said, the callers except to have a log error and abort, if you remove
> one of them you are altering callers, IMO they do not handle errors and
> they do not log errors, they rely on assert_se(). As you know in systemd
> we do log errors...
>
>> I think the performance impact is negligible, I just want to drop all
>> the strings.
>> Perhaps we could replace the logging with backtrace() in this case?
> Don't know are you sure that if you add a backrack() routine there it
> will be better than the current assert_se(), actually without real
> numbers it is hard to say...
>
> IMO we should not touch this macro, the fix should be in the callers, so
> you have to make sure that callers are robust and can handle errors
> correctly (log them too perhaps)...
>
The point is that assert() and assert_se() should only be used for
programming errors,
this is covered in CODING_STYLE.
>
> --
> 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