259003 - port NSS to Solaris AMD64

Status: RESOLVED FIXED

(NSS :: Libraries, enhancement, P2)

Description

[Julien Pierre]

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.

Comment 1

[Julien Pierre]

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.

Comment 2

[Julien Pierre]

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

Comment 3

[Julien Pierre]

Attachment: also allow gcc to build 64-bit when setting NS_USE_GCC to 1

Comment 4

[Wan-Teh Chang]

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.

Comment 5

[Julien Pierre]

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.

Comment 6

[Julien Pierre]

Attachment: updated patch. applies to tip

Comment 7

[Julien Pierre]

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.

Comment 8

[Julien Pierre]

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

[Julien Pierre]

Comment on attachment 159676 [details] [diff] [review]
updated patch for NSS tip

I can't seem to attach the proper patch. Long day.

Comment 10

[Julien Pierre]

Attachment: patch for NSS tip

Comment 11

[Julien Pierre]

Attachment: patch for NSS_3_9_BRANCH

Comment 12

[Julien Pierre]

FYI, the Sun Studio9 compiler defines __x86_64 and __amd64, but not __x86_64__ .

Comment 13

[Wan-Teh Chang]

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 14

[Wan-Teh Chang]

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?!

Comment 15

[Julien Pierre]

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.

Comment 16

[Julien Pierre]

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

Comment 17

[Julien Pierre]

Attachment: updated patch for NSS_3_9_BRANCH

same changes as for the tip

Comment 18

[Wan-Teh Chang]

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 19

[Wan-Teh Chang]

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.

Comment 20

[Julien Pierre]

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: 1.1.82.1; previous revision: 1.1
done
Checking in coreconf/SunOS5.mk;
/cvsroot/mozilla/security/coreconf/SunOS5.mk,v  <--  SunOS5.mk
new revision: 1.13.16.1; previous revision: 1.13
done
Checking in nss/lib/freebl/Makefile;
/cvsroot/mozilla/security/nss/lib/freebl/Makefile,v  <--  Makefile
new revision: 1.53.14.4; previous revision: 1.53.14.3
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