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

[Djalal Harouni]

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


-- 
Djalal Harouni
http://opendz.org