use pdb files for debug symbols

RESOLVED FIXED

Status

SeaMonkey
Build Config
RESOLVED FIXED
14 years ago
14 years ago

People

(Reporter: Brian Ryner (not reading), Assigned: Brian Ryner (not reading))

Tracking

({fixed-aviary1.0, fixed1.7.5})

Trunk
x86
Windows XP
fixed-aviary1.0, fixed1.7.5

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 3 obsolete attachments)

(Assignee)

Description

14 years ago
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.
(Assignee)

Comment 1

14 years ago
Created attachment 149004 [details] [diff] [review]
patch

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

Updated

14 years ago
Attachment #149004 - Flags: review?(leaf)
(Assignee)

Comment 2

14 years ago
Created attachment 149369 [details] [diff] [review]
patch "make deliver"

Comment 3

14 years ago
Comment on attachment 149004 [details] [diff] [review]
patch

r=leaf
Attachment #149004 - Flags: review?(leaf) → review+

Comment 4

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

this one, too.
Attachment #149369 - Flags: review+

Comment 5

14 years ago
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.
(Assignee)

Comment 6

14 years ago
Comment on attachment 149004 [details] [diff] [review]
patch

wtc, we really need this for firefox trunk
Attachment #149004 - Flags: superreview?(wchang0222)

Comment 7

14 years ago
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

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

Comment 9

14 years ago
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.
(Assignee)

Comment 10

14 years ago
(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

14 years ago
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.
(Assignee)

Comment 12

14 years ago
Created attachment 154882 [details] [diff] [review]
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.
(Assignee)

Updated

14 years ago
Attachment #149004 - Attachment is obsolete: true
Attachment #149369 - Attachment is obsolete: true
(Assignee)

Updated

14 years ago
Attachment #154882 - Flags: review?(wchang0222)

Comment 13

14 years ago
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

14 years ago
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.
(Assignee)

Comment 15

14 years ago
Created attachment 156864 [details] [diff] [review]
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.
Attachment #154882 - Attachment is obsolete: true
(Assignee)

Comment 16

14 years ago
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.
(Assignee)

Comment 17

14 years ago
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
Attachment #156864 - Flags: review?(wchang0222)

Comment 18

14 years ago
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.
Attachment #156864 - Flags: review?(wchang0222) → review+
(Assignee)

Comment 19

14 years ago
> 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.
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

14 years ago
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

14 years ago
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.
(Assignee)

Comment 23

14 years ago
Created attachment 157289 [details] [diff] [review]
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.
(Assignee)

Updated

14 years ago
Attachment #157289 - Flags: approval-aviary?

Comment 24

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

a=asa for branch checkin.
Attachment #157289 - Flags: approval-aviary? → approval-aviary+

Comment 25

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

to keep them in sync, approving for 1.7.x too
Attachment #157289 - Flags: approval1.7.x+
(Assignee)

Comment 26

14 years ago
Comment on attachment 149004 [details] [diff] [review]
patch

removing obsolete review request
Attachment #149004 - Flags: superreview?(wchang0222)
(Assignee)

Comment 27

14 years ago
Comment on attachment 154882 [details] [diff] [review]
revised patch

removing obsolete review request
Attachment #154882 - Flags: review?(wchang0222)
(Assignee)

Comment 28

14 years ago
checked in on trunk, 1.7 branch, and aviary branch.
Status: NEW → RESOLVED
Last Resolved: 14 years ago
Keywords: fixed-aviary1.0, fixed1.7.x
Resolution: --- → FIXED
Product: Browser → Seamonkey
You need to log in before you can comment on or make changes to this bug.