517097 - make enabling debug symbols more sane

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[(not currently active) Ted Mielczarek]

Currently to enable debug symbols you can:
--enable-debugger-info-modules - which gets you debug symbols for everything but NSPR and NSS (but you can specify individual modules to enable, if you really wanted)
export MOZ_DEBUG_SYMBOLS=1 - which gets you debug symbols for NSS on all platforms, everything else on Windows only

These are both kind of stupid. We should deprecate them and replace them with --enable-debug-symbols, which should also enable symbols for NSPR and NSS.

Comment 1

[(not currently active) Ted Mielczarek]

Mitch: should be a relatively easy patch, if you're interested.

Comment 2

[Mitchell Field [:Mitch]]

Attachment: Patch (v1)

Comment 3

[Mitchell Field [:Mitch]]

Attachment: Patch for js/src (v1)

Comment 4

[(not currently active) Ted Mielczarek]

Comment on attachment 424250 [details] [diff] [review]
Patch (v1)

This looks pretty good, but I have a few things I'd like to see changed. Also, you can combine this patch with the js/src patch when you update it, they don't need to be reviewed separately.

>diff --git a/configure.in b/configure.in
>--- a/configure.in
>+++ b/configure.in
>-dnl ========================================================
>-dnl = Enable/disable generation of debugger info for specific modules only
>-dnl =    the special module name ALL_MODULES can be used to denote all modules
>-dnl =    module names beginning with ^ will be disabled
>-dnl ========================================================
>-MOZ_ARG_ENABLE_STRING(debugger-info-modules,
>-[  --enable-debugger-info-modules
>-                          Enable/disable debugger info for specific modules],

I think we should keep this temporarily, since there may be people using it. Just make it set MOZ_DEBUG_SYMBOLS=1, and AC_MSG_WARN([--enable-debugger-info modules is deprecated, please use --enable-debug-symbols]).

>+dnl = Enable generation of debug symbols
>+dnl ========================================================
>+MOZ_ARG_ENABLE_BOOL(debug-symbols,
>+[  --enable-debug-symbols  Enable debug symbols],

I think I'd like this to be an MOZ_ARG_ENABLE_STRING, like --enable-debug, so we can do things like --enable-debug-symbols="-gdwarf2".

>+    MOZ_DEBUG_SYMBOLS=1,
>+    MOZ_DEBUG_SYMBOLS=)
>+if test -z "$MOZ_DEBUG"; then
>+    if test -n "$MOZ_DEBUG_SYMBOLS"; then
>+        AC_DEFINE(MOZ_DEBUG_SYMBOLS)
>+    fi
>+else
>+    AC_DEFINE(MOZ_DEBUG_SYMBOLS)
>+fi

I think we may also want to export MOZ_DEBUG_SYMBOLS here, so that the value makes its way down to NSPR. I believe this will work fine for NSS as written, because we already export MOZ_DEBUG_SYMBOLS in the makefile that starts the NSS build.

Other than those few things this looks great, nice code removal!

Comment 5

[(not currently active) Ted Mielczarek]

Comment on attachment 424251 [details] [diff] [review]
Patch for js/src (v1)

r- as above, please roll this into the top-level patch when you resubmit.

Comment 6

[Mitchell Field [:Mitch]]

Attachment: Patch v2

Comment 7

[Mitchell Field [:Mitch]]

Attachment: Patch v3

Allows for specification of debug symbol format on Mac OS X.

Comment 8

[(not currently active) Ted Mielczarek]

Comment on attachment 426164 [details] [diff] [review]
Patch v3

>diff --git a/configure.in b/configure.in
>--- a/configure.in
>+++ b/configure.in
>@@ -1873,20 +1873,25 @@ case "$target" in
> 	    _WARNINGS_CXXFLAGS="-Wall"
> 	    # The test above doesn't work properly, at least on 3.1.
> 	    MKSHLIB_FORCE_ALL=''
> 	    MKSHLIB_UNFORCE_ALL=''
> 	;;
>     esac
>     ;;
> 
>-*-darwin*) 
>+*-darwin*)
>     MKSHLIB='$(CXX) $(CXXFLAGS) $(DSO_PIC_CFLAGS) $(DSO_LDOPTS) -o $@'
>     MKCSHLIB='$(CC) $(CFLAGS) $(DSO_PIC_CFLAGS) $(DSO_LDOPTS) -o $@'
>     MOZ_OPTIMIZE_FLAGS="-O3"
>+    if test "$MOZ_DEBUG_SYMBOLS" != "1"; then
>+        MOZ_DEBUG_FLAGS="-g$MOZ_DEBUG_SYMBOLS"
>+    else
>+        MOZ_DEBUG_FLAGS="-g"
>+    fi

You shouldn't need to fiddle with these default settings for MOZ_DEBUG_FLAGS, it should just get overridden with whatever the user passes with --enable-debug-symbols. Also, I think these code blocks are above the --enable-debug-symbols block, so they will be executed before the option is ever seen.

See:
http://mxr.mozilla.org/mozilla-central/source/configure.in#6536
for how --enable-debug works.

Also, you should let the user pass the full -gwhatever string, like --enable-debug-symbols="-gstabs".

>+[ if test "$enableval" != "no"; then
>+    AC_MSG_WARN([--enable-debugger-info-modules is deprecated by --enable-debug-symbols])
>+    MOZ_DEBUG_SYMBOLS=1

Thanks for adding this. Slight phrasing tweak: "--enable-debugger-info-modules is deprecated, use --enable-debug-symbols instead".

Clean up those two things and you're good.

Comment 9

[Mitchell Field [:Mitch]]

Attachment: Patch v4

Comment 10

[Mitchell Field [:Mitch]]

Attachment: Patch v5

Now with less redundancy.

Comment 11

[(not currently active) Ted Mielczarek]

Comment on attachment 426240 [details] [diff] [review]
Patch v5

>diff --git a/configure.in b/configure.in
>--- a/configure.in
>+++ b/configure.in
>+MOZ_ARG_ENABLE_STRING(debug-symbols,
>+[ if test "$enableval" != "no"; then
>+      MOZ_DEBUG_SYMBOLS=1
>+      if test -n "$enableval" && if test "$enableval" != "yes"; then

You've got a second 'if' there that you don't need.

>+          if test -z "$_MOZ_DEBUG_FLAGS_SET"; then
>+              MOZ_DEBUG_FLAGS=`echo $enableval | sed -e 's|\\\ | |g'`
>+          else
>+              AC_MSG_ERROR([--enable-debug-symbols flags cannot be used with --enable-debug flags])
>+          fi
>       fi
>-      if test "$i" = "yes"; then
>-        i="ALL_MODULES"
>-      fi
>-      MOZ_DBGRINFO_MODULES="$i $MOZ_DBGRINFO_MODULES";
>-    done ])
>+  else
>+      MOZ_DEBUG_SYMBOLS=
>+  fi ],
>+  MOZ_DEBUG=)

This looks like a copy/paste leftover?

>+
>+if test -z "$MOZ_DEBUG"; then
>+    if test -n "$MOZ_DEBUG_SYMBOLS"; then
>+        AC_DEFINE(MOZ_DEBUG_SYMBOLS)
>+        export MOZ_DEBUG_SYMBOLS
>+    fi
>+else
>+    AC_DEFINE(MOZ_DEBUG_SYMBOLS)
>+    export MOZ_DEBUG_SYMBOLS
>+fi

You could just combine these into one check, like:
if test -n "$MOZ_DEBUG" -o -n "$MOZ_DEBUG_SYMBOLS"; then

r=me with those changes.

Comment 12

[(not currently active) Ted Mielczarek]

Also, we'll need a followup to make this work on NSPR on non-Windows.

Comment 13

[Mitchell Field [:Mitch]]

Attachment: Patch v6

Comment 14

[(not currently active) Ted Mielczarek]

Comment on attachment 426254 [details] [diff] [review]
Patch v6

Looks good, thanks!

Comment 15

[Mitchell Field [:Mitch]]

Attachment: Patch v7

I forgot --enable-debug-symbols' --help description.

Comment 16

[(not currently active) Ted Mielczarek]

Comment on attachment 426394 [details] [diff] [review]
Patch v7

I realized that you probably should have removed --enable-debug-modules while you were in there, since you removed the machinery that makes it work anyway. Could you update this patch one last time with that? Otherwise this looks ready to land.

Comment 17

[Mitchell Field [:Mitch]]

Attachment: Patch v8

Comment 18

[(not currently active) Ted Mielczarek]

Pushed to m-c:
http://hg.mozilla.org/mozilla-central/rev/fe08cc555248

Comment 19

[Benjamin Smedberg]

This patch caused -fno-inline to be added to opt builds, so it was backed out. 50% perf regression and an extra 1MB of codesize and all...

Comment 20

[(not currently active) Ted Mielczarek]

Yeah, our opt builds set MOZ_DEBUG_SYMBOLS=1, so they get the default debug symbols...

We can either:
a) Fix the mozconfigs to do --enable-debug-symbols="whatever", where "whatever" is the stuff we currently have in CFLAGS/CXXFLAGS
b) Ditch -fno-inline from the default debug flags, not sure if that matters or not.

Comment 21

[Ben Turner (not reading bugmail, use the needinfo flag!)]

> b) Ditch -fno-inline from the default debug flags

Hm. Can we not tie |-fno-inline| to |--enable-debug| rather than |--enable-debug-symbols| ?

Comment 22

[(not currently active) Ted Mielczarek]

It might be possible. Right now there's a default set of "enable debug symbols" flags for each compiler/arch, and GCC/Linux has -fno-inline in there. This patch just uses those same flags as the defaults for when you --enable-debug-symbols.

Comment 23

[Mitchell Field [:Mitch]]

Attachment: Patch v9

Comment 24

[(not currently active) Ted Mielczarek]

Comment on attachment 426668 [details] [diff] [review]
Patch v9

This won't work, unfortunately, because you're using $MOZ_DEBUG before it's defined (below,in the MOZ_ARG_ENABLE_BOOL(debug) block). This patch will basically make it such that we never use -fno-inline.

Comment 25

[Mitchell Field [:Mitch]]

Attachment: Patch v10

Comment 26

[(not currently active) Ted Mielczarek]

Comment on attachment 429722 [details] [diff] [review]
Patch v10

Ok, this looks good. Thanks for sticking with it!

I think we can't quite land this still, because it will probably screw up our breakpad symbols on tinderbox. Since the mozconfigs have MOZ_DEBUG_SYMBOLS=1, which is the same as --enable-debug-symbols with your patch. However, we still need to pass specific -g flags to gcc to get stabs on Linux and DWARF on Mac, and with your patch we'll wind up with an extra -g in the compile commandline which will screw things up. This should be a matter of simply adding
--enable-debug-symbols="-gwhatever" to the mozconfigs.

Comment 27

[(not currently active) Ted Mielczarek]

Pushed to m-c:
http://hg.mozilla.org/mozilla-central/rev/bedd1bb8ea77

Comment 28

[Jim Mathies [:jimm]]

This patch resulted in build bustage on linux - 
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1268236064.1268236572.18165.gz

checking for conic... checking for valid debug flags... yes
configure: error: --enable-debug-symbols flags cannot be used with --enable-debug flags
*** Fix above errors and then restart with               "make -f client.mk build"
make[1]: *** [configure] Error 1
make[1]: Leaving directory `/builds/moz2_slave/mozilla-central-linux-debug/build'
make: *** [obj-firefox/Makefile] Error 2

Comment 29

[Jim Mathies [:jimm]]

backed out.

Comment 30

[(not currently active) Ted Mielczarek]

*sigh* This broke our debug builds, because they do --enable-debug="foo", so they got:
configure: error: --enable-debug-symbols flags cannot be used with --enable-debug flags

I'll have to make another patch.

Comment 31

[(not currently active) Ted Mielczarek]

Third time's the charm:
http://hg.mozilla.org/mozilla-central/rev/79443803350c