[Buildroot] [PATCH 0/7] Introduce BR2_ENABLE_RUNTIME_DEBUG

[Buildroot] [PATCH 0/7] Introduce BR2_ENABLE_RUNTIME_DEBUG

[Thomas De Schampheleire]

From: Thomas De Schampheleire <thomas.de_schampheleire@nokia.com>

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.

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            |  2 --
 8 files changed, 19 insertions(+), 12 deletions(-)

-- 
2.26.2

[Buildroot] [PATCH 1/7] core: introduce BR2_ENABLE_RUNTIME_DEBUG

[Thomas De Schampheleire]

From: Thomas De Schampheleire <thomas.de_schampheleire@nokia.com>

Some packages have optional runtime assertions, extra traces, or other
elements that can help in debugging problems. However, such runtime elements
can negatively influence performance.

In a test program performing 100K gRPC calls from a client to a local server
and receiving the returned response, we see following execution time:

    - runtime debug enabled: 1065 seconds
    - runtime debug disabled:  48 seconds

This is more than a factor 20 (!) difference. Analysis shows that the
problem mostly stems from libabseil-cpp (a dependency of gRPC) which enables
mutex deadlock analysis when the preprocessor flag 'NDEBUG' is not set,
which adds a 'backtrace()' call on every lock/unlock.  Potentially worse,
when libunwind is enabled and linked with the test program, 'backtrace()' is
not provided by glibc but by libunwind itself.

For production systems, users expect good performance out-of-the-box. In the
example above, the difference is huge and unless explicitly tested and
analyzed, users may not realize that the performance could be much better.

Address this problem by introducing a new option BR2_ENABLE_RUNTIME_DEBUG,
which can be used by packages or package infrastructures to set the
necessary flags.

Note that BR2_ENABLE_RUNTIME_DEBUG is orthogonal to BR2_ENABLE_DEBUG: the
former changes runtime behavior, while the latter is only expected to add
debug symbols to the build. Today, the cmake build system does introduce a
runtime impact when BR2_ENABLE_DEBUG is set, but that will be rectified in a
subsequent commit.

Signed-off-by: Thomas De Schampheleire <thomas.de_schampheleire@nokia.com>
---
 Config.in | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/Config.in b/Config.in
index e35a78fb71..94e3076ca7 100644
--- a/Config.in
+++ b/Config.in
@@ -412,6 +412,19 @@ config BR2_DEBUG_3
 endchoice
 endif
 
+config BR2_ENABLE_RUNTIME_DEBUG
+	bool "build packages with runtime debugging info"
+	help
+	  Some packages may have runtime assertions, extra traces, and
+	  similar runtime elements that can help debugging. However,
+	  these elements may negatively influence performance so should
+	  normally not be enabled on production systems.
+
+	  Enable this option to enable such runtime debugging.
+
+	  Note: disabling this option is not a guarantee that all
+	  packages effectively removed these runtime debugging elements.
+
 config BR2_STRIP_strip
 	bool "strip target binaries"
 	default y
-- 
2.26.2

[Buildroot] [PATCH 2/7] core: enable 'NDEBUG' unless BR2_ENABLE_RUNTIME_DEBUG is set

[Thomas De Schampheleire]

From: Thomas De Schampheleire <thomas.de_schampheleire@nokia.com>

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

Signed-off-by: Thomas De Schampheleire <thomas.de_schampheleire@nokia.com>
---
 package/Makefile.in | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/package/Makefile.in b/package/Makefile.in
index 51f5cbce4f..9a18c05c6f 100644
--- a/package/Makefile.in
+++ b/package/Makefile.in
@@ -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

[Buildroot] [PATCH 3/7] package/pkg-cmake.mk: determine CMAKE_BUILD_TYPE depending on BR2_ENABLE_RUNTIME_DEBUG

[Thomas De Schampheleire]

From: Thomas De Schampheleire <thomas.de_schampheleire@nokia.com>

The CMAKE_BUILD_TYPE is currently set as 'Debug' in case BR2_ENABLE_DEBUG is
set, and as 'Release' in other cases. However, while the description of
BR2_ENABLE_DEBUG is to enable debug symbols (no runtime impact), the 'Debug'
build type in CMake can actually have runtime impact. For one, because it
does not set -DNDEBUG like is done for 'Release', but also because packages
may do custom things based on it.

The question of which CMAKE_BUILD_TYPE Buildroot should set, be it 'Debug',
'Release', 'RelWithDebInfo' or others, has come up several times in the
past. See some references below:

- July 2016: switch from Debug to RelWithDebInfo:
  https://git.buildroot.org/buildroot/commit/?id=4b0120183404913f7f7788ef4f0f6b51498ef363

- October 2016: switch from RelWithDebInfo back to Debug:
  https://git.buildroot.org/buildroot/commit/?id=104bb29e0490bfb487e2e665448dd3ca07fcc2b5
  and changes to make sure Buildroot's flags are respected:
  https://git.buildroot.org/buildroot/commit/?id=12494ef48f893684d0800e7f6fe39a2ceaed0451

- August 2017: bug #10246 - "BR2_ENABLE_DEBUG does not have the expected
  effect for cmake packages"
  https://bugs.busybox.net/show_bug.cgi?id=10246

- August 2017: mail thread following bug #10246:
  http://lists.busybox.net/pipermail/buildroot/2017-August/200778.html

In the last mail thread, Samuel Martin confirmed that the 'Release' build
type could be used in all cases, because Buildroot is actually making sure
that the optimization flags are those determined by Buildroot, not the
defaults of cmake, thanks to commit 12494ef48f.
But Arnout Vandecappelle objected to using always 'Release', stating that
users may actually want the extra assertions.

With the introduction of BR2_ENABLE_RUNTIME_DEBUG, Buildroot can now cater
for all cases:

- use CMAKE_BUILD_TYPE=Release by default. This makes sure that there is no
  unexpected performance degradation triggered by enabling BR2_ENABLE_DEBUG.

- users can optionally enable BR2_ENABLE_RUNTIME_DEBUG if they want runtime
  debug info like assertions, at the risk of introducing performance
  degradation. In this case, we switch to CMAKE_BUILD_TYPE=Debug.

- orthogonally to the above, BR2_ENABLE_DEBUG still determines passing the
  '-g' flag to enable debug symbols, and BR2_OPTIMIZE_X still determines the
  used optimization flags.

Signed-off-by: Thomas De Schampheleire <thomas.de_schampheleire@nokia.com>
---
 docs/manual/adding-packages-cmake.txt | 2 +-
 package/pkg-cmake.mk                  | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/docs/manual/adding-packages-cmake.txt b/docs/manual/adding-packages-cmake.txt
index 73f0943024..541d7422cf 100644
--- a/docs/manual/adding-packages-cmake.txt
+++ b/docs/manual/adding-packages-cmake.txt
@@ -100,7 +100,7 @@ typical packages will therefore only use a few of them.
   necessary to set them in the package's +*.mk+ file unless you want
   to override them:
 
-** +CMAKE_BUILD_TYPE+ is driven by +BR2_ENABLE_DEBUG+;
+** +CMAKE_BUILD_TYPE+ is driven by +BR2_ENABLE_RUNTIME_DEBUG+;
 ** +CMAKE_INSTALL_PREFIX+;
 ** +BUILD_SHARED_LIBS+ is driven by +BR2_STATIC_LIBS+;
 ** +BUILD_DOC+, +BUILD_DOCS+ are disabled;
diff --git a/package/pkg-cmake.mk b/package/pkg-cmake.mk
index c001051002..07b8091117 100644
--- a/package/pkg-cmake.mk
+++ b/package/pkg-cmake.mk
@@ -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

[Buildroot] [PATCH 4/7] package/libjson: drop explicit '-DNDEBUG'

[Thomas De Schampheleire]

From: Thomas De Schampheleire <thomas.de_schampheleire@nokia.com>

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

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

diff --git a/package/libjson/libjson.mk b/package/libjson/libjson.mk
index d04ddc40f3..9d064e53c3 100644
--- a/package/libjson/libjson.mk
+++ b/package/libjson/libjson.mk
@@ -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

[Buildroot] [PATCH 5/7] package/flare-engine: remove explicit setting of CMAKE_BUILD_TYPE

[Thomas De Schampheleire]

From: Thomas De Schampheleire <thomas.de_schampheleire@nokia.com>

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.

Signed-off-by: Thomas De Schampheleire <thomas.de_schampheleire@nokia.com>
---
 package/flare-engine/flare-engine.mk | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/package/flare-engine/flare-engine.mk b/package/flare-engine/flare-engine.mk
index e2f3eefa28..7022eac7ae 100644
--- a/package/flare-engine/flare-engine.mk
+++ b/package/flare-engine/flare-engine.mk
@@ -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

[Buildroot] [PATCH 6/7] package/supertux: remove explicit setting of CMAKE_BUILD_TYPE

[Thomas De Schampheleire]

From: Thomas De Schampheleire <thomas.de_schampheleire@nokia.com>

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.

Signed-off-by: Thomas De Schampheleire <thomas.de_schampheleire@nokia.com>
---
 package/supertux/supertux.mk | 2 --
 1 file changed, 2 deletions(-)

diff --git a/package/supertux/supertux.mk b/package/supertux/supertux.mk
index e4a4630918..a339c42e42 100644
--- a/package/supertux/supertux.mk
+++ b/package/supertux/supertux.mk
@@ -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

[Buildroot] [PATCH 7/7] package/sysrepo: remove explicit setting of CMAKE_BUILD_TYPE

[Thomas De Schampheleire]

From: Thomas De Schampheleire <thomas.de_schampheleire@nokia.com>

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

Signed-off-by: Thomas De Schampheleire <thomas.de_schampheleire@nokia.com>
---
 package/sysrepo/sysrepo.mk | 2 --
 1 file changed, 2 deletions(-)

diff --git a/package/sysrepo/sysrepo.mk b/package/sysrepo/sysrepo.mk
index e4d553cdbd..e0ac4cd245 100644
--- a/package/sysrepo/sysrepo.mk
+++ b/package/sysrepo/sysrepo.mk
@@ -13,7 +13,6 @@ 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)
 
 ifeq ($(BR2_TOOLCHAIN_HAS_LIBATOMIC),y)
@@ -26,7 +25,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

[Buildroot] [PATCH 7/7] package/sysrepo: remove explicit setting of CMAKE_BUILD_TYPE

[Jan Kundrát]

On st?eda 10. ?nora 2021 15:12:56 CET, Thomas De Schampheleire wrote:
> From: Thomas De Schampheleire <thomas.de_schampheleire@nokia.com>
>
> 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).
>
> Signed-off-by: Thomas De Schampheleire <thomas.de_schampheleire@nokia.com>
> ---
>  package/sysrepo/sysrepo.mk | 2 --
>  1 file changed, 2 deletions(-)
>
> diff --git a/package/sysrepo/sysrepo.mk b/package/sysrepo/sysrepo.mk
> index e4d553cdbd..e0ac4cd245 100644
> --- a/package/sysrepo/sysrepo.mk
> +++ b/package/sysrepo/sysrepo.mk
> @@ -13,7 +13,6 @@ 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)

You'll need to pass -DREPO_PATH=/etc/sysrepo here so that sysrepo on the 
target does not accidentally try to use a path that only exists on the 
build host. See 
https://github.com/sysrepo/sysrepo/blob/v1.4.104/CMakeLists.txt#L138-L144 
for details.

After that, feel free to add my Reviewed-by.

Jan

[Buildroot] [PATCH 7/7] package/sysrepo: remove explicit setting of CMAKE_BUILD_TYPE

[Thomas De Schampheleire]

El jue, 11 feb 2021 a las 15:16, Jan Kundr?t (<jan.kundrat@cesnet.cz>) escribi?:
>
> On st?eda 10. ?nora 2021 15:12:56 CET, Thomas De Schampheleire wrote:
> > From: Thomas De Schampheleire <thomas.de_schampheleire@nokia.com>
> >
> > 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).
> >
> > Signed-off-by: Thomas De Schampheleire <thomas.de_schampheleire@nokia.com>
> > ---
> >  package/sysrepo/sysrepo.mk | 2 --
> >  1 file changed, 2 deletions(-)
> >
> > diff --git a/package/sysrepo/sysrepo.mk b/package/sysrepo/sysrepo.mk
> > index e4d553cdbd..e0ac4cd245 100644
> > --- a/package/sysrepo/sysrepo.mk
> > +++ b/package/sysrepo/sysrepo.mk
> > @@ -13,7 +13,6 @@ 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)
>
> You'll need to pass -DREPO_PATH=/etc/sysrepo here so that sysrepo on the
> target does not accidentally try to use a path that only exists on the
> build host. See
> https://github.com/sysrepo/sysrepo/blob/v1.4.104/CMakeLists.txt#L138-L144
> for details.
>
> After that, feel free to add my Reviewed-by.

Thanks a lot, I will send a v2.