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.
See commit message in patch 3/7 for references to this prior discussion.

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!)

Best regards,
Thomas

Thomas De Schampheleire (7):
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/libjson: drop explicit '-DNDEBUG' package/flare-engine: remove explicit setting of CMAKE_BUILD_TYPE package/supertux: remove explicit setting of CMAKE_BUILD_TYPE package/sysrepo: remove explicit setting of CMAKE_BUILD_TYPE

 Config.in                             | 13 +++++++++++++
 docs/manual/adding-packages-cmake.txt |  2 +-
 package/Makefile.in                   |  3 +++
 package/flare-engine/flare-engine.mk  |  5 -----
 package/libjson/libjson.mk            |  2 +-
 package/pkg-cmake.mk                  |  2 +-
 package/supertux/supertux.mk          |  2 --
 package/sysrepo/sysrepo.mk            |  5 ++---
 8 files changed, 21 insertions(+), 13 deletions(-)

2.26.2

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(+)

index e35a78fb71..94e3076ca7 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

2.26.2

From: Thomas De Schampheleire <[email protected]>

A common way to disable runtime assertions is by honoring the 'NDEBUG' preprocessor flag. Set it when BR2_ENABLE_RUNTIME_DEBUG is disabled (the default case).

---
 package/Makefile.in | 3 +++
 1 file changed, 3 insertions(+)

index 51f5cbce4f..9a18c05c6f 100644
@@ -145,6 +145,9 @@ endif
 ifeq ($(BR2_DEBUG_3),y)
 TARGET_DEBUGGING = -g3
 endif
+ifeq ($(BR2_ENABLE_RUNTIME_DEBUG),)
+TARGET_DEBUGGING += -DNDEBUG
+endif
 
 TARGET_LDFLAGS = $(call qstrip,$(BR2_TARGET_LDFLAGS))
 

2.26.2

How "common" is that? It feels a pretty "random" definition to me. I'm not sure we want to randomly pass -DNDEBUG to all our packages, without knowing what the meaning of NDEBUG is for all those packages.

Thomas

Thomas Petazzoni, CTO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Hi Thomas,

El s?b, 13 feb 2021 a las 22:56, Thomas Petazzoni
(<[email protected]>) escribi?:

'Originally', NDEBUG is the macro used by assert() to decide whether or not assertions actually do anything.
See https://www.man7.org/linux/man-pages/man3/assert.3.html

cmake links this parameter to the CMAKE_BUILD_TYPE, setting NDEBUG for 'Release', and not setting it for 'Debug'.

Based on these two elements, I find NDEBUG a fairly standard mechanism.

But it is indeed possible that some package uses this in an unexpected way. For this reason I would not merge this series for 2021.02 but instead opt for -next so we have some more time to catch any problems.

For the severe performance degradation we observed in gRPC/libabseil, the patch changing the cmake build system would be sufficient. But since the problem is generic, I opted to also make this patch for other packages.

Best regards,
Thomas

Hello,

El lun, 15 feb 2021 a las 10:11, Thomas De Schampheleire (<[email protected]>) escribi?:

I wanted to come back to this patch and clarify that I removed it internally. By passing -NDEBUG in TARGET_DEBUGGING, it will be passed to the compiler wrapper, which means it is in place also for compilations of project-specific applications.
As explained, the 'assert' function provided by glibc will remove assertions if NDEBUG is specified.
Aside from the fact that you may not want that for your project-specific applications, it may also trigger compilation failures like "variable X is not used" if X is only used in context of the assertion. These may be solvable problems but in any case it means this is not a transparent change.

I will send a v3.

/Thomas

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(-)

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;

index c001051002..07b8091117 100644
@@ -265,7 +265,7 @@ define TOOLCHAIN_CMAKE_INSTALL_FILES
 		-e 's#@@TARGET_FC@@#$(subst $(HOST_DIR)/,,$(call qstrip,$(TARGET_FC)))#' \
 		-e 's#@@CMAKE_SYSTEM_PROCESSOR@@#$(call qstrip,$(CMAKE_SYSTEM_PROCESSOR))#' \
 		-e 's#@@TOOLCHAIN_HAS_FORTRAN@@#$(if $(BR2_TOOLCHAIN_HAS_FORTRAN),1,0)#' \
-		-e 's#@@CMAKE_BUILD_TYPE@@#$(if $(BR2_ENABLE_DEBUG),Debug,Release)#' \
+		-e 's#@@CMAKE_BUILD_TYPE@@#$(if $(BR2_ENABLE_RUNTIME_DEBUG),Debug,Release)#' \
 		$(TOPDIR)/support/misc/toolchainfile.cmake.in \
 		> $(HOST_DIR)/share/buildroot/toolchainfile.cmake
 	$(Q)$(INSTALL) -D -m 0644 support/misc/Buildroot.cmake \

2.26.2

Hei hei,

disclaimer: I'm no heavy buildroot user, but I have some experience with CMake and embedded builds for real world products built with a competing build system.

For that description I would consider "RelWithDebInfo" the correct option.

Why? What we want for devices in the field: no performance penalty, but being able to analyze core dumps or even use gdb/gdbserver (which is not ideal if you have optimization _and_ debug flags on at the same time, but better than nothing).

Since you can have all the debug symbols detached (in general, don't know if buildroot allows this), you don't need to copy them to the target and we store it with our release builds and can access them later for debugging.

/"\ ASCII RIBBON | ?With the first link, the chain is forged. The first
\ / CAMPAIGN     | speech censured, the first thought forbidden, the
 X  AGAINST      | first freedom denied, chains us all irrevocably.?
/ \ HTML MAIL    | (Jean-Luc Picard, quoting Judge Aaron Satie)
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://lists.busybox.net/pipermail/buildroot/attachments/20210213/13f55777/attachment.asc>

Hi Alex,

El s?b, 13 feb 2021 a las 16:56, Alexander Dahl (<[email protected]>) escribi?:

It was also my original thought, but the references below did not like that direction.

Buildroot already has BR2_ENABLE_DEBUG, which sets the '-g' flag during compilation. This is done even regardless of the CMAKE_BUILD_TYPE.
Originally, I would just have set 'Release' unconditionally (which is basically 'RelWithDebInfo' since we also pass '-g' when BR2_ENABLE_DEBUG is set).

But given the prior discussion, it seems there are people who desire to have runtime assertions enabled. So instead of trying to force one way or another, I propose a second option BR2_ENABLE_RUNTIME_DEBUG to make that choice.

Debugging symbols are anyway stripped from target builds. They will still be present in the 'staging' directory. So this is in a way 'detached' even though they are not really in a separate file.

Best regards,
Thomas

I think this is indeed the way to go.

However, note that this means that BR2_ENABLE_RUNTIME_DEBUG is *not* orthogonal
to BR2_ENABLE_DEBUG. If BR2_ENABLE_DEBUG is not set, *no* -g option is forced on the compiler, so whatever gets passed by the package build system will be used. So for CMake packages, that means that BR2_ENABLE_DEBUG=n with BR2_ENABLE_RUNTIME_DEBUG=y will still result in a -g option in the compile.

Maybe we should do an explicit -g0 in case BR2_ENALBE_DEBUG is not set.

Regards,
Arnout

From: Thomas De Schampheleire <[email protected]>

The passing of 'NDEBUG' is now steered by BR2_ENABLE_RUNTIME_DEBUG and commonly set from package/Makefile.in.

---
 package/libjson/libjson.mk | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index d04ddc40f3..9d064e53c3 100644
@@ -11,7 +11,7 @@ LIBJSON_INSTALL_STAGING = YES
 LIBJSON_LICENSE = BSD-2-Clause
 LIBJSON_LICENSE_FILES = License.txt
 
-LIBJSON_CXXFLAGS = $(TARGET_CFLAGS) -DNDEBUG
+LIBJSON_CXXFLAGS = $(TARGET_CFLAGS)
 
 ifeq ($(BR2_STATIC_LIBS),y)
 LIBJSON_MAKE_OPTS += SHARED=0

2.26.2

From: Thomas De Schampheleire <[email protected]>

flare-engine set CMAKE_BUILD_TYPE=RelWithDebInfo to avoid '-pg' for profiling.

With the introduction of BR2_ENABLE_RUNTIME_DEBUG, this change should no longer be necessary. Users that do not wish to have profiling information, just keep BR2_ENABLE_RUNTIME_DEBUG disabled (default value), and those that enable BR2_ENABLE_RUNTIME_DEBUG will get profiling.

---
 package/flare-engine/flare-engine.mk | 5 -----
 1 file changed, 5 deletions(-)

index e2f3eefa28..7022eac7ae 100644
@@ -14,11 +14,6 @@ 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
-
 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.

2.26.2

From: Thomas De Schampheleire <[email protected]>

supertux explicitly set 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 profiling information, just keep BR2_ENABLE_RUNTIME_DEBUG disabled (default value), and those that enable BR2_ENABLE_RUNTIME_DEBUG will get profiling.

---
 package/supertux/supertux.mk | 2 --
 1 file changed, 2 deletions(-)

index e4a4630918..a339c42e42 100644
@@ -18,7 +18,6 @@ SUPERTUX_LICENSE_FILES = LICENSE.txt data/AUTHORS
 SUPERTUX_DEPENDENCIES = host-pkgconf boost freetype libcurl libgl libglew \
 	libogg libpng libvorbis openal physfs sdl2 sdl2_image
 
-# CMAKE_BUILD_TYPE=Release: disable profiling code (-pg)
 # ENABLE_BOOST_STATIC_LIBS=OFF: use boost shared libraries since supertux
 # depends on !BR2_STATIC_LIBS and boost provide only shared libraries with
 # BR2_SHARED_LIBS.

@@ -29,7 +28,6 @@ SUPERTUX_DEPENDENCIES = host-pkgconf boost freetype libcurl libgl libglew \
 # in physfs.h (CHECK_SYMBOL_EXISTS) doesn't work.
 # ENABLE_OPENGLES2=OFF: Disable opengles2 for now.
 SUPERTUX_CONF_OPTS += \
-	-DCMAKE_BUILD_TYPE=Release \
 	-DCMAKE_CXX_FLAGS="$(TARGET_CXXFLAGS) -DGLEW_NO_GLU" \
 	-DENABLE_BOOST_STATIC_LIBS=OFF \
 	-DBUILD_DOCUMENTATION=OFF \

2.26.2

From: Thomas De Schampheleire <[email protected]>

sysrepo explicitly set 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).

---
v2: Jan Kundr?t:
- explicitly set REPO_PATH, which is set to a host path in case of CMAKE_BUILD_TYPE=Debug (see e.g.
https://github.com/sysrepo/sysrepo/blob/v1.4.104/CMakeLists.txt#L138-L144)

 package/sysrepo/sysrepo.mk | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

index e4d553cdbd..768cde8d35 100644
@@ -13,8 +13,8 @@ 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) \
+	-DREPO_PATH=/etc/sysrepo
 
 ifeq ($(BR2_TOOLCHAIN_HAS_LIBATOMIC),y)
 SYSREPO_CONF_OPTS += -DCMAKE_EXE_LINKER_FLAGS=-latomic

@@ -26,7 +26,6 @@ define SYSREPO_INSTALL_INIT_SYSV
 endef
 
 HOST_SYSREPO_CONF_OPTS = \
-	-DCMAKE_BUILD_TYPE=Release \
 	-DBUILD_EXAMPLES=OFF \
 	-DREPO_PATH=$(TARGET_DIR)/etc/sysrepo
 

2.26.2

Jan