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

Status: RESOLVED FIXED

(NSS :: Build, defect, P2)

Description

[Julien Pierre]

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.

Comment 1

[Nelson Bolyard (seldom reads bugmail)]

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.

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

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.)

Comment 3

[Julien Pierre]

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.

Comment 4

[Nelson Bolyard (seldom reads bugmail)]

Attachment: 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.

Comment 5

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 369943 [details] [diff] [review]
patch v1

I tested this patch in both debug and optimized builds.  Looks good to me.

Comment 6

[Julien Pierre]

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 ?

Comment 7

[Julien Pierre]

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.

Comment 8

[Nelson Bolyard (seldom reads bugmail)]

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.

Comment 9

[Julien Pierre]

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.

Comment 10

[Julien Pierre]

One more thing, please remove the comment before checkin.

Comment 11

[Nelson Bolyard (seldom reads bugmail)]

Checking in WIN32.mk; new revision: 1.34; previous revision: 1.33