265501 - Merge 32 and 64 bit versions of prcpucfg.h on Solaris

Status: RESOLVED FIXED

(NSPR :: NSPR, defect, P1)

Description

[Christophe Ravel]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7.3) Gecko/20040913
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7.3) Gecko/20040913

Currently, prcpucfg.h is different for 32 and 64 bit Solaris.

32: prcpucfg.h = md/_solaris32.cfg
64: prcpucfg.h = md/_solaris64.cfg

The expected behaviour is to have only one version (md/_solaris.cfg) and use the
flag _LP64 to select the right definitions.

_LP64 is the flag used by all the Solaris headers included by NSPR.


Reproducible: Always
Steps to Reproduce:
1.
2.
3.

Actual Results:  
:-(

Expected Results:  
:-)

Comment 1

[Christophe Ravel]

Attachment: new file mozilla/nsprpub/pr/include/md/_solaris.cfg

New file _solaris.cfg = merge of _solaris32.cfg and _solaris64.cfg

Comment 2

[Christophe Ravel]

Attachment: patch for mozilla/nsprpub/configure

MDCPUCFG_H=_solaris.cfg

Comment 3

[Christophe Ravel]

Attachment: patch for mozilla/nsprpub/configure.in

MDCPUCFG_H=_solaris.cfg

Comment 4

[Julien Pierre]

Thanks, Christophe. Please also create patches for the tip of NSPR in addition
to NSPR_4_5_BRANCH.

Comment 5

[Christophe Ravel]

remove files:
mozilla/nsprpub/pr/include/md/_solaris32.cfg
mozilla/nsprpub/pr/include/md/_solaris64.cfg

Comment 6

[Christophe Ravel]

Attachment: new file for tip mozilla/nsprpub/pr/include/md/_solaris.cfg

New file _solaris.cfg = merge of _solaris32.cfg and _solaris64.cfg

For Tip. Only Copyright differences with the NSPR_4_5_BRANCH version.

Comment 7

[Christophe Ravel]

Comment on attachment 162895 [details]
new file mozilla/nsprpub/pr/include/md/_solaris.cfg

For NSPR_4_5_BRANCH

Comment 8

[Christophe Ravel]

Comment on attachment 162896 [details] [diff] [review]
patch for mozilla/nsprpub/configure

For both tip and NSPR_4_5_BRANCH

Comment 9

[Christophe Ravel]

Comment on attachment 162897 [details] [diff] [review]
patch for mozilla/nsprpub/configure.in

For both tip and NSPR_4_5_BRANCH

Comment 10

[Wan-Teh Chang]

Is it causing problems to have 32 and 64 bit versions of
prcpucfg.h on Solaris?

Comment 11

[Julien Pierre]

In a word, yes.

A Solaris component depends on NSPR/NSS, and builds both 32 and 64 bits, on
Solaris and x86 architectures, but we were only delivering a single set of
header files, the 64 bit header for Sparc, and the 32 bit header for x86.

The error went unnoticed for Sparc 32 bit and 64 bits as apparently the 64-bit
header file did not break the 32-bit Sparc compilation, although I can't imagine
how the resulting binaries ran (they probably didn't, but Solaris defaults to
booting 64 bit).

When I added 64-bit AMD support, the 32-bit header we used to package was
replaced with the 64-bit header, which broke the 32-bit x86 compilation .

One possible solution that I was advocating was to ship two sets of include
directories, and let the Solaris component choose which one to use accordingly,
but this went against the common usage in Solaris, so in the end it was
preferred to ship just one header that would work for all 4 cases (Sparc/x86,
32/64 bits). I must say it wasn't very logical to have one header for 32-bit
Sparc and 32-bit x86, and another header for 64-bit Sparc and 64-bit AMD.
Historic NSPR usage would probably dictate to have 4 files I suppose ...

Comment 12

[Wan-Teh Chang]

I reviewed and checked in the patch, added the new file,
and removed the two files on NSPR tip and NSPR_4_5_BRANCH.

I regenerated configure using autoconf as opposed to
modifying it directly.

I made some minor white space and comment changes to
_solaris.cfg and moved the definition of the PR_AF_INET6
macro outside the ifdef _LP64.

Comment 13

[Julien Pierre]

Wan-Teh,

Not so quick :-(

It doesn't appear that _LP64 is properly defined by the 64-bit compilers, but
only in the Solaris build rather. We are looking for an alternate 32/64 bit
differentiator .

Comment 14

[Julien Pierre]

Turns out there isn't a generic compiler define for 32 / 64 bit check. _LP64 is
only defined after one includes a Solaris header file.

Christophe, I believe the defines we need to check for are :

__sparc and __sparcv9 : 64 bit sparc
__sparc and __sparcv8 : 32 bit sparc

__x86_64 or __amd_64 : 64-bit AMD
__i386 or i386 : 32-bit x86

Be careful with the logic in the x86 case, as gcc defines both __x86_64 and
__i386 in 64-bit mode.

The above defines should work with either gcc or studio in the 4 different
platform cases. The only case I didn't test was gcc with 64-bit Solaris, as the
old version of gcc I have on sparc only does 32 bit.

Comment 15

[Julien Pierre]

Attachment: updated _solaris.cfg for NSPR_4_5_BRANCH

I tested 12 variants of Solaris NSPR builds successfully with this update.

Comment 16

[Christophe Ravel]

Comment on attachment 162957 [details] [diff] [review]
updated _solaris.cfg for NSPR_4_5_BRANCH

Julien,

You could put
#define PR_AF_INET6 26	/* same as AF_INET6 */
outside the 32/64 bit area as suggested by WTC.
Otherwise OK.

Comment 17

[Christophe Ravel]

Comment on attachment 162957 [details] [diff] [review]
updated _solaris.cfg for NSPR_4_5_BRANCH

Checked in NSPR_4_5_BRANCH with #define PR_AF_INET6 26 outside the IS_64 if and
else areas.

Comment 18

[Julien Pierre]

Christophe, thanks for the review and branch check-in.

Wan-Teh, could you please check the equivalent _solaris.cfg (it should differ
from the branch only by license and your comment changes) to the tip ?

Thanks.

Comment 19

[Wan-Teh Chang]

I checked in the patch for _solaris.cfg on the
NSPR trunk (NSPR 4.6) and NSPRPUB_PRE_4_2_CLIENT_BRANCH
(post Mozilla 1.8 Alpha 4).

Julien, could you email me the <sys/isa_defs.h> header
from Solaris 10?

Comment 20

[Julien Pierre]

Wan-Teh,

Thanks for checking in this patch. I have sent you the header file.

Comment 21

[Wan-Teh Chang]

Julien, the _solaris.cfg I checked in on the NSPR
trunk is equivalent to but slightly different from
the _solaris.cfg you checked in on the NSPR_4_5_BRANCH.