Closed Bug 517097 Opened 10 years ago Closed 10 years ago

make enabling debug symbols more sane

Categories

(Firefox Build System :: General, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
mozilla1.9.3a3

People

(Reporter: ted, Assigned: Mitch)

References

Details

Attachments

(1 file, 10 obsolete files)

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.
Mitch: should be a relatively easy patch, if you're interested.
Assignee: nobody → mitch_1_2
Status: NEW → ASSIGNED
Attached patch Patch (v1) (obsolete) — Splinter Review
Attachment #424250 - Flags: review?(ted.mielczarek)
Attached patch Patch for js/src (v1) (obsolete) — Splinter Review
Attachment #424251 - Flags: review?(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!
Attachment #424250 - Flags: review?(ted.mielczarek) → review-
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.
Attachment #424251 - Flags: review?(ted.mielczarek) → review-
Attached patch Patch v2 (obsolete) — Splinter Review
Attachment #424250 - Attachment is obsolete: true
Attachment #424251 - Attachment is obsolete: true
Attachment #425970 - Flags: review?(ted.mielczarek)
Attached patch Patch v3 (obsolete) — Splinter Review
Allows for specification of debug symbol format on Mac OS X.
Attachment #425970 - Attachment is obsolete: true
Attachment #426164 - Flags: review?(ted.mielczarek)
Attachment #425970 - Flags: review?(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.
Attachment #426164 - Flags: review?(ted.mielczarek) → review-
Attached patch Patch v4 (obsolete) — Splinter Review
Attachment #426164 - Attachment is obsolete: true
Attachment #426239 - Flags: review?(ted.mielczarek)
Attached patch Patch v5 (obsolete) — Splinter Review
Now with less redundancy.
Attachment #426239 - Attachment is obsolete: true
Attachment #426240 - Flags: review?(ted.mielczarek)
Attachment #426239 - Flags: review?(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.
Attachment #426240 - Flags: review?(ted.mielczarek) → review+
Also, we'll need a followup to make this work on NSPR on non-Windows.
Attached patch Patch v6 (obsolete) — Splinter Review
Attachment #426240 - Attachment is obsolete: true
Attachment #426254 - Flags: review?(ted.mielczarek)
Comment on attachment 426254 [details] [diff] [review]
Patch v6

Looks good, thanks!
Attachment #426254 - Flags: review?(ted.mielczarek) → review+
Attached patch Patch v7 (obsolete) — Splinter Review
I forgot --enable-debug-symbols' --help description.
Attachment #426254 - Attachment is obsolete: true
Attachment #426394 - Flags: review?(ted.mielczarek)
Blocks: 338224
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.
Attachment #426394 - Flags: review?(ted.mielczarek) → review+
Attached patch Patch v8 (obsolete) — Splinter Review
Attachment #426394 - Attachment is obsolete: true
Attachment #426504 - Flags: review?(ted.mielczarek)
Attachment #426504 - Flags: review?(ted.mielczarek) → review+
Pushed to m-c:
http://hg.mozilla.org/mozilla-central/rev/fe08cc555248
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
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...
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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.
> b) Ditch -fno-inline from the default debug flags

Hm. Can we not tie |-fno-inline| to |--enable-debug| rather than |--enable-debug-symbols| ?
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.
Attached patch Patch v9 (obsolete) — Splinter Review
Attachment #426504 - Attachment is obsolete: true
Attachment #426668 - Flags: review?(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.
Attachment #426668 - Flags: review?(ted.mielczarek) → review-
Attached patch Patch v10Splinter Review
Attachment #426668 - Attachment is obsolete: true
Attachment #429722 - Flags: review?(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.
Attachment #429722 - Flags: review?(ted.mielczarek) → review+
Depends on: 550241
Pushed to m-c:
http://hg.mozilla.org/mozilla-central/rev/bedd1bb8ea77
Status: REOPENED → RESOLVED
Closed: 10 years ago10 years ago
Resolution: --- → FIXED
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
backed out.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
*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.
Depends on: 551501
Third time's the charm:
http://hg.mozilla.org/mozilla-central/rev/79443803350c
Status: REOPENED → RESOLVED
Closed: 10 years ago10 years ago
Resolution: --- → FIXED
Flags: in-testsuite-
Target Milestone: --- → mozilla1.9.3a3
Blocks: 575179
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.