[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 2:40 PM, Djalal Harouni <tixxdz at opendz.org> wrote:
> 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.
Well the patch I last submitted still calls abort() (did you see that
patch?). but yeah I
don't have strong opinions about this....
>
> Thanks!
>
> --
> Djalal Harouni
> _______________________________________________
> systemd-devel mailing list
> systemd-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/systemd-devel



-- 
Shawn Landden