244231 - use pdb files for debug symbols

Status: RESOLVED FIXED

(SeaMonkey :: Build Config, defect)

Description

[Brian Ryner (not reading)]

We should use pdb files (rather than CV debug info) for debug symbols on
Windows, since CV has a type data limitation that breaks it for static builds. 
The Talkback server supports pdb symbols now.

Comment 1

[Brian Ryner (not reading)]

Attachment: patch

Support MOZ_PDB, which can be set instead of MOZ_PROFILE when doing release
builds.

Comment 2

[Brian Ryner (not reading)]

Attachment: patch "make deliver"

Comment 3

[Daniel (Leaf) Nunes]

Comment on attachment 149004 [details] [diff] [review]
patch

r=leaf

Comment 4

[Daniel (Leaf) Nunes]

Comment on attachment 149369 [details] [diff] [review]
patch "make deliver"

this one, too.

Comment 5

[Wan-Teh Chang]

Comment on attachment 149004 [details] [diff] [review]
patch

Why are you using -Z7 in NSPR and -Zi in Mozilla?

Is Mozilla using MOZ_PROFILE?  It seems that we
should just pick one debug info format and use
the original MOZ_PROFILE variable to turn that on.

Comment 6

[Brian Ryner (not reading)]

Comment on attachment 149004 [details] [diff] [review]
patch

wtc, we really need this for firefox trunk

Comment 7

[Wan-Teh Chang]

Comment on attachment 149004 [details] [diff] [review]
patch

The idea of the patch is okay, but I'd like you
to rename MOZ_PDB so its name reflects its purpose,
and add comment to explain what it is and how to use
it (like the comment that explains MOZ_PROFILE in
mozilla/config/config.mk).  Otherwise other people
won't know what the MOZ_PDB variable is for.

Don't you need to modify mozilla/security/coreconf
(for NSS) and mozilla/directory/c-sdk, too?

Comment 8

[Wan-Teh Chang]

Let me clarify: it seems that MOZ_PDB is intended for
generating Talkback symbols, so the variable's name
should reflect that.

Comment 9

[Brian Ryner (not reading)]

It certainly has uses other than talkback, for example, pdb files can be used
when instrumenting with Quantify or Purify (so can CV debug symbols, hence the
origin of MOZ_PROFILE).  MOZ_DEBUG_SYMBOLS is about the most descriptive I could
be.  Is that name ok with you?

Up to this point we haven't worried about getting debug symbols for NSS... as
far as I can tell it's not currently possible to compile NSS with -g (linux/mac)
when optimization is turned on either.  I'd propose doing debug symbol support
for NSS, on all platforms, as a later patch.  You're right about ldap c-sdk
though, it currently understands MOZ_PROFILE and should be made to understand
MOZ_DEBUG_SYMBOLS as well.

Also, just to clarify, I think we can deprecate MOZ_PROFILE but I'm hesitant to
remove it entirely, not knowing who may be depending on it with external tools
that only understand CV debug symbols.

Comment 10

[Brian Ryner (not reading)]

(In reply to comment #9)
> Up to this point we haven't worried about getting debug symbols for NSS

Oops.  NSS does support MOZ_PROFILE.  I'll add MOZ_DEBUG_SYMBOLS to it in my
next patch.

Comment 11

[Wan-Teh Chang]

Brian, this comment suggests that one use of MOZ_PROFILE
is for Quantify:
http://lxr.mozilla.org/seamonkey/source/config/config.mk#307

I've used MOZ_PROFILE with Purify.

Since the /PDB:NONE linker flag is not supported by MSVC 7.1,
it seems that PDB files are the way to go and I think the
major tools should support PDB files.  So it is okay for
current versions of our code to only use PDB files.

Comment 12

[Brian Ryner (not reading)]

Attachment: revised patch

This changes to MOZ_DEBUG_SYMBOLS, which I think is a better name than
MOZ_PROFILE.  I've included patches for directory c-sdk and nss as well.  I
didn't remove MOZ_PROFILE in this patch but we can do that at a later time if
we want to.

Comment 13

[Wan-Teh Chang]

Comment on attachment 154882 [details] [diff] [review]
revised patch

Sorry about the late review.  Just came back from
a three-week vacation.

>Index: Makefile.in
>===================================================================
>RCS file: /cvsroot/mozilla/Makefile.in,v
>retrieving revision 1.259
>diff -u -p -r1.259 Makefile.in
>--- Makefile.in	19 Jul 2004 00:28:15 -0000	1.259
>+++ Makefile.in	30 Jul 2004 23:28:17 -0000
>@@ -506,12 +506,20 @@ endif
> 
> splitsymbols:
> ifdef MOZILLA_OFFICIAL
>+ifdef MOZ_DEBUG_SYMBOLS
>+	echo finding pdb files
>+	mkdir -p $(DIST)/$(BUILDID)
>+	-cp `/bin/find . -path "./dist" -prune -o -name "*.dll" | sed "s/\.dll$$/\.pdb/" | xargs` $(DIST)/$(BUILDID)
>+	-cp `/bin/find . -path "./dist" -prune -o -name "*.exe" | sed "s/\.exe$$/\.pdb/" | xargs` $(DIST)/$(BUILDID)
>+	-cp `/bin/find . -path "./dist" -prune -o -name "*.EXE" | sed "s/\.EXE$$/\.pdb/" | xargs` $(DIST)/$(BUILDID)
>+else
> 	echo splitting symbols out of binaries
> 	/bin/find $(DIST) -name "*.dll" -exec splitsym {} \;
> 	/bin/find $(DIST) -name "*.exe" -exec splitsym {} \;
> 	/bin/find $(DIST) -name "*.EXE" -exec splitsym {} \;
> 	mkdir -p $(DIST)/$(BUILDID)
> 	/bin/find $(DIST) -name "*.dbg" -exec mv {} $(DIST)/$(BUILDID) \;
>+endif # MOZ_DEBUG_SYMBOLS
> endif # MOZILLA_OFFICIAL
> 
> signnss:

The MOZ_DEBUG_SYMBOLS variable name makes the above
makefile code look confusing.  In the innermost
'else' branch, MOZ_DEBUG_SYMBOLS is not defined, which
suggests that there are no debug symbols, but the code
shows we extract symbols out of the binaries!  If the
variable name is something like MOZ_DEBUG_INFO_IN_PDB,
it will make the code clearer.

There may be other ways to make the code clearer. For
example, we should find out what variable needs to be
defined for the old splitsymbols code to work. I'm sure
that by default optimized binaries do not have symbols
in them.

>Index: config/config.mk
>===================================================================
>RCS file: /cvsroot/mozilla/config/config.mk,v
>retrieving revision 3.291
>diff -u -p -r3.291 config.mk
>--- config/config.mk	21 Jun 2004 20:56:13 -0000	3.291
>+++ config/config.mk	30 Jul 2004 23:28:19 -0000
>@@ -324,6 +324,11 @@ endif
> endif
> # MOZ_COVERAGE
> 
>+ifdef MOZ_DEBUG_SYMBOLS
>+MOZ_OPTIMIZE_FLAGS=-Zi -O1 -UDEBUG -DNDEBUG
>+OS_LDFLAGS = /DEBUG
>+endif
>+
> #
> # Handle trace-malloc in optimized builds.
> # No opt to give sane callstacks.

Please add a comment explaining what MOZ_DEBUG_SYMBOLS
is, along the lines of the comment at lines 307-309 for
MOZ_PROFILE.

The value of OS_LDFLAGS for the MOZ_DEBUG_SYMBOLS case
is very different from the value of OS_LDFLAGS for the
MOZ_PROFILE case (/DEBUG /DEBUGTYPE:CV /PDB:NONE /OPT:REF /OPT:nowin98).
Since /DEBUGTYPE:CV is the default if /DEBUG is specified
and we do not want /PDB:NONE for the MOZ_DEBUG_SYMBOLS
case, I am wondering if we should still have 
/OPT:REF /OPT:nowin98 in the value of OS_LDFLAGS.

>Index: directory/c-sdk/build.mk
>===================================================================
>RCS file: /cvsroot/mozilla/directory/c-sdk/build.mk,v
>retrieving revision 5.0.2.11
>diff -u -p -r5.0.2.11 build.mk
>--- directory/c-sdk/build.mk	25 Mar 2003 13:57:30 -0000	5.0.2.11
>+++ directory/c-sdk/build.mk	30 Jul 2004 23:28:20 -0000
>@@ -375,12 +375,18 @@ ifeq ($(BUILD_OPT), 1)
> endif
> 
> SUBSYSTEM=CONSOLE
>+ifdef MOZ_DEBUG_SYMBOLS
>+DEBUG_FLAGS=/DEBUG
>+else
>+DEBUG_FLAGS=/PDB:NONE /DEBUGTYPE:BOTH
>+endif

This file sets DEBUG_LINK_OPT to /DEBUG:FULL if
BUILD_OPT is not defined.

I can't find in Microsoft documentation what
/DEBUG:FULL is.  If /DEBUG:FULL is the same
as /DEBUG, you might want to say something
like:

DEBUG_LINK_OPT=/DEBUG
ifeq ($(BUILD_OPT), 1)
  ifndef MOZ_DEBUG_SYMBOLS
    DEBUG_LINK_OPT=
  endif
endif

ifndef MOZ_DEBUG_SYMBOLS
DEBUG_FLAGS=/PDB:NONE /DEBUGTYPE:BOTH
endif

>Index: directory/c-sdk/configure.in
>===================================================================
>RCS file: /cvsroot/mozilla/directory/c-sdk/configure.in,v
>retrieving revision 5.0.2.27
>diff -u -p -r5.0.2.27 configure.in
>--- directory/c-sdk/configure.in	20 May 2004 20:20:25 -0000	5.0.2.27
>+++ directory/c-sdk/configure.in	30 Jul 2004 23:28:20 -0000
>@@ -1243,6 +1243,11 @@ case "$target" in
>                 DLLFLAGS="$DLLFLAGS -DEBUG -DEBUGTYPE:CV"
>                 LDFLAGS="$LDFLAGS -DEBUG -DEBUGTYPE:CV"
>             fi
>+            if test -n "$MOZ_DEBUG_SYMBOLS"; then
>+               _OPTIMIZE_FLAGS="$_OPTIMIZE_FLAGS -Z7"
>+               DLLFLAGS="$DLLFLAGS -DEBUG"
>+               LDFLAGS="$LDFLAGS -DEBUG"
>+            fi
>         fi
> 
>         if test -n "$MOZ_DEBUG"; then

Since -DEBUGTYPE:CV is the default if -DEBUG is specified
(please verify that), the new code is identical to the
preceding code for MOZ_PROFILE and I urge you to simply
modify the preceding code to also test for -n "$MOZ_DEBUG_SYMBOLS".

>Index: nsprpub/configure.in
>===================================================================
>RCS file: /cvsroot/mozilla/nsprpub/configure.in,v
>retrieving revision 1.83.2.89
>diff -u -p -r1.83.2.89 configure.in
>--- nsprpub/configure.in	21 May 2004 21:26:18 -0000	1.83.2.89
>+++ nsprpub/configure.in	30 Jul 2004 23:28:35 -0000
>@@ -1293,6 +1293,11 @@ case "$target" in
>                 DLLFLAGS="$DLLFLAGS -DEBUG -DEBUGTYPE:CV"
>                 LDFLAGS="$LDFLAGS -DEBUG -DEBUGTYPE:CV"
>             fi
>+            if test -n "$MOZ_DEBUG_SYMBOLS"; then
>+                _OPTIMIZE_FLAGS="$_OPTIMIZE_FLAGS -Z7"
>+                DLLFLAGS="$DLLFLAGS -DEBUG"
>+                LDFLAGS="$LDFLAGS -DEBUG"
>+            fi
>         fi
> 
>         if test -n "$MOZ_DEBUG"; then

Same comment as above.

>@@ -1337,7 +1342,7 @@ case "$target" in
>     fi
> 
>     OS_DLLFLAGS="-nologo -DLL -SUBSYSTEM:WINDOWS"
>-    if test "$MSC_VER" = "1200"; then
>+    if test "$MSC_VER" = "1200" -a -z "$MOZ_DEBUG_SYMBOLS"; then
>         OS_DLLFLAGS="$OS_DLLFLAGS -PDB:NONE"
>     fi
>

Here a variable name with PDB in it MIGHT also make the
code a little easier to understand.

>Index: security/coreconf/WIN32.mk
>===================================================================
>RCS file: /cvsroot/mozilla/security/coreconf/WIN32.mk,v
>retrieving revision 1.17
>diff -u -p -r1.17 WIN32.mk
>--- security/coreconf/WIN32.mk	6 Dec 2003 06:34:20 -0000	1.17
>+++ security/coreconf/WIN32.mk	30 Jul 2004 23:28:37 -0000
>@@ -114,6 +114,10 @@ else # !NS_USE_GCC
> 	#
> 	# Add symbolic information for a profiler
> 	#
>+	ifdef MOZ_DEBUG_SYMBOLS
>+		OPTIMIZER += -Z7
>+		DLLFLAGS += -DEBUG
>+	endif
> 	ifdef MOZ_PROFILE
> 		OPTIMIZER += -Z7
> 		DLLFLAGS += -DEBUG -DEBUGTYPE:CV

The MOZ_DEBUG_SYMBOLS and MOZ_PROFILE cases should
be combined because the code is the same (-DEBUGTYPE:CV
is the default).  See my comment for
directory/c-sdk/configure.in.

Comment 14

[Wan-Teh Chang]

This MSDN page says:
  On the command line, if /DEBUG is specified,
  the default type is /DEBUGTYPE:CV; if /DEBUG
  is not specified, /DEBUGTYPE is ignored.

http://msdn.microsoft.com/library/default.asp?url=/library/en-us/vccore98/html/_core_.2f.debugtype.asp

This is why I think the only key difference between
MOZ_PROFILE and MOZ_DEBUG_SYMBOLS is the /PDB:NO
linker flag.

Comment 15

[Brian Ryner (not reading)]

Attachment: revised patch

I think this addresses all of your comments.

One thing I found is that we do probably want to use /Zi (rather than /Z7)
everywhere to cause the compiler to place debug info into an external pdb file
instead of the .obj file.  Does this seem reasonable?  Oddly, nspr4.pdb ends up
being somewhat smaller if /Zi is used instead of /Z7, though in theory they're
both producing the same debug info.  I'm pretty confident that /Zi gives us
everything we need, but if talkback reports end up missing symbols, we can try
changing it to /Z7 (I don't have a strong preference one way or the other but
I'd like to use the same thing on all modules).

I'm going to see if I can get a Firefox trunk build kicked off with this patch
to ensure that Talkback works as expected.

Comment 16

[Brian Ryner (not reading)]

Test talkback build with latest patch:
http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2004-08-24-00-trunk/FirefoxSetup.exe

I'll look tomorrow and see whether incidents from this build are getting good
stack traces.

Comment 17

[Brian Ryner (not reading)]

Comment on attachment 156864 [details] [diff] [review]
revised patch

talkback incident from a build done with this patch:

http://talkback-public.mozilla.org/talkback/fastfind.jsp?search=2&type=iid&id=6
42093

Comment 18

[Wan-Teh Chang]

Comment on attachment 156864 [details] [diff] [review]
revised patch

r=wtc.

>Index: Makefile.in
>===================================================================
>RCS file: /cvsroot/mozilla/Makefile.in,v
>retrieving revision 1.259
>diff -u -p -r1.259 Makefile.in
>--- Makefile.in	19 Jul 2004 00:28:15 -0000	1.259
>+++ Makefile.in	24 Aug 2004 06:05:00 -0000
>@@ -506,12 +506,21 @@ endif
> 
> splitsymbols:
> ifdef MOZILLA_OFFICIAL
>+ifdef MOZ_DEBUG_SYMBOLS
>+	echo finding pdb files
>+	mkdir -p $(DIST)/$(BUILDID)
>+	-cp `/bin/find . -path "./dist" -prune -o -name "*.dll" | sed "s/\.dll$$/\.pdb/" | xargs` $(DIST)/$(BUILDID)
>+	-cp `/bin/find . -path "./dist" -prune -o -name "*.exe" | sed "s/\.exe$$/\.pdb/" | xargs` $(DIST)/$(BUILDID)
>+	-cp `/bin/find . -path "./dist" -prune -o -name "*.EXE" | sed "s/\.EXE$$/\.pdb/" | xargs` $(DIST)/$(BUILDID)
>+endif # MOZ_DEBUG_SYMBOLS
>+ifdef MOZ_PROFILE
> 	echo splitting symbols out of binaries
> 	/bin/find $(DIST) -name "*.dll" -exec splitsym {} \;
> 	/bin/find $(DIST) -name "*.exe" -exec splitsym {} \;
> 	/bin/find $(DIST) -name "*.EXE" -exec splitsym {} \;
> 	mkdir -p $(DIST)/$(BUILDID)
> 	/bin/find $(DIST) -name "*.dbg" -exec mv {} $(DIST)/$(BUILDID) \;
>+endif # MOZ_PROFILE
> endif # MOZILLA_OFFICIAL
> 
> signnss:

Please have mozilla.org's release engineer review the
above change.  He needs to verify that the ifdef MOZ_PROFILE
change is correct (i.e., currently MOZ_PROFILE is defined
when he does official Mozilla builds.)

I noticed that you add -OPT:REF wherever we use -DEBUG
on optimized builds.  Is this because -DEBUG turns off
-OPT:REF?  (-OPT:REF is the default.)

We may want to omit -DEBUGTYPE:CV if -DEBUG is used.
First, -DEBUGTYPE:CV is the default.  Second, the
-DEBUGTYPE flag is removed in the upcoming MSVC .NET
2005.

Comment 19

[Brian Ryner (not reading)]

> Please have mozilla.org's release engineer review the
> above change.  He needs to verify that the ifdef MOZ_PROFILE
> change is correct (i.e., currently MOZ_PROFILE is defined
> when he does official Mozilla builds.)

I will double-check that.

> 
> I noticed that you add -OPT:REF wherever we use -DEBUG
> on optimized builds.  Is this because -DEBUG turns off
> -OPT:REF?  (-OPT:REF is the default.)

Yes.

> 
> We may want to omit -DEBUGTYPE:CV if -DEBUG is used.
> First, -DEBUGTYPE:CV is the default.  Second, the
> -DEBUGTYPE flag is removed in the upcoming MSVC .NET
> 2005.
> 

Ah, ok.  In that case, I'll remove it.  Thanks for the thorough review.

Comment 20

[Dan Mosedale (:dmosedale, :dmose)]

r=dmose for checkin of the directory/c-sdk portions of the patch to the ldap
client C sdk branch as well the ldap trunk.

Comment 21

[Wan-Teh Chang]

Comment on attachment 156864 [details] [diff] [review]
revised patch

I checked in the NSS (mozilla/security/coreconf) changes
(after minor editing and with -DEBUGTYPE:CV removed) on
the NSS trunk (NSS 3.10), NSS_3_9_BRANCH (NSS 3.9.3), and
NSS_CLIENT_TAG (Mozilla 1.8a4).

Comment 22

[Mark Smith [:mcs]]

I committed the mozilla/directory/c-sdk changes to the trunk as well:

mozilla/directory/c-sdk/configure
  new revision: 5.25; previous revision: 5.24
mozilla/directory/c-sdk/configure.in
  new revision: 5.26; previous revision: 5.25
mozilla/directory/c-sdk/build.mk
  new revision: 5.15; previous revision: 5.14
    Commit fix for bug #244231 from ldapcsdk_50_client_branch:
     Add support for generating PDB-format debug symbols with MSVC,
     by setting MOZ_DEBUG_SYMBOLS=1 in the environment. This is to
     be used in place of MOZ_PROFILE. r=wtc, dmose.

Comment 23

[Brian Ryner (not reading)]

Attachment: merge to aviary branch

This removes the previous PDB support that had been put in on
AVIARY_1_0_20040515_BRANCH and replaces it with a backport of the above patch. 
I'd like to get this in so that we can sync up local tinderbox script changes.

Comment 24

[Asa Dotzler [:asa]]

Comment on attachment 157289 [details] [diff] [review]
merge to aviary branch

a=asa for branch checkin.

Comment 25

[Asa Dotzler [:asa]]

Comment on attachment 157289 [details] [diff] [review]
merge to aviary branch

to keep them in sync, approving for 1.7.x too

Comment 26

[Brian Ryner (not reading)]

Comment on attachment 149004 [details] [diff] [review]
patch

removing obsolete review request

Comment 27

[Brian Ryner (not reading)]

Comment on attachment 154882 [details] [diff] [review]
revised patch

removing obsolete review request

Comment 28

[Brian Ryner (not reading)]

checked in on trunk, 1.7 branch, and aviary branch.