[pulseaudio-discuss] [PATCH] build-sys: Add an --enable-debug option

[Tanu Kaskinen]

On Tue, 2014-04-22 at 13:09 +0530, Arun Raghavan wrote:
> This stops the abuse of __OPTIMIZE__ in builds, and adds an explicit
> configure-time option to enable a debug-friendly build. We can extend
> this with more debug-related options as needed.
> 
> One small side-effect that this change has is that non-debug builds no
> longer have fastpath asserts enabled (which is how it should be).
> ---
>  README                           |  6 +-----
>  bootstrap.sh                     |  2 +-
>  configure.ac                     | 12 +++++-------
>  src/daemon/main.c                |  6 ++----
>  src/modules/gconf/module-gconf.c |  2 +-
>  src/pulsecore/pid.c              |  4 ++--
>  6 files changed, 12 insertions(+), 20 deletions(-)
> 
> diff --git a/README b/README
> index 66c1847..904d1c1 100644
> --- a/README
> +++ b/README
> @@ -34,11 +34,7 @@ AUTHORS:
>  	Several
>  
>  HACKING:
> -	In order to run pulseaudio from the build dir __OPTIMIZE__ should be
> -	disabled (look at src/pulsecore/core-util.h::pa_run_from_build_tree()),
> -	this can be done by passing "CFLAGS=-O0" to the configure script:
> -	  ./autogen.sh
> -	  CFLAGS="-ggdb3 -O0" LDFLAGS="-ggdb3" ./configure
> +	  ./configure
>  	  make
>  	  ./src/pulseaudio -n -F src/default.pa -p $(pwd)/src/
>  
> diff --git a/bootstrap.sh b/bootstrap.sh
> index 08e0fa4..56ddd2a 100755
> --- a/bootstrap.sh
> +++ b/bootstrap.sh
> @@ -55,6 +55,6 @@ autopoint --force
>  AUTOPOINT='intltoolize --automake --copy' autoreconf --force --install --verbose
>  
>  if test "x$NOCONFIGURE" = "x"; then
> -    CFLAGS="$CFLAGS -g -O0" ./configure --sysconfdir=/etc --localstatedir=/var --enable-force-preopen "$@"
> +    CFLAGS="$CFLAGS -g -O0" ./configure --sysconfdir=/etc --localstatedir=/var --enable-force-preopen --enable-debug "$@"
>      make clean
>  fi
> diff --git a/configure.ac b/configure.ac
> index e75973f..75d76b2 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -177,13 +177,11 @@ AX_APPEND_COMPILE_FLAGS(
>      [-Wall -W -Wextra -pipe -Wno-long-long -Wno-overlength-strings -Wunsafe-loop-optimizations -Wundef -Wformat=2 -Wlogical-op -Wsign-compare -Wformat-security -Wmissing-include-dirs -Wformat-nonliteral -Wold-style-definition -Wpointer-arith -Winit-self -Wdeclaration-after-statement -Wfloat-equal -Wmissing-prototypes -Wstrict-prototypes -Wredundant-decls -Wmissing-declarations -Wmissing-noreturn -Wshadow -Wendif-labels -Wcast-align -Wstrict-aliasing -Wwrite-strings -Wno-unused-parameter -ffast-math -fno-common -fdiagnostics-show-option],
>      [], [-pedantic -Werror])
>  
> -# Only enable fastpath asserts when doing a debug build, e.g. from bootstrap.sh.
> -AS_CASE([" $CFLAGS "], [*" -O0 "*], [], [AX_APPEND_FLAG(["-DFASTPATH"], [CPPFLAGS])])
> -
> -# Only set _FORTIFY_SOURCE when optimizations are enabled. If optimizations
> -# are disabled, _FORTIFY_SOURCE doesn't do anything, and causes tons of
> -# warnings during compiling on some distributions (at least Fedora).
> -AS_CASE([" $CFLAGS "], [*" -O0 "*], [], [AX_APPEND_FLAG(["-D_FORTIFY_SOURCE=2"], [CPPFLAGS])])
> +AC_ARG_ENABLE([debug],
> +    AS_HELP_STRING([--enable-debug], [Enable a build suitable for debugging]))
> +AS_IF([test "x$enable_debug" != "xno"],
> +    [AX_APPEND_FLAG(["-DDEBUG -D_FORTIFY_SOURCE=2"], [CPPFLAGS])],
> +    [AX_APPEND_FLAG(["-DFASTPATH"], [CPPFLAGS])])

You probably meant != "xyes" in the test, this doesn't make sense
otherwise.

I think the _FORTIFY_SOURCE check should be left as it was, because
_FORTIFY_SOURCE depends on optimizations, not on whether the "debug
mode" is enabled.

>  
> 
>  #### Linker flags ####
> diff --git a/src/daemon/main.c b/src/daemon/main.c
> index 02a8ea6..d0345c9 100644
> --- a/src/daemon/main.c
> +++ b/src/daemon/main.c
> @@ -427,12 +427,10 @@ int main(int argc, char *argv[]) {
>      pa_log_set_level(PA_LOG_NOTICE);
>      pa_log_set_flags(PA_LOG_COLORS|PA_LOG_PRINT_FILE|PA_LOG_PRINT_LEVEL, PA_LOG_RESET);
>  
> -#if defined(__linux__) && defined(__OPTIMIZE__)
> +#ifndef DEBUG
>      /*
>         Disable lazy relocations to make usage of external libraries
> -       more deterministic for our RT threads. We abuse __OPTIMIZE__ as
> -       a check whether we are a debug build or not. This all is
> -       admittedly a bit snake-oilish.
> +       more deterministic for our RT threads.
>      */
>  
>      if (!getenv("LD_BIND_NOW")) {
> diff --git a/src/modules/gconf/module-gconf.c b/src/modules/gconf/module-gconf.c
> index d791b00..61ac6bf 100644
> --- a/src/modules/gconf/module-gconf.c
> +++ b/src/modules/gconf/module-gconf.c
> @@ -339,7 +339,7 @@ int pa__init(pa_module*m) {
>      u->buf_fill = 0;
>  
>      if ((u->fd = pa_start_child_for_read(
> -#if defined(__linux__) && !defined(__OPTIMIZE__)
> +#ifdef DEBUG
>                                pa_run_from_build_tree() ? PA_BUILDDIR "/gconf-helper" :
>  #endif
>                   PA_GCONF_HELPER, NULL, &u->pid)) < 0)
> diff --git a/src/pulsecore/pid.c b/src/pulsecore/pid.c
> index e347884..cae4234 100644
> --- a/src/pulsecore/pid.c
> +++ b/src/pulsecore/pid.c
> @@ -165,14 +165,14 @@ static int proc_name_ours(pid_t pid, const char *procname) {
>          good = pa_startswith(stored, expected);
>          pa_xfree(expected);
>  
> -/*#if !defined(__OPTIMIZE__)*/
> +#ifdef DEBUG
>          if (!good) {
>              /* libtool likes to rename our binary names ... */
>              expected = pa_sprintf_malloc("%lu (lt-%s)", (unsigned long) pid, procname);
>              good = pa_startswith(stored, expected);
>              pa_xfree(expected);
>          }
> -/*#endif*/
> +#endif

The previous #if was probably commented out for a reason. I'd prefer you
to not disable this logic on non-debug builds, because I don't see
significant benefit in doing so, and it can potentially cause trouble
for someone.

-- 
Tanu