There will be two ports, one with the Studio9 compiler, and another with gcc 3.4.1 . Currently, only the former has patches ready, which I will attach.
I'm not sure if this will make NSS 3.9.3, which we will probably cut in less than 10 days . I'd like to target 3.9.4, but there is no such milestone in bugzilla.
Created attachment 158665 [details] [diff] [review] patch to compile with the Sun Studio9 compiler You need to set USE_64 to 1 to compile 64-bit with this patch. There is currently a problem with the SHA384 and SHA512 tests failing in all.sh, but everything else passes.
Created attachment 159142 [details] [diff] [review] also allow gcc to build 64-bit when setting NS_USE_GCC to 1
Comment on attachment 159142 [details] [diff] [review] also allow gcc to build 64-bit when setting NS_USE_GCC to 1 Here are my unsolicited review comments. Hope you don't mind. 1. coreconf/SunOS5.10_i86pc.mk >+ifeq ($(USE_64),1) >+AMD64 = 1 >+OS_DEFINES += -D__x86_64__ >+else >+OS_DEFINES += -Di386 >+endif I encourage you to avoid introducing the new make variable AMD64. It is not necessary. I think __x86_64__ is implicitly defined by compilers. > include $(CORE_DEPTH)/coreconf/SunOS5.mk > > CPU_ARCH = x86 >+ifndef AMD64 > ARCHFLAG = >-OS_DEFINES += -Di386 >+endif Perhaps CPU_ARCH needs to be "x86-64" or "x86_64" if USE_64 equals 1? "ARCHFLAG = " probably should not be inside an ifdef. 2. coreconf/SunOS5.mk > ifeq ($(USE_64), 1) > ifdef NS_USE_GCC >- ARCHFLAG= UNKNOWN >+ ARCHFLAG= Are you sure about this change? > ifdef NS_USE_GCC > DSO_LDOPTS += -shared -h $(notdir $@) >+ifeq ($(USE_64), 1) >+ DSO_LDOPTS += -m64 >+ OS_CFLAGS += -m64 >+endif > else > ifeq ($(USE_64), 1) >+ifdef AMD64 >+ DSO_LDOPTS += -xarch=amd64 >+else > DSO_LDOPTS += -xarch=v9 > endif >+endif > DSO_LDOPTS += -G -h $(notdir $@) > endif I think "OS_CFLAGS += -m64" should be deleted. Instead, replace the "ARCHFLAG= UNKNOWN" above by "ARCHFLAG = -m64". For NS_USE_GCC, you should add the new code before "DSO_LDOPTS += -shared -h $(notdir $@) to be analogous to the non-gcc case.
1. __x86_64__ is not implicitly defined by the Sun studio compiler, only by gcc. Changing CPUARCH may be an option, but I think there are other parts of coreconf that check for i86pc . 2. The "UNKNOWN" was showing up in the OBJDIR . That's why I reset it. Note that previously, ARCHFLAG was cleared in SunOS5.10_i86pc.mk . I just moved that to SunOS5.mk . I'll try your suggestion.
Created attachment 159672 [details] [diff] [review] updated patch. applies to tip
Comment on attachment 159672 [details] [diff] [review] updated patch. applies to tip Actually this still has a few problems. Some of the many solaris builds (12 total!) are failing. One more coming.
Created attachment 159676 [details] [diff] [review] updated patch for NSS tip Implements many suggestions from Wan-Teh's and Saul's review. Allows building on x86 and AMD64, either 32-bit or 64-bit, with gcc or Sun's Studio 9. Applies to the tip.
Comment on attachment 159676 [details] [diff] [review] updated patch for NSS tip I can't seem to attach the proper patch. Long day.
Created attachment 159683 [details] [diff] [review] patch for NSS tip
FYI, the Sun Studio9 compiler defines __x86_64 and __amd64, but not __x86_64__ .
Then I think our C code should test for __x86_64 instead of __x86_64__. Now I remember this same difference between gcc and Sun compiler for SPARC. In any case, I think defining -D__x86_64__ for the Sun Studio9 compiler is fine.
Comment on attachment 159685 [details] [diff] [review] patch for NSS_3_9_BRANCH 1. coreconf/SunOS5.10_i86pc.mk >+ifeq ($(USE_64),1) >+CPU_ARCH = amd64 >+OS_DEFINES += -D__x86_64__ >+else > CPU_ARCH = x86 >-ARCHFLAG = > OS_DEFINES += -Di386 >+endif "x86-64" or "x86_64" might be a better value for CPU_ARCH than "amd64" because Intel has a compatible architecture called EM64T. You may want to indent the code inside ifeq and else. Do we need to have -Di386 for the USE_64 case? (See my question/comment about this in the NSPR Solaris/AMD64 bug.) 2. coreconf/SunOS5.mk >+# default to Sparc architecture >+ifeq ($(CPU_ARCH),) >+CPU_ARCH = sparc >+endif This change requires modifying all the older SunOS5.*_i86pc.mk to set CPU_ARCH to x86 before including SunOS5.mk. This can be avoided if in this file you use $(OS_TEST) instead of $(CPU_ARCH) to determine the CPU type. > ifeq ($(USE_64), 1) > ifdef NS_USE_GCC >- ARCHFLAG= UNKNOWN >+ ARCHFLAG= -m64 Does -m64 work for both AMD64 and 64-bit SPARC? > else >- ARCHFLAG=-xarch=v9 >+ ifeq ($(CPU_ARCH),amd64) >+ ARCHFLAG=-xarch=amd64 >+ else >+ ARCHFLAG=-xarch=v9 >+ endif > endif Instead of comparing $(CPU_ARCH) with amd64, it may be better to compare $(OS_TEST) with i86pc to be consistent with NSPR. Also may want to indent inside ifeq and else. > else >- ifdef NS_USE_GCC >- ifdef USE_HYBRID >- ARCHFLAG=-mcpu=v9 -Wa,-xarch=v8plus >+ ifeq ($(CPU_ARCH),sparc) >+ ifdef NS_USE_GCC >+ ifdef USE_HYBRID >+ ARCHFLAG=-mcpu=v9 -Wa,-xarch=v8plus >+ else >+ ARCHFLAG=-mcpu=v8 >+ endif > else >- ARCHFLAG=-mcpu=v8 >- endif >- else >- ifdef USE_HYBRID >- ARCHFLAG=-xarch=v8plus >- else >- ARCHFLAG=-xarch=v8 >+ ifdef USE_HYBRID >+ ARCHFLAG=-xarch=v8plus >+ else >+ ARCHFLAG=-xarch=v8 >+ endif > endif > endif > endif Does this change (adding ifeq ($(CPU_ARCH),sparc) around the whole block of code) mean we were compiling with -xarch=v8 on Solaris x86?!
Thanks for the review, Wan-Teh. 1. I wanted to have a CPU_ARCH that was different. amd64 seems innocuous enough, IMO, even if Intel has a compatible instruction set. I don't think we need -Di386 for the USE_64, nor do we want to. This can actually tell some parts of the code to optimize structures for 32 bits integers, which is undesirable. Sun studio does not define this macro in 64-bit mode, but unfortunately gcc does. 2. If I use OS_TEST instead of OS_ARCH, then I also have to check USE_64 . But it's OK since it means not having to modify config files for each new version of solaris. gcc -m64 works for both AMD64 and 64-bit Sparc indeed . However, on Sparc, the gcc build fails for other reasons not related to the patch, even in 32 bit mode. Actually the NSPR build fails with gcc on Sparc as it seems to be using the Sun compiler syntax. The NSS build may be OK, but I didn't get that far. As far as the test on CPU_ARCH for sparc, yes, I have seen cases when we were compiling with -xarch=v8 on Solaris x86, although that only generated a warning, as the Sun compiler is not a cross-compiler. Those cases may only have occurred when I had the patch in progress, and not with what was checked in before; but regardless, I don't think this change hurts anything.
Created attachment 161337 [details] [diff] [review] updated patch for tip 1. use CPU_ARCH = x86_64 instead of AMD64 2. Don't define __x86_64__ 3. Don't define i386 for 64 bit casse 4. Don't define CPU_ARCH to Sparc by default 5. Use OS_TEST = i86pc and USE_64 to test for x86 64 6. remove tabs
Created attachment 161346 [details] [diff] [review] updated patch for NSS_3_9_BRANCH same changes as for the tip
Comment on attachment 161337 [details] [diff] [review] updated patch for tip r=wtc. There are some white space problems in your changes. You should try to follow the indentation styles in those makefiles, including the use of tabs. coreconf uses tabs more than our C files do. We should avoid a mixture of indentation styles in coreconf.
Comment on attachment 161346 [details] [diff] [review] updated patch for NSS_3_9_BRANCH r=wtc. Same comments about the use of white spaces. Please follow the prevalent indentation style and use of tabs in coreconf makefiles.
I fixed the indenting. Checked in to NSS_3_9_BRANCH : Checking in coreconf/SunOS5.10_i86pc.mk; /cvsroot/mozilla/security/coreconf/SunOS5.10_i86pc.mk,v <-- SunOS5.10_i86pc.mk new revision: 184.108.40.206; previous revision: 1.1 done Checking in coreconf/SunOS5.mk; /cvsroot/mozilla/security/coreconf/SunOS5.mk,v <-- SunOS5.mk new revision: 220.127.116.11; previous revision: 1.13 done Checking in nss/lib/freebl/Makefile; /cvsroot/mozilla/security/nss/lib/freebl/Makefile,v <-- Makefile new revision: 18.104.22.168; previous revision: 22.214.171.124 done And to the tip : Checking in coreconf/SunOS5.10_i86pc.mk; /cvsroot/mozilla/security/coreconf/SunOS5.10_i86pc.mk,v <-- SunOS5.10_i86pc.mk new revision: 1.3; previous revision: 1.2 done Checking in coreconf/SunOS5.mk; /cvsroot/mozilla/security/coreconf/SunOS5.mk,v <-- SunOS5.mk new revision: 1.16; previous revision: 1.15 done Checking in nss/lib/freebl/Makefile; /cvsroot/mozilla/security/nss/lib/freebl/Makefile,v <-- Makefile new revision: 1.58; previous revision: 1.57 done