Re: [systemd-devel] [PATCH 2/2] macro: allow assert_se() assertions to also be optimized out when NDEBUG is set
On Tue, 24.03.15 11:11, Shawn Landden (sh...@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!
Lennart
> ---
> 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@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Lennart
--
Lennart Poettering, Red Hat
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [PATCH 2/2] macro: allow assert_se() assertions to also be optimized out when NDEBUG is set
On Thu, Mar 26, 2015 at 9:19 AM, Lennart Poettering
wrote:
> On Tue, 24.03.15 11:11, Shawn Landden (sh...@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.
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@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/systemd-devel
>
>
> Lennart
>
> --
> Lennart Poettering, Red Hat
> ___
> systemd-devel mailing list
> systemd-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/systemd-devel
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [PATCH 2/2] macro: allow assert_se() assertions to also be optimized out when NDEBUG is set
On Fri, Mar 27, 2015 at 12:30:53AM +0100, Tom Gundersen wrote:
> On Thu, Mar 26, 2015 at 9:19 AM, Lennart Poettering
> wrote:
> > On Tue, 24.03.15 11:11, Shawn Landden (sh...@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.
> 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@lists.freedesktop.org
> >> http://lists.freedesktop.org/mailman/listinfo/systemd-devel
> >
> >
> > Lennart
> >
> > --
> > Lennart Poettering, Red Hat
> > ___
> > systemd-devel mailing list
> > systemd-devel@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/systemd-devel
> ___
> systemd-devel mailing list
> systemd-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/systemd-devel
--
Djalal Harouni
http://opendz.org
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [PATCH 2/2] macro: allow assert_se() assertions to also be optimized out when NDEBUG is set
On Thu, Mar 26, 2015 at 1:19 AM, Lennart Poettering
wrote:
> On Tue, 24.03.15 11:11, Shawn Landden (sh...@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!
+#define assert_se(expr) do {expr} while(false)
#define assert(expr) do {} while(false)
No it does not. see "{expr}", it still is doing the side effect. It
just doesn't check if the return value is as expected.
>
> Lennart
>
>> ---
>> 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@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/systemd-devel
>
>
> Lennart
>
> --
> Lennart Poettering, Red Hat
> ___
> systemd-devel mailing list
> systemd-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/systemd-devel
--
Shawn Landden
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [PATCH 2/2] macro: allow assert_se() assertions to also be optimized out when NDEBUG is set
On Thu, Mar 26, 2015 at 5:47 PM, Djalal Harouni 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
>> wrote:
>> > On Tue, 24.03.15 11:11, Shawn Landden (sh...@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@lists.freedesktop.org
>> >> http://lists.freedesktop.org/mailman/listinfo/systemd-devel
>> >
>> >
>> > Lennart
>> >
>> > --
>> > Lennart Poettering, Red Hat
>> > ___
>> > systemd-devel mailing list
>> > systemd-devel@lists.freedesktop.org
>> > http://lists.freedesktop.org/mailman/listinfo/systemd-devel
>> ___
>> systemd-devel mailing list
>> systemd-devel@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/systemd-devel
>
> --
> Djalal Harouni
> http://opendz.org
> ___
> systemd-devel mailing list
> systemd-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/systemd-devel
--
Shawn Landden
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [PATCH 2/2] macro: allow assert_se() assertions to also be optimized out when NDEBUG is set
Hi Shawn, On Thu, Mar 26, 2015 at 11:21:54PM -0700, Shawn Landden wrote: > On Thu, Mar 26, 2015 at 5:47 PM, Djalal Harouni 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 > >> wrote: > >> > On Tue, 24.03.15 11:11, Shawn Landden (sh...@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. Yes but there are also the following points: * assert() is simple, assert_se() is more complex and it may modify other global sate. I don't think that we can break down side effects of assert_se() into: 1) side effects of assert() 2) side effects of other expressions. And then remove that part 1) So given the fact that asser_se() do not depend on NDEBUG, did you consider the following: * You don't have control over what the expression do, it may just call abort() or exit() in case of fatal errors, so even if you remove the explicit abort() call, expressions may just call it. * Some expressions were perhaps depending on the logging mechanism which is part of the side effect. Callers to assert_se() are perhaps using it for a reason, are you sure that you are not introducing a regression here ?! it will be difficult to debug the error if we don't have logs. * 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(). * Did you grep for assert_se() uses in the source ? I really do not think this is an improvement, we can't predict the semantics of expression here, perhaps we can with simple assert() but not with assert_se(). -- Djalal Harouni http://opendz.org ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [PATCH 2/2] macro: allow assert_se() assertions to also be optimized out when NDEBUG is set
On Fri, Mar 27, 2015 at 2:04 PM, Djalal Harouni wrote: > Hi Shawn, > > On Thu, Mar 26, 2015 at 11:21:54PM -0700, Shawn Landden wrote: >> On Thu, Mar 26, 2015 at 5:47 PM, Djalal Harouni 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 >> >> wrote: >> >> > On Tue, 24.03.15 11:11, Shawn Landden (sh...@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. > Yes but there are also the following points: > * assert() is simple, assert_se() is more complex and it may modify other > global sate. I don't think that we can break down side effects of > assert_se() into: > 1) side effects of assert() > 2) side effects of other expressions. > > And then remove that part 1) > > > So given the fact that asser_se() do not depend on NDEBUG, did you > consider the following: > > * You don't have control over what the expression do, it may just call > abort() or exit() in case of fatal errors, so even if you remove the > explicit abort() call, expressions may just call it. If expr() aborts or exits, then it really does not matter whether we abort() explicitly or not, so I don't see what this has to do with this patch. Sure, even with NDEBUG the program may abort if that is called explicitly... > * Some expressions were perhaps depending on the logging mechanism which > is part of the side effect. Callers to assert_se() are perhaps using > it for a reason, are you sure that you are not introducing a > regression here ?! it will be difficult to debug the error if we don't > have logs. As far as I can tell all the relevant logging functions are pure. They should not interact with global state, but maybe I'm missing something? > * 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); > * Did you grep for assert_se() uses in the source ? > > I really do not think this is an improvement, we can't predict the > semantics of expression here, perhaps we can with simple assert() but > not with assert_se(). > > -- > Djalal Harouni > http://opendz.org ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [PATCH 2/2] macro: allow assert_se() assertions to also be optimized out when NDEBUG is set
On Fri, Mar 27, 2015 at 8:16 AM, Tom Gundersen wrote: > On Fri, Mar 27, 2015 at 2:04 PM, Djalal Harouni wrote: >> Hi Shawn, >> >> On Thu, Mar 26, 2015 at 11:21:54PM -0700, Shawn Landden wrote: >>> On Thu, Mar 26, 2015 at 5:47 PM, Djalal Harouni 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 >>> >> wrote: >>> >> > On Tue, 24.03.15 11:11, Shawn Landden (sh...@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. >> Yes but there are also the following points: >> * assert() is simple, assert_se() is more complex and it may modify other >> global sate. I don't think that we can break down side effects of >> assert_se() into: >> 1) side effects of assert() >> 2) side effects of other expressions. >> >> And then remove that part 1) >> >> >> So given the fact that asser_se() do not depend on NDEBUG, did you >> consider the following: >> >> * You don't have control over what the expression do, it may just call >> abort() or exit() in case of fatal errors, so even if you remove the >> explicit abort() call, expressions may just call it. > > If expr() aborts or exits, then it really does not matter whether we > abort() explicitly or not, so I don't see what this has to do with > this patch. Sure, even with NDEBUG the program may abort if that is > called explicitly... > >> * Some expressions were perhaps depending on the logging mechanism which >> is part of the side effect. Callers to assert_se() are perhaps using >> it for a reason, are you sure that you are not introducing a >> regression here ?! it will be difficult to debug the error if we don't >> have logs. > > As far as I can tell all the relevant logging functions are pure. They > should not interact with global state, but maybe I'm missing > something? > >> * 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. > >> * Did you grep for assert_se() uses in the source ? >> >> I really do not think this is an improvement, we can't predict the >> semantics of expression here, perhaps we can with simple assert() but >> not with assert_se(). >> >> -- >> Djalal Harouni >> http://opendz.org > ___ > systemd-devel mailing list > systemd-devel@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/systemd-devel -- Shawn Landden ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [PATCH 2/2] macro: allow assert_se() assertions to also be optimized out when NDEBUG is set
On Fri, Mar 27, 2015 at 04:16:55PM +0100, Tom Gundersen wrote: > On Fri, Mar 27, 2015 at 2:04 PM, Djalal Harouni wrote: > > Hi Shawn, > > > > On Thu, Mar 26, 2015 at 11:21:54PM -0700, Shawn Landden wrote: > >> On Thu, Mar 26, 2015 at 5:47 PM, Djalal Harouni 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 > >> >> wrote: > >> >> > On Tue, 24.03.15 11:11, Shawn Landden (sh...@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. > > Yes but there are also the following points: > > * assert() is simple, assert_se() is more complex and it may modify other > > global sate. I don't think that we can break down side effects of > > assert_se() into: > > 1) side effects of assert() > > 2) side effects of other expressions. > > > > And then remove that part 1) > > > > > > So given the fact that asser_se() do not depend on NDEBUG, did you > > consider the following: > > > > * You don't have control over what the expression do, it may just call > > abort() or exit() in case of fatal errors, so even if you remove the > > explicit abort() call, expressions may just call it. > > If expr() aborts or exits, then it really does not matter whether we > abort() explicitly or not, so I don't see what this has to do with > this patch. Sure, even with NDEBUG the program may abort if that is > called explicitly... Hmm If expr() aborts or do some other complex things on its own then it should not be used with assert(), right ? that comment was in regard of this. Since it aborts this is a side effect, and perhaps followup instructions depend on the state if it is correct or not otherwise things will break later. If you use assert() here and if NDEBUG is set, assert will just be a nop and everything should continue to work. The patch seems simple but it is introducing *new* semantics for assert_se() if NDEBUG is set that current callers do not expect. > > * Some expressions were perhaps depending on the logging mechanism which > > is part of the side effect. Callers to assert_se() are perhaps using > > it for a reason, are you sure that you are not introducing a > > regression here ?! it will be difficult to debug the error if we don't > > have logs. > > As far as I can tell all the relevant logging functions are pure. They > should not interact with global state, but maybe I'm missing > something? Hmm, we are opening, closing and logging to file descriptors and virtual logging devices... And callers expect to find a log message why the function failed. > > * 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. Yes, perhaps if this was the case from the start! but currently assert_se() depends on the aborting mechanism which is not the case of assert(). > 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. Indeed, yes. > 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); => nop Yes sure, but since assert_se() did not interact with NDEBUG, to do this you have to modify _all_ callers, the second assert() w
Re: [systemd-devel] [PATCH 2/2] macro: allow assert_se() assertions to also be optimized out when NDEBUG is set
On Fri, Mar 27, 2015 at 09:51:26AM -0700, Shawn Landden wrote: > On Fri, Mar 27, 2015 at 8:16 AM, Tom Gundersen 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... ? Thanks! -- Djalal Harouni http://opendz.org ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [PATCH 2/2] macro: allow assert_se() assertions to also be optimized out when NDEBUG is set
On Mon, Mar 30, 2015 at 5:04 PM, Djalal Harouni 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 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? 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? > > Thanks! > > -- > Djalal Harouni > http://opendz.org > ___ > systemd-devel mailing list > systemd-devel@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/systemd-devel -- Shawn Landden ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [PATCH 2/2] macro: allow assert_se() assertions to also be optimized out when NDEBUG is set
On Mon, Mar 30, 2015 at 07:32:35PM -0700, Shawn Landden wrote: > On Mon, Mar 30, 2015 at 5:04 PM, Djalal Harouni 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 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 ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [PATCH 2/2] macro: allow assert_se() assertions to also be optimized out when NDEBUG is set
On Tue, Mar 31, 2015 at 8:38 AM, Djalal Harouni 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 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 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@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/systemd-devel -- Shawn Landden ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [PATCH 2/2] macro: allow assert_se() assertions to also be optimized out when NDEBUG is set
On Tue, Mar 31, 2015 at 8:38 AM, Djalal Harouni 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 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 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. I submitted a new version of this patch, with numbers (10kB smaller) to the mailing list. > > > -- > Djalal Harouni > http://opendz.org > ___ > systemd-devel mailing list > systemd-devel@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/systemd-devel -- Shawn Landden ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [PATCH 2/2] macro: allow assert_se() assertions to also be optimized out when NDEBUG is set
On Tue, Mar 31, 2015 at 11:10:34AM -0700, Shawn Landden wrote: > On Tue, Mar 31, 2015 at 8:38 AM, Djalal Harouni 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 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 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 ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [PATCH 2/2] macro: allow assert_se() assertions to also be optimized out when NDEBUG is set
On Tue, Mar 31, 2015 at 2:40 PM, Djalal Harouni 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 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 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 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.
Re: [systemd-devel] [PATCH 2/2] macro: allow assert_se() assertions to also be optimized out when NDEBUG is set
Hi Shawn, On Tue, Mar 31, 2015 at 04:59:29PM -0700, Shawn Landden wrote: > On Tue, Mar 31, 2015 at 2:40 PM, Djalal Harouni wrote: > > On Tue, Mar 31, 2015 at 11:10:34AM -0700, Shawn Landden wrote: [...] > >> 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 Yes I saw the patch and the numbers, thank you! I just didn't comment to let the thread clean, and see what others may think. Thanks! -- Djalal Harouni http://opendz.org ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [PATCH 2/2] macro: allow assert_se() assertions to also be optimized out when NDEBUG is set
On Thu, 26.03.15 23:18, Shawn Landden (sh...@churchofgit.com) wrote:
> On Thu, Mar 26, 2015 at 1:19 AM, Lennart Poettering
> wrote:
> > On Tue, 24.03.15 11:11, Shawn Landden (sh...@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!
>
> +#define assert_se(expr) do {expr} while(false)
> #define assert(expr) do {} while(false)
>
> No it does not. see "{expr}", it still is doing the side effect. It
> just doesn't check if the return value is as expected.
I see.
Well, in general I think:
a) actually using NDEBUG is crazy. It's mostly theoretical thing, not
something people should really do.
b) we don't optimize 3k or 7k away. It's negligible.
c) We don't optimize error paths. Optimization we only do for inner
loops.
> >> #ifdef NDEBUG
> >> +#define assert_se(expr) do {expr} while(false)
Does this even work? I mean, lines within the {} of a do/while block
need to end in a semicolon to be useful. Did you actually test this
with -DNDEBUG during compilation?
Also, if somebody uses this:
assert_se(a == 7);
Then you turn this into
do { a == 7 } while(false);
Which (ignoring the fact that the semicolon is missing) will result in
a compiler warning, given that we make a comparison without using its
result. The only way this could work would be this:
#define assert_se(expr) do { (void) (expr) } while(false)
i.e. explicitly casting the result of the expression to (void)...
That all said, I am not convinced that it is worth doing this change
at all.
--
Lennart Poettering, Red Hat
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel
