* [Buildroot] [PATCHv4 01/16] core: introduce BR2_ENABLE_RUNTIME_DEBUG
2021-06-01 14:34 [Buildroot] [PATCHv4 00/16] Introduce BR2_ENABLE_RUNTIME_DEBUG Thomas De Schampheleire
@ 2021-06-01 14:34 ` Thomas De Schampheleire
2021-06-01 20:32 ` Yann E. MORIN
2021-06-01 14:34 ` [Buildroot] [PATCHv4 02/16] core: enable 'NDEBUG' unless BR2_ENABLE_RUNTIME_DEBUG is set Thomas De Schampheleire
` (15 subsequent siblings)
16 siblings, 1 reply; 31+ messages in thread
From: Thomas De Schampheleire @ 2021-06-01 14:34 UTC (permalink / raw)
To: buildroot
From: Thomas De Schampheleire <thomas.de_schampheleire@nokia.com>
Some packages have optional runtime assertions, extra traces, or other
elements that can help in debugging problems. However, such runtime elements
can negatively influence performance.
In a test program performing 100K gRPC calls from a client to a local server
and receiving the returned response, we see following execution time:
- runtime debug enabled: 1065 seconds
- runtime debug disabled: 48 seconds
This is more than a factor 20 (!) difference. Analysis shows that the
problem mostly stems from libabseil-cpp (a dependency of gRPC) which enables
mutex deadlock analysis when the preprocessor flag 'NDEBUG' is not set,
which adds a 'backtrace()' call on every lock/unlock. Potentially worse,
when libunwind is enabled and linked with the test program, 'backtrace()' is
not provided by glibc but by libunwind itself.
For production systems, users expect good performance out-of-the-box. In the
example above, the difference is huge and unless explicitly tested and
analyzed, users may not realize that the performance could be much better.
Address this problem by introducing a new option BR2_ENABLE_RUNTIME_DEBUG,
which can be used by packages or package infrastructures to set the
necessary flags.
Note that BR2_ENABLE_RUNTIME_DEBUG is orthogonal to BR2_ENABLE_DEBUG: the
former changes runtime behavior, while the latter is only expected to add
debug symbols to the build. Today, the cmake build system does introduce a
runtime impact when BR2_ENABLE_DEBUG is set, but that will be rectified in a
subsequent commit.
Signed-off-by: Thomas De Schampheleire <thomas.de_schampheleire@nokia.com>
Acked-by: Arnout Vandecappelle (Essensium/Mind) <arnout@mind.be>
---
Config.in | 13 +++++++++++++
1 file changed, 13 insertions(+)
diff --git a/Config.in b/Config.in
index c65e34bd5e..a008425688 100644
--- a/Config.in
+++ b/Config.in
@@ -412,6 +412,19 @@ config BR2_DEBUG_3
endchoice
endif
+config BR2_ENABLE_RUNTIME_DEBUG
+ bool "build packages with runtime debugging info"
+ help
+ Some packages may have runtime assertions, extra traces, and
+ similar runtime elements that can help debugging. However,
+ these elements may negatively influence performance so should
+ normally not be enabled on production systems.
+
+ Enable this option to enable such runtime debugging.
+
+ Note: disabling this option is not a guarantee that all
+ packages effectively removed these runtime debugging elements.
+
config BR2_STRIP_strip
bool "strip target binaries"
default y
--
2.26.3
^ permalink raw reply related [flat|nested] 31+ messages in thread* [Buildroot] [PATCHv4 01/16] core: introduce BR2_ENABLE_RUNTIME_DEBUG
2021-06-01 14:34 ` [Buildroot] [PATCHv4 01/16] core: introduce BR2_ENABLE_RUNTIME_DEBUG Thomas De Schampheleire
@ 2021-06-01 20:32 ` Yann E. MORIN
0 siblings, 0 replies; 31+ messages in thread
From: Yann E. MORIN @ 2021-06-01 20:32 UTC (permalink / raw)
To: buildroot
Thomas, All,
On 2021-06-01 16:34 +0200, Thomas De Schampheleire spake thusly:
> From: Thomas De Schampheleire <thomas.de_schampheleire@nokia.com>
>
> Some packages have optional runtime assertions, extra traces, or other
> elements that can help in debugging problems. However, such runtime elements
> can negatively influence performance.
>
> In a test program performing 100K gRPC calls from a client to a local server
> and receiving the returned response, we see following execution time:
>
> - runtime debug enabled: 1065 seconds
> - runtime debug disabled: 48 seconds
>
> This is more than a factor 20 (!) difference. Analysis shows that the
> problem mostly stems from libabseil-cpp (a dependency of gRPC) which enables
> mutex deadlock analysis when the preprocessor flag 'NDEBUG' is not set,
> which adds a 'backtrace()' call on every lock/unlock. Potentially worse,
> when libunwind is enabled and linked with the test program, 'backtrace()' is
> not provided by glibc but by libunwind itself.
>
> For production systems, users expect good performance out-of-the-box. In the
> example above, the difference is huge and unless explicitly tested and
> analyzed, users may not realize that the performance could be much better.
>
> Address this problem by introducing a new option BR2_ENABLE_RUNTIME_DEBUG,
> which can be used by packages or package infrastructures to set the
> necessary flags.
>
> Note that BR2_ENABLE_RUNTIME_DEBUG is orthogonal to BR2_ENABLE_DEBUG: the
> former changes runtime behavior, while the latter is only expected to add
> debug symbols to the build. Today, the cmake build system does introduce a
> runtime impact when BR2_ENABLE_DEBUG is set, but that will be rectified in a
> subsequent commit.
>
> Signed-off-by: Thomas De Schampheleire <thomas.de_schampheleire@nokia.com>
> Acked-by: Arnout Vandecappelle (Essensium/Mind) <arnout@mind.be>
Reviewed-by: Yann E. MORIN <yann.morin.1998@free.fr>
No, I am *not* going to bikeshed on the prompt... ;-)
Although yes, I can see how it could be a little bit confusing...
Regards,
Yann E. MORIN.
> ---
> Config.in | 13 +++++++++++++
> 1 file changed, 13 insertions(+)
>
> diff --git a/Config.in b/Config.in
> index c65e34bd5e..a008425688 100644
> --- a/Config.in
> +++ b/Config.in
> @@ -412,6 +412,19 @@ config BR2_DEBUG_3
> endchoice
> endif
>
> +config BR2_ENABLE_RUNTIME_DEBUG
> + bool "build packages with runtime debugging info"
> + help
> + Some packages may have runtime assertions, extra traces, and
> + similar runtime elements that can help debugging. However,
> + these elements may negatively influence performance so should
> + normally not be enabled on production systems.
> +
> + Enable this option to enable such runtime debugging.
> +
> + Note: disabling this option is not a guarantee that all
> + packages effectively removed these runtime debugging elements.
> +
> config BR2_STRIP_strip
> bool "strip target binaries"
> default y
> --
> 2.26.3
>
> _______________________________________________
> buildroot mailing list
> buildroot at busybox.net
> http://lists.busybox.net/mailman/listinfo/buildroot
--
.-----------------.--------------------.------------------.--------------------.
| Yann E. MORIN | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software Designer | \ / CAMPAIGN | ___ |
| +33 561 099 427 `------------.-------: X AGAINST | \e/ There is no |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL | v conspiracy. |
'------------------------------^-------^------------------^--------------------'
^ permalink raw reply [flat|nested] 31+ messages in thread
* [Buildroot] [PATCHv4 02/16] core: enable 'NDEBUG' unless BR2_ENABLE_RUNTIME_DEBUG is set
2021-06-01 14:34 [Buildroot] [PATCHv4 00/16] Introduce BR2_ENABLE_RUNTIME_DEBUG Thomas De Schampheleire
2021-06-01 14:34 ` [Buildroot] [PATCHv4 01/16] core: introduce BR2_ENABLE_RUNTIME_DEBUG Thomas De Schampheleire
@ 2021-06-01 14:34 ` Thomas De Schampheleire
2021-06-01 20:36 ` Yann E. MORIN
2021-07-04 9:15 ` Thomas Petazzoni
2021-06-01 14:34 ` [Buildroot] [PATCHv4 03/16] package/pkg-cmake.mk: determine CMAKE_BUILD_TYPE depending on BR2_ENABLE_RUNTIME_DEBUG Thomas De Schampheleire
` (14 subsequent siblings)
16 siblings, 2 replies; 31+ messages in thread
From: Thomas De Schampheleire @ 2021-06-01 14:34 UTC (permalink / raw)
To: buildroot
From: Thomas De Schampheleire <thomas.de_schampheleire@nokia.com>
The 'assert' statement in glibc honors the 'NDEBUG' preprocessor macro: if
it is set, then the assert statement is compiled away.
Define this 'NDEBUG' macro when BR2_ENABLE_RUNTIME_DEBUG is disabled (the
default case).
Signed-off-by: Thomas De Schampheleire <thomas.de_schampheleire@nokia.com>
---
v4: reintroduced patch on suggestion of Arnout. I had previously dropped it from
the series based on an incorrect conclusion.
package/Makefile.in | 3 +++
1 file changed, 3 insertions(+)
diff --git a/package/Makefile.in b/package/Makefile.in
index 86db62ba5b..955e6a8e8c 100644
--- a/package/Makefile.in
+++ b/package/Makefile.in
@@ -148,6 +148,9 @@ endif
ifeq ($(BR2_DEBUG_3),y)
TARGET_DEBUGGING = -g3
endif
+ifeq ($(BR2_ENABLE_RUNTIME_DEBUG),)
+TARGET_DEBUGGING += -DNDEBUG
+endif
TARGET_LDFLAGS = $(call qstrip,$(BR2_TARGET_LDFLAGS))
--
2.26.3
^ permalink raw reply related [flat|nested] 31+ messages in thread* [Buildroot] [PATCHv4 02/16] core: enable 'NDEBUG' unless BR2_ENABLE_RUNTIME_DEBUG is set
2021-06-01 14:34 ` [Buildroot] [PATCHv4 02/16] core: enable 'NDEBUG' unless BR2_ENABLE_RUNTIME_DEBUG is set Thomas De Schampheleire
@ 2021-06-01 20:36 ` Yann E. MORIN
2021-06-05 16:50 ` D. Olsson
2021-07-04 9:15 ` Thomas Petazzoni
1 sibling, 1 reply; 31+ messages in thread
From: Yann E. MORIN @ 2021-06-01 20:36 UTC (permalink / raw)
To: buildroot
Thomas, All,
On 2021-06-01 16:34 +0200, Thomas De Schampheleire spake thusly:
> From: Thomas De Schampheleire <thomas.de_schampheleire@nokia.com>
>
> The 'assert' statement in glibc honors the 'NDEBUG' preprocessor macro: if
> it is set, then the assert statement is compiled away.
>
> Define this 'NDEBUG' macro when BR2_ENABLE_RUNTIME_DEBUG is disabled (the
> default case).
>
> Signed-off-by: Thomas De Schampheleire <thomas.de_schampheleire@nokia.com>
Reviewed-by: Yann E. MORIN <yann.morin.1998@free.fr>
However, see a little nit, below...
> ---
> v4: reintroduced patch on suggestion of Arnout. I had previously dropped it from
> the series based on an incorrect conclusion.
>
>
> package/Makefile.in | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/package/Makefile.in b/package/Makefile.in
> index 86db62ba5b..955e6a8e8c 100644
> --- a/package/Makefile.in
> +++ b/package/Makefile.in
> @@ -148,6 +148,9 @@ endif
> ifeq ($(BR2_DEBUG_3),y)
> TARGET_DEBUGGING = -g3
Here, we have a simple assignment in a conditional block, while...
> endif
> +ifeq ($(BR2_ENABLE_RUNTIME_DEBUG),)
> +TARGET_DEBUGGING += -DNDEBUG
... here we have an append-assignment in a conditional.
In this case the order ensures it works, but this is inconsistent and
prone to errors should we need to expand TARGET_DEBUGGING futher...
Regards,
Yann E. MORIN.
> +endif
>
> TARGET_LDFLAGS = $(call qstrip,$(BR2_TARGET_LDFLAGS))
>
> --
> 2.26.3
>
> _______________________________________________
> buildroot mailing list
> buildroot at busybox.net
> http://lists.busybox.net/mailman/listinfo/buildroot
--
.-----------------.--------------------.------------------.--------------------.
| Yann E. MORIN | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software Designer | \ / CAMPAIGN | ___ |
| +33 561 099 427 `------------.-------: X AGAINST | \e/ There is no |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL | v conspiracy. |
'------------------------------^-------^------------------^--------------------'
^ permalink raw reply [flat|nested] 31+ messages in thread
* [Buildroot] [PATCHv4 02/16] core: enable 'NDEBUG' unless BR2_ENABLE_RUNTIME_DEBUG is set
2021-06-01 20:36 ` Yann E. MORIN
@ 2021-06-05 16:50 ` D. Olsson
2021-06-05 17:11 ` Thomas De Schampheleire
0 siblings, 1 reply; 31+ messages in thread
From: D. Olsson @ 2021-06-05 16:50 UTC (permalink / raw)
To: buildroot
Hi Thomas, Yann, all,
> On 2021-06-01 16:34 +0200, Thomas De Schampheleire spake thusly:
> +ifeq ($(BR2_ENABLE_RUNTIME_DEBUG),)
> +TARGET_DEBUGGING += -DNDEBUG
This broke bitcoin builds. I haven't looked deeply into the
details, but it is my understanding that NDEBUG changes the
behaviour of assert() in a way that Bitcoin explicitly does
not support.
See: https://github.com/bitcoin/bitcoin/blob/707ba8692b0013f4824dc3c2ea6554ccad5d20c2/src/compat/assumptions.h#L13
I don't know if there is a workaround for this, but I have a
feeling that passing NDEBUG for all packages might not be a
good idea?
---
D. Olsson
PGP: 8204A8CD
^ permalink raw reply [flat|nested] 31+ messages in thread
* [Buildroot] [PATCHv4 02/16] core: enable 'NDEBUG' unless BR2_ENABLE_RUNTIME_DEBUG is set
2021-06-05 16:50 ` D. Olsson
@ 2021-06-05 17:11 ` Thomas De Schampheleire
0 siblings, 0 replies; 31+ messages in thread
From: Thomas De Schampheleire @ 2021-06-05 17:11 UTC (permalink / raw)
To: buildroot
Hello,
On Sat, Jun 5, 2021, 18:50 D. Olsson <hi@senzilla.io> wrote:
> Hi Thomas, Yann, all,
>
> > On 2021-06-01 16:34 +0200, Thomas De Schampheleire spake thusly:
> > +ifeq ($(BR2_ENABLE_RUNTIME_DEBUG),)
> > +TARGET_DEBUGGING += -DNDEBUG
>
> This broke bitcoin builds. I haven't looked deeply into the
> details, but it is my understanding that NDEBUG changes the
> behaviour of assert() in a way that Bitcoin explicitly does
> not support.
>
> See:
> https://github.com/bitcoin/bitcoin/blob/707ba8692b0013f4824dc3c2ea6554ccad5d20c2/src/compat/assumptions.h#L13
>
> I don't know if there is a workaround for this, but I have a
> feeling that passing NDEBUG for all packages might not be a
> good idea?
>
NDEBUG is a macro that, when set, will neutralize assert statements (see
man assert). The rationale is that assert statements can help developers
validate assumptions in a test phase, but that this code is better disabled
in release builds, for reasons of performance. At that time, the code is
expected to be correct.
It's odd but not unacceptable that bitcoin expects NDEBUG not to be set.
The workaround/solution would be to explicitly undefine NDEBUG, via
-UNDEBUG. This should become part of the flags set in bitcoin.mk on
Buildroot.
It is possible that there are other packages that need this change, but
generally one wpuld expect projects to allow both scenarios.
Best regards
Thomas
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.busybox.net/pipermail/buildroot/attachments/20210605/07fc05c1/attachment.html>
^ permalink raw reply [flat|nested] 31+ messages in thread
* [Buildroot] [PATCHv4 02/16] core: enable 'NDEBUG' unless BR2_ENABLE_RUNTIME_DEBUG is set
2021-06-01 14:34 ` [Buildroot] [PATCHv4 02/16] core: enable 'NDEBUG' unless BR2_ENABLE_RUNTIME_DEBUG is set Thomas De Schampheleire
2021-06-01 20:36 ` Yann E. MORIN
@ 2021-07-04 9:15 ` Thomas Petazzoni
2021-07-04 11:01 ` Peter Korsgaard
2021-07-04 14:58 ` Arnout Vandecappelle
1 sibling, 2 replies; 31+ messages in thread
From: Thomas Petazzoni @ 2021-07-04 9:15 UTC (permalink / raw)
To: buildroot
Hello,
On Tue, 1 Jun 2021 16:34:07 +0200
Thomas De Schampheleire <patrickdepinguin@gmail.com> wrote:
> From: Thomas De Schampheleire <thomas.de_schampheleire@nokia.com>
>
> The 'assert' statement in glibc honors the 'NDEBUG' preprocessor macro: if
> it is set, then the assert statement is compiled away.
>
> Define this 'NDEBUG' macro when BR2_ENABLE_RUNTIME_DEBUG is disabled (the
> default case).
>
> Signed-off-by: Thomas De Schampheleire <thomas.de_schampheleire@nokia.com>
So, I would like to challenge whether this change is really a good
idea. Since this has been merged, we had to add a significant number of
patches to undefine NDEBUG.
The manpage of assert() tells us:
If the macro NDEBUG is defined at the moment <assert.h> was last in?
cluded, the macro assert() generates no code, and hence does nothing at
all. It is not recommended to define NDEBUG if using assert() to de?
tect error conditions since the software may behave non-deterministi?
cally.
So there is a clear recommandation to *not* define NDEBUG if assert is
used to detect error conditions. And later on, it also says:
BUGS
assert() is implemented as a macro; if the expression tested has side-
effects, program behavior will be different depending on whether NDEBUG
is defined. This may create Heisenbugs which go away when debugging is
turned on.
We have catched a number of packages where defining NDEBUG is causing
build issues, but I am worried about those packages where it doesn't
cause build issues, but assert() is used to run something that has
side-effects, and where the issue would only be visible at runtime.
Yes, ideally, assert() should only be used to verify expressions that
have no side effects. But practically speaking, I am not sure how many
programmers are really aware of the fact that assert() expressions
should not have side effects, because assert() can be disabled by
defining NDEBUG.
I believe this change is too dangerous, has caused a number of build
breakage + can cause run-time breakage that is difficult to predict.
What do you think ?
Thomas
--
Thomas Petazzoni, co-owner and CEO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
^ permalink raw reply [flat|nested] 31+ messages in thread* [Buildroot] [PATCHv4 02/16] core: enable 'NDEBUG' unless BR2_ENABLE_RUNTIME_DEBUG is set
2021-07-04 9:15 ` Thomas Petazzoni
@ 2021-07-04 11:01 ` Peter Korsgaard
2021-07-04 11:32 ` Yann E. MORIN
2021-07-04 14:58 ` Arnout Vandecappelle
1 sibling, 1 reply; 31+ messages in thread
From: Peter Korsgaard @ 2021-07-04 11:01 UTC (permalink / raw)
To: buildroot
>>>>> "Thomas" == Thomas Petazzoni <thomas.petazzoni@bootlin.com> writes:
> Hello,
> On Tue, 1 Jun 2021 16:34:07 +0200
> Thomas De Schampheleire <patrickdepinguin@gmail.com> wrote:
>> From: Thomas De Schampheleire <thomas.de_schampheleire@nokia.com>
>>
>> The 'assert' statement in glibc honors the 'NDEBUG' preprocessor macro: if
>> it is set, then the assert statement is compiled away.
>>
>> Define this 'NDEBUG' macro when BR2_ENABLE_RUNTIME_DEBUG is disabled (the
>> default case).
>>
>> Signed-off-by: Thomas De Schampheleire <thomas.de_schampheleire@nokia.com>
> So, I would like to challenge whether this change is really a good
> idea. Since this has been merged, we had to add a significant number of
> patches to undefine NDEBUG.
Indeed.
> I believe this change is too dangerous, has caused a number of build
> breakage + can cause run-time breakage that is difficult to predict.
> What do you think ?
I agree, I don't think it ends up being a net positive regarding
cost/benefit. I would prefer to revert it.
--
Bye, Peter Korsgaard
^ permalink raw reply [flat|nested] 31+ messages in thread
* [Buildroot] [PATCHv4 02/16] core: enable 'NDEBUG' unless BR2_ENABLE_RUNTIME_DEBUG is set
2021-07-04 11:01 ` Peter Korsgaard
@ 2021-07-04 11:32 ` Yann E. MORIN
2021-07-04 12:09 ` Yann E. MORIN
0 siblings, 1 reply; 31+ messages in thread
From: Yann E. MORIN @ 2021-07-04 11:32 UTC (permalink / raw)
To: buildroot
Peter, Thomas?, All,
On 2021-07-04 13:01 +0200, Peter Korsgaard spake thusly:
> >>>>> "Thomas" == Thomas Petazzoni <thomas.petazzoni@bootlin.com> writes:
> > Hello,
> > On Tue, 1 Jun 2021 16:34:07 +0200
> > Thomas De Schampheleire <patrickdepinguin@gmail.com> wrote:
> >> From: Thomas De Schampheleire <thomas.de_schampheleire@nokia.com>
> >>
> >> The 'assert' statement in glibc honors the 'NDEBUG' preprocessor macro: if
> >> it is set, then the assert statement is compiled away.
> >>
> >> Define this 'NDEBUG' macro when BR2_ENABLE_RUNTIME_DEBUG is disabled (the
> >> default case).
> >>
> >> Signed-off-by: Thomas De Schampheleire <thomas.de_schampheleire@nokia.com>
> > So, I would like to challenge whether this change is really a good
> > idea. Since this has been merged, we had to add a significant number of
> > patches to undefine NDEBUG.
> Indeed.
Compiling without assert() should never have had an impact on the
behaviour of the program. Semantically, assert() should only be used to
test pre- and post-conditions (e.g. a value we were passed is in an
acceptable range), i.e. to ensure the contract of the API.
The problem is that developpers have started using assert() as a mean to
test expected error conditions (e.g. the socket was closed by the remote
end), instead of handling them gracefully.
As such, the semantic of assert() has shifted enough that the original
intent is mush less prominent than what it is currently used for.
So yes, this does more harm than it brings benefits.
> > I believe this change is too dangerous, has caused a number of build
> > breakage + can cause run-time breakage that is difficult to predict.
> > What do you think ?
> I agree, I don't think it ends up being a net positive regarding
> cost/benefit. I would prefer to revert it.
In the end, for packages that do use assert() can add NDEBUG to their
CFLAGS if so they wish.
I will revert all those changes, since I was the one who applied the
original changes.
Regards,
Yann E. MORIN.
--
.-----------------.--------------------.------------------.--------------------.
| Yann E. MORIN | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software Designer | \ / CAMPAIGN | ___ |
| +33 561 099 427 `------------.-------: X AGAINST | \e/ There is no |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL | v conspiracy. |
'------------------------------^-------^------------------^--------------------'
^ permalink raw reply [flat|nested] 31+ messages in thread
* [Buildroot] [PATCHv4 02/16] core: enable 'NDEBUG' unless BR2_ENABLE_RUNTIME_DEBUG is set
2021-07-04 11:32 ` Yann E. MORIN
@ 2021-07-04 12:09 ` Yann E. MORIN
0 siblings, 0 replies; 31+ messages in thread
From: Yann E. MORIN @ 2021-07-04 12:09 UTC (permalink / raw)
To: buildroot
Peter, Thomas?, All,
On 2021-07-04 13:32 +0200, Yann E. MORIN spake thusly:
> On 2021-07-04 13:01 +0200, Peter Korsgaard spake thusly:
> > >>>>> "Thomas" == Thomas Petazzoni <thomas.petazzoni@bootlin.com> writes:
> > > On Tue, 1 Jun 2021 16:34:07 +0200
> > > Thomas De Schampheleire <patrickdepinguin@gmail.com> wrote:
> > >> From: Thomas De Schampheleire <thomas.de_schampheleire@nokia.com>
> > >>
> > >> The 'assert' statement in glibc honors the 'NDEBUG' preprocessor macro: if
> > >> it is set, then the assert statement is compiled away.
> > >>
> > >> Define this 'NDEBUG' macro when BR2_ENABLE_RUNTIME_DEBUG is disabled (the
> > >> default case).
> > >>
> > >> Signed-off-by: Thomas De Schampheleire <thomas.de_schampheleire@nokia.com>
> > > So, I would like to challenge whether this change is really a good
> > > idea. Since this has been merged, we had to add a significant number of
> > > patches to undefine NDEBUG.
"Significant", being just 2 packages I had to revert as they do not have
corresponding upstream changes, or upstream expressly refused to support
NDEBUGi: bitcoin and weston. A few others have had the NDEBUG changes
accepted upstream, and for some wee even have updated to a version with
those patches.
Plus 6 patches pending in patchwork:
- gcc: the issue is not trivial: -Werror or -DNDEBUG?
- sane-backends: merged upstream
- tor: can't build with NDEBUG
- casync: can't with NDEBUG
- qemu: can't with NDEBUG
- xen: is already using NDEBUG, but causes -Werror issues
So, that's 8 packages affected by NDEBUG, of which 6 or 7 are going to be
an issue. I would not call that 'significant' yet.
However, as I said earlier, I still think this is not worth the effort,
indeed.
Regards,
Yann E. MORIN.
--
.-----------------.--------------------.------------------.--------------------.
| Yann E. MORIN | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software Designer | \ / CAMPAIGN | ___ |
| +33 561 099 427 `------------.-------: X AGAINST | \e/ There is no |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL | v conspiracy. |
'------------------------------^-------^------------------^--------------------'
^ permalink raw reply [flat|nested] 31+ messages in thread
* [Buildroot] [PATCHv4 02/16] core: enable 'NDEBUG' unless BR2_ENABLE_RUNTIME_DEBUG is set
2021-07-04 9:15 ` Thomas Petazzoni
2021-07-04 11:01 ` Peter Korsgaard
@ 2021-07-04 14:58 ` Arnout Vandecappelle
2021-07-04 19:52 ` Yann E. MORIN
1 sibling, 1 reply; 31+ messages in thread
From: Arnout Vandecappelle @ 2021-07-04 14:58 UTC (permalink / raw)
To: buildroot
On 04/07/2021 11:15, Thomas Petazzoni wrote:
> Hello,
>
> On Tue, 1 Jun 2021 16:34:07 +0200
> Thomas De Schampheleire <patrickdepinguin@gmail.com> wrote:
>
>> From: Thomas De Schampheleire <thomas.de_schampheleire@nokia.com>
>>
>> The 'assert' statement in glibc honors the 'NDEBUG' preprocessor macro: if
>> it is set, then the assert statement is compiled away.
>>
>> Define this 'NDEBUG' macro when BR2_ENABLE_RUNTIME_DEBUG is disabled (the
>> default case).
>>
>> Signed-off-by: Thomas De Schampheleire <thomas.de_schampheleire@nokia.com>
>
> So, I would like to challenge whether this change is really a good
> idea.
I'm the one who originally proposed this (I think) so I better pipe up.
> Since this has been merged, we had to add a significant number of
> patches to undefine NDEBUG.
>
> The manpage of assert() tells us:
>
> If the macro NDEBUG is defined at the moment <assert.h> was last in?
> cluded, the macro assert() generates no code, and hence does nothing at
> all. It is not recommended to define NDEBUG if using assert() to de?
> tect error conditions since the software may behave non-deterministi?
> cally.
In practice, the build failures are mostly due to variables that become unused
because the assert() is gone, and that triggers -Werror.
> So there is a clear recommandation to *not* define NDEBUG if assert is
> used to detect error conditions. And later on, it also says:
>
> BUGS
> assert() is implemented as a macro; if the expression tested has side-
> effects, program behavior will be different depending on whether NDEBUG
> is defined. This may create Heisenbugs which go away when debugging is
> turned on.
>
> We have catched a number of packages where defining NDEBUG is causing
> build issues, but I am worried about those packages where it doesn't
> cause build issues, but assert() is used to run something that has
> side-effects, and where the issue would only be visible at runtime.
I don't think it's very likely that this happens, but indeed it's possibly. You
just have to "optimise":
bool result = f();
assert(result);
into
assert(f());
and you get hit by this issue...
> Yes, ideally, assert() should only be used to verify expressions that
> have no side effects. But practically speaking, I am not sure how many
> programmers are really aware of the fact that assert() expressions
> should not have side effects, because assert() can be disabled by
> defining NDEBUG.
Well, packages that use meson or CMake *do* get NDEBUG set (because that's done
by the release variants), so those packages at least seem to behave sanely...
That said:
> I believe this change is too dangerous,
This is definitely true. So I also think reverting it is the safest option.
Regards,
Arnout
> has caused a number of build
> breakage + can cause run-time breakage that is difficult to predict.
>
> What do you think ?
>
> Thomas
>
^ permalink raw reply [flat|nested] 31+ messages in thread* [Buildroot] [PATCHv4 02/16] core: enable 'NDEBUG' unless BR2_ENABLE_RUNTIME_DEBUG is set
2021-07-04 14:58 ` Arnout Vandecappelle
@ 2021-07-04 19:52 ` Yann E. MORIN
0 siblings, 0 replies; 31+ messages in thread
From: Yann E. MORIN @ 2021-07-04 19:52 UTC (permalink / raw)
To: buildroot
Arnout, Peter, Thomas?, All,
On 2021-07-04 16:58 +0200, Arnout Vandecappelle spake thusly:
> On 04/07/2021 11:15, Thomas Petazzoni wrote:
> > On Tue, 1 Jun 2021 16:34:07 +0200
> > Thomas De Schampheleire <patrickdepinguin@gmail.com> wrote:
> >> From: Thomas De Schampheleire <thomas.de_schampheleire@nokia.com>
> >> The 'assert' statement in glibc honors the 'NDEBUG' preprocessor macro: if
> >> it is set, then the assert statement is compiled away.
> >> Define this 'NDEBUG' macro when BR2_ENABLE_RUNTIME_DEBUG is disabled (the
> >> default case).
> > So, I would like to challenge whether this change is really a good
> > idea.
> In practice, the build failures are mostly due to variables that become unused
> because the assert() is gone, and that triggers -Werror.
[--SNIP--]
> I don't think it's very likely that this happens, but indeed it's possibly. You
> just have to "optimise":
> bool result = f();
> assert(result);
> into
> assert(f());
> and you get hit by this issue...
And this is way too easy an optimisation to do. ;-)
> > Yes, ideally, assert() should only be used to verify expressions that
> > have no side effects. But practically speaking, I am not sure how many
> > programmers are really aware of the fact that assert() expressions
> > should not have side effects, because assert() can be disabled by
> > defining NDEBUG.
> Well, packages that use meson or CMake *do* get NDEBUG set (because that's done
> by the release variants), so those packages at least seem to behave sanely...
> That said:
>
> > I believe this change is too dangerous,
>
> This is definitely true. So I also think reverting it is the safest option.
I've now done so: I reverted the change itself, and reverted the two
packages that had workarounds for this;
fc0fb99f0307 Revert "package/weston: disable -NDEBUG"
1aa3cb109067 Revert "package/bitcoin: unset the NDEBUG flag"
a1c7cff1a081 Revert "core: enable 'NDEBUG' unless BR2_ENABLE_RUNTIME_DEBUG is set"
The other NDEBUG-related changes, I have left in the tree:
b7939fe2a0b1 core: introduce BR2_ENABLE_RUNTIME_DEBUG
=> used by meson and cmake
61c5e0319c5 package/pkg-cmake.mk: determine CMAKE_BUILD_TYPE depending on BR2_ENABLE_RUNTIME_DEBUG
=> see commit log for details
fb12adbb761 package/sofia-sip: correct passing of '--enable-ndebug'
0993954814e package/sofia-sip: don't set 'NDEBUG' explicitly
=> the first was a fix; the second drops an explicit handling
maybe we could revert 0993954814e, but I don't see the point
ea02ac3e061 package/daemon: fix build without BR2_ENABLE_RUNTIME_DEBUG
=> very similar fix accepted upstream, so we will drop the patch
when we bump
bf32928bbb0 package/gnutls: disable tests
=> we still want to disable tests even without NDEBUG
1f1d536e7e3 package/pdbg: fix build with -DNDEBUG
932f6a0a2ad package/pdbg: Bump version to v3.3
=> a patch we had was accepted upstream, and we updated the
version
cc1c8c3bb1c package/openswan: disable -Werror
=> we still want to disable Werror even without NDEBUG
12551386027 package/filemq: bump to af4768dcaf2fcb8083a32bad107a22ecb7a5d954
=> is a version bump anyway
I will now reply to the individual pending patches telated to NDEBUG,
and reject them.
Regards,
Yann E. MORIN.
--
.-----------------.--------------------.------------------.--------------------.
| Yann E. MORIN | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software Designer | \ / CAMPAIGN | ___ |
| +33 561 099 427 `------------.-------: X AGAINST | \e/ There is no |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL | v conspiracy. |
'------------------------------^-------^------------------^--------------------'
^ permalink raw reply [flat|nested] 31+ messages in thread
* [Buildroot] [PATCHv4 03/16] package/pkg-cmake.mk: determine CMAKE_BUILD_TYPE depending on BR2_ENABLE_RUNTIME_DEBUG
2021-06-01 14:34 [Buildroot] [PATCHv4 00/16] Introduce BR2_ENABLE_RUNTIME_DEBUG Thomas De Schampheleire
2021-06-01 14:34 ` [Buildroot] [PATCHv4 01/16] core: introduce BR2_ENABLE_RUNTIME_DEBUG Thomas De Schampheleire
2021-06-01 14:34 ` [Buildroot] [PATCHv4 02/16] core: enable 'NDEBUG' unless BR2_ENABLE_RUNTIME_DEBUG is set Thomas De Schampheleire
@ 2021-06-01 14:34 ` Thomas De Schampheleire
2021-06-01 20:37 ` Yann E. MORIN
2021-06-01 14:34 ` [Buildroot] [PATCHv4 04/16] package/flare-engine: disable effect of CMAKE_BUILD_TYPE Thomas De Schampheleire
` (13 subsequent siblings)
16 siblings, 1 reply; 31+ messages in thread
From: Thomas De Schampheleire @ 2021-06-01 14:34 UTC (permalink / raw)
To: buildroot
From: Thomas De Schampheleire <thomas.de_schampheleire@nokia.com>
The CMAKE_BUILD_TYPE is currently set as 'Debug' in case BR2_ENABLE_DEBUG is
set, and as 'Release' in other cases. However, while the description of
BR2_ENABLE_DEBUG is to enable debug symbols (no runtime impact), the 'Debug'
build type in CMake can actually have runtime impact. For one, because it
does not set -DNDEBUG like is done for 'Release', but also because packages
may do custom things based on it.
The question of which CMAKE_BUILD_TYPE Buildroot should set, be it 'Debug',
'Release', 'RelWithDebInfo' or others, has come up several times in the
past. See some references below:
- July 2016: switch from Debug to RelWithDebInfo:
https://git.buildroot.org/buildroot/commit/?id=4b0120183404913f7f7788ef4f0f6b51498ef363
- October 2016: switch from RelWithDebInfo back to Debug:
https://git.buildroot.org/buildroot/commit/?id=104bb29e0490bfb487e2e665448dd3ca07fcc2b5
and changes to make sure Buildroot's flags are respected:
https://git.buildroot.org/buildroot/commit/?id=12494ef48f893684d0800e7f6fe39a2ceaed0451
- August 2017: bug #10246 - "BR2_ENABLE_DEBUG does not have the expected
effect for cmake packages"
https://bugs.busybox.net/show_bug.cgi?id=10246
- August 2017: mail thread following bug #10246:
http://lists.busybox.net/pipermail/buildroot/2017-August/200778.html
In the last mail thread, Samuel Martin confirmed that the 'Release' build
type could be used in all cases, because Buildroot is actually making sure
that the optimization flags are those determined by Buildroot, not the
defaults of cmake, thanks to commit 12494ef48f.
But Arnout Vandecappelle objected to using always 'Release', stating that
users may actually want the extra assertions.
With the introduction of BR2_ENABLE_RUNTIME_DEBUG, Buildroot can now cater
for all cases:
- use CMAKE_BUILD_TYPE=Release by default. This makes sure that there is no
unexpected performance degradation triggered by enabling BR2_ENABLE_DEBUG.
- users can optionally enable BR2_ENABLE_RUNTIME_DEBUG if they want runtime
debug info like assertions, at the risk of introducing performance
degradation. In this case, we switch to CMAKE_BUILD_TYPE=Debug.
- orthogonally to the above, BR2_ENABLE_DEBUG still determines passing the
'-g' flag to enable debug symbols, and BR2_OPTIMIZE_X still determines the
used optimization flags.
Signed-off-by: Thomas De Schampheleire <thomas.de_schampheleire@nokia.com>
Acked-by: Arnout Vandecappelle (Essensium/Mind) <arnout@mind.be>
---
docs/manual/adding-packages-cmake.txt | 2 +-
package/pkg-cmake.mk | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/docs/manual/adding-packages-cmake.txt b/docs/manual/adding-packages-cmake.txt
index 73f0943024..541d7422cf 100644
--- a/docs/manual/adding-packages-cmake.txt
+++ b/docs/manual/adding-packages-cmake.txt
@@ -100,7 +100,7 @@ typical packages will therefore only use a few of them.
necessary to set them in the package's +*.mk+ file unless you want
to override them:
-** +CMAKE_BUILD_TYPE+ is driven by +BR2_ENABLE_DEBUG+;
+** +CMAKE_BUILD_TYPE+ is driven by +BR2_ENABLE_RUNTIME_DEBUG+;
** +CMAKE_INSTALL_PREFIX+;
** +BUILD_SHARED_LIBS+ is driven by +BR2_STATIC_LIBS+;
** +BUILD_DOC+, +BUILD_DOCS+ are disabled;
diff --git a/package/pkg-cmake.mk b/package/pkg-cmake.mk
index 37078f3c34..4ee100a0c6 100644
--- a/package/pkg-cmake.mk
+++ b/package/pkg-cmake.mk
@@ -266,7 +266,7 @@ define TOOLCHAIN_CMAKE_INSTALL_FILES
-e 's#@@CMAKE_SYSTEM_PROCESSOR@@#$(call qstrip,$(CMAKE_SYSTEM_PROCESSOR))#' \
-e 's#@@TOOLCHAIN_HAS_CXX@@#$(if $(BR2_INSTALL_LIBSTDCPP),1,0)#' \
-e 's#@@TOOLCHAIN_HAS_FORTRAN@@#$(if $(BR2_TOOLCHAIN_HAS_FORTRAN),1,0)#' \
- -e 's#@@CMAKE_BUILD_TYPE@@#$(if $(BR2_ENABLE_DEBUG),Debug,Release)#' \
+ -e 's#@@CMAKE_BUILD_TYPE@@#$(if $(BR2_ENABLE_RUNTIME_DEBUG),Debug,Release)#' \
$(TOPDIR)/support/misc/toolchainfile.cmake.in \
> $(HOST_DIR)/share/buildroot/toolchainfile.cmake
$(Q)$(INSTALL) -D -m 0644 support/misc/Buildroot.cmake \
--
2.26.3
^ permalink raw reply related [flat|nested] 31+ messages in thread* [Buildroot] [PATCHv4 03/16] package/pkg-cmake.mk: determine CMAKE_BUILD_TYPE depending on BR2_ENABLE_RUNTIME_DEBUG
2021-06-01 14:34 ` [Buildroot] [PATCHv4 03/16] package/pkg-cmake.mk: determine CMAKE_BUILD_TYPE depending on BR2_ENABLE_RUNTIME_DEBUG Thomas De Schampheleire
@ 2021-06-01 20:37 ` Yann E. MORIN
0 siblings, 0 replies; 31+ messages in thread
From: Yann E. MORIN @ 2021-06-01 20:37 UTC (permalink / raw)
To: buildroot
Thomas, All,
On 2021-06-01 16:34 +0200, Thomas De Schampheleire spake thusly:
> From: Thomas De Schampheleire <thomas.de_schampheleire@nokia.com>
>
> The CMAKE_BUILD_TYPE is currently set as 'Debug' in case BR2_ENABLE_DEBUG is
> set, and as 'Release' in other cases. However, while the description of
> BR2_ENABLE_DEBUG is to enable debug symbols (no runtime impact), the 'Debug'
> build type in CMake can actually have runtime impact. For one, because it
> does not set -DNDEBUG like is done for 'Release', but also because packages
> may do custom things based on it.
>
> The question of which CMAKE_BUILD_TYPE Buildroot should set, be it 'Debug',
> 'Release', 'RelWithDebInfo' or others, has come up several times in the
> past. See some references below:
>
> - July 2016: switch from Debug to RelWithDebInfo:
> https://git.buildroot.org/buildroot/commit/?id=4b0120183404913f7f7788ef4f0f6b51498ef363
>
> - October 2016: switch from RelWithDebInfo back to Debug:
> https://git.buildroot.org/buildroot/commit/?id=104bb29e0490bfb487e2e665448dd3ca07fcc2b5
> and changes to make sure Buildroot's flags are respected:
> https://git.buildroot.org/buildroot/commit/?id=12494ef48f893684d0800e7f6fe39a2ceaed0451
>
> - August 2017: bug #10246 - "BR2_ENABLE_DEBUG does not have the expected
> effect for cmake packages"
> https://bugs.busybox.net/show_bug.cgi?id=10246
>
> - August 2017: mail thread following bug #10246:
> http://lists.busybox.net/pipermail/buildroot/2017-August/200778.html
>
> In the last mail thread, Samuel Martin confirmed that the 'Release' build
> type could be used in all cases, because Buildroot is actually making sure
> that the optimization flags are those determined by Buildroot, not the
> defaults of cmake, thanks to commit 12494ef48f.
> But Arnout Vandecappelle objected to using always 'Release', stating that
> users may actually want the extra assertions.
>
> With the introduction of BR2_ENABLE_RUNTIME_DEBUG, Buildroot can now cater
> for all cases:
>
> - use CMAKE_BUILD_TYPE=Release by default. This makes sure that there is no
> unexpected performance degradation triggered by enabling BR2_ENABLE_DEBUG.
>
> - users can optionally enable BR2_ENABLE_RUNTIME_DEBUG if they want runtime
> debug info like assertions, at the risk of introducing performance
> degradation. In this case, we switch to CMAKE_BUILD_TYPE=Debug.
>
> - orthogonally to the above, BR2_ENABLE_DEBUG still determines passing the
> '-g' flag to enable debug symbols, and BR2_OPTIMIZE_X still determines the
> used optimization flags.
>
> Signed-off-by: Thomas De Schampheleire <thomas.de_schampheleire@nokia.com>
> Acked-by: Arnout Vandecappelle (Essensium/Mind) <arnout@mind.be>
Reviewed-by: Yann E. MORIN <yann.morin.1998@free.fr>
Regards,
Yann E. MORIN.
> ---
> docs/manual/adding-packages-cmake.txt | 2 +-
> package/pkg-cmake.mk | 2 +-
> 2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/docs/manual/adding-packages-cmake.txt b/docs/manual/adding-packages-cmake.txt
> index 73f0943024..541d7422cf 100644
> --- a/docs/manual/adding-packages-cmake.txt
> +++ b/docs/manual/adding-packages-cmake.txt
> @@ -100,7 +100,7 @@ typical packages will therefore only use a few of them.
> necessary to set them in the package's +*.mk+ file unless you want
> to override them:
>
> -** +CMAKE_BUILD_TYPE+ is driven by +BR2_ENABLE_DEBUG+;
> +** +CMAKE_BUILD_TYPE+ is driven by +BR2_ENABLE_RUNTIME_DEBUG+;
> ** +CMAKE_INSTALL_PREFIX+;
> ** +BUILD_SHARED_LIBS+ is driven by +BR2_STATIC_LIBS+;
> ** +BUILD_DOC+, +BUILD_DOCS+ are disabled;
> diff --git a/package/pkg-cmake.mk b/package/pkg-cmake.mk
> index 37078f3c34..4ee100a0c6 100644
> --- a/package/pkg-cmake.mk
> +++ b/package/pkg-cmake.mk
> @@ -266,7 +266,7 @@ define TOOLCHAIN_CMAKE_INSTALL_FILES
> -e 's#@@CMAKE_SYSTEM_PROCESSOR@@#$(call qstrip,$(CMAKE_SYSTEM_PROCESSOR))#' \
> -e 's#@@TOOLCHAIN_HAS_CXX@@#$(if $(BR2_INSTALL_LIBSTDCPP),1,0)#' \
> -e 's#@@TOOLCHAIN_HAS_FORTRAN@@#$(if $(BR2_TOOLCHAIN_HAS_FORTRAN),1,0)#' \
> - -e 's#@@CMAKE_BUILD_TYPE@@#$(if $(BR2_ENABLE_DEBUG),Debug,Release)#' \
> + -e 's#@@CMAKE_BUILD_TYPE@@#$(if $(BR2_ENABLE_RUNTIME_DEBUG),Debug,Release)#' \
> $(TOPDIR)/support/misc/toolchainfile.cmake.in \
> > $(HOST_DIR)/share/buildroot/toolchainfile.cmake
> $(Q)$(INSTALL) -D -m 0644 support/misc/Buildroot.cmake \
> --
> 2.26.3
>
> _______________________________________________
> buildroot mailing list
> buildroot at busybox.net
> http://lists.busybox.net/mailman/listinfo/buildroot
--
.-----------------.--------------------.------------------.--------------------.
| Yann E. MORIN | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software Designer | \ / CAMPAIGN | ___ |
| +33 561 099 427 `------------.-------: X AGAINST | \e/ There is no |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL | v conspiracy. |
'------------------------------^-------^------------------^--------------------'
^ permalink raw reply [flat|nested] 31+ messages in thread
* [Buildroot] [PATCHv4 04/16] package/flare-engine: disable effect of CMAKE_BUILD_TYPE
2021-06-01 14:34 [Buildroot] [PATCHv4 00/16] Introduce BR2_ENABLE_RUNTIME_DEBUG Thomas De Schampheleire
` (2 preceding siblings ...)
2021-06-01 14:34 ` [Buildroot] [PATCHv4 03/16] package/pkg-cmake.mk: determine CMAKE_BUILD_TYPE depending on BR2_ENABLE_RUNTIME_DEBUG Thomas De Schampheleire
@ 2021-06-01 14:34 ` Thomas De Schampheleire
2021-06-01 14:34 ` [Buildroot] [PATCHv4 05/16] package/sysrepo: use default CMAKE_BUILD_TYPE for host package Thomas De Schampheleire
` (12 subsequent siblings)
16 siblings, 0 replies; 31+ messages in thread
From: Thomas De Schampheleire @ 2021-06-01 14:34 UTC (permalink / raw)
To: buildroot
From: Thomas De Schampheleire <thomas.de_schampheleire@nokia.com>
flare-engine enables profiling when CMAKE_BUILD_TYPE is 'Debug'. The
Buildroot package explicitly avoided that by forcing CMAKE_BUILD_TYPE to
'RelWithDebInfo' when pkg-cmake.mk would normally set it to 'Debug'. Until
recently, this was the case when BR2_ENABLE_DEBUG was enabled.
A previous commit changed the condition under which CMAKE_BUILD_TYPE=Debug
was set, from BR2_ENABLE_DEBUG=y to BR2_ENABLE_RUNTIME_DEBUG=y, so logically
the flare-engine package would have to be updated accordingly.
However, apart from the profiling flag, the flare-engine package only uses
CMAKE_BUILD_TYPE to determine flags that Buildroot wants to control itself,
like optimization and debugging flags.
This means we can fake CMAKE_BUILD_TYPE to a value that has no meaning for
flare-engine itself, without needing to check BR2_ENABLE_DEBUG nor
BR2_ENABLE_RUNTIME_DEBUG.
Incidentally, this trick was already done in case
BR2_TOOLCHAIN_HAS_GCC_BUG_85180 was true, so move that line out of this
condition.
Signed-off-by: Thomas De Schampheleire <thomas.de_schampheleire@nokia.com>
---
v4:
- use a custom build type as suggested by Arnout.
package/flare-engine/flare-engine.mk | 12 ++++--------
1 file changed, 4 insertions(+), 8 deletions(-)
diff --git a/package/flare-engine/flare-engine.mk b/package/flare-engine/flare-engine.mk
index e2f3eefa28..b91fc16af3 100644
--- a/package/flare-engine/flare-engine.mk
+++ b/package/flare-engine/flare-engine.mk
@@ -14,16 +14,12 @@ FLARE_ENGINE_DEPENDENCIES += sdl2 sdl2_image sdl2_mixer sdl2_ttf
# Don't use /usr/games and /usr/share/games
FLARE_ENGINE_CONF_OPTS += -DBINDIR=bin -DDATADIR=share/flare
-# Don't use the default Debug type as it adds -pg (gprof)
-ifeq ($(BR2_ENABLE_DEBUG),y)
-FLARE_ENGINE_CONF_OPTS += -DCMAKE_BUILD_TYPE=RelWithDebInfo
-endif
+# CMAKE_BUILD_TYPE is only used to set optimization and debug flags, all of
+# which we want Buildroot to steer explicitly. Explicitly set a fake build type
+# to get this control.
+FLARE_ENGINE_CONF_OPTS += -DCMAKE_BUILD_TYPE=Buildroot
ifeq ($(BR2_TOOLCHAIN_HAS_GCC_BUG_85180),y)
-# CMakeLists.txt sets CMAKE_CXX_FLAGS_<BUILD_TYPE> depending on
-# BUILD_TYPE, and this comes after the generic CMAKE_CXX_FLAGS.
-# Override CMAKE_BUILD_TYPE so no overrides are applied.
-FLARE_ENGINE_CONF_OPTS += -DCMAKE_BUILD_TYPE=Buildroot
FLARE_ENGINE_CONF_OPTS += -DCMAKE_CXX_FLAGS="$(TARGET_CXXFLAGS) -O0"
endif
--
2.26.3
^ permalink raw reply related [flat|nested] 31+ messages in thread* [Buildroot] [PATCHv4 05/16] package/sysrepo: use default CMAKE_BUILD_TYPE for host package
2021-06-01 14:34 [Buildroot] [PATCHv4 00/16] Introduce BR2_ENABLE_RUNTIME_DEBUG Thomas De Schampheleire
` (3 preceding siblings ...)
2021-06-01 14:34 ` [Buildroot] [PATCHv4 04/16] package/flare-engine: disable effect of CMAKE_BUILD_TYPE Thomas De Schampheleire
@ 2021-06-01 14:34 ` Thomas De Schampheleire
2021-06-01 14:34 ` [Buildroot] [PATCHv4 06/16] package/sysrepo: remove explicit setting of CMAKE_BUILD_TYPE Thomas De Schampheleire
` (11 subsequent siblings)
16 siblings, 0 replies; 31+ messages in thread
From: Thomas De Schampheleire @ 2021-06-01 14:34 UTC (permalink / raw)
To: buildroot
From: Thomas De Schampheleire <thomas.de_schampheleire@nokia.com>
Use the default build type for host-sysrepo.
In the current version of sysrepo, this happens to be 'Debug'.
As 'Debug' also enables tests, explicitly disable them.
Suggested-by: Arnout Vandecappelle (Essensium/Mind) <arnout@mind.be>
Signed-off-by: Thomas De Schampheleire <thomas.de_schampheleire@nokia.com>
Acked-by: Arnout Vandecappelle (Essensium/Mind) <arnout@mind.be>
Acked-by: Heiko Thiery <heiko.thiery@gmail.com>
---
package/sysrepo/sysrepo.mk | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/package/sysrepo/sysrepo.mk b/package/sysrepo/sysrepo.mk
index dbc7b16b63..f5be29d2c9 100644
--- a/package/sysrepo/sysrepo.mk
+++ b/package/sysrepo/sysrepo.mk
@@ -26,8 +26,9 @@ define SYSREPO_INSTALL_INIT_SYSV
endef
HOST_SYSREPO_CONF_OPTS = \
- -DCMAKE_BUILD_TYPE=Release \
-DBUILD_EXAMPLES=OFF \
+ -DENABLE_TESTS=OFF \
+ -DENABLE_VALGRIND_TESTS=OFF \
-DREPO_PATH=$(TARGET_DIR)/etc/sysrepo
$(eval $(cmake-package))
--
2.26.3
^ permalink raw reply related [flat|nested] 31+ messages in thread* [Buildroot] [PATCHv4 06/16] package/sysrepo: remove explicit setting of CMAKE_BUILD_TYPE
2021-06-01 14:34 [Buildroot] [PATCHv4 00/16] Introduce BR2_ENABLE_RUNTIME_DEBUG Thomas De Schampheleire
` (4 preceding siblings ...)
2021-06-01 14:34 ` [Buildroot] [PATCHv4 05/16] package/sysrepo: use default CMAKE_BUILD_TYPE for host package Thomas De Schampheleire
@ 2021-06-01 14:34 ` Thomas De Schampheleire
2021-06-01 14:34 ` [Buildroot] [PATCHv4 07/16] package/boost: use BR2_ENABLE_RUNTIME_DEBUG iso BR2_ENABLE_DEBUG Thomas De Schampheleire
` (10 subsequent siblings)
16 siblings, 0 replies; 31+ messages in thread
From: Thomas De Schampheleire @ 2021-06-01 14:34 UTC (permalink / raw)
To: buildroot
From: Thomas De Schampheleire <thomas.de_schampheleire@nokia.com>
sysrepo explicitly sets CMAKE_BUILD_TYPE=Release, ignoring any possible
value of BR2_ENABLE_DEBUG (previously) or BR2_ENABLE_RUNTIME_DEBUG (now).
With the introduction of BR2_ENABLE_RUNTIME_DEBUG, this change should no
longer be necessary. Users that do not wish to have additional runtime
debugging just keep BR2_ENABLE_RUNTIME_DEBUG disabled (default value).
As the 'Debug' build type enables tests, disable them explicitly.
As the 'Debug' build type uses a custom REPO_PATH which does not exist on
target, force /etc/sysrepo like in the 'Release' build type.
Signed-off-by: Thomas De Schampheleire <thomas.de_schampheleire@nokia.com>
Acked-by: Arnout Vandecappelle (Essensium/Mind) <arnout@mind.be>
Acked-by: Heiko Thiery <heiko.thiery@gmail.com>
---
package/sysrepo/sysrepo.mk | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/package/sysrepo/sysrepo.mk b/package/sysrepo/sysrepo.mk
index f5be29d2c9..c0db05e52c 100644
--- a/package/sysrepo/sysrepo.mk
+++ b/package/sysrepo/sysrepo.mk
@@ -13,8 +13,10 @@ SYSREPO_DEPENDENCIES = libyang pcre host-sysrepo
HOST_SYSREPO_DEPENDENCIES = host-libyang host-pcre
SYSREPO_CONF_OPTS = \
- -DCMAKE_BUILD_TYPE=Release \
- -DBUILD_EXAMPLES=$(if $(BR2_PACKAGE_SYSREPO_EXAMPLES),ON,OFF)
+ -DBUILD_EXAMPLES=$(if $(BR2_PACKAGE_SYSREPO_EXAMPLES),ON,OFF) \
+ -DENABLE_TESTS=OFF \
+ -DENABLE_VALGRIND_TESTS=OFF \
+ -DREPO_PATH=/etc/sysrepo
ifeq ($(BR2_TOOLCHAIN_HAS_LIBATOMIC),y)
SYSREPO_CONF_OPTS += -DCMAKE_EXE_LINKER_FLAGS=-latomic
--
2.26.3
^ permalink raw reply related [flat|nested] 31+ messages in thread* [Buildroot] [PATCHv4 07/16] package/boost: use BR2_ENABLE_RUNTIME_DEBUG iso BR2_ENABLE_DEBUG
2021-06-01 14:34 [Buildroot] [PATCHv4 00/16] Introduce BR2_ENABLE_RUNTIME_DEBUG Thomas De Schampheleire
` (5 preceding siblings ...)
2021-06-01 14:34 ` [Buildroot] [PATCHv4 06/16] package/sysrepo: remove explicit setting of CMAKE_BUILD_TYPE Thomas De Schampheleire
@ 2021-06-01 14:34 ` Thomas De Schampheleire
2021-06-01 14:34 ` [Buildroot] [PATCHv4 08/16] package/oracle-mysql: " Thomas De Schampheleire
` (9 subsequent siblings)
16 siblings, 0 replies; 31+ messages in thread
From: Thomas De Schampheleire @ 2021-06-01 14:34 UTC (permalink / raw)
To: buildroot
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/boost/boost.mk | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/package/boost/boost.mk b/package/boost/boost.mk
index 8d745ecc72..e72ddf897c 100644
--- a/package/boost/boost.mk
+++ b/package/boost/boost.mk
@@ -95,7 +95,7 @@ BOOST_OPTS += --no-cmake-config \
toolset=gcc \
threading=multi \
abi=$(BOOST_ABI) \
- variant=$(if $(BR2_ENABLE_DEBUG),debug,release)
+ variant=$(if $(BR2_ENABLE_RUNTIME_DEBUG),debug,release)
ifeq ($(BR2_sparc64),y)
BOOST_OPTS += architecture=sparc instruction-set=ultrasparc
--
2.26.3
^ permalink raw reply related [flat|nested] 31+ messages in thread* [Buildroot] [PATCHv4 08/16] package/oracle-mysql: use BR2_ENABLE_RUNTIME_DEBUG iso BR2_ENABLE_DEBUG
2021-06-01 14:34 [Buildroot] [PATCHv4 00/16] Introduce BR2_ENABLE_RUNTIME_DEBUG Thomas De Schampheleire
` (6 preceding siblings ...)
2021-06-01 14:34 ` [Buildroot] [PATCHv4 07/16] package/boost: use BR2_ENABLE_RUNTIME_DEBUG iso BR2_ENABLE_DEBUG Thomas De Schampheleire
@ 2021-06-01 14:34 ` Thomas De Schampheleire
2021-06-01 14:34 ` [Buildroot] [PATCHv4 09/16] package/qt5: " Thomas De Schampheleire
` (8 subsequent siblings)
16 siblings, 0 replies; 31+ messages in thread
From: Thomas De Schampheleire @ 2021-06-01 14:34 UTC (permalink / raw)
To: buildroot
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>
Acked-by: Arnout Vandecappelle (Essensium/Mind) <arnout@mind.be>
---
package/oracle-mysql/oracle-mysql.mk | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/package/oracle-mysql/oracle-mysql.mk b/package/oracle-mysql/oracle-mysql.mk
index ccfa40cfb1..e7d6354ed8 100644
--- a/package/oracle-mysql/oracle-mysql.mk
+++ b/package/oracle-mysql/oracle-mysql.mk
@@ -95,7 +95,7 @@ ORACLE_MYSQL_CONF_OPTS += \
# Debugging is only available for the server, so no need for
# this if-block outside of the server if-block
-ifeq ($(BR2_ENABLE_DEBUG),y)
+ifeq ($(BR2_ENABLE_RUNTIME_DEBUG),y)
ORACLE_MYSQL_CONF_OPTS += --with-debug=full
else
ORACLE_MYSQL_CONF_OPTS += --without-debug
--
2.26.3
^ permalink raw reply related [flat|nested] 31+ messages in thread* [Buildroot] [PATCHv4 09/16] package/qt5: use BR2_ENABLE_RUNTIME_DEBUG iso BR2_ENABLE_DEBUG
2021-06-01 14:34 [Buildroot] [PATCHv4 00/16] Introduce BR2_ENABLE_RUNTIME_DEBUG Thomas De Schampheleire
` (7 preceding siblings ...)
2021-06-01 14:34 ` [Buildroot] [PATCHv4 08/16] package/oracle-mysql: " Thomas De Schampheleire
@ 2021-06-01 14:34 ` Thomas De Schampheleire
2021-06-01 14:34 ` [Buildroot] [PATCHv4 10/16] package/ripgrep: " Thomas De Schampheleire
` (7 subsequent siblings)
16 siblings, 0 replies; 31+ messages in thread
From: Thomas De Schampheleire @ 2021-06-01 14:34 UTC (permalink / raw)
To: buildroot
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>
Acked-by: Arnout Vandecappelle (Essensium/Mind) <arnout@mind.be>
---
package/qt5/qt5base/qt5base.mk | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/package/qt5/qt5base/qt5base.mk b/package/qt5/qt5base/qt5base.mk
index 84e9fa4edb..8b55aa3098 100644
--- a/package/qt5/qt5base/qt5base.mk
+++ b/package/qt5/qt5base/qt5base.mk
@@ -86,7 +86,7 @@ else
QT5BASE_CONFIGURE_OPTS += -no-gbm
endif
-ifeq ($(BR2_ENABLE_DEBUG),y)
+ifeq ($(BR2_ENABLE_RUNTIME_DEBUG),y)
QT5BASE_CONFIGURE_OPTS += -debug
else
QT5BASE_CONFIGURE_OPTS += -release
--
2.26.3
^ permalink raw reply related [flat|nested] 31+ messages in thread* [Buildroot] [PATCHv4 10/16] package/ripgrep: use BR2_ENABLE_RUNTIME_DEBUG iso BR2_ENABLE_DEBUG
2021-06-01 14:34 [Buildroot] [PATCHv4 00/16] Introduce BR2_ENABLE_RUNTIME_DEBUG Thomas De Schampheleire
` (8 preceding siblings ...)
2021-06-01 14:34 ` [Buildroot] [PATCHv4 09/16] package/qt5: " Thomas De Schampheleire
@ 2021-06-01 14:34 ` Thomas De Schampheleire
2021-06-01 14:34 ` [Buildroot] [PATCHv4 11/16] package/sofia-sip: correct passing of '--enable-ndebug' Thomas De Schampheleire
` (6 subsequent siblings)
16 siblings, 0 replies; 31+ messages in thread
From: Thomas De Schampheleire @ 2021-06-01 14:34 UTC (permalink / raw)
To: buildroot
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>
Acked-by: Arnout Vandecappelle (Essensium/Mind) <arnout@mind.be>
---
package/ripgrep/ripgrep.mk | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/package/ripgrep/ripgrep.mk b/package/ripgrep/ripgrep.mk
index 9dd8d58de1..c3c12fd185 100644
--- a/package/ripgrep/ripgrep.mk
+++ b/package/ripgrep/ripgrep.mk
@@ -18,7 +18,7 @@ RIPGREP_CARGO_OPTS = \
--target=$(RUSTC_TARGET_NAME) \
--manifest-path=$(@D)/Cargo.toml
-ifeq ($(BR2_ENABLE_DEBUG),y)
+ifeq ($(BR2_ENABLE_RUNTIME_DEBUG),y)
RIPGREP_CARGO_BIN_SUBDIR = debug
else
RIPGREP_CARGO_OPTS += --release
--
2.26.3
^ permalink raw reply related [flat|nested] 31+ messages in thread* [Buildroot] [PATCHv4 11/16] package/sofia-sip: correct passing of '--enable-ndebug'
2021-06-01 14:34 [Buildroot] [PATCHv4 00/16] Introduce BR2_ENABLE_RUNTIME_DEBUG Thomas De Schampheleire
` (9 preceding siblings ...)
2021-06-01 14:34 ` [Buildroot] [PATCHv4 10/16] package/ripgrep: " Thomas De Schampheleire
@ 2021-06-01 14:34 ` Thomas De Schampheleire
2021-06-01 14:34 ` [Buildroot] [PATCHv4 12/16] package/sofia-sip: don't set 'NDEBUG' explicitly Thomas De Schampheleire
` (5 subsequent siblings)
16 siblings, 0 replies; 31+ messages in thread
From: Thomas De Schampheleire @ 2021-06-01 14:34 UTC (permalink / raw)
To: buildroot
From: Thomas De Schampheleire <thomas.de_schampheleire@nokia.com>
The preprocessor option NDEBUG, triggered by the configure option
'--enable-ndebug', should be read as 'no-debug'. When NDEBUG is set, asserts
are _disabled_.
The sofia-sip package had inverted logic, and set '--enable-ndebug' when
BR2_ENABLE_DEBUG was enabled, while it should be the other way around.
Reported-by: Arnout Vandecappelle (Essensium/Mind) <arnout@mind.be>
Signed-off-by: Thomas De Schampheleire <thomas.de_schampheleire@nokia.com>
---
v4: new patch;
Even though the next patch will remove this block altogether, I keep this
correction as a separate patch to make it clear that the original code was
incorrect, and allow potential backporting to other branches.
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..274c72fce8 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_DEBUG),)
SOFIA_SIP_CONF_OPTS += --enable-ndebug
endif
--
2.26.3
^ permalink raw reply related [flat|nested] 31+ messages in thread* [Buildroot] [PATCHv4 12/16] package/sofia-sip: don't set 'NDEBUG' explicitly
2021-06-01 14:34 [Buildroot] [PATCHv4 00/16] Introduce BR2_ENABLE_RUNTIME_DEBUG Thomas De Schampheleire
` (10 preceding siblings ...)
2021-06-01 14:34 ` [Buildroot] [PATCHv4 11/16] package/sofia-sip: correct passing of '--enable-ndebug' Thomas De Schampheleire
@ 2021-06-01 14:34 ` Thomas De Schampheleire
2021-06-01 14:34 ` [Buildroot] [PATCHv4 13/16] package/zmqpp: don't set CONFIG=debug Thomas De Schampheleire
` (4 subsequent siblings)
16 siblings, 0 replies; 31+ messages in thread
From: Thomas De Schampheleire @ 2021-06-01 14:34 UTC (permalink / raw)
To: buildroot
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.
The sofia-sip package was setting '--enable-ndebug' conditionally based on
BR2_ENABLE_DEBUG, and this would have to be updated to be based on
BR2_ENABLE_RUNTIME_DEBUG.
However, the sofia-sip option '--enable-ndebug' only sets the 'NDEBUG'
preprocessor macro, and the core package infrastructure already sets this
macro correctly based on BR2_ENABLE_RUNTIME_DEBUG.
This means that the explicit '--enable-ndebug' flag can be removed.
Suggested-by: Arnout Vandecappelle (Essensium/Mind) <arnout@mind.be>
Signed-off-by: Thomas De Schampheleire <thomas.de_schampheleire@nokia.com>
---
package/sofia-sip/sofia-sip.mk | 4 ----
1 file changed, 4 deletions(-)
diff --git a/package/sofia-sip/sofia-sip.mk b/package/sofia-sip/sofia-sip.mk
index 274c72fce8..fb565637eb 100644
--- a/package/sofia-sip/sofia-sip.mk
+++ b/package/sofia-sip/sofia-sip.mk
@@ -30,8 +30,4 @@ SOFIA_SIP_CONF_OPTS += \
--without-openssl
endif
-ifeq ($(BR2_ENABLE_DEBUG),)
-SOFIA_SIP_CONF_OPTS += --enable-ndebug
-endif
-
$(eval $(autotools-package))
--
2.26.3
^ permalink raw reply related [flat|nested] 31+ messages in thread* [Buildroot] [PATCHv4 13/16] package/zmqpp: don't set CONFIG=debug
2021-06-01 14:34 [Buildroot] [PATCHv4 00/16] Introduce BR2_ENABLE_RUNTIME_DEBUG Thomas De Schampheleire
` (11 preceding siblings ...)
2021-06-01 14:34 ` [Buildroot] [PATCHv4 12/16] package/sofia-sip: don't set 'NDEBUG' explicitly Thomas De Schampheleire
@ 2021-06-01 14:34 ` Thomas De Schampheleire
2021-06-01 14:34 ` [Buildroot] [PATCHv4 14/16] package/zmqpp: use BR2_ENABLE_RUNTIME_DEBUG iso BR2_ENABLE_DEBUG Thomas De Schampheleire
` (3 subsequent siblings)
16 siblings, 0 replies; 31+ messages in thread
From: Thomas De Schampheleire @ 2021-06-01 14:34 UTC (permalink / raw)
To: buildroot
From: Thomas De Schampheleire <thomas.de_schampheleire@nokia.com>
Below are the flags set by zmqpp depending on the specified CONFIG variable:
CONFIG_FLAGS =
ifeq ($(CONFIG),debug)
CONFIG_FLAGS = -g -fno-inline -ftemplate-depth-1000
endif
ifeq ($(CONFIG),valgrind)
CONFIG_FLAGS = -g -O1 -DNO_DEBUG_LOG -DNO_TRACE_LOG
endif
ifeq ($(CONFIG),max)
CONFIG_FLAGS = -O3 -funroll-loops -ffast-math -finline-functions \
-fomit-frame-pointer -DNDEBUG
endif
ifneq (,$(findstring $(CONFIG),release loadtest))
CONFIG_FLAGS = -O3 -funroll-loops -ffast-math -finline-functions \
-fomit-frame-pointer -DNO_DEBUG_LOG -DNO_TRACE_LOG -DNDEBUG
endif
For the flags added with CONFIG=debug, '-g' is to be steered by the core
infrastructure (could be '-g1', '-g2' etc.)
The flag '-ftemplate-depth' is only a protection against incorrect code and
not really needed in Buildroot context.
Finally, the flag '-fno-inline' may be useful when really stepping through
zmqpp code, but is a very specific use case.
With the above in mind, not passing CONFIG=debug may actually be better.
Use 'CONFIG=buildroot' instead.
Note that we don't pass an empty 'CONFIG' to avoid confusion, as this
variable is also passed through to the variable BUILD_ENV, even though it is
currently unused.
Signed-off-by: Thomas De Schampheleire <thomas.de_schampheleire@nokia.com>
---
package/zmqpp/zmqpp.mk | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/package/zmqpp/zmqpp.mk b/package/zmqpp/zmqpp.mk
index 3cd19d644a..c629f14920 100644
--- a/package/zmqpp/zmqpp.mk
+++ b/package/zmqpp/zmqpp.mk
@@ -19,7 +19,7 @@ ZMQPP_LDFLAGS = $(TARGET_LDFLAGS) -lpthread
# -ffast-math -finline-functions -fomit-frame-pointer are disabled,
# so only set CONFIG for the non-affected cases.
ifneq ($(BR2_or1k):$(BR2_TOOLCHAIN_GCC_AT_LEAST_6),y:)
-ZMQPP_CONFIG = $(if $(BR2_ENABLE_DEBUG),debug,release)
+ZMQPP_CONFIG = $(if $(BR2_ENABLE_DEBUG),buildroot,release)
endif
ifeq ($(BR2_TOOLCHAIN_HAS_LIBATOMIC),y)
--
2.26.3
^ permalink raw reply related [flat|nested] 31+ messages in thread* [Buildroot] [PATCHv4 14/16] package/zmqpp: use BR2_ENABLE_RUNTIME_DEBUG iso BR2_ENABLE_DEBUG
2021-06-01 14:34 [Buildroot] [PATCHv4 00/16] Introduce BR2_ENABLE_RUNTIME_DEBUG Thomas De Schampheleire
` (12 preceding siblings ...)
2021-06-01 14:34 ` [Buildroot] [PATCHv4 13/16] package/zmqpp: don't set CONFIG=debug Thomas De Schampheleire
@ 2021-06-01 14:34 ` Thomas De Schampheleire
2021-06-01 14:34 ` [Buildroot] [PATCHv4 15/16] package/pkg-meson.mk: determine 'buildtype' based on " Thomas De Schampheleire
` (2 subsequent siblings)
16 siblings, 0 replies; 31+ messages in thread
From: Thomas De Schampheleire @ 2021-06-01 14:34 UTC (permalink / raw)
To: buildroot
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/zmqpp/zmqpp.mk | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/package/zmqpp/zmqpp.mk b/package/zmqpp/zmqpp.mk
index c629f14920..32fabf26ef 100644
--- a/package/zmqpp/zmqpp.mk
+++ b/package/zmqpp/zmqpp.mk
@@ -19,7 +19,7 @@ ZMQPP_LDFLAGS = $(TARGET_LDFLAGS) -lpthread
# -ffast-math -finline-functions -fomit-frame-pointer are disabled,
# so only set CONFIG for the non-affected cases.
ifneq ($(BR2_or1k):$(BR2_TOOLCHAIN_GCC_AT_LEAST_6),y:)
-ZMQPP_CONFIG = $(if $(BR2_ENABLE_DEBUG),buildroot,release)
+ZMQPP_CONFIG = $(if $(BR2_ENABLE_RUNTIME_DEBUG),buildroot,release)
endif
ifeq ($(BR2_TOOLCHAIN_HAS_LIBATOMIC),y)
--
2.26.3
^ permalink raw reply related [flat|nested] 31+ messages in thread* [Buildroot] [PATCHv4 15/16] package/pkg-meson.mk: determine 'buildtype' based on BR2_ENABLE_RUNTIME_DEBUG iso BR2_ENABLE_DEBUG
2021-06-01 14:34 [Buildroot] [PATCHv4 00/16] Introduce BR2_ENABLE_RUNTIME_DEBUG Thomas De Schampheleire
` (13 preceding siblings ...)
2021-06-01 14:34 ` [Buildroot] [PATCHv4 14/16] package/zmqpp: use BR2_ENABLE_RUNTIME_DEBUG iso BR2_ENABLE_DEBUG Thomas De Schampheleire
@ 2021-06-01 14:34 ` Thomas De Schampheleire
2021-06-01 20:30 ` Yann E. MORIN
2021-06-01 14:34 ` [Buildroot] [PATCHv4 16/16] utils/genrandconfig: also test BR2_ENABLE_RUNTIME_DEBUG Thomas De Schampheleire
2021-06-01 20:54 ` [Buildroot] [PATCHv4 00/16] Introduce BR2_ENABLE_RUNTIME_DEBUG Yann E. MORIN
16 siblings, 1 reply; 31+ messages in thread
From: Thomas De Schampheleire @ 2021-06-01 14:34 UTC (permalink / raw)
To: buildroot
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/pkg-meson.mk | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/package/pkg-meson.mk b/package/pkg-meson.mk
index a57820d4d2..dafad3b1eb 100644
--- a/package/pkg-meson.mk
+++ b/package/pkg-meson.mk
@@ -89,7 +89,7 @@ define $(2)_CONFIGURE_CMDS
--prefix=/usr \
--libdir=lib \
--default-library=$(if $(BR2_STATIC_LIBS),static,shared) \
- --buildtype=$(if $(BR2_ENABLE_DEBUG),debug,release) \
+ --buildtype=$(if $(BR2_ENABLE_RUNTIME_DEBUG),debug,release) \
--cross-file=$$($$(PKG)_SRCDIR)/build/cross-compilation.conf \
-Dstrip=false \
-Dbuild.pkg_config_path=$$(HOST_DIR)/lib/pkgconfig \
--
2.26.3
^ permalink raw reply related [flat|nested] 31+ messages in thread* [Buildroot] [PATCHv4 15/16] package/pkg-meson.mk: determine 'buildtype' based on BR2_ENABLE_RUNTIME_DEBUG iso BR2_ENABLE_DEBUG
2021-06-01 14:34 ` [Buildroot] [PATCHv4 15/16] package/pkg-meson.mk: determine 'buildtype' based on " Thomas De Schampheleire
@ 2021-06-01 20:30 ` Yann E. MORIN
2021-06-01 21:13 ` Arnout Vandecappelle
0 siblings, 1 reply; 31+ messages in thread
From: Yann E. MORIN @ 2021-06-01 20:30 UTC (permalink / raw)
To: buildroot
Thomas, All,
Eric, and other meson experts: question for you below...
On 2021-06-01 16:34 +0200, Thomas De Schampheleire spake thusly:
> 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/pkg-meson.mk | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/package/pkg-meson.mk b/package/pkg-meson.mk
> index a57820d4d2..dafad3b1eb 100644
> --- a/package/pkg-meson.mk
> +++ b/package/pkg-meson.mk
> @@ -89,7 +89,7 @@ define $(2)_CONFIGURE_CMDS
> --prefix=/usr \
> --libdir=lib \
> --default-library=$(if $(BR2_STATIC_LIBS),static,shared) \
> - --buildtype=$(if $(BR2_ENABLE_DEBUG),debug,release) \
> + --buildtype=$(if $(BR2_ENABLE_RUNTIME_DEBUG),debug,release) \
I agree on this specific change.
However, more globally: 'buildtype' governs two settings [0]: 'debug'
and 'optimisation'. The 'optimisation' part is already part of our
CFLAGS, so we don't need to drive it from meson.
So, I think that what we need, is to just set the 'debug' option, rather
than the full 'buildtype'.
Thoughts?
[0] https://mesonbuild.com/Builtin-options.html#core-options
> --cross-file=$$($$(PKG)_SRCDIR)/build/cross-compilation.conf \
> -Dstrip=false \
> -Dbuild.pkg_config_path=$$(HOST_DIR)/lib/pkgconfig \
> --
> 2.26.3
>
> _______________________________________________
> buildroot mailing list
> buildroot at busybox.net
> http://lists.busybox.net/mailman/listinfo/buildroot
--
.-----------------.--------------------.------------------.--------------------.
| Yann E. MORIN | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software Designer | \ / CAMPAIGN | ___ |
| +33 561 099 427 `------------.-------: X AGAINST | \e/ There is no |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL | v conspiracy. |
'------------------------------^-------^------------------^--------------------'
^ permalink raw reply [flat|nested] 31+ messages in thread
* [Buildroot] [PATCHv4 15/16] package/pkg-meson.mk: determine 'buildtype' based on BR2_ENABLE_RUNTIME_DEBUG iso BR2_ENABLE_DEBUG
2021-06-01 20:30 ` Yann E. MORIN
@ 2021-06-01 21:13 ` Arnout Vandecappelle
0 siblings, 0 replies; 31+ messages in thread
From: Arnout Vandecappelle @ 2021-06-01 21:13 UTC (permalink / raw)
To: buildroot
On 01/06/2021 22:30, Yann E. MORIN wrote:
> Thomas, All,
>
> Eric, and other meson experts: question for you below...
>
> On 2021-06-01 16:34 +0200, Thomas De Schampheleire spake thusly:
>> 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/pkg-meson.mk | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/package/pkg-meson.mk b/package/pkg-meson.mk
>> index a57820d4d2..dafad3b1eb 100644
>> --- a/package/pkg-meson.mk
>> +++ b/package/pkg-meson.mk
>> @@ -89,7 +89,7 @@ define $(2)_CONFIGURE_CMDS
>> --prefix=/usr \
>> --libdir=lib \
>> --default-library=$(if $(BR2_STATIC_LIBS),static,shared) \
>> - --buildtype=$(if $(BR2_ENABLE_DEBUG),debug,release) \
>> + --buildtype=$(if $(BR2_ENABLE_RUNTIME_DEBUG),debug,release) \
>
> I agree on this specific change.
>
> However, more globally: 'buildtype' governs two settings [0]: 'debug'
> and 'optimisation'. The 'optimisation' part is already part of our
> CFLAGS, so we don't need to drive it from meson.
>
> So, I think that what we need, is to just set the 'debug' option, rather
> than the full 'buildtype'.
I discussed this at length with Thomas in v3 of the patch.
Basically, buildtype governs 3 things (for gcc - the situation is vastly more
complicated on windows):
- debug
- optimisation
- b_ndebug
Ideally, we'd like to set buildtype to "do nothing", otherwise it's going to
set debug/optimisation flags that conflict with what we set in cflags. Of
course, there's no such option.
Alternatively, we set debug, optimisatoin and b_ndebug explicitly. However, for
debug there's just on or off, while we have levels. For optimisation, we'd have
to map our optimisation levels to the meson optimisation levels, so that's
possible. And b_ndebug is what we could set based on BR2_ENABLE_RUNTIME_DEBUG.
And if we in the end decide to set -DNDEBUG globally, then we could simply leave
b_ndebug alone. But this is a lot of complexity for not really any gain.
So Thomas and I said: let's keep things like they are at the moment, i.e. this
patch.
Regards,
Arnout
>
> Thoughts?
>
> [0] https://mesonbuild.com/Builtin-options.html#core-options
>
>> --cross-file=$$($$(PKG)_SRCDIR)/build/cross-compilation.conf \
>> -Dstrip=false \
>> -Dbuild.pkg_config_path=$$(HOST_DIR)/lib/pkgconfig \
>> --
>> 2.26.3
>>
>> _______________________________________________
>> buildroot mailing list
>> buildroot at busybox.net
>> http://lists.busybox.net/mailman/listinfo/buildroot
>
^ permalink raw reply [flat|nested] 31+ messages in thread
* [Buildroot] [PATCHv4 16/16] utils/genrandconfig: also test BR2_ENABLE_RUNTIME_DEBUG
2021-06-01 14:34 [Buildroot] [PATCHv4 00/16] Introduce BR2_ENABLE_RUNTIME_DEBUG Thomas De Schampheleire
` (14 preceding siblings ...)
2021-06-01 14:34 ` [Buildroot] [PATCHv4 15/16] package/pkg-meson.mk: determine 'buildtype' based on " Thomas De Schampheleire
@ 2021-06-01 14:34 ` Thomas De Schampheleire
2021-06-01 20:54 ` [Buildroot] [PATCHv4 00/16] Introduce BR2_ENABLE_RUNTIME_DEBUG Yann E. MORIN
16 siblings, 0 replies; 31+ messages in thread
From: Thomas De Schampheleire @ 2021-06-01 14:34 UTC (permalink / raw)
To: buildroot
From: Thomas De Schampheleire <thomas.de_schampheleire@nokia.com>
... similar to BR2_ENABLE_DEBUG.
Signed-off-by: Thomas De Schampheleire <thomas.de_schampheleire@nokia.com>
Acked-by: Arnout Vandecappelle (Essensium/Mind) <arnout@mind.be>
---
utils/genrandconfig | 2 ++
1 file changed, 2 insertions(+)
diff --git a/utils/genrandconfig b/utils/genrandconfig
index 93dc6d898b..ca3e77e3e3 100755
--- a/utils/genrandconfig
+++ b/utils/genrandconfig
@@ -357,6 +357,8 @@ def gen_config(args):
# Amend the configuration with a few things.
if randint(0, 20) == 0:
configlines.append("BR2_ENABLE_DEBUG=y\n")
+ if randint(0, 20) == 0:
+ configlines.append("BR2_ENABLE_RUNTIME_DEBUG=y\n")
if randint(0, 1) == 0:
configlines.append("BR2_INIT_BUSYBOX=y\n")
elif randint(0, 15) == 0:
--
2.26.3
^ permalink raw reply related [flat|nested] 31+ messages in thread* [Buildroot] [PATCHv4 00/16] Introduce BR2_ENABLE_RUNTIME_DEBUG
2021-06-01 14:34 [Buildroot] [PATCHv4 00/16] Introduce BR2_ENABLE_RUNTIME_DEBUG Thomas De Schampheleire
` (15 preceding siblings ...)
2021-06-01 14:34 ` [Buildroot] [PATCHv4 16/16] utils/genrandconfig: also test BR2_ENABLE_RUNTIME_DEBUG Thomas De Schampheleire
@ 2021-06-01 20:54 ` Yann E. MORIN
16 siblings, 0 replies; 31+ messages in thread
From: Yann E. MORIN @ 2021-06-01 20:54 UTC (permalink / raw)
To: buildroot
Thomas, All,
On 2021-06-01 16:34 +0200, Thomas De Schampheleire spake thusly:
> This patch series started with as main goal to fix the performance degradation
> found when testing gRPC in combination with BR2_ENABLE_DEBUG.
>
> Its implementation strives to settle a discussion that has happened several
> times, regarding the CMAKE_BUILD_TYPE that Buildroot should set, by not forcing
> anything upon the user but allowing the choice.
>
> Additionally, it cleans up some related changes done in specific packages.
>
> After this series, there are still packages that set CMAKE_BUILD_TYPE
> explicitly, but the associated comments indicate that they are needed to avoid
> build failures, so they are not touched.
I had started to add my rev-tag to individual commits, until Arnout just
suggested I apply the full series instead. So I did just that: applied
to next, thanks.
However, please see a little comment about the penultimate commit in
your series, about the meson package infra...
Regards,
Yann E. MORIN.
> v2: fix sysrepo after comment by Jan Kundr?t (thanks!)
> v3:
> - new patch setting -g0 (Arnout)
> - don't set -DNDEBUG globally anymore, and drop related libjson change
> - flare-engine, supertux: retain disabling of -pg option (Arnout)
> - sysrepo: new patch for host-sysrepo, disable tests for target (Arnout)
> - update several packages to use BR2_ENABLE_RUNTIME_DEBUG instead of
> BR2_ENABLE_DEBUG
> v4:
> - drop uclibc patch (Arnout)
> - reintroduce global -DNDEBUG
> - sofia-sip: fix then remove the handling of NDEBUG
> - zmqpp: change buildtype to 'buildroot' in case of BR2_ENABLE_RUNTIME_DEBUG
>
> Thomas De Schampheleire (16):
> core: introduce BR2_ENABLE_RUNTIME_DEBUG
> core: enable 'NDEBUG' unless BR2_ENABLE_RUNTIME_DEBUG is set
> package/pkg-cmake.mk: determine CMAKE_BUILD_TYPE depending on
> BR2_ENABLE_RUNTIME_DEBUG
> package/flare-engine: disable effect of CMAKE_BUILD_TYPE
> package/sysrepo: use default CMAKE_BUILD_TYPE for host package
> package/sysrepo: remove explicit setting of CMAKE_BUILD_TYPE
> package/boost: use BR2_ENABLE_RUNTIME_DEBUG iso BR2_ENABLE_DEBUG
> package/oracle-mysql: use BR2_ENABLE_RUNTIME_DEBUG iso
> BR2_ENABLE_DEBUG
> package/qt5: use BR2_ENABLE_RUNTIME_DEBUG iso BR2_ENABLE_DEBUG
> package/ripgrep: use BR2_ENABLE_RUNTIME_DEBUG iso BR2_ENABLE_DEBUG
> package/sofia-sip: correct passing of '--enable-ndebug'
> package/sofia-sip: don't set 'NDEBUG' explicitly
> package/zmqpp: don't set CONFIG=debug
> package/zmqpp: use BR2_ENABLE_RUNTIME_DEBUG iso BR2_ENABLE_DEBUG
> package/pkg-meson.mk: determine 'buildtype' based on
> BR2_ENABLE_RUNTIME_DEBUG iso BR2_ENABLE_DEBUG
> utils/genrandconfig: also test BR2_ENABLE_RUNTIME_DEBUG
>
> Config.in | 13 +++++++++++++
> docs/manual/adding-packages-cmake.txt | 2 +-
> package/Makefile.in | 3 +++
> package/boost/boost.mk | 2 +-
> package/flare-engine/flare-engine.mk | 12 ++++--------
> package/oracle-mysql/oracle-mysql.mk | 2 +-
> package/pkg-cmake.mk | 2 +-
> package/pkg-meson.mk | 2 +-
> package/qt5/qt5base/qt5base.mk | 2 +-
> package/ripgrep/ripgrep.mk | 2 +-
> package/sofia-sip/sofia-sip.mk | 4 ----
> package/sysrepo/sysrepo.mk | 9 ++++++---
> package/zmqpp/zmqpp.mk | 2 +-
> utils/genrandconfig | 2 ++
> 14 files changed, 36 insertions(+), 23 deletions(-)
>
> --
> 2.26.3
>
> _______________________________________________
> buildroot mailing list
> buildroot at busybox.net
> http://lists.busybox.net/mailman/listinfo/buildroot
--
.-----------------.--------------------.------------------.--------------------.
| Yann E. MORIN | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software Designer | \ / CAMPAIGN | ___ |
| +33 561 099 427 `------------.-------: X AGAINST | \e/ There is no |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL | v conspiracy. |
'------------------------------^-------^------------------^--------------------'
^ permalink raw reply [flat|nested] 31+ messages in thread