diff mbox series

[PATCHv3,11/15] package/sofia-sip: use BR2_ENABLE_RUNTIME_DEBUG iso BR2_ENABLE_DEBUG

Message ID 20210525122750.5022-12-patrickdepinguin@gmail.com
State Changes Requested
Headers show
Series Introduce BR2_ENABLE_RUNTIME_DEBUG | expand

Commit Message

Thomas De Schampheleire May 25, 2021, 12:27 p.m. UTC 
From: Thomas De Schampheleire <thomas.de_schampheleire@nokia.com>

BR2_ENABLE_DEBUG should just steer the availability of debug symbols and
should have no negative effect on performance.

Introduction of 'assert' statements, 'debug'-type builds with additional
logging, etc. should be steered by BR2_ENABLE_RUNTIME_DEBUG instead.

Signed-off-by: Thomas De Schampheleire <thomas.de_schampheleire@nokia.com>
---
 package/sofia-sip/sofia-sip.mk | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Arnout Vandecappelle May 25, 2021, 9:32 p.m. UTC | #1 
On 25/05/2021 14:27, Thomas De Schampheleire wrote:
> From: Thomas De Schampheleire <thomas.de_schampheleire@nokia.com>
> 
> BR2_ENABLE_DEBUG should just steer the availability of debug symbols and
> should have no negative effect on performance.
> 
> Introduction of 'assert' statements, 'debug'-type builds with additional
> logging, etc. should be steered by BR2_ENABLE_RUNTIME_DEBUG instead.
> 
> Signed-off-by: Thomas De Schampheleire <thomas.de_schampheleire@nokia.com>
> ---
>  package/sofia-sip/sofia-sip.mk | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/package/sofia-sip/sofia-sip.mk b/package/sofia-sip/sofia-sip.mk
> index 5c383400ff..cb867ba0dc 100644
> --- a/package/sofia-sip/sofia-sip.mk
> +++ b/package/sofia-sip/sofia-sip.mk
> @@ -30,7 +30,7 @@ SOFIA_SIP_CONF_OPTS += \
>  	--without-openssl
>  endif
>  
> -ifeq ($(BR2_ENABLE_DEBUG),y)
> +ifeq ($(BR2_ENABLE_RUNTIME_DEBUG),y)
>  SOFIA_SIP_CONF_OPTS += --enable-ndebug

 Actually, the logic was inverted here (already wrong in the current situation):
--enable-ndebug should be given when RUNTIME_DEBUG is *not* set.

 However, I think we should define -DNDEBUG in TARGET_CPPFLAGS, so it wouldn't
be necessary to pass it explicitly here (--enable-ndebug does nothing more than
defining NDEBUG).


 Regards,
 Arnout

>  endif
>  
>
Thomas De Schampheleire May 28, 2021, 6:35 p.m. UTC | #2 
El mar, 25 may 2021 a las 23:32, Arnout Vandecappelle
(<arnout@mind.be>) escribió:
>
>
>
> On 25/05/2021 14:27, Thomas De Schampheleire wrote:
> > From: Thomas De Schampheleire <thomas.de_schampheleire@nokia.com>
> >
> > BR2_ENABLE_DEBUG should just steer the availability of debug symbols and
> > should have no negative effect on performance.
> >
> > Introduction of 'assert' statements, 'debug'-type builds with additional
> > logging, etc. should be steered by BR2_ENABLE_RUNTIME_DEBUG instead.
> >
> > Signed-off-by: Thomas De Schampheleire <thomas.de_schampheleire@nokia.com>
> > ---
> >  package/sofia-sip/sofia-sip.mk | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/package/sofia-sip/sofia-sip.mk b/package/sofia-sip/sofia-sip.mk
> > index 5c383400ff..cb867ba0dc 100644
> > --- a/package/sofia-sip/sofia-sip.mk
> > +++ b/package/sofia-sip/sofia-sip.mk
> > @@ -30,7 +30,7 @@ SOFIA_SIP_CONF_OPTS += \
> >       --without-openssl
> >  endif
> >
> > -ifeq ($(BR2_ENABLE_DEBUG),y)
> > +ifeq ($(BR2_ENABLE_RUNTIME_DEBUG),y)
> >  SOFIA_SIP_CONF_OPTS += --enable-ndebug
>
>  Actually, the logic was inverted here (already wrong in the current situation):
> --enable-ndebug should be given when RUNTIME_DEBUG is *not* set.

Indeed, you're right. I'll add a patch to fix that first.

>
>  However, I think we should define -DNDEBUG in TARGET_CPPFLAGS, so it wouldn't
> be necessary to pass it explicitly here (--enable-ndebug does nothing more than
> defining NDEBUG).
>

In a previous iteration I basically did that, but dropped that patch
because it also had effect in the toolchain wrapper, which may be used
to build project-specific projects, and which caused compilation
failures when NDEBUG was unexpectedly set (e.g. variables only used
for assert statements, which got compiled away with NDEBUG). Granted,
this is actually a problem in such code, but nevertheless it was my
reality and I expect those of others too.
See http://patchwork.ozlabs.org/project/buildroot/patch/20210212135451.22786-3-patrickdepinguin@gmail.com/
   and my last comment on it.

Best regards,
Thomas
Arnout Vandecappelle May 28, 2021, 9:07 p.m. UTC | #3 
On 28/05/2021 20:35, Thomas De Schampheleire wrote:
> El mar, 25 may 2021 a las 23:32, Arnout Vandecappelle
> (<arnout@mind.be>) escribió:
>>
>>
>> On 25/05/2021 14:27, Thomas De Schampheleire wrote:
>>> From: Thomas De Schampheleire <thomas.de_schampheleire@nokia.com>
>>>
>>> BR2_ENABLE_DEBUG should just steer the availability of debug symbols and
>>> should have no negative effect on performance.
>>>
>>> Introduction of 'assert' statements, 'debug'-type builds with additional
>>> logging, etc. should be steered by BR2_ENABLE_RUNTIME_DEBUG instead.
>>>
>>> Signed-off-by: Thomas De Schampheleire <thomas.de_schampheleire@nokia.com>
>>> ---
>>>  package/sofia-sip/sofia-sip.mk | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/package/sofia-sip/sofia-sip.mk b/package/sofia-sip/sofia-sip.mk
>>> index 5c383400ff..cb867ba0dc 100644
>>> --- a/package/sofia-sip/sofia-sip.mk
>>> +++ b/package/sofia-sip/sofia-sip.mk
>>> @@ -30,7 +30,7 @@ SOFIA_SIP_CONF_OPTS += \
>>>       --without-openssl
>>>  endif
>>>
>>> -ifeq ($(BR2_ENABLE_DEBUG),y)
>>> +ifeq ($(BR2_ENABLE_RUNTIME_DEBUG),y)
>>>  SOFIA_SIP_CONF_OPTS += --enable-ndebug
>>  Actually, the logic was inverted here (already wrong in the current situation):
>> --enable-ndebug should be given when RUNTIME_DEBUG is *not* set.
> Indeed, you're right. I'll add a patch to fix that first.
> 
>>  However, I think we should define -DNDEBUG in TARGET_CPPFLAGS, so it wouldn't
>> be necessary to pass it explicitly here (--enable-ndebug does nothing more than
>> defining NDEBUG).
> In a previous iteration I basically did that, but dropped that patch
> because it also had effect in the toolchain wrapper,

 No it doesn't... TARGET_CFLAGS isn't set in the toolchain wrapper. Only
arch-specific options are there, and BR2_TARGET_OPTIMIZATION (which is intended
for arch-specific -m flags).

> which may be used
> to build project-specific projects, and which caused compilation
> failures when NDEBUG was unexpectedly set

 If it's built by Buildroot, then of course it *is* going to be passed in
through CFLAGS.

> (e.g. variables only used
> for assert statements, which got compiled away with NDEBUG). Granted,
> this is actually a problem in such code, but nevertheless it was my
> reality and I expect those of others too.

 If you don't want to fix the actual issue, you can always add -UNDEBUG in the
package-specific CFLAGS or in the package build system.

 Regards,
 Arnout

> See http://patchwork.ozlabs.org/project/buildroot/patch/20210212135451.22786-3-patrickdepinguin@gmail.com/
>    and my last comment on it.
Thomas De Schampheleire May 31, 2021, 10:44 a.m. UTC | #4 
Hello,

El vie, 28 may 2021 a las 23:07, Arnout Vandecappelle
(<arnout@mind.be>) escribió:
>
>
>
> On 28/05/2021 20:35, Thomas De Schampheleire wrote:
> > El mar, 25 may 2021 a las 23:32, Arnout Vandecappelle
> > (<arnout@mind.be>) escribió:
> >>
> >>
> >> On 25/05/2021 14:27, Thomas De Schampheleire wrote:
> >>> From: Thomas De Schampheleire <thomas.de_schampheleire@nokia.com>
> >>>
> >>> BR2_ENABLE_DEBUG should just steer the availability of debug symbols and
> >>> should have no negative effect on performance.
> >>>
> >>> Introduction of 'assert' statements, 'debug'-type builds with additional
> >>> logging, etc. should be steered by BR2_ENABLE_RUNTIME_DEBUG instead.
> >>>
> >>> Signed-off-by: Thomas De Schampheleire <thomas.de_schampheleire@nokia.com>
> >>> ---
> >>>  package/sofia-sip/sofia-sip.mk | 2 +-
> >>>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>>
> >>> diff --git a/package/sofia-sip/sofia-sip.mk b/package/sofia-sip/sofia-sip.mk
> >>> index 5c383400ff..cb867ba0dc 100644
> >>> --- a/package/sofia-sip/sofia-sip.mk
> >>> +++ b/package/sofia-sip/sofia-sip.mk
> >>> @@ -30,7 +30,7 @@ SOFIA_SIP_CONF_OPTS += \
> >>>       --without-openssl
> >>>  endif
> >>>
> >>> -ifeq ($(BR2_ENABLE_DEBUG),y)
> >>> +ifeq ($(BR2_ENABLE_RUNTIME_DEBUG),y)
> >>>  SOFIA_SIP_CONF_OPTS += --enable-ndebug
> >>  Actually, the logic was inverted here (already wrong in the current situation):
> >> --enable-ndebug should be given when RUNTIME_DEBUG is *not* set.
> > Indeed, you're right. I'll add a patch to fix that first.
> >
> >>  However, I think we should define -DNDEBUG in TARGET_CPPFLAGS, so it wouldn't
> >> be necessary to pass it explicitly here (--enable-ndebug does nothing more than
> >> defining NDEBUG).
> > In a previous iteration I basically did that, but dropped that patch
> > because it also had effect in the toolchain wrapper,
>
>  No it doesn't... TARGET_CFLAGS isn't set in the toolchain wrapper. Only
> arch-specific options are there, and BR2_TARGET_OPTIMIZATION (which is intended
> for arch-specific -m flags).

I had to reproduce the issue in order to understand where my
conclusion was wrong.

The compiler wrapper indeed does not pass these flags.
The problem in our case is that the -DNDEBUG is passed via a
<project>-config script to external compilations. In our case, xenomai
is used, and it provides a 'xeno-config' script to get the correct
compilation and linker flags. The generated file contains the flags
set by Buildroot in XENO_BASE_CFLAGS . With the mentioned patch, this
would include -DNDEBUG.
And the output of 'xeno-config [...] --cflags' was used in the
compilation of the external project.

We can work around that by filtering the output of 'xeno-config' in
our local makefiles.

Note that Thomas Petazzoni had initial doubts on the mentioned patch
(http://patchwork.ozlabs.org/project/buildroot/patch/20210212135451.22786-3-patrickdepinguin@gmail.com/).

If you think we should re-add it to the series, I will do it.

Thanks,
Thomas
diff mbox series

Patch

diff --git a/package/sofia-sip/sofia-sip.mk b/package/sofia-sip/sofia-sip.mk
index 5c383400ff..cb867ba0dc 100644
--- a/package/sofia-sip/sofia-sip.mk
+++ b/package/sofia-sip/sofia-sip.mk
@@ -30,7 +30,7 @@  SOFIA_SIP_CONF_OPTS += \
 	--without-openssl
 endif
 
-ifeq ($(BR2_ENABLE_DEBUG),y)
+ifeq ($(BR2_ENABLE_RUNTIME_DEBUG),y)
 SOFIA_SIP_CONF_OPTS += --enable-ndebug
 endif