Closed Bug 338224 Opened 15 years ago Closed 10 years ago

implement --enable-debug-symbols (nspr4.dll and --disable-debug yields "no rule to make nspr4.pdb needed by export")

Categories

(NSPR :: NSPR, defect)

4.7.3
x86
Windows XP
defect
Not set
normal

Tracking

(status1.9.2 final-fixed)

RESOLVED FIXED
Tracking Status
status1.9.2 --- final-fixed

People

(Reporter: bugzilla, Assigned: Mitch)

References

Details

Attachments

(2 files, 10 obsolete files)

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.0.3) Gecko/20060426 Firefox/1.5.0.3
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.0.3) Gecko/20060426 Firefox/1.5.0.3

This actually applies to Thunderbird CVS (where I first noticed it) and Firefox CVS, where I confirmed.

I tried (honest!) to track down the rule that "export" was complaining about, to clear the $(LIB).pdb dependency, but was stymied.

Reproducible: Always

Steps to Reproduce:
1. Checkout from CVS
2. include "ac_add_options --disable-debug" in your %MOZCONFIG%
3. make -f client.mk build
4. kaboom
5. rm -rf objdir
6. switch to "ac_add_options --enable-debug"
7. observe since .pdb file is created, build is happy; makes product REALLY verbose




Here is the build environment, FWIW:

set VCVARS=c:\Program Files\Microsoft Visual Studio 8\VC\bin\vcvars32.bat
set MSSDK=c:\Program Files\Microsoft Platform SDK for Windows Server 2003 R2\
set MOZ_TOOLS=c:\moz\moztools
set CYGWINBASE=c:\cygwin
set PATH=%CYGWINBASE%\bin;%PATH%
call "%VCVARS%"
set PATH=%MSSDK%\bin;%PATH%
set INCLUDE=%MSSDK%\include;%INCLUDE%
set LIB=%MSSDK%\lib;%LIB%
set PATH=%PATH%;%MOZ_TOOLS%\bin
set NO_MFC=1
I can reproduce this problem.

--disable-optimize and --disable-debug ... failed
--enable-optimize  and --disable-debug ... succeeded
--disable-optimize and --enable-debug  ... succeeded

I cannot build the non-optimezed and non-debug build.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Version: unspecified → Trunk
I've worked around this problem every couple of years.

wtc: link -DEBUG != BE debug. and pdbs don't have to be shipped. they should always be generated.
Assignee: nobody → wtchang
Component: Build Config → NSPR
Product: Firefox → NSPR
QA Contact: build.config → nspr
Version: Trunk → 4.7.3
Assignee: wtchang → timeless
Status: NEW → ASSIGNED
Attachment #254456 - Flags: review?(wtchang)
Any chance of getting the patch reviewed and checked in?
Ugh, and now I'm even getting this build error after I applied the patch.
(In reply to comment #1)
> I can reproduce this problem.
> 
> --disable-optimize and --disable-debug ... failed
> --enable-optimize  and --disable-debug ... succeeded
> --disable-optimize and --enable-debug  ... succeeded

That's what I get with current FF/mozilla-central (and SM/comm-central).

(In reply to comment #5)
> Ugh, and now I'm even getting this build error after I applied the patch.

This patch works for me :-)
Attachment #331392 - Flags: review?(wtc)
(In reply to comment #6)
> > --disable-optimize and --disable-debug ... failed
> > --enable-optimize  and --disable-debug ... succeeded
> > --disable-optimize and --enable-debug  ... succeeded

And, fwiw,
--enable-optimize  and --enable-debug ... succeeded
Surely it would be better for nspr's configure to set BUILD_OPT depending on whether MOZ_DEBUG is unset rather than blindly copying MOZ_OPTIMIZE?
Can someone tell me what the current status of this bug is? I need to build 
--disable-optimize and --disable-debug
so I can used the debugger when I need it. Is it possible?
Thanks,
John
BTW I get the nspr4.pdb missing with
ac_add_options --disable-vista-sdk-requirements
ac_add_options --enable-application=browser
ac_add_options --disable-optimize 
ac_add_options --disable-parental-controls
ac_add_options --enable-debugger-info-modules=yes
export MOZ_DEBUG_SYMBOLS=1
mk_add_options MOZ_OBJDIR=@TOPSRCDIR@/obj-@CONFIG_GUESS@

what can I do?
downloaded second attachment to this bug as nsprbug.diff
cd src/nsprpub
patch -p1  configure.in  ../../nsprbug.diff
cd ..
make -f client.mk build

result was
 *** No rule to make target `nspr4.pdb', needed by `export'.  Stop.
This .mozconfig builds (added --enable-debug)
ac_add_options --disable-vista-sdk-requirements
ac_add_options --enable-application=browser
ac_add_options --disable-optimize 
ac_add_options --disable-parental-controls
ac_add_options --enable-debug
ac_add_options --enable-debugger-info-modules=yes
export MOZ_DEBUG_SYMBOLS=1
mk_add_options MOZ_OBJDIR=@TOPSRCDIR@/obj-@CONFIG_GUESS@

However if I delete nsprpub/pr/src/nspr4.pdb and run make, then I get the No rule message.

So in the successful case the .pdb file is not created by a rule, but rather as a side effect of the compile leading to nspr.dll. Indeed if I delete the nspr4.dll file and make, then nspr4.pdb is built. Looks like the 'link' command extracts the debug info from the .obj files and puts it into the .pdb file. The link has no explicit "/Fd" to namem the pdb, rather it seems to be using the .dll name.  Presumably some setting on the compile (Zi) leading to the .obj files will cause the .pdb to result from the link. 

If this is all correct, then pdb should never be required by export. If you compile with Zi you get pdb, else not.
In nsprpub directory under the .mozconfig of comment 13, the compile option that causes debug output is probably "Z7" rather than Zi.
client.mk shows that nsprpub has its own configure:
# 'configure' scripts generated by autoconf.
CONFIGURES := $(TOPSRCDIR)/configure
CONFIGURES += $(TOPSRCDIR)/nsprpub/configure

That may explain why this problem only occurs in the nsprpub directory.

In nsprpub/configure MOZ_DEBUG_SYMBOLS is only used for MOZ_OPTIMIZE
        if test -n "$MOZ_OPTIMIZE"; then
            if test -n "$MOZ_PROFILE"; then
                _OPTIMIZE_FLAGS="$_OPTIMIZE_FLAGS -Z7"
            fi
            if test -n "$MOZ_DEBUG_SYMBOLS"; then
                _OPTIMIZE_FLAGS="$_OPTIMIZE_FLAGS -Zi"
            fi
            if test -n "$MOZ_PROFILE" -o -n "$MOZ_DEBUG_SYMBOLS"; then
                DLLFLAGS="$DLLFLAGS -OPT:REF"
                LDFLAGS="$LDFLAGS -OPT:REF"
            fi
        fi

But I am assuming that MOZ_DEBUG_SYMBOLS should be independent of MOZ_OPTIMIZE.
(In reply to comment #8)
> (In reply to comment #6)
> > > --disable-optimize and --disable-debug ... failed
> > > --enable-optimize  and --disable-debug ... succeeded
> > > --disable-optimize and --enable-debug  ... succeeded
> 
> And, fwiw,
> --enable-optimize  and --enable-debug ... succeeded

That makes sense because nsprpub/configure says:
if test -n "$MOZ_OPTIMIZE"; then
    CFLAGS="$CFLAGS $_OPTIMIZE_FLAGS"
    CXXFLAGS="$CXXFLAGS $_OPTIMIZE_FLAGS"
fi

if test -n "$MOZ_DEBUG"; then
    CFLAGS="$CFLAGS $_DEBUG_FLAGS"
    CXXFLAGS="$CXXFLAGS $_DEBUG_FLAGS"
fi
So if you disable both, then Z7 is not added. If you want pdb you have to optimize or debug.
wtc, ping ?
I found a solution that allows debugging with Visual Studio 2005 but no stream of tracing on startup:

ac_add_options --disable-vista-sdk-requirements
ac_add_options --enable-application=browser
ac_add_options --disable-optimize
ac_add_options --disable-parental-controls
export MOZ_DEBUG_SYMBOLS=1
ac_add_options --enable-debugger-info-modules=yes
mk_add_options MOZ_OBJDIR=@TOPSRCDIR@/obj-@CONFIG_GUESS@

The only difference between here and Comment 11 is the position of the export. I don't remember reading anything about order of the mozconfig but if it is essentially a script it makes sense that the constants have to be defined first.

My guess is that some people have the export in their environment variables so they never have this bug. (More evidence that environment variables should never be used in a shared build system).

To come to this conclusion I did:
make -f client.mk clean
make -f client.mk build
If the clean is not perfect it is possible that the conclusion would be incorrect.
I'm not sure I consider this bug valid.
A .pdb file is a file for debugging.  It contains the debugging symbols.
When you say --disable-debug, you're saying "I don't want debugging stuff"
and that includes debugging symbols.
I don't agree. I want some debugging stuff: symbols. I don't want other debugging stuff: tracing. Various attempts to accomplish this include trying --disable-debug as in the title, but it is not in the end needed. 

In addition the build should not fail with "no rule ..." with any combination of options.  Its a bug in the make system.  Maybe too hard to fix that but I in my opinion no target should depend upon .pdb since it is a side-effect of compile, not a result of any command.

I think the minimal resolution is to ensure that 
export MOZ_DEBUG_SYMBOLS=1
is set early in the .mozconfig file.
You didn't tell us what makefile reports the error you cited in comment 12
I'm not sure what you are asking. The makefile was, as I said in comment 12, "client.mk". The .mozconfig I used in comment 12 was the one in comment 11. The error occurs when make enters nsprpub as a describe in comment 15.
John, Make invokes itself recursively as it traverses the tree of source 
file directories.  In each directory, it invokes that directory's makefile.
The error you reported occurred in some makefile for some directory.
It seems very unlikely that the error occurred in client.mk, but more 
likely that it occurred in some subordinate makefile.  The question is:
which one?
Since the error occurs in nsprpub, I presume the subordinate make file is nsprpub/Makefile. But since these are generated make files, you have to look into the config system to fix the problem.
John, there's no need to presume anything.  The output of the make should 
make it very explicitly clear where the problem occurs.  There should be a 
LOT more lines of output surrounding the one you quoted:
>  *** No rule to make target `nspr4.pdb', needed by `export'.  Stop.
please share them with us.
Sorry Nelson, I already did a lot of investigation on this problem and posted my findings. I have a working solution now so I don't want to undo it all unless there is a good reason.  If you try the .mozconfig files I posted and get different results, I'll be happy to try to sort that out.
(In reply to comment #25)
> John, there's no need to presume anything.  The output of the make should 
> make it very explicitly clear where the problem occurs.  There should be a 
> LOT more lines of output surrounding the one you quoted:
> >  *** No rule to make target `nspr4.pdb', needed by `export'.  Stop.
> please share them with us.
Here is the make -d output
$ tail -40 make.out
    Trying pattern rule with stem `nspr4.pdb.cpp'.
    Trying implicit prerequisite `RCS/nspr4.pdb.cpp,v'.
    Trying pattern rule with stem `nspr4.pdb.cpp'.
    Trying implicit prerequisite `RCS/nspr4.pdb.cpp'.
    Trying pattern rule with stem `nspr4.pdb.cpp'.
    Trying implicit prerequisite `s.nspr4.pdb.cpp'.
    Trying pattern rule with stem `nspr4.pdb.cpp'.
    Trying implicit prerequisite `SCCS/s.nspr4.pdb.cpp'.
   Trying pattern rule with stem `nspr4.pdb'.
   Trying implicit prerequisite `nspr4.pdb.s'.
   Looking for a rule with intermediate file `nspr4.pdb.s'.
    Avoiding implicit rule recursion.
    Trying pattern rule with stem `nspr4.pdb.s'.
    Trying implicit prerequisite `nspr4.pdb.s,v'.
    Trying pattern rule with stem `nspr4.pdb.s'.
    Trying implicit prerequisite `RCS/nspr4.pdb.s,v'.
    Trying pattern rule with stem `nspr4.pdb.s'.
    Trying implicit prerequisite `RCS/nspr4.pdb.s'.
    Trying pattern rule with stem `nspr4.pdb.s'.
    Trying implicit prerequisite `s.nspr4.pdb.s'.
    Trying pattern rule with stem `nspr4.pdb.s'.
    Trying implicit prerequisite `SCCS/s.nspr4.pdb.s'.
   No implicit rule found for `nspr4.pdb'.
   Finished prerequisites of target file `nspr4.pdb'.
  Must remake target `nspr4.pdb'.
make[5]: Leaving directory `/g/mozilla/mozilla-central/src/obj-i686-pc-mingw32/n
sprpub/pr/src'
Reaping losing child 0x0a0334f0 PID 3288
Removing child 0x0a0334f0 PID 3288 from chain.
make[4]: Leaving directory `/g/mozilla/mozilla-central/src/obj-i686-pc-mingw32/n
sprpub/pr'
Reaping losing child 0x0a034600 PID 2776
Removing child 0x0a034600 PID 2776 from chain.
make[3]: Leaving directory `/g/mozilla/mozilla-central/src/obj-i686-pc-mingw32/n
sprpub'
Reaping losing child 0x0a056948 PID 3092
Removing child 0x0a056948 PID 3092 from chain.
make[2]: Leaving directory `/g/mozilla/mozilla-central/src/obj-i686-pc-mingw32'
Reaping losing child 0x0a0567e8 PID 2428
Removing child 0x0a0567e8 PID 2428 from chain.
make[1]: Leaving directory `/g/mozilla/mozilla-central/src/obj-i686-pc-mingw32'
Reaping losing child 0x0a0445e0 PID 2628
Removing child 0x0a0445e0 PID 2628 from chain.
(In reply to comment #20)
...
> I think the minimal resolution is to ensure that 
> export MOZ_DEBUG_SYMBOLS=1
> is set early in the .mozconfig file.

I re-cloned hg mozilla-central in a new directory and copied the .mozconfig from comment 18 and the build fails with the same pdb message.

So something I changed in the config files fixed the problem, not the change to .mozconfig. 

And once again I am not able to build.
This error message means the .pdb file with debug symbols has not been built.
   (Based on the error message and MS docs)
The setting that creates the .pdb is -Zi to C compiler
   (Based on the MS docs and line 1583 of src/nsprpub/configure.in
   http://mxr.mozilla.org/mozilla-central/source/nsprpub/configure.in#1562)
The C compiler gets the setting from either $_OPTIMIZE_FLAGS *or* $_DEBUG_FLAGS in configure.in, line 2661
    http://mxr.mozilla.org/mozilla-central/source/nsprpub/configure.in#2661

MOZ_OPTIMIZE controls the application of _OPTIMIZE_FLAGS; MOZ_DEBUG controls _DEBUG_FLAGS. In the .mozconfig files I use, *neither* value is set.

The flag to control debug symbols in other modules is MOZ_DEBUG_SYMBOLS
    http://mxr.mozilla.org/mozilla-central/source/config/config.mk#253

My goal is to set the -Zi in the case that  
   1) MOZ_OPTIMIZE is not set,
   2) MOZ_DEBUG is not set,
   3) MOZ_DEBUG_SYMBOLS is set,
for nsprpub directories

It looks to me as if I should change two places in src/nsprpub/configure.in. 
1) In the "Override of system specific target options", case
*-mingw*|*-cygwin*|*-msvc*|*-mks*)
...
        # want symbols but not debug tracing or optimize
        if test -n "$MOZ_DEBUG_SYMBOLS"; then  
            if test -z "$MOZ_OPTIMIZE" -a -z "$MOZ_DEBUG"; then
                _DEBUG_SYMBOLS = "$_DEBUG_SYMBOLS -Zi"
            fi
        fi
...
And then near the bottom of the file, use the new _DEBUG_SYMBOLS:

if test -z "$MOZ_DEBUG" -a -z "$MOZ_OPTIMIZE"; then 
    if test -n "$_DEBUG_SYMBOLS"; then
        CFLAGS="$CFLAGS $_DEBUG_SYMBOLS"
        CXXFLAGS="$CXXFLAGS $_DEBUG_SYMBOLS"
    fi
fi      

But I can't get configure to work, the second bit fails with
/g/mozilla/mozilla-central/src/nsprpub/configure: _DEBUG_SYMBOLS: command not fo
und
Attached patch Patch to nsprpub/config/rules.mk (obsolete) — Splinter Review
only tested with the .mozconfig in comment 18 and only on windows
Ok I've found a solution that works.  In nsprpub/config/rules.mk we can postpend the flags to create the .pdb files with the correct names:
OS_CFLAGS:=$(OS_CFLAGS) -Zi -Fd$(SHARED_LIB_PDB)

This can be done in the 'if' block that applies to Windows and OS/2 only, just after the value of SHARED_LIB_PDB is created.

The reset of OS_CFLAGS can be done only within if MOZ_DEBUG_SYMBOLS, however no other use of similar variable appears in rules.mk.

If any code after this point changes components of OS_CFLAGS, you lose.
Attachment #348289 - Attachment is patch: true
Attachment #348289 - Attachment mime type: application/octet-stream → text/plain
nelson: your view is wrong.

gecko supports:
--enable-debugging-info-modules --disable-optimize --disable-debug

PDBs let you debug things. this includes optimized things and non optimized things.

NSPR (and sadly NSS which is why i keep trying to fix NSPR) have this broken view of the world which is basically described by your incorrect comments.

i should be able to debug *anything*.

in mozilla, --enable-debug means two things:

--enable-debugging-info
--enable-debugging-code

where info is stuff the compiler/linker provide so that a debugger will work
and code is arbitrary stuff hidden behind #ifdef DEBUG and the like.

wtc: please review this. i've had review requests to you for nearly 2 years on this bug. that's entirely unacceptable.
Attachment #354732 - Flags: review?(wtc)
Comment on attachment 354732 [details] [diff] [review]
fix --enable-debugger-info-modules

ted: if this doesn't get a review in a reasonable time. i'd like permission to commit this to Mercurial *ANYWAY* and just merge it each time nspr lands.
Attachment #354732 - Flags: review?(ted.mielczarek)
Comment on attachment 354732 [details] [diff] [review]
fix --enable-debugger-info-modules

We don't take local patches to NSPR in m-c. We'll have to ping wtc about these, he's been pretty responsive as of late, these might just have fallen off his radar.
Attachment #354732 - Flags: review?(ted.mielczarek)
Jan Odvarko just succeeded with the patch in comment 30 and 
export MOZ_DEBUG_SYMBOLS=1
Attachment #354732 - Flags: review?(wtc) → review?(ted.mielczarek)
Comment on attachment 354732 [details] [diff] [review]
fix --enable-debugger-info-modules

Ted, could you review this patch?

Some background: NSPR developers only do two kinds of build:
- debug build (the default): --enable-debug --disable-optimize
- optimized build: --disable-debug --enable-optimize

We don't use other combinations of these two configure options,
and we don't use the MOZ_PROFILE and MOZ_DEBUG_SYMBOLS
environment variables.  The support of those two environment
variables was added to NSPR at Mozilla developers' request,
and over time I've forgot what they meant, and I couldn't
figure out by inspecting Mozilla's build system how Mozilla
uses them.

So, if the patch doesn't break how the NSPR developers build
NSPR, it's fine.  It'd be nice to add some comments to document
the meaning of MOZ_PROFILE and MOZ_DEBUG_SYMBOLS and how they
should be set (as environment variables, at configure time, or
at build time).  Thanks.
Actually, I build with MOZ_DEBUG_SYMBOLS all the time.  But I can be 
flexible there, if needs be.
Nelson, you're relying on an unintended effect of MOZ_DEBUG_SYMBOLS
to turn off the -PDB:NONE linker flag, which is not supported by
the latest versions of Visual C++.  The proper fix is to check the
Visual C++ version before adding the -PDB:NONE linker flag.  I
implemented the fix in nsprpub/configure.in but not in
security/coreconf:
http://mxr.mozilla.org/nspr/source/nsprpub/configure.in#1591

1591         if test "$MSC_VER" -le "1200" -a -z "$MOZ_DEBUG_SYMBOLS"; then
1592             OS_DLLFLAGS="$OS_DLLFLAGS -PDB:NONE"
1593         fi

So you don't need to set MOZ_DEBUG_SYMBOLS when building NSPR.

Contrast that with the corresponding code in security/coreconf:

http://mxr.mozilla.org/security/source/security/coreconf/WIN954.0.mk#70

70 ifndef MOZ_DEBUG_SYMBOLS
71         OS_DLLFLAGS += -PDB:NONE
72 endif

http://mxr.mozilla.org/security/source/security/coreconf/WIN32.mk#162

162 ifndef MOZ_DEBUG_SYMBOLS
163         LDFLAGS    += -PDB:NONE 
164 endif
(In reply to comment #37)
> Actually, I build with MOZ_DEBUG_SYMBOLS all the time.  But I can be 
> flexible there, if needs be.

As do all of our nightly builds. I can't be flexible about that. :) However, I did remove MOZ_PROFILE from the Mozilla build system, as it was duplicate functionality.
Comment on attachment 354732 [details] [diff] [review]
fix --enable-debugger-info-modules

fwiw, this fixes things for me.
Ted: MXR shows that MOZ_PROFILE still exists in mozilla-central.
When you say you removed MOZ_PROFILE from the Mozilla build system,
did you mean that you stopped using MOZ_PROFILE, rather than
removing it from the source tree?

Should we remove MOZ_PROFILE from the NSPR and NSS source trees?
The fewer (poorly documented) build variables, the better.
I only see two actual references to it:
http://mxr.mozilla.org/mozilla-central/source/configure.in#7958
http://mxr.mozilla.org/mozilla-central/source/Makefile.in#137

These are just leftovers that were missed, I suspect. We might as well clean it out of NSPR/NSS as well, if it was only added for Mozilla use anyway.
Yes, those are the two references to MOZ_PROFILE I found. There is
one more reference at
http://mxr.mozilla.org/mozilla-central/source/config/autoconf.mk.in#590

MOZ_PROFILE is also mentioned in this comment:
http://mxr.mozilla.org/mozilla-central/source/config/config.mk#246

MOZ_PROFILE and MOZ_DEBUG_SYMBOLS were only added to NSPR/NSS for Mozilla
use.  Nelson has since found a side effect of MOZ_DEBUG_SYMBOLS to
turn off the obsolete -PDB:NONE linker flag, but we're not using
MOZ_PROFILE.
I filed bug 479978 on removing the last traces of MOZ_PROFILE from the Mozilla build system.
Comment on attachment 354732 [details] [diff] [review]
fix --enable-debugger-info-modules

+    if test -n "$NSPR_DEBUG_INFO"; then
+        if test -z "$MOZ_DEBUG_SYMBOLS"; then
+            export MOZ_DEBUG_SYMBOLS=1

The problem with this is that this will make --enable-debugger-info-modules=nspr work on Win32, but nowhere else. That doesn't seem great to me. You can already accomplish this (on Windows) by exporting MOZ_DEBUG_SYMBOLS=1.

The other part seems fine, since it's the actual fix to the bug here.

I wonder if we shouldn't just make nspr's configure understand --enable-debugger-info-modules instead? (on all platforms)
Attachment #354732 - Flags: review?(ted.mielczarek) → review-
Ted: NSPR doesn't have submodules.  Does the --enable-debugger-info-modules
option still make sense for NSPR?
I think we could just make it turn on debug symbols, like MOZ_DEBUG_SYMBOLS does. Except MOZ_DEBUG_SYMBOLS only works for Windows.
OK, for NSPR I think the option should be named
--enable-debugger-info because NSPR consists of only
one module (itself).

--enable-debug implies --enable-debugger-info but also
turns on code ifdef'd with DEBUG.

Do I understand this correctly?
Yes. It feels like we should combine the codepaths for MOZ_DEBUG_SYMBOLS, too, since they accomplish that same task.

The only reason I suggested using --enable-debugger-info-modules is so that it would silently pass from the top-level configure and work, but it's not a big deal either way.
Assignee: timeless → ted.mielczarek
Attachment #418861 - Flags: review?(ted.mielczarek)
See Also: → 504175
Attachment #418861 - Flags: review?(ted.mielczarek) → review-
Comment on attachment 418861 [details] [diff] [review]
fix build rules for creating pdb files when needed

I don't think this is correct. If you compile with this patch and --enable-optimize and MOZ_DEBUG_SYMBOLS=1, you'll wind up with -DEBUG twice on the link line, because of this:
http://mxr.mozilla.org/mozilla-central/source/nsprpub/configure.in#1565

I think the configure part of timeless' patch above is actually correct.
bug 517097 is adding --enable-debug-symbols to the top-level configure, we should just port that to NSPR, refactor all these codepaths, and fix it right.
Attached patch --enable-debug-symbols (obsolete) — Splinter Review
Port of bug 517097's patch. This patch depends on the aforementioned patch.
Attachment #426395 - Flags: review?(ted.mielczarek)
Depends on: 517097
Assignee: ted.mielczarek → mitch_1_2
Comment on attachment 426395 [details] [diff] [review]
--enable-debug-symbols

This is broken.
Attachment #426395 - Attachment is obsolete: true
Attachment #426395 - Flags: review?(ted.mielczarek)
Attached patch --enable-debug-symbols (v2) (obsolete) — Splinter Review
Attachment #432001 - Flags: review?(ted.mielczarek)
(In reply to comment #32)

> i should be able to debug *anything*.
> 
> in mozilla, --enable-debug means two things:
> 
> --enable-debugging-info
> --enable-debugging-code
> 
> where info is stuff the compiler/linker provide so that a debugger will work
> and code is arbitrary stuff hidden behind #ifdef DEBUG and the like.

I absolutely agree.  Enabling debugging symbols etc (so that debuggers
can navigate) and enabling assertions/#ifdef-DEBUG code should be
independent.  Glomming them together is a mistake.
(In reply to comment #56)
> Created an attachment (id=432001) [details]
> --enable-debug-symbols (v2)

Does not work for me, WinXP, VC2008 Express, mozilla-central.
(In reply to comment #57)
> (In reply to comment #32)
> 
> > i should be able to debug *anything*.
> > 
> > in mozilla, --enable-debug means two things:
> > 
> > --enable-debugging-info
> > --enable-debugging-code
> > 
> > where info is stuff the compiler/linker provide so that a debugger will work
> > and code is arbitrary stuff hidden behind #ifdef DEBUG and the like.
> 
> I absolutely agree.  Enabling debugging symbols etc (so that debuggers
> can navigate) and enabling assertions/#ifdef-DEBUG code should be
> independent.  Glomming them together is a mistake.

Doesn't --enable-debugger-info-modules already provide symbols without DEBUG code?
NSPR doesn't actually support that (and this is an NSPR bug). We replaced that with --enable-debug-symbols in the Mozilla configure in bug 517097, Mitch's patch here adds it to NSPR's configure as well. (but apparently doesn't fix the current bug here.)
Attached patch --enable-debug-symbols (v3) (obsolete) — Splinter Review
Attachment #432001 - Attachment is obsolete: true
Attachment #435128 - Flags: review?(ted.mielczarek)
Attachment #432001 - Flags: review?(ted.mielczarek)
Attached patch --enable-debug-symbols (v4) (obsolete) — Splinter Review
Attachment #435128 - Attachment is obsolete: true
Attachment #435592 - Flags: review?(ted.mielczarek)
Attachment #435128 - Flags: review?(ted.mielczarek)
(In reply to comment #62)
> Created an attachment (id=435592) [details]
> --enable-debug-symbols (v4)

Tested on Windows 7, with all -debug, -debug-symbols and -optimize enable/disable combinations.
Attachment #254456 - Attachment is obsolete: true
Attachment #254456 - Flags: review?(wtc)
Attachment #331392 - Attachment is obsolete: true
Attachment #331392 - Flags: review?(wtc)
Attachment #348289 - Attachment is obsolete: true
Attachment #354732 - Attachment is obsolete: true
Attachment #418861 - Attachment is obsolete: true
Comment on attachment 435592 [details] [diff] [review]
--enable-debug-symbols (v4)

>diff --git a/nsprpub/config/autoconf.mk.in b/nsprpub/config/autoconf.mk.in
>--- a/nsprpub/config/autoconf.mk.in
>+++ b/nsprpub/config/autoconf.mk.in
>@@ -32,16 +32,18 @@ MOD_MAJOR_VERSION = @MOD_MAJOR_VERSION@
> MOD_MINOR_VERSION = @MOD_MINOR_VERSION@
> MOD_PATCH_VERSION = @MOD_PATCH_VERSION@
> 
> LIBNSPR		= @LIBNSPR@
> LIBPLC		= @LIBPLC@
> 
> CROSS_COMPILE	= @CROSS_COMPILE@
> BUILD_OPT	= @MOZ_OPTIMIZE@
>+MOZ_DEBUG	= @MOZ_DEBUG@
>+MOZ_DEBUG_SYMBOLS	= @MOZ_DEBUG_SYMBOLS@

Do these line up in a sensible way in the actual file? Hard to tell with actual tab characters in a diff.

>diff --git a/nsprpub/configure.in b/nsprpub/configure.in
>--- a/nsprpub/configure.in
>+++ b/nsprpub/configure.in
>@@ -176,39 +176,58 @@ AC_ARG_WITH(mozilla,
> 	    else
> 	        MOZILLA_CLIENT=
> 	    fi],
>     [	if test -n "$MOZILLA_CLIENT"; then
> 	        AC_DEFINE(MOZILLA_CLIENT)
> 	    fi])
> 
> AC_ARG_ENABLE(optimize,
>-    [  --enable-optimize(=val) Enable code optimizations (val, ie. -O2) ],
>+    [  --enable-optimize[=OPT] Enable code optimizations (ie. -O2) ],
>     [ if test "$enableval" != "no"; then
>-        MOZ_OPTIMIZE=1
>-        if test -n "$enableval" && test "$enableval" != "yes"; then
>-    	    _OPTIMIZE_FLAGS=`echo $enableval | sed -e 's|\\\ | |g'`
>+          MOZ_OPTIMIZE=1
>+          if test -n "$enableval" -a "$enableval" != "yes"; then
>+            _OPTIMIZE_FLAGS=`echo $enableval | sed -e 's|\\\ | |g'`
>             _SAVE_OPTIMIZE_FLAGS=$_OPTIMIZE_FLAGS
>-        fi
>+          fi

Is this all just reindentation? Is this for consistency with something else? I'm not a fan of indentation changes unless you're already touching the code for another reason.

>+if test -n "$MOZ_DEBUG" -o -n "$MOZ_DEBUG_SYMBOLS"; then
>+    MOZ_DEBUG_SYMBOLS=1
>+fi

Why not just set MOZ_DEBUG_SYMBOLS=1 in the ENABLE(debug block where you set MOZ_DEBUG=1? Also, is there a reason you check that MOZ_DEBUG_SYMBOLS is set and then set it to 1 here?

>-        if test -n "$MOZ_OPTIMIZE"; then
>-            if test -n "$MOZ_DEBUG_SYMBOLS"; then
>-                _OPTIMIZE_FLAGS="$_OPTIMIZE_FLAGS -Zi"
>+        if test -n "$MOZ_DEBUG_SYMBOLS" -o -n "$MOZ_DEBUG"; then

Since you're setting MOZ_DEBUG_SYMBOLS if MOZ_DEBUG is defined up above, I think you should only have to test MOZ_DEBUG_SYMBOLS here. (And in the other places below.)

r=me with those things fixed. Thanks for the patch!
Attachment #435592 - Flags: review?(ted.mielczarek) → review+
(In reply to comment #64)
> (From update of attachment 435592 [details] [diff] [review])
> Do these line up in a sensible way in the actual file? Hard to tell with actual
> tab characters in a diff.
The whole file is a mess. I've tried to line them up better now.

> Is this all just reindentation? Is this for consistency with something else?
> I'm not a fan of indentation changes unless you're already touching the code
> for another reason.
I reindented and inverted the logic, to be consistent with the --enable-debug and --enable-debug-symbols options nearby.

> Why not just set MOZ_DEBUG_SYMBOLS=1 in the ENABLE(debug block where you set
> MOZ_DEBUG=1? Also, is there a reason you check that MOZ_DEBUG_SYMBOLS is set
> and then set it to 1 here?
Fixed.

> Since you're setting MOZ_DEBUG_SYMBOLS if MOZ_DEBUG is defined up above, I
> think you should only have to test MOZ_DEBUG_SYMBOLS here. (And in the other
> places below.)
Done.
Attachment #435592 - Attachment is obsolete: true
Attachment #435933 - Flags: review?(ted.mielczarek)
Comment on attachment 435933 [details] [diff] [review]
--enable-debug-symbols (v5)

Ok, this looks good. Thanks! I'll land it for you shortly.
Attachment #435933 - Flags: review?(ted.mielczarek) → review+
Landed in CVS:
Checking in configure;
/cvsroot/mozilla/nsprpub/configure,v  <--  configure
new revision: 1.272; previous revision: 1.271
done
Checking in configure.in;
/cvsroot/mozilla/nsprpub/configure.in,v  <--  configure.in
new revision: 1.276; previous revision: 1.275
done
Checking in config/autoconf.mk.in;
/cvsroot/mozilla/nsprpub/config/autoconf.mk.in,v  <--  autoconf.mk.in
new revision: 1.39; previous revision: 1.38
done
Checking in config/rules.mk;
/cvsroot/mozilla/nsprpub/config/rules.mk,v  <--  rules.mk
new revision: 3.77; previous revision: 3.76
done
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Summary: nspr4.dll and --disable-debug yields "no rule to make nspr4.pdb needed by export" → implement --enable-debug-symbols (nspr4.dll and --disable-debug yields "no rule to make nspr4.pdb needed by export")
Blocks: 504175
See Also: 504175
Today for the first time in more than a month I did hg pull on 1.9.2 and updated. Then I set my .mozconfig to avoid 'debug':

export MOZ_DEBUG_SYMBOLS=1
mk_add_options MOZ_OBJDIR=@TOPSRCDIR@/obj-ff-dbg
ac_add_options --enable-application=browser
mk_add_options MOZ_CO_PROJECT=browser
ac_add_options --disable-optimize
ac_add_options --disable-debug
ac_add_options --disable-tests
ac_add_options --enable-debugger-info-modules=yes

 My build fails again with " *** No rule to make target `nspr4.pdb', needed by `export'."

Is the patch here on 1.9.2?  If not will it work on 1.9.2? If yes, how to activate it?
(In reply to comment #68)

> Is the patch here on 1.9.2?  If not will it work on 1.9.2? If yes, how to
> activate it?

I applied the patch to 1.9.2 but two hunks fail. I could not see how to fix them, I could not find the lines specified by the diff. If I run with the partial patch I still fail.
My old workaround in comment 30 still works for me. So hopefully this problem will be fixed on 1.9.3.
This has only landed in NSPR CVS (and mozilla-central) so far. It's possible that we'll pick up a NSPR update on 1.9.2 at some point, but this isn't high-priority enough to drive an update there.
Mitchell, Ted: thank you for the patch.  I'd like you to make
one improvement to your patch: --enable-debug-symbols should
have a "smart" default value for backward compatibility.

Recall that the NSPR and NSS developers build NSPR with only
these two combinations of the debug/optimize configure options:

    --enable-debug --disable-optimize (called the "debug" build)
    --disable-debug --enable-optimize (called the "optimized" build)

I'd like --enable-debug-symbols to be the default in debug builds,
and --disable-debug-symbols to be the default in optimized builds.

You can tie the default of the debug-symbols configure option
to either the debug or the optimize configure option.

Looking at your patch v5 (attachment 435933 [details] [diff] [review]), I think you intended
to tie the default of the debug-symbols option to the debug configure
option:

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/nsprpub/configure.in&rev=1.277&mark=261,263-264#261

For some reason, it's not working well.  If I do a debug build
on Linux (just run "nsprpub/configure" without any options), I
don't get any debug symbols.  In fact, none of BUILD_OPT,
MOZ_DEBUG, and MOZ_DEBUG_SYMBOLS is defined in the generated
nsprpub/config/autoconf.mk:

    CROSS_COMPILE           =
    BUILD_OPT                       =
    MOZ_DEBUG                       =
    MOZ_DEBUG_SYMBOLS       =

Perhaps you didn't know that debug build is the default for
NSPR's configure?

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/nsprpub/configure.in&rev=1.277&mark=67-68#65
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: --- → 4.8.5
Attached patch Followup (obsolete) — Splinter Review
> I'd like --enable-debug-symbols to be the default in debug builds,
> and --disable-debug-symbols to be the default in optimized builds.

That's what I was aiming for.
 
> Looking at your patch v5 (attachment 435933 [details] [diff] [review]), I think you intended
> to tie the default of the debug-symbols option to the debug configure
> option:

Correct.

> Perhaps you didn't know that debug build is the default for
> NSPR's configure?

I did know, but I simply misunderstood AC_ARG_ENABLE'S behaviour when an option is not specified.

This patch sets the defaults as I originally intended, and also renames the few BUILD_OPT instances to MOZ_OPTIMIZE, as BUILD_OPT was set to @MOZ_OPTIMIZE@ anyway.
Attachment #441688 - Flags: review?(ted.mielczarek)
Comment on attachment 441688 [details] [diff] [review]
Followup

Sorry, I guess I missed those on reviewing the first patch.
Attachment #441688 - Flags: review?(ted.mielczarek) → review+
I edited Mitchell's Followup patch a bit:
- Rely on the default value of MOZ_DEBUG set at the top of configure.in.
- Fix TABs; in NSPR the default tab stop is 8.

I checked in the patch on the NSPR trunk (NSPR 4.8.6).

Checking in configure;
/cvsroot/mozilla/nsprpub/configure,v  <--  configure
new revision: 1.282; previous revision: 1.281
done
Checking in configure.in;
/cvsroot/mozilla/nsprpub/configure.in,v  <--  configure.in
new revision: 1.286; previous revision: 1.285
done
Checking in config/autoconf.mk.in;
/cvsroot/mozilla/nsprpub/config/autoconf.mk.in,v  <--  autoconf.mk.in
new revision: 1.42; previous revision: 1.41
done
Checking in config/config.mk;
/cvsroot/mozilla/nsprpub/config/config.mk,v  <--  config.mk
new revision: 3.33; previous revision: 3.32
done
Checking in pr/src/md/windows/Makefile.in;
/cvsroot/mozilla/nsprpub/pr/src/md/windows/Makefile.in,v  <--  Makefile.in
new revision: 1.21; previous revision: 1.20
done

Checking in configure;
/cvsroot/mozilla/nsprpub/configure,v  <--  configure
new revision: 1.283; previous revision: 1.282
done
Checking in configure.in;
/cvsroot/mozilla/nsprpub/configure.in,v  <--  configure.in
new revision: 1.287; previous revision: 1.286
done
Attachment #441688 - Attachment is obsolete: true
Status: REOPENED → RESOLVED
Closed: 11 years ago10 years ago
Resolution: --- → FIXED
Target Milestone: 4.8.5 → 4.8.6
Duplicate of this bug: 590032
(In reply to comment #71)
> This has only landed in NSPR CVS (and mozilla-central) so far. It's possible
> that we'll pick up a NSPR update on 1.9.2 at some point, but this isn't
> high-priority enough to drive an update there.

Requesting 1.9.2 support on the basis of duplicate bug 590032, which bit me unexpectedly.  (I really, really hate it when I can't build in some variant of configure options.)
status1.9.2: --- → ?
No, this has too many odd dependencies to backport to 1.9.2
You need to log in before you can comment on or make changes to this bug.