Introduce BR2_ENABLE_RUNTIME_DEBUG
v4 cover Introduce BR2_ENABLE_RUNTIME_DEBUG
From: Thomas De Schampheleire <[email protected]>
Hello,
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.
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(-)
Signature · 1 line
2.26.3
PATCH v4 1/16 core: introduce BR2_ENABLE_RUNTIME_DEBUG
From: Thomas De Schampheleire <[email protected]>
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.
--- Config.in | 13 +++++++++++++ 1 file changed, 13 insertions(+)
Config.in +13 -0
@@ -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
Signature · 1 line
2.26.3
PATCH v4 1/16 core: introduce BR2_ENABLE_RUNTIME_DEBUG
Thomas, All,
On 2021-06-01 16:34 +0200, Thomas De Schampheleire spake thusly:
Quoted 27 lines
From: Thomas De Schampheleire <[email protected]>
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 secondsThis 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.
Quoted 8 lines
Note that
BR2_ENABLE_RUNTIME_DEBUGis orthogonal toBR2_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 whenBR2_ENABLE_DEBUGis set, but that will be rectified in a subsequent commit.
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.
Quoted 35 lines
--- Config.in | 13 +++++++++++++ 1 file changed, 13 insertions(+)Config.in +13
-0@@ -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 ySignature · 6 lines
2.26.3 _______________________________________________ buildroot mailing list buildroot at busybox.net http://lists.busybox.net/mailman/listinfo/buildroot
Signature · 6 lines
.-----------------.--------------------.------------------.--------------------. | 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. | '------------------------------^-------^------------------^--------------------'
PATCH v4 2/16 core: enable 'NDEBUG' unless BR2_ENABLE_RUNTIME_DEBUG is set
In reply to Thomas De SchampheleireFrom: Thomas De Schampheleire <[email protected]>
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).
---
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(+)
package/Makefile.in +3 -0
@@ -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))
Signature · 1 line
2.26.3
PATCH v4 2/16 core: enable 'NDEBUG' unless BR2_ENABLE_RUNTIME_DEBUG is set
Thomas, All,
On 2021-06-01 16:34 +0200, Thomas De Schampheleire spake thusly:
Quoted 9 lines
From: Thomas De Schampheleire <[email protected]>
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_DEBUGis disabled (the default case).Signed-off-by Thomas De Schampheleire <[email protected]>
However, see a little nit, below...
Quoted 13 lines
---
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(+)package/Makefile.in +0
-0
package/Makefile.in endif · line 149
Quoted 2 lines
package/Makefile.in +0
-0@@ -148,6 +148,9 @@ endif ifeq ($(BR2_DEBUG_3),y) TARGET_DEBUGGING = -g3
Here, we have a simple assignment in a conditional block, while...
package/Makefile.in endif · line 152
Quoted 3 lines
package/Makefile.in +2
-0endif +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.
Quoted 11 lines
package/Makefile.in +1
-0+endif TARGET_LDFLAGS = $(call qstrip,$(BR2_TARGET_LDFLAGS))Signature · 6 lines
2.26.3 _______________________________________________ buildroot mailing list buildroot at busybox.net http://lists.busybox.net/mailman/listinfo/buildroot
Signature · 6 lines
.-----------------.--------------------.------------------.--------------------. | 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. | '------------------------------^-------^------------------^--------------------'
PATCH v4 2/16 core: enable 'NDEBUG' unless BR2_ENABLE_RUNTIME_DEBUG is set
Hi Thomas, Yann, all,
package/Makefile.in endif · line 152
Quoted 3 lines
On 2021-06-01 16:34 +0200, Thomas De Schampheleire spake thusly:
package/Makefile.in +2
-0+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.
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
PATCH v4 2/16 core: enable 'NDEBUG' unless BR2_ENABLE_RUNTIME_DEBUG is set
Hello,
Quoted 11 lines · On Sat, Jun 5, 2021, 18:50 D. Olsson <[email protected]> wrote:
Hi Thomas, Yann, all,
package/Makefile.in endif · line 152
Quoted 3 lines
On 2021-06-01 16:34 +0200, Thomas De Schampheleire spake thusly:
package/Makefile.in +2
-0+ifeq ($(BR2_ENABLE_RUNTIME_DEBUG),) +TARGET_DEBUGGING += -DNDEBUGThis 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.
Quoted 7 lines
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>
PATCH v4 2/16 core: enable 'NDEBUG' unless BR2_ENABLE_RUNTIME_DEBUG is set
In reply to Thomas De SchampheleireHello,
Quoted 9 lines · On Tue, 1 Jun 2021 16:34:07 +0200 Thomas De Schampheleire…
From: Thomas De Schampheleire <[email protected]>
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_DEBUGis disabled (the default case).Signed-off-by Thomas De Schampheleire <[email protected]>
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
Signature · 3 lines
Thomas Petazzoni, co-owner and CEO, Bootlin Embedded Linux and Kernel engineering https://bootlin.com
PATCH v4 2/16 core: enable 'NDEBUG' unless BR2_ENABLE_RUNTIME_DEBUG is set
Quoted 11 lines
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
"Thomas" == Thomas Petazzoni <[email protected]> writes:
Hello,
Quoted 5 lines · On Tue, 1 Jun 2021 16:34:07 +0200 Thomas De Schampheleire…
From: Thomas De Schampheleire <[email protected]>
The 'assert' statement in glibc honors the 'NDEBUG' preprocessor macro: if it is set, then the assert statement is compiled away.
Quoted 8 lines
Quoted 4 lines
Define this 'NDEBUG' macro when
BR2_ENABLE_RUNTIME_DEBUGis disabled (the default case).Signed-off-by Thomas De Schampheleire <[email protected]>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.
Quoted 4 lines
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.
Signature · 1 line
Bye, Peter Korsgaard
PATCH v4 2/16 core: enable 'NDEBUG' unless BR2_ENABLE_RUNTIME_DEBUG is set
Peter, Thomas?, All,
On 2021-07-04 13:01 +0200, Peter Korsgaard spake thusly:
Quoted 9 lines
Quoted 9 lines
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
"Thomas" == Thomas Petazzoni <[email protected]> writes:
Hello,
Quoted 5 lines · On Tue, 1 Jun 2021 16:34:07 +0200 Thomas De Schampheleire…
From: Thomas De Schampheleire <[email protected]>
The 'assert' statement in glibc honors the 'NDEBUG' preprocessor macro: if it is set, then the assert statement is compiled away.
Quoted 8 lines
Quoted 7 lines
Quoted 4 lines
Define this 'NDEBUG' macro when
BR2_ENABLE_RUNTIME_DEBUGis disabled (the default case).Signed-off-by Thomas De Schampheleire <[email protected]>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.
Quoted 5 lines
Quoted 3 lines
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.
Signature · 6 lines
.-----------------.--------------------.------------------.--------------------. | 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. | '------------------------------^-------^------------------^--------------------'
PATCH v4 2/16 core: enable 'NDEBUG' unless BR2_ENABLE_RUNTIME_DEBUG is set
Peter, Thomas?, All,
On 2021-07-04 13:32 +0200, Yann E. MORIN spake thusly:
Quoted 16 lines
On 2021-07-04 13:01 +0200, Peter Korsgaard spake thusly:
Quoted 15 lines
Quoted 15 lines
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
"Thomas" == Thomas Petazzoni <[email protected]> writes:
Quoted 9 lines · On Tue, 1 Jun 2021 16:34:07 +0200 Thomas De Schampheleire…
From: Thomas De Schampheleire <[email protected]>
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_DEBUGis disabled (the default case).Signed-off-by Thomas De Schampheleire <[email protected]>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.
Signature · 6 lines
.-----------------.--------------------.------------------.--------------------. | 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. | '------------------------------^-------^------------------^--------------------'
PATCH v4 2/16 core: enable 'NDEBUG' unless BR2_ENABLE_RUNTIME_DEBUG is set
In reply to Thomas PetazzoniQuoted 10 lines · Thomas Petazzoni
Hello,
Quoted 5 lines · On Tue, 1 Jun 2021 16:34:07 +0200 Thomas De Schampheleire…
From: Thomas De Schampheleire <[email protected]>
The 'assert' statement in glibc honors the 'NDEBUG' preprocessor macro: if it is set, then the assert statement is compiled away.
Quoted 7 lines
Quoted 4 lines
Define this 'NDEBUG' macro when
BR2_ENABLE_RUNTIME_DEBUGis disabled (the default case).Signed-off-by Thomas De Schampheleire <[email protected]>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.
Quoted 10 lines
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 macroassert()generates no code, and hence does nothing at
all. It is not recommended to define NDEBUG if usingassert()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.
Quoted 9 lines
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.
Quoted 4 lines
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...
Quoted 5 lines
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 thatassert()expressions should not have side effects, becauseassert()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...
Quoted 1 line · That
I believe this change is too dangerous,
This is definitely true. So I also think reverting it is the safest option.
Regards,
Arnout
Quoted 7 lines
has caused a number of build
breakage + can cause run-time breakage that is difficult to predict.What do you think ?
Thomas
PATCH v4 2/16 core: enable 'NDEBUG' unless BR2_ENABLE_RUNTIME_DEBUG is set
Arnout, Peter, Thomas?, All,
On 2021-07-04 16:58 +0200, Arnout Vandecappelle spake thusly:
Quoted 8 lines · Thomas Petazzoni
Quoted 7 lines
Quoted 5 lines · On Tue, 1 Jun 2021 16:34:07 +0200 Thomas De Schampheleire…
From: Thomas De Schampheleire <[email protected]> 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 whenBR2_ENABLE_RUNTIME_DEBUGis disabled (the default case).
Quoted 4 lines
Quoted 2 lines
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 theassert()is gone, and that triggers -Werror.
[--SNIP--]
Quoted 7 lines
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. ;-)
Quoted 5 lines
Quoted 5 lines
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 thatassert()expressions should not have side effects, becauseassert()can be disabled by defining NDEBUG.
Quoted 8 lines
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...Quoted 1 line · That
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.
Signature · 6 lines
.-----------------.--------------------.------------------.--------------------. | 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. | '------------------------------^-------^------------------^--------------------'
PATCH v4 3/16 package/pkg-cmake.mk: determine CMAKE_BUILD_TYPE depending on BR2_ENABLE_RUNTIME_DEBUG
In reply to Thomas De SchampheleireFrom: Thomas De Schampheleire <[email protected]>
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.
--- docs/manual/adding-packages-cmake.txt | 2 +- package/pkg-cmake.mk | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-)
docs/manual/adding-packages-cmake.txt +1 -1
@@ -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;
package/pkg-cmake.mk +1 -1
@@ -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 \
Signature · 1 line
2.26.3
PATCH v4 3/16 package/pkg-cmake.mk: determine CMAKE_BUILD_TYPE depending on BR2_ENABLE_RUNTIME_DEBUG
Thomas, All,
On 2021-06-01 16:34 +0200, Thomas De Schampheleire spake thusly:
Quoted 45 lines
From: Thomas De Schampheleire <[email protected]>
The
CMAKE_BUILD_TYPEis currently set as 'Debug' in caseBR2_ENABLE_DEBUGis set, and as 'Release' in other cases. However, while the description ofBR2_ENABLE_DEBUGis 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_TYPEBuildroot 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_DEBUGdoes 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.htmlIn 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_DEBUGif they want runtime debug info like assertions, at the risk of introducing performance degradation. In this case, we switch to CMAKE_BUILD_TYPE=Debug.
Quoted 6 lines
- orthogonally to the above,
BR2_ENABLE_DEBUGstill determines passing the '-g' flag to enable debug symbols, andBR2_OPTIMIZE_Xstill determines the used optimization flags.
Regards,
Yann E. MORIN.
Quoted 38 lines
--- docs/manual/adding-packages-cmake.txt | 2 +- package/pkg-cmake.mk | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-)docs/manual/adding-packages-cmake.txt +1
-1@@ -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;package/pkg-cmake.mk +1
-1@@ -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 \Signature · 6 lines
2.26.3 _______________________________________________ buildroot mailing list buildroot at busybox.net http://lists.busybox.net/mailman/listinfo/buildroot
Signature · 6 lines
.-----------------.--------------------.------------------.--------------------. | 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. | '------------------------------^-------^------------------^--------------------'
PATCH v4 4/16 package/flare-engine: disable effect of CMAKE_BUILD_TYPE
In reply to Thomas De SchampheleireFrom: Thomas De Schampheleire <[email protected]>
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.
---
v4:
- use a custom build type as suggested by Arnout.
package/flare-engine/flare-engine.mk | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-)
package/flare-engine/flare-engine.mk +4 -8
@@ -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
Signature · 1 line
2.26.3
PATCH v4 5/16 package/sysrepo: use default CMAKE_BUILD_TYPE for host package
In reply to Thomas De SchampheleireFrom: Thomas De Schampheleire <[email protected]>
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.
--- package/sysrepo/sysrepo.mk | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
package/sysrepo/sysrepo.mk +2 -1
@@ -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))
Signature · 1 line
2.26.3
PATCH v4 6/16 package/sysrepo: remove explicit setting of CMAKE_BUILD_TYPE
In reply to Thomas De SchampheleireFrom: Thomas De Schampheleire <[email protected]>
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.
--- package/sysrepo/sysrepo.mk | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)
package/sysrepo/sysrepo.mk +4 -2
@@ -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
Signature · 1 line
2.26.3
PATCH v4 7/16 package/boost: use BR2_ENABLE_RUNTIME_DEBUG iso BR2_ENABLE_DEBUG
In reply to Thomas De SchampheleireFrom: Thomas De Schampheleire <[email protected]>
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.
--- package/boost/boost.mk | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
package/boost/boost.mk +1 -1
@@ -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
Signature · 1 line
2.26.3
PATCH v4 8/16 package/oracle-mysql: use BR2_ENABLE_RUNTIME_DEBUG iso BR2_ENABLE_DEBUG
In reply to Thomas De SchampheleireFrom: Thomas De Schampheleire <[email protected]>
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.
--- package/oracle-mysql/oracle-mysql.mk | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
package/oracle-mysql/oracle-mysql.mk +1 -1
@@ -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
Signature · 1 line
2.26.3
PATCH v4 9/16 package/qt5: use BR2_ENABLE_RUNTIME_DEBUG iso BR2_ENABLE_DEBUG
In reply to Thomas De SchampheleireFrom: Thomas De Schampheleire <[email protected]>
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.
--- package/qt5/qt5base/qt5base.mk | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
package/qt5/qt5base/qt5base.mk +1 -1
@@ -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
Signature · 1 line
2.26.3
PATCH v4 10/16 package/ripgrep: use BR2_ENABLE_RUNTIME_DEBUG iso BR2_ENABLE_DEBUG
In reply to Thomas De SchampheleireFrom: Thomas De Schampheleire <[email protected]>
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.
--- package/ripgrep/ripgrep.mk | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
package/ripgrep/ripgrep.mk +1 -1
@@ -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
Signature · 1 line
2.26.3
PATCH v4 11/16 package/sofia-sip: correct passing of '--enable-ndebug'
In reply to Thomas De SchampheleireFrom: Thomas De Schampheleire <[email protected]>
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.
---
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(-)
package/sofia-sip/sofia-sip.mk +1 -1
@@ -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
Signature · 1 line
2.26.3
PATCH v4 12/16 package/sofia-sip: don't set 'NDEBUG' explicitly
In reply to Thomas De SchampheleireFrom: Thomas De Schampheleire <[email protected]>
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.
--- package/sofia-sip/sofia-sip.mk | 4 ---- 1 file changed, 4 deletions(-)
package/sofia-sip/sofia-sip.mk +0 -4
@@ -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))
Signature · 1 line
2.26.3
PATCH v4 13/16 package/zmqpp: don't set CONFIG=debug
In reply to Thomas De SchampheleireFrom: Thomas De Schampheleire <[email protected]>
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 -DNDEBUGendif
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.
--- package/zmqpp/zmqpp.mk | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
package/zmqpp/zmqpp.mk +1 -1
@@ -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)
Signature · 1 line
2.26.3
PATCH v4 14/16 package/zmqpp: use BR2_ENABLE_RUNTIME_DEBUG iso BR2_ENABLE_DEBUG
In reply to Thomas De SchampheleireFrom: Thomas De Schampheleire <[email protected]>
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.
--- package/zmqpp/zmqpp.mk | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
package/zmqpp/zmqpp.mk +1 -1
@@ -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)
Signature · 1 line
2.26.3
PATCH v4 15/16 package/pkg-meson.mk: determine 'buildtype' based on BR2_ENABLE_RUNTIME_DEBUG iso BR2_ENABLE_DEBUG
In reply to Thomas De SchampheleireFrom: Thomas De Schampheleire <[email protected]>
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.
--- package/pkg-meson.mk | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
package/pkg-meson.mk +1 -1
@@ -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 \
Signature · 1 line
2.26.3
PATCH v4 15/16 package/pkg-meson.mk: determine 'buildtype' based on BR2_ENABLE_RUNTIME_DEBUG iso BR2_ENABLE_DEBUG
Thomas, All,
Eric, and other meson experts: question for you below...
On 2021-06-01 16:34 +0200, Thomas De Schampheleire spake thusly:
Quoted 18 lines
From: Thomas De Schampheleire <[email protected]>
BR2_ENABLE_DEBUGshould 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_DEBUGinstead.Signed-off-by Thomas De Schampheleire <[email protected]>--- package/pkg-meson.mk | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)package/pkg-meson.mk +0
-0
package/pkg-meson.mk define $(2)_CONFIGURE_CMDS · line 92
Quoted 5 lines
package/pkg-meson.mk +1
-1@@ -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
Quoted 10 lines
package/pkg-meson.mk +0
-0--cross-file=$$($$(PKG)_SRCDIR)/build/cross-compilation.conf \ -Dstrip=false \ -Dbuild.pkg_config_path=$$(HOST_DIR)/lib/pkgconfig \Signature · 6 lines
2.26.3 _______________________________________________ buildroot mailing list buildroot at busybox.net http://lists.busybox.net/mailman/listinfo/buildroot
Signature · 6 lines
.-----------------.--------------------.------------------.--------------------. | 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. | '------------------------------^-------^------------------^--------------------'
PATCH v4 15/16 package/pkg-meson.mk: determine 'buildtype' based on BR2_ENABLE_RUNTIME_DEBUG iso BR2_ENABLE_DEBUG
Quoted 31 lines · Yann E. MORIN
Thomas, All,
Eric, and other meson experts: question for you below...
On 2021-06-01 16:34 +0200, Thomas De Schampheleire spake thusly:
Quoted 18 lines
From: Thomas De Schampheleire <[email protected]>
BR2_ENABLE_DEBUGshould 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_DEBUGinstead.Signed-off-by Thomas De Schampheleire <[email protected]>--- package/pkg-meson.mk | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)package/pkg-meson.mk +0
-0package/pkg-meson.mk define $(2)_CONFIGURE_CMDS · line 92
Quoted 5 lines
package/pkg-meson.mk +1
-1@@ -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.
Quoted 6 lines
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
Quoted 16 lines
Thoughts?
[0] https://mesonbuild.com/Builtin-options.html#core-options
Quoted 10 lines
package/pkg-meson.mk +0
-0--cross-file=$$($$(PKG)_SRCDIR)/build/cross-compilation.conf \ -Dstrip=false \ -Dbuild.pkg_config_path=$$(HOST_DIR)/lib/pkgconfig \Signature · 6 lines
2.26.3 _______________________________________________ buildroot mailing list buildroot at busybox.net http://lists.busybox.net/mailman/listinfo/buildroot
PATCH v4 16/16 utils/genrandconfig: also test BR2_ENABLE_RUNTIME_DEBUG
In reply to Thomas De SchampheleireFrom: Thomas De Schampheleire <[email protected]>
... similar to BR2_ENABLE_DEBUG.
--- utils/genrandconfig | 2 ++ 1 file changed, 2 insertions(+)
utils/genrandconfig +2 -0
@@ -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:
Signature · 1 line
2.26.3
v4 cover Introduce BR2_ENABLE_RUNTIME_DEBUG
In reply to Thomas De SchampheleireThomas, All,
On 2021-06-01 16:34 +0200, Thomas De Schampheleire spake thusly:
Quoted 7 lines
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_TYPEthat Buildroot should set, by not forcing anything upon the user but allowing the choice.
Quoted 5 lines
Additionally, it cleans up some related changes done in specific packages.
After this series, there are still packages that set
CMAKE_BUILD_TYPEexplicitly, 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.
Quoted 58 lines
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 useBR2_ENABLE_RUNTIME_DEBUGinstead ofBR2_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 ofBR2_ENABLE_RUNTIME_DEBUGThomas De Schampheleire (16):
core: introduceBR2_ENABLE_RUNTIME_DEBUG
core: enable 'NDEBUG' unlessBR2_ENABLE_RUNTIME_DEBUGis setpackage/pkg-cmake.mk: determineCMAKE_BUILD_TYPEdepending on
BR2_ENABLE_RUNTIME_DEBUG
package/flare-engine: disable effect ofCMAKE_BUILD_TYPEpackage/sysrepo: use defaultCMAKE_BUILD_TYPEfor host package package/sysrepo: remove explicit setting ofCMAKE_BUILD_TYPEpackage/boost: useBR2_ENABLE_RUNTIME_DEBUGisoBR2_ENABLE_DEBUGpackage/oracle-mysql: useBR2_ENABLE_RUNTIME_DEBUGiso
BR2_ENABLE_DEBUG
package/qt5: useBR2_ENABLE_RUNTIME_DEBUGisoBR2_ENABLE_DEBUGpackage/ripgrep: useBR2_ENABLE_RUNTIME_DEBUGisoBR2_ENABLE_DEBUGpackage/sofia-sip: correct passing of '--enable-ndebug' package/sofia-sip: don't set 'NDEBUG' explicitly package/zmqpp: don't set CONFIG=debug
package/zmqpp: useBR2_ENABLE_RUNTIME_DEBUGisoBR2_ENABLE_DEBUGpackage/pkg-meson.mk: determine 'buildtype' based on
BR2_ENABLE_RUNTIME_DEBUGisoBR2_ENABLE_DEBUG
utils/genrandconfig: also testBR2_ENABLE_RUNTIME_DEBUGConfig.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(-)Signature · 6 lines
2.26.3 _______________________________________________ buildroot mailing list buildroot at busybox.net http://lists.busybox.net/mailman/listinfo/buildroot
Signature · 6 lines
.-----------------.--------------------.------------------.--------------------. | 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. | '------------------------------^-------^------------------^--------------------'