[pacman-dev] Enable additional debug flags/logging with debugoptimized builds

Message ID 20181203143636.1559-1-dreisner@archlinux.org
State Accepted, archived
Headers show
Series
  • [pacman-dev] Enable additional debug flags/logging with debugoptimized builds
Related show

Commit Message

Dave Reisner Dec. 3, 2018, 2:36 p.m. UTC
This lets developers run a local build with optimizations but also the
added debug logging that comes with PACMAN_DEBUG being defined.
---
 meson.build | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Allan McRae Dec. 4, 2018, 7:14 a.m. UTC | #1
On 4/12/18 12:36 am, Dave Reisner wrote:
> This lets developers run a local build with optimizations but also the
> added debug logging that comes with PACMAN_DEBUG being defined.
> ---

So, we only test for "debug" to enable anything.  What options starting
with debug would be used here?

Also, all those warning flags in this test are set with --enable-warning
flags in autotools based builds.  Strangely, all the options from
--enable-debug are not set here...

>  meson.build | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/meson.build b/meson.build
> index 8837816f..394bd1f5 100644
> --- a/meson.build
> +++ b/meson.build
> @@ -211,7 +211,7 @@ elif conf.has('HAVE_STRUCT_STATFS_F_FLAGS')
>    conf.set('FSSTATSTYPE', 'struct statfs')
>  endif
>  
> -if get_option('buildtype') == 'debug'
> +if get_option('buildtype').startswith('debug')
>    extra_cflags = [
>      '-Wcast-align',
>      '-Wclobbered',
>
Dave Reisner Dec. 4, 2018, 12:56 p.m. UTC | #2
On Tue, Dec 04, 2018 at 05:14:50PM +1000, Allan McRae wrote:
> On 4/12/18 12:36 am, Dave Reisner wrote:
> > This lets developers run a local build with optimizations but also the
> > added debug logging that comes with PACMAN_DEBUG being defined.
> > ---
> 
> So, we only test for "debug" to enable anything.  What options starting
> with debug would be used here?

meson understands --buildtype=debug and --buildtype=debugoptimized when
you configure the build. Before this patch, only the former buildtype is
accepted in order to trigger these extra flags. After, both buildtypes
are.

> Also, all those warning flags in this test are set with --enable-warning
> flags in autotools based builds.  Strangely, all the options from
> --enable-debug are not set here...

I chose not to add a -Ddebug=true flag to the meson build and instead
key off of the buildtype. With this patch, I'm essentially suggesting
that developers who want a suitable debugging build should just build as
--buildtype=(debug|debugoptimized). That gives you the extra warning
flags and extra timestamp logging just the same as --enable-debug and
--enable-warning-flags gave you with autotools.

Do you feel there's reason to separate these concerns?

> >  meson.build | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/meson.build b/meson.build
> > index 8837816f..394bd1f5 100644
> > --- a/meson.build
> > +++ b/meson.build
> > @@ -211,7 +211,7 @@ elif conf.has('HAVE_STRUCT_STATFS_F_FLAGS')
> >    conf.set('FSSTATSTYPE', 'struct statfs')
> >  endif
> >  
> > -if get_option('buildtype') == 'debug'
> > +if get_option('buildtype').startswith('debug')
> >    extra_cflags = [
> >      '-Wcast-align',
> >      '-Wclobbered',
> >
Eli Schwartz Dec. 4, 2018, 2:27 p.m. UTC | #3
On 12/4/18 7:56 AM, Dave Reisner wrote:
> On Tue, Dec 04, 2018 at 05:14:50PM +1000, Allan McRae wrote:
>> On 4/12/18 12:36 am, Dave Reisner wrote:
>>> This lets developers run a local build with optimizations but also the
>>> added debug logging that comes with PACMAN_DEBUG being defined.
>>> ---
>>
>> So, we only test for "debug" to enable anything.  What options starting
>> with debug would be used here?
> 
> meson understands --buildtype=debug and --buildtype=debugoptimized when
> you configure the build. Before this patch, only the former buildtype is
> accepted in order to trigger these extra flags. After, both buildtypes
> are.
> 
>> Also, all those warning flags in this test are set with --enable-warning
>> flags in autotools based builds.  Strangely, all the options from
>> --enable-debug are not set here...
> 
> I chose not to add a -Ddebug=true flag to the meson build and instead
> key off of the buildtype. With this patch, I'm essentially suggesting
> that developers who want a suitable debugging build should just build as
> --buildtype=(debug|debugoptimized). That gives you the extra warning
> flags and extra timestamp logging just the same as --enable-debug and
> --enable-warning-flags gave you with autotools.
> 
> Do you feel there's reason to separate these concerns?
When using makepkg to build pacman-git, I use --buildtype=plain and let
makepkg.conf handle debug builds. But maybe I still want the timestamping.

Conversely, whenever we get around to enabling debug packages in the
official Arch repositories, the people-whose-decision-I-disagree-with
are not unlikely to begin hardcoding --buildtype=debug in the entire
repository tree (as they currently hardcode --buildtype=release) which
sort of obviates the purpose of guarding this flag at all.
Dave Reisner Dec. 4, 2018, 2:50 p.m. UTC | #4
On Tue, Dec 04, 2018 at 09:27:52AM -0500, Eli Schwartz wrote:
> On 12/4/18 7:56 AM, Dave Reisner wrote:
> > On Tue, Dec 04, 2018 at 05:14:50PM +1000, Allan McRae wrote:
> >> On 4/12/18 12:36 am, Dave Reisner wrote:
> >>> This lets developers run a local build with optimizations but also the
> >>> added debug logging that comes with PACMAN_DEBUG being defined.
> >>> ---
> >>
> >> So, we only test for "debug" to enable anything.  What options starting
> >> with debug would be used here?
> > 
> > meson understands --buildtype=debug and --buildtype=debugoptimized when
> > you configure the build. Before this patch, only the former buildtype is
> > accepted in order to trigger these extra flags. After, both buildtypes
> > are.
> > 
> >> Also, all those warning flags in this test are set with --enable-warning
> >> flags in autotools based builds.  Strangely, all the options from
> >> --enable-debug are not set here...
> > 
> > I chose not to add a -Ddebug=true flag to the meson build and instead
> > key off of the buildtype. With this patch, I'm essentially suggesting
> > that developers who want a suitable debugging build should just build as
> > --buildtype=(debug|debugoptimized). That gives you the extra warning
> > flags and extra timestamp logging just the same as --enable-debug and
> > --enable-warning-flags gave you with autotools.
> > 
> > Do you feel there's reason to separate these concerns?
> When using makepkg to build pacman-git, I use --buildtype=plain and let
> makepkg.conf handle debug builds. But maybe I still want the timestamping.

For c/c++ builds, --buildtype=debug isn't more than --buildtype=plain
with an added -g flag. I'm not sure how this would get in your way.
Allan McRae Dec. 10, 2018, 1:19 a.m. UTC | #5
On 4/12/18 10:56 pm, Dave Reisner wrote:
> On Tue, Dec 04, 2018 at 05:14:50PM +1000, Allan McRae wrote:
>> On 4/12/18 12:36 am, Dave Reisner wrote:
>>> This lets developers run a local build with optimizations but also the
>>> added debug logging that comes with PACMAN_DEBUG being defined.
>>> ---
>>
>> So, we only test for "debug" to enable anything.  What options starting
>> with debug would be used here?
> 
> meson understands --buildtype=debug and --buildtype=debugoptimized when
> you configure the build. Before this patch, only the former buildtype is
> accepted in order to trigger these extra flags. After, both buildtypes
> are.
> 
>> Also, all those warning flags in this test are set with --enable-warning
>> flags in autotools based builds.  Strangely, all the options from
>> --enable-debug are not set here...
> 
> I chose not to add a -Ddebug=true flag to the meson build and instead
> key off of the buildtype. With this patch, I'm essentially suggesting
> that developers who want a suitable debugging build should just build as
> --buildtype=(debug|debugoptimized). That gives you the extra warning
> flags and extra timestamp logging just the same as --enable-debug and
> --enable-warning-flags gave you with autotools.
> 
> Do you feel there's reason to separate these concerns?
> 

I think we separated these due to compiler updates making lots of
warnings.  Then we could have debug builds without erroring on new
warning until they were fixed.  However...  meson debug does not add
-Werror, so separating them is not really needed.  Happy with the patch
as is.


As an aside, looking at the configure.ac:

if test "x$debug" = "xyes" ; then
	AC_MSG_RESULT(yes)
	AC_DEFINE([PACMAN_DEBUG], , [Enable debug code])
	# Check for -fstack-protector availability
	GCC_STACK_PROTECT_LIB
	GCC_STACK_PROTECT_CC
	GCC_FORTIFY_SOURCE_CC
	WARNING_CFLAGS="-g -Wall -Werror"
else
	AC_MSG_RESULT(no)
	WARNING_CFLAGS="-Wall"
fi

Is -Wall default in meson builds?  Should we enforce stack protection
stuff in our debug builds (for those of us not building with default
Arch makepkg flags)?


Allan
Eli Schwartz Dec. 10, 2018, 1:25 a.m. UTC | #6
On 12/4/18 9:50 AM, Dave Reisner wrote:
>> When using makepkg to build pacman-git, I use --buildtype=plain and let
>> makepkg.conf handle debug builds. But maybe I still want the timestamping.
> 
> For c/c++ builds, --buildtype=debug isn't more than --buildtype=plain
> with an added -g flag. I'm not sure how this would get in your way.

Maybe we should just add_global_arguments('-g', language : 'c')

Since it doesn't get in peoples' way, there should be no harm in forcing
this on all the time, right?

Patch

diff --git a/meson.build b/meson.build
index 8837816f..394bd1f5 100644
--- a/meson.build
+++ b/meson.build
@@ -211,7 +211,7 @@  elif conf.has('HAVE_STRUCT_STATFS_F_FLAGS')
   conf.set('FSSTATSTYPE', 'struct statfs')
 endif
 
-if get_option('buildtype') == 'debug'
+if get_option('buildtype').startswith('debug')
   extra_cflags = [
     '-Wcast-align',
     '-Wclobbered',