vc90.pdb files are output in source directory instead of OBJDIR

RESOLVED FIXED in 3.12.3

Status

NSS
Build
P2
normal
RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: Julien Pierre, Assigned: Nelson Bolyard (seldom reads bugmail))

Tracking

trunk
3.12.3
x86
Windows Vista

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Reporter)

Description

8 years ago
When building with MSVC 9 (Visual studio 2008), one has to define MOZ_DEBUG_SYMBOLS since /PDB:NONE isn't supported. But then, the compiler generates PDB files in the source directory instead of OBJDIR .

Even though I don't need the PDB files, I ran into the following build race condition while simultaneously building 32-bit and 64-bit binaries on Windows :

c:/NSS/tip/mozilla/security/nss/lib/freebl/loader.c : fatal error C1033: cannot
open program database 'c:\nss\tip\mozilla\security\nss\lib\freebl\vc90.pdb'
make[2]: *** [WINNT6.0_64_OPT.OBJ/loader.obj] Error 2
make[2]: Leaving directory `/c/NSS/tip/mozilla/security/nss/lib/freebl'
make[1]: *** [libs] Error 2
make[1]: Leaving directory `/c/NSS/tip/mozilla/security/nss/lib'
make: *** [libs] Error 2

That's because two concurrent freebl builds were being done, and they each needed access to the vc90.pdb . It means the PDB file is being overwritten by each build.

IMO, this should be resolved , even for non-concurrent builds. If some customer asks us for PDB files, it will only be the PDB from the last build.

There seems to be other, named, PDB files output in OBJDIR, though.

Perhaps we don't need those VC90.PDB files at all, and we need to turn off their generation altogether.
There's a trivial fix to this that puts the pdb files in the objdir directory.
It's a one-line change to coreconf.  I have it in some of my workareas.
Let me find it.
Actually, it should no longer be true that it is necessary to define
MOZ_DEBUG_SYMBOLS to prevent PDB:NONE.  Wan-Teh committed a change to 
coreconf to detect the version of MSVC and do the right thing.  
(Well, Wan-Teh wrote such a patch, and I reviewed it.  I assume it was 
committed.)
(Reporter)

Comment 3

8 years ago
Nelson,
I believe the autodetection is for nspr only with autoconf. I don't think it works with coreconf. 
The environment variable is still needed for nss.
Created attachment 369943 [details] [diff] [review]
patch v1

Julien, Wan-Teh and I discussed the patch for cl version detection in several 
bugs. The bug for NSS is Bug 478563.  The patch attached to that bug was not 
committed because it was a "work in progress" and several suggested changes 
appear in the comments.  

The patch attached here should fix the problem with the location of the 
vcx0.pdb files.  Please test it in a clobber build.
Attachment #369943 - Flags: review?(julien.pierre.boogz)
(Assignee)

Updated

8 years ago
Assignee: nobody → nelson
Comment on attachment 369943 [details] [diff] [review]
patch v1

I tested this patch in both debug and optimized builds.  Looks good to me.
Attachment #369943 - Attachment description: untested patch v1 → patch v1
(Reporter)

Comment 6

8 years ago
Comment on attachment 369943 [details] [diff] [review]
patch v1

Nelson,

I will test this patch, but I have two questions :

1) is -Fd$(OBJDIR)/ supported in all versions of MSVC (including VC6) ? We don't require VC6 support on the trunk at Sun, but I don't know if anyone else needs it.

2) In the second hunk, I note that the patch was already there, but commented. What was the reason for it ?

Also, -Z7 got replaced with -Zi. Will this cause issues with compilers older than VC9 ?
(Reporter)

Comment 7

8 years ago
Comment on attachment 369943 [details] [diff] [review]
patch v1

I tested this patch and it's fine for VC9 (MSVC 2008). I don't know if it's got any unwanted side effects with previous versions of MSVC which is why I haven't given r+ yet.

I may be able to test VC6 in the office tomorrow, but I don't have anything else in between VC6 and VC9 to test.

We should clarify whether we need pre-VC9 support on the NSS trunk, and if so, for which versions.
VC8 builds fine with this patch.  
I don't know about VC6, but don't care much about it, either.
Mozilla requires VC8 for FF on Windows.  And since Sun uses VC9 now, 
I see no reason to continue worrying about VC6.
(Reporter)

Comment 9

8 years ago
Comment on attachment 369943 [details] [diff] [review]
patch v1

I agree we don't need vc6 support on the trunk. Vc7.x shouldn't be needed either.
Attachment #369943 - Flags: review?(julien.pierre.boogz) → review+
(Reporter)

Comment 10

8 years ago
One more thing, please remove the comment before checkin.
Checking in WIN32.mk; new revision: 1.34; previous revision: 1.33
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Priority: -- → P2
Resolution: --- → FIXED
Target Milestone: --- → 3.12.3
You need to log in before you can comment on or make changes to this bug.