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