[Buildroot] [PATCHv2 0/7] Introduce BR2_ENABLE_RUNTIME_DEBUG

[Buildroot] [PATCHv2 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.
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

[Buildroot] [PATCHv2 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] [PATCHv2 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] [PATCHv2 2/7] core: enable 'NDEBUG' unless BR2_ENABLE_RUNTIME_DEBUG is set

[Thomas Petazzoni]

On Fri, 12 Feb 2021 14:54:45 +0100
Thomas De Schampheleire <patrickdepinguin@gmail.com> wrote:

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

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

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

[Thomas De Schampheleire]

Hi Thomas,

El s?b, 13 feb 2021 a las 22:56, Thomas Petazzoni
(<thomas.petazzoni@bootlin.com>) escribi?:
>
> On Fri, 12 Feb 2021 14:54:45 +0100
> Thomas De Schampheleire <patrickdepinguin@gmail.com> wrote:
>
> > 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).
>
> 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.

'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

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

[Thomas De Schampheleire]

Hello,

El lun, 15 feb 2021 a las 10:11, Thomas De Schampheleire
(<patrickdepinguin@gmail.com>) escribi?:
>
> Hi Thomas,
>
> El s?b, 13 feb 2021 a las 22:56, Thomas Petazzoni
> (<thomas.petazzoni@bootlin.com>) escribi?:
> >
> > On Fri, 12 Feb 2021 14:54:45 +0100
> > Thomas De Schampheleire <patrickdepinguin@gmail.com> wrote:
> >
> > > 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).
> >
> > 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.
>
> '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.

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

[Buildroot] [PATCHv2 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] [PATCHv2 3/7] package/pkg-cmake.mk: determine CMAKE_BUILD_TYPE depending on BR2_ENABLE_RUNTIME_DEBUG

[Alexander Dahl]

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.

On Fri, Feb 12, 2021 at 02:54:46PM +0100, Thomas De Schampheleire wrote:
> 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.

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

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

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.

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

That said, I see no use anymore in using "Release" at all.
"RelWithDebInfo" in combination with detached debug symbols works as
well. This means, NDEBUG is set for both of those builds, so no real
difference. (Yes of course, it might affect build time and those
symbols take space.)

And for "real" debug you can turn on "Debug" with all the stuff
masked by NDEBUG before.

Greets
Alex

-- 
/"\ 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>

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

[Thomas De Schampheleire]

Hi Alex,

El s?b, 13 feb 2021 a las 16:56, Alexander Dahl (<post@lespocky.de>) escribi?:
>
> 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.
>
> On Fri, Feb 12, 2021 at 02:54:46PM +0100, Thomas De Schampheleire wrote:
> > 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.
>
> For that description I would consider "RelWithDebInfo" the correct
> option.

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

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

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.

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

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

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

[Arnout Vandecappelle]

On 12/02/2021 14:54, Thomas De Schampheleire wrote:
> 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.

 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

[Buildroot] [PATCHv2 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] [PATCHv2 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] [PATCHv2 5/7] package/flare-engine: remove explicit setting of CMAKE_BUILD_TYPE

[Arnout Vandecappelle]

On 12/02/2021 14:54, Thomas De Schampheleire wrote:
> 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

 NACK to this one. This is required to fix a build issue:

commit aa9d77c8518c5245b4e220d5f15e7c3733aa78be
Author: Fabrice Fontaine <fontaine.fabrice@gmail.com>
Date:   Sun Nov 18 22:34:39 2018

    flare-engine: fix debug build

    If BR2_ENABLE_DEBUG is set, use RelWithDebInfo instead of default Debug
    as Debug will add -pg (gprof) which is not always available on toolchain

    Fixes:
      -
http://autobuild.buildroot.org/results/a12ae622a44bbe025c3a8b7e8e4c253b52927ae8


 Unless you have a way to know that the toolchain has the profiling library,
this has to be kept.

 And anyway, profiling has little to do with runtime debugging IMHO.

 Regards,
 Arnout

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

[Buildroot] [PATCHv2 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] [PATCHv2 6/7] package/supertux: remove explicit setting of CMAKE_BUILD_TYPE

[Arnout Vandecappelle]

On 12/02/2021 14:54, Thomas De Schampheleire wrote:
> 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)

 Same here: it may fail to build if no profiling is available in the toolchain.

 Regards,
 Arnout

>  # 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 \
> 

[Buildroot] [PATCHv2 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>
---
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(-)

diff --git a/package/sysrepo/sysrepo.mk b/package/sysrepo/sysrepo.mk
index e4d553cdbd..768cde8d35 100644
--- a/package/sysrepo/sysrepo.mk
+++ b/package/sysrepo/sysrepo.mk
@@ -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

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

[Jan Kundrát]

On p?tek 12. ?nora 2021 14:54:50 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>
> ---
> 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)

Reviewed-by: Jan Kundr?t <jan.kundrat@cesnet.cz>

Jan

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

[Arnout Vandecappelle]

On 12/02/2021 14:54, 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>
> ---
> 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(-)
> 
> diff --git a/package/sysrepo/sysrepo.mk b/package/sysrepo/sysrepo.mk
> index e4d553cdbd..768cde8d35 100644
> --- a/package/sysrepo/sysrepo.mk
> +++ b/package/sysrepo/sysrepo.mk
> @@ -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

 Another effect of CMAKE_BUILD_TYPE=Debug for this package is that it enables
the tests, so we should probably also add

	-DENABLE_TESTS=OFF \
	-DENABLE_VALGRIND_TESTS=OFF


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

 Note that this change could be made independent of the rest of the series,
because for host builds we always use the default build type for that package.

 Regards,
 Arnout

>  	-DBUILD_EXAMPLES=OFF \
>  	-DREPO_PATH=$(TARGET_DIR)/etc/sysrepo
>  
>