Skip to contentSkip to conversation

Introduce BR2_ENABLE_RUNTIME_DEBUG

PATCH New Thomas De Schampheleire 6 participants 31 messages Jun 1, 2021 – Jul 4, 2021 Arm/Amlogic Meson SoC support

lore
Share & tools
lore
#0 lore raw

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
#1 lore raw

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
index c65e34bd5e..a008425688 100644
@@ -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
#2 lore raw

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 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.

Quoted 8 lines

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.

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
index c65e34bd5e..a008425688 100644
@@ -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 · 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.  |
'------------------------------^-------^------------------^--------------------'
#3 lore raw

PATCH v4 2/16 core: enable 'NDEBUG' unless BR2_ENABLE_RUNTIME_DEBUG is set

In reply to 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_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
index 86db62ba5b..955e6a8e8c 100644
@@ -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
#4 lore raw

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_DEBUG is disabled (the default case).

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
index 86db62ba5b..955e6a8e8c 100644

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 -0
 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.

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.  |
'------------------------------^-------^------------------^--------------------'
#5 lore raw

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.

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

#6 lore raw

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 += -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.

Quoted 7 lines

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>

#7 lore raw

PATCH v4 2/16 core: enable 'NDEBUG' unless BR2_ENABLE_RUNTIME_DEBUG is set

In reply to Thomas De Schampheleire

Hello,

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_DEBUG is disabled (the default case).

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
#8 lore raw

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_DEBUG is disabled (the default case).

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
#9 lore raw

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_DEBUG is disabled (the default case).

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.  |
'------------------------------^-------^------------------^--------------------'
#10 lore raw

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_DEBUG is disabled (the default case).

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.  |
'------------------------------^-------^------------------^--------------------'
#11 lore raw

PATCH v4 2/16 core: enable 'NDEBUG' unless BR2_ENABLE_RUNTIME_DEBUG is set

In reply to Thomas Petazzoni
Quoted 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_DEBUG is disabled (the default case).

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 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.

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 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...

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

#12 lore raw

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 when BR2_ENABLE_RUNTIME_DEBUG is 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 the assert() 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 that assert() expressions should not have side effects, because assert() 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.  |
'------------------------------^-------^------------------^--------------------'
#13 lore raw

PATCH v4 3/16 package/pkg-cmake.mk: determine CMAKE_BUILD_TYPE depending on BR2_ENABLE_RUNTIME_DEBUG

In reply to Thomas De Schampheleire

From: 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
index 73f0943024..541d7422cf 100644
@@ -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
index 37078f3c34..4ee100a0c6 100644
@@ -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
#14 lore raw

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_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.

Quoted 6 lines

- 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.

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
index 73f0943024..541d7422cf 100644
@@ -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
index 37078f3c34..4ee100a0c6 100644
@@ -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.  |
'------------------------------^-------^------------------^--------------------'
#15 lore raw

PATCH v4 4/16 package/flare-engine: disable effect of CMAKE_BUILD_TYPE

In reply to Thomas De Schampheleire

From: 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
index e2f3eefa28..b91fc16af3 100644
@@ -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
#16 lore raw

PATCH v4 5/16 package/sysrepo: use default CMAKE_BUILD_TYPE for host package

In reply to Thomas De Schampheleire

From: 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
index dbc7b16b63..f5be29d2c9 100644
@@ -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
#17 lore raw

PATCH v4 6/16 package/sysrepo: remove explicit setting of CMAKE_BUILD_TYPE

In reply to Thomas De Schampheleire

From: 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
index f5be29d2c9..c0db05e52c 100644
@@ -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
#18 lore raw

PATCH v4 7/16 package/boost: use BR2_ENABLE_RUNTIME_DEBUG iso BR2_ENABLE_DEBUG

In reply to Thomas De Schampheleire

From: 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
index 8d745ecc72..e72ddf897c 100644
@@ -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
#19 lore raw

PATCH v4 8/16 package/oracle-mysql: use BR2_ENABLE_RUNTIME_DEBUG iso BR2_ENABLE_DEBUG

In reply to Thomas De Schampheleire

From: 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
index ccfa40cfb1..e7d6354ed8 100644
@@ -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
#20 lore raw

PATCH v4 9/16 package/qt5: use BR2_ENABLE_RUNTIME_DEBUG iso BR2_ENABLE_DEBUG

In reply to Thomas De Schampheleire

From: 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
index 84e9fa4edb..8b55aa3098 100644
@@ -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
#21 lore raw

PATCH v4 10/16 package/ripgrep: use BR2_ENABLE_RUNTIME_DEBUG iso BR2_ENABLE_DEBUG

In reply to Thomas De Schampheleire

From: 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
index 9dd8d58de1..c3c12fd185 100644
@@ -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
#22 lore raw

PATCH v4 11/16 package/sofia-sip: correct passing of '--enable-ndebug'

In reply to Thomas De Schampheleire

From: 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
index 5c383400ff..274c72fce8 100644
@@ -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
#23 lore raw

PATCH v4 12/16 package/sofia-sip: don't set 'NDEBUG' explicitly

In reply to Thomas De Schampheleire

From: 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
index 274c72fce8..fb565637eb 100644
@@ -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
#24 lore raw

PATCH v4 13/16 package/zmqpp: don't set CONFIG=debug

In reply to Thomas De Schampheleire

From: 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 -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.

---
 package/zmqpp/zmqpp.mk | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
package/zmqpp/zmqpp.mk +1 -1
index 3cd19d644a..c629f14920 100644
@@ -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
#25 lore raw

PATCH v4 14/16 package/zmqpp: use BR2_ENABLE_RUNTIME_DEBUG iso BR2_ENABLE_DEBUG

In reply to Thomas De Schampheleire

From: 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
index c629f14920..32fabf26ef 100644
@@ -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
#26 lore raw

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 Schampheleire

From: 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
index a57820d4d2..dafad3b1eb 100644
@@ -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
#27 lore raw

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_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 +0 -0
index a57820d4d2..dafad3b1eb 100644

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.  |
'------------------------------^-------^------------------^--------------------'
#28 lore raw

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_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 +0 -0
index a57820d4d2..dafad3b1eb 100644

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.

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
#29 lore raw

PATCH v4 16/16 utils/genrandconfig: also test BR2_ENABLE_RUNTIME_DEBUG

In reply to Thomas De Schampheleire

From: Thomas De Schampheleire <[email protected]>

... similar to BR2_ENABLE_DEBUG.

---
 utils/genrandconfig | 2 ++
 1 file changed, 2 insertions(+)
utils/genrandconfig +2 -0
index 93dc6d898b..ca3e77e3e3 100755
@@ -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
#30 lore raw

v4 cover Introduce BR2_ENABLE_RUNTIME_DEBUG

In reply to Thomas De Schampheleire

Thomas, 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_TYPE that 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_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.

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 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 · 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.  |
'------------------------------^-------^------------------^--------------------'