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

[Djalal Harouni]

On Tue, Mar 31, 2015 at 11:10:34AM -0700, Shawn Landden wrote:
> 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.
No, I don't see any mention of assert_se().


With this you are breaking the logic that commes with the following
commits:
0c0cdb06c139b52ff1
787784c4c1b24a132
e80afdb3e4a123
288c0991d5a0b240
b680a194bf9ed4c6
bdf7026e9557349c
8d95631ea6c039a60b

And the list is long... Please, check the code and the commit logs...

You are also hiding the error logs. A simple "grep -r "assert_se" src/"
will show you perhaps the ones that can be improved without affecting
the semantics of other callers.

You do realize that some of these are tests to make sure that systemd
works fine ? please read the commit logs, you will clearly see that the
authors do *not* want assert_se() to be optimized, and some authors will
even use assert_se() to handle errors in the core code.

IOW this patch will *weaken* all the test infrastructure and perhaps
other areas.

How tests are supposed to log errors after this ?


> I submitted a new version of this patch, with numbers (10kB smaller)
> to the mailing list.
Hmm thanks for the patch, this seems a valid number, but as I have said
I still think this is not the best approach.

You are affecting all the callers, if you want to do this then update
the parts that really need to be updated, the real callers, IMHO before
you update a function, first check callers.

So here if you think that the caller worth it and it is robust, then yeh
remove the assert_se() from there and make it handle the error correctly,
hmm but perhaps others may think differently...

I will let others comment.

Thanks!

-- 
Djalal Harouni