| Message ID | 1463861314-15547-1-git-send-email-ckhardin@exablox.com |
|---|---|
| State | Accepted |
| Headers | show |
Samuel, Please review/ack this patch if appropriate. If there is no feedback in the next days, I'll assume it's good and I'll apply. Thanks, Thomas On Sat, 21 May 2016 13:08:34 -0700, Charles Hardin wrote: > From the documentation on BR2_ENABLE_DEBUG, the intention is to get > a build with debug symbols and not a "debug build" since that can have > the unintended consequence of being a different code path then a > release build type definition. > > Switch the "Debug" to "RelWithDebInfo" in the cmake package support > to accomodate getting the debug symbols and still be a release > build. > > Signed-off-by: Charles Hardin <ckhardin@exablox.com> > --- > package/pkg-cmake.mk | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/package/pkg-cmake.mk b/package/pkg-cmake.mk > index 81dcfcc..bca3603 100644 > --- a/package/pkg-cmake.mk > +++ b/package/pkg-cmake.mk > @@ -87,7 +87,7 @@ define $(2)_CONFIGURE_CMDS > PATH=$$(BR_PATH) \ > $$($$(PKG)_CONF_ENV) $$(HOST_DIR)/usr/bin/cmake $$($$(PKG)_SRCDIR) \ > -DCMAKE_TOOLCHAIN_FILE="$$(HOST_DIR)/usr/share/buildroot/toolchainfile.cmake" \ > - -DCMAKE_BUILD_TYPE=$$(if $$(BR2_ENABLE_DEBUG),Debug,Release) \ > + -DCMAKE_BUILD_TYPE=$$(if $$(BR2_ENABLE_DEBUG),RelWithDebInfo,Release) \ > -DCMAKE_INSTALL_PREFIX="/usr" \ > -DCMAKE_COLOR_MAKEFILE=OFF \ > -DBUILD_DOC=OFF \
Hi all, On Sat, Jun 11, 2016 at 4:19 PM, Thomas Petazzoni <thomas.petazzoni@free-electrons.com> wrote: > Samuel, > > Please review/ack this patch if appropriate. > > If there is no feedback in the next days, I'll assume it's good and > I'll apply. Ooops, I missed this patch :-/ > > Thanks, > > Thomas > > On Sat, 21 May 2016 13:08:34 -0700, Charles Hardin wrote: >> From the documentation on BR2_ENABLE_DEBUG, the intention is to get >> a build with debug symbols and not a "debug build" since that can have >> the unintended consequence of being a different code path then a >> release build type definition. >> >> Switch the "Debug" to "RelWithDebInfo" in the cmake package support >> to accomodate getting the debug symbols and still be a release >> build. >> By default CMake sets the CFLAGS like this: CMAKE_C_FLAGS_DEBUG=-g CMAKE_C_FLAGS_RELWITHDEBINFO=-O2 -g -DNDEBUG CMAKE_C_FLAGS_RELEASE=-O3 -DNDEBUG I do not see what could be wrong with this patch, hence: Reviewed-by: Samuel Martin <s.martin49@gmail.com> >> Signed-off-by: Charles Hardin <ckhardin@exablox.com> >> --- >> package/pkg-cmake.mk | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/package/pkg-cmake.mk b/package/pkg-cmake.mk >> index 81dcfcc..bca3603 100644 >> --- a/package/pkg-cmake.mk >> +++ b/package/pkg-cmake.mk >> @@ -87,7 +87,7 @@ define $(2)_CONFIGURE_CMDS >> PATH=$$(BR_PATH) \ >> $$($$(PKG)_CONF_ENV) $$(HOST_DIR)/usr/bin/cmake $$($$(PKG)_SRCDIR) \ >> -DCMAKE_TOOLCHAIN_FILE="$$(HOST_DIR)/usr/share/buildroot/toolchainfile.cmake" \ >> - -DCMAKE_BUILD_TYPE=$$(if $$(BR2_ENABLE_DEBUG),Debug,Release) \ >> + -DCMAKE_BUILD_TYPE=$$(if $$(BR2_ENABLE_DEBUG),RelWithDebInfo,Release) \ >> -DCMAKE_INSTALL_PREFIX="/usr" \ >> -DCMAKE_COLOR_MAKEFILE=OFF \ >> -DBUILD_DOC=OFF \ > > > > -- > Thomas Petazzoni, CTO, Free Electrons > Embedded Linux, Kernel and Android engineering > http://free-electrons.com Regards,
Hello, On Sat, 11 Jun 2016 17:22:24 +0200, Samuel Martin wrote: > By default CMake sets the CFLAGS like this: > CMAKE_C_FLAGS_DEBUG=-g > CMAKE_C_FLAGS_RELWITHDEBINFO=-O2 -g -DNDEBUG Is -O2 -g really what we want for debugging? Optimized code very often doesn't play well with debugging. Thomas
On Sat, Jun 11, 2016 at 7:06 PM, Thomas Petazzoni <thomas.petazzoni@free-electrons.com> wrote: > Hello, > > On Sat, 11 Jun 2016 17:22:24 +0200, Samuel Martin wrote: > >> By default CMake sets the CFLAGS like this: >> CMAKE_C_FLAGS_DEBUG=-g >> CMAKE_C_FLAGS_RELWITHDEBINFO=-O2 -g -DNDEBUG > > Is -O2 -g really what we want for debugging? Optimized code very often > doesn't play well with debugging. To be honest, I don't have a strong opinion about it. Note that this is the default flags CMake sets; if you add to the configure opts: -DCMAKE_C_FLAGS_RELWITHDEBINFO=-g -O1 -UNDEBUG then these flags are taken into account, not the default ones. OTOH, applying no optimization at all in debug build may lead to some size issues for (very) tiny targets... Who knows? though I don't remember any bug report/complaint about such a situation. Regards,
We hit the size and the NDEBUG issue as well. Sent from my iPhone > On Jun 11, 2016, at 10:22 AM, Samuel Martin <s.martin49@gmail.com> wrote: > > On Sat, Jun 11, 2016 at 7:06 PM, Thomas Petazzoni > <thomas.petazzoni@free-electrons.com> wrote: >> Hello, >> >>> On Sat, 11 Jun 2016 17:22:24 +0200, Samuel Martin wrote: >>> >>> By default CMake sets the CFLAGS like this: >>> CMAKE_C_FLAGS_DEBUG=-g >>> CMAKE_C_FLAGS_RELWITHDEBINFO=-O2 -g -DNDEBUG >> >> Is -O2 -g really what we want for debugging? Optimized code very often >> doesn't play well with debugging. > > To be honest, I don't have a strong opinion about it. > > Note that this is the default flags CMake sets; if you add to the > configure opts: > -DCMAKE_C_FLAGS_RELWITHDEBINFO=-g -O1 -UNDEBUG > then these flags are taken into account, not the default ones. > > OTOH, applying no optimization at all in debug build may lead to some > size issues for (very) tiny targets... Who knows? though I don't > remember any bug report/complaint about such a situation. > > Regards, > > -- > Samuel
Hello, On Sat, 21 May 2016 13:08:34 -0700, Charles Hardin wrote: > From the documentation on BR2_ENABLE_DEBUG, the intention is to get > a build with debug symbols and not a "debug build" since that can have > the unintended consequence of being a different code path then a > release build type definition. > > Switch the "Debug" to "RelWithDebInfo" in the cmake package support > to accomodate getting the debug symbols and still be a release > build. > > Signed-off-by: Charles Hardin <ckhardin@exablox.com> > --- > package/pkg-cmake.mk | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) Applied to master, thanks. Thomas
diff --git a/package/pkg-cmake.mk b/package/pkg-cmake.mk index 81dcfcc..bca3603 100644 --- a/package/pkg-cmake.mk +++ b/package/pkg-cmake.mk @@ -87,7 +87,7 @@ define $(2)_CONFIGURE_CMDS PATH=$$(BR_PATH) \ $$($$(PKG)_CONF_ENV) $$(HOST_DIR)/usr/bin/cmake $$($$(PKG)_SRCDIR) \ -DCMAKE_TOOLCHAIN_FILE="$$(HOST_DIR)/usr/share/buildroot/toolchainfile.cmake" \ - -DCMAKE_BUILD_TYPE=$$(if $$(BR2_ENABLE_DEBUG),Debug,Release) \ + -DCMAKE_BUILD_TYPE=$$(if $$(BR2_ENABLE_DEBUG),RelWithDebInfo,Release) \ -DCMAKE_INSTALL_PREFIX="/usr" \ -DCMAKE_COLOR_MAKEFILE=OFF \ -DBUILD_DOC=OFF \
From the documentation on BR2_ENABLE_DEBUG, the intention is to get a build with debug symbols and not a "debug build" since that can have the unintended consequence of being a different code path then a release build type definition. Switch the "Debug" to "RelWithDebInfo" in the cmake package support to accomodate getting the debug symbols and still be a release build. Signed-off-by: Charles Hardin <ckhardin@exablox.com> --- package/pkg-cmake.mk | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)