225859 - port NSPR to Windows XP / Server 2003 64bit for AMD64

Status: RESOLVED FIXED

(NSPR :: NSPR, defect, P1)

Description

[Makoto Kato [:m_kato]]

User-Agent:       Mozilla/4.0 (compatible; MSIE 6.0; Windows NT 5.2; Win64; AMD64)
Build Identifier: 

Currently code isn't safe for 64bits-Windows.

ex) pointer structure is unsigned long (32bits).  But 64bit-Windows uses 64bits 
pointer.

Reproducible: Always

Steps to Reproduce:
This is build issue and port issue.  So this doesn't need.
Actual Results:  
cannot build.

Expected Results:  
build and work fine.

Comment 1

[Makoto Kato [:m_kato]]

Attachment: NSPR patch for Windows XP for AMD64

Comment 2

[Christian :Biesinger (don't email me, ping me on IRC)]

confirming...

Comment 3

[Makoto Kato [:m_kato]]

Attachment: mozilla/nsprpub new diff @ 2003-11-21

I re-wrote several code by bug #226094.

Comment 4

[timeless]

Comment on attachment 136000 [details] [diff] [review]
mozilla/nsprpub new diff @ 2003-11-21

for reference you can generally skip attaching diffs to configure, since they
should be generated from the configure.in changes.

>Index: pr/include/prtypes.h
>===================================================================
>RCS file: /cvsroot/mozilla/nsprpub/pr/include/prtypes.h,v
>retrieving revision 3.20.4.6
>diff -u -r3.20.4.6 prtypes.h
>--- pr/include/prtypes.h	16 Sep 2003 20:30:38 -0000	3.20.4.6
>+++ pr/include/prtypes.h	20 Nov 2003 14:17:55 -0000
>@@ -416,7 +416,11 @@
> **  A type for pointer difference. Variables of this type are suitable
> **      for storing a pointer or pointer sutraction. 
> ************************************************************************/
>+#if PR_BYTES_PER_WORD == 8 && PR_BYTES_PER_LONG != 8
>+typedef PRUint64 PRUptrdiff;
>+#else
> typedef unsigned long PRUptrdiff;
>+#endif
I wonder why we don't simply use typedef PRUint64 PRUptrdiff for both cases?

>@@ -469,8 +473,13 @@
> ** Specification, Addison-Wesley, September 1996.
> ** http://java.sun.com/docs/books/vmspec/index.html.)
> */
>+#if PR_BYTES_PER_WORD == 8 && PR_BYTES_PER_LONG != 8
>+typedef PRInt64 PRWord;
>+typedef PRUint64 PRUword;
>+#else
> typedef long PRWord;
> typedef unsigned long PRUword;
>+#endif
same...

you didn't write patches for the _winnt files, you should :). Or at least
indicate that the changes to the _win95 files can be applied to the _winnt
files (please test first though...)

Comment 5

[Makoto Kato [:m_kato]]

> I wonder why we don't simply use typedef PRUint64 PRUptrdiff for both cases?

Usual 32bit platform is that different of pointer is 32bits.  But 64bit platform
is 64bit and WinXP 64bit is LLP64 model.  So we need this change.

Also, I have it for WINNT defined, too.  So I will submit after testing more.

Comment 6

[Makoto Kato [:m_kato]]

Attachment: nsprpub diff @ 2004-02-01

I tested to build on RedHat Fedora Core1 and Win32.

Comment 7

[Asa Dotzler [:asa]]

not going to happen for 1.7. Maybe 1.8 or a 1.7.x release can incorporate these
changes. 

Comment 8

[Wan-Teh Chang]

Comment on attachment 140341 [details] [diff] [review]
nsprpub diff @ 2004-02-01

In mozilla/nsprpub/pr/src/Makefile.in, you have:

>+ifeq ($(TARGET_CPU),x86_64)
>+DLLBASE=/BASE:0x300000000000
>+else
> DLLBASE=/BASE:0x30000000
>+endif # x86_64

Is it beneficial to use /BASE:0x300000000000 on
the AMD64 architecture?  If not, I'd like to
remove the /BASE linker option.

Comment 9

[Wan-Teh Chang]

Attachment: nsprpub diff @ 2004-05-07

Please try this patch on mozilla/nsprpub.

The main change I made is to use the new type
'PROsfd' instead of 'PRWord' for osfd.

I also made some other small changes.

Comment 10

[Wan-Teh Chang]

Comment on attachment 147952 [details] [diff] [review]
nsprpub diff @ 2004-05-07

I have only compiled mozilla/nsprpub with this
patch applied.	I don't have an AMD64 system, so
I can't run the NSPR tests.

Comment 11

[Makoto Kato [:m_kato]]

Althouth new patch is OK, I cannot edit this bug due to "You are not authorised
to edit attachment 147952 [details] [diff] [review]" error.

Comment 12

[Wan-Teh Chang]

*** Bug 293580 has been marked as a duplicate of this bug. ***

Comment 13

[Wan-Teh Chang]

Attachment: nsprpub diff @ 2005-05-10

This is the same patch updated for the current
NSPRPUB_PRE_4_2_CLIENT_BRANCH, which is used by
the Mozilla and Firefox trunks.

This patch omits the changes to mozilla/nsprpub/configure.
You should run autoconf (version 2.13) in mozilla/nsprpub
after applying this patch to regenerate mozilla/nsprpub/configure.
I will attach a patch for mozilla/nsprpub/configure next
for those who don't have autoconf, but that patch is likely
to fail to apply as soon as we change mozilla/nsprpub/configure.

Comment 14

[Wan-Teh Chang]

Attachment: nsprpub/configure diff @ 2005-05-10

Comment 15

[dackz]

(In reply to comment #13)
> Created an attachment (id=183172) [edit]
> nsprpub diff @ 2005-05-10

Your patch doesn't take into account MSVC8, unless I'm doing something 
drastically wrong. In MSVC8 the architecture is _M_X64. I don't see anything 
about _AMD64_ with MSVC8 and compilation with your patch fails due to this.

Here is a list of predefined macros in MSVC8: http://msdn2.microsoft.com/
library/b0084kay(en-us,vs.80).aspx

Comment 16

[Michael Moy]

I think that X64 is for Windows 64. Are you building on/for Windows 64?
I'm running Windows XP 32 with MSVC8 but I haven't tried the patch yet but will
give it a shot today. It will take me a while to get my Windows 64 development
environment set up.

(In reply to comment #15)
> (In reply to comment #13)
> > Created an attachment (id=183172) [edit] [edit]
> > nsprpub diff @ 2005-05-10
> 
> Your patch doesn't take into account MSVC8, unless I'm doing something 
> drastically wrong. In MSVC8 the architecture is _M_X64. I don't see anything 
> about _AMD64_ with MSVC8 and compilation with your patch fails due to this.
> 
> Here is a list of predefined macros in MSVC8: http://msdn2.microsoft.com/
> library/b0084kay(en-us,vs.80).aspx

Comment 17

[dackz]

(In reply to comment #16)
> I think that X64 is for Windows 64. Are you building on/for Windows 64?
> I'm running Windows XP 32 with MSVC8 but I haven't tried the patch yet but 
will
> give it a shot today. It will take me a while to get my Windows 64 development
> environment set up.

Yes, I forgot to mention that. This is with building X64 code in an X64 
environment.

Comment 18

[Michael Moy]

(In reply to comment #17)
> (In reply to comment #16)
> > I think that X64 is for Windows 64. Are you building on/for Windows 64?
> > I'm running Windows XP 32 with MSVC8 but I haven't tried the patch yet but 
> will
> > give it a shot today. It will take me a while to get my Windows 64 development
> > environment set up.
> 
> Yes, I forgot to mention that. This is with building X64 code in an X64 
> environment.


Sorry, I was in the wrong bug.

Comment 19

[dackz]

(In reply to comment #15)
> Your patch doesn't take into account MSVC8, unless I'm doing something 
> drastically wrong. In MSVC8 the architecture is _M_X64. I don't see anything 
> about _AMD64_ with MSVC8 and compilation with your patch fails due to this.
> 
> Here is a list of predefined macros in MSVC8: http://msdn2.microsoft.com/
> library/b0084kay(en-us,vs.80).aspx

Sorry, this was my fault. The AC_DEFINE(_AMD64_) in configure.in takes care of
things.

Comment 20

[Makoto Kato [:m_kato]]

(In reply to comment #13)

Wan, looks OK.

Comment 21

[Wan-Teh Chang]

Attachment: nsprpub diff @ 2005-10-11

I'm ready to ask for a code review of this patch now.
Who should review this patch?

Here is a summary of the changes.

1. prtypes.h: I added definitions of the PRUptrdiff, PRWord,
and PRUword for _WIN64.  The definitions of these three types
are not changed for all other platforms.

2. private/pprio.h: I added a new type PROsfd to replace the
PRInt32 type that we've used for osfd.	I also added macros
for printing and scanning the PROsfd type.

3. In other files, most of the changes are to replace
"PRInt32 osfd" by "PROsfd osfd".  Other changes include
dealing with the larger size of size_t on WIN64, and to
pass 0 as the first argument to the select-like functions.

Could someone test this patch on Windows x64 Edition or
IA64 Edition?  Thanks.

Comment 22

[Wan-Teh Chang]

Attachment: nsprpub diff @ 2005-10-11 (second one)

I noticed that the PR_SI_ARCHITECTURE macro in _winnt.h
and _win95.h also needs to be defined for AMD64 and IA64.

Comment 23

[Doug Turner (:dougt)]

Comment on attachment 199234 [details] [diff] [review]
nsprpub diff @ 2005-10-11 (second one)

In the .cfg, you can combined the _AMD64_ and _IA64_ sections.


In nsprpub/pr/src/Makefile.in,

Will we losed the -W1 flag when NS_USE_GCC is defined?	Do we need any DLLBASE
flags when USE_64 is defined?


I noticed some changes like:

-		 rv = _PR_NTFiberSafeSelect(osfd + 1, &rd, NULL, NULL, tvp);
+		 rv = _PR_NTFiberSafeSelect(0, &rd, NULL, NULL, tvp);

I am not sure why 0 is the value.  In command #21, you mention that you should
pass zero to select-like functions.  Could you explain why?

There is some space vs. tab inconsistency in nsprpub/pr/tests/prpoll.c.  Do you
want to fix that?

Comment 24

[Wan-Teh Chang]

Doug: thanks for the code review.

In the .cfg, the _AMD64_ and _IA64_ sections are intentionally
separate because in general different CPU architectures may
have different values for some of the macros.

In nsprpub/pr/src/Makefile.in, the use of DLLBASE is optional.
That flag came from the old Netscape days when we could
assign 0x30000000 to NSPR.  NSPR is now open source, so
there is no cental authority that assign the DLLBASE addresses
to DLLs.  In fact, Mozilla rebases all of its DLLs anyway.
Originally the patch used a DLLBASE of 0x300000000000 for x86-64
(see nsprpub diff@2005-05-10) but decided to remove it because
I don't know where that value came from.

> Will we losed the -W1 flag when NS_USE_GCC is defined?

I don't understand this question.

Regarding passing 0 as the first argument to select-like functions,
this is okay on Windows because Winsock's select ignores its first
argument.  Would you like me to remove changes like
-		 rv = _PR_NTFiberSafeSelect(osfd + 1, &rd, NULL, NULL, tvp);
+		 rv = _PR_NTFiberSafeSelect(0, &rd, NULL, NULL, tvp);
because they have nothing to do with the 64-bit Windows port?

Regarding space vs. tab inconsistency in nsprpub/pr/tests/prpoll.c,
I don't fix those problems in general because those changes clutter
the diffs, which I read a lot.

Comment 25

[Wan-Teh Chang]

Doug: see
http://msdn.microsoft.com/library/default.asp?url=/library/en-us/winsock/winsock/recvfrom_2.asp
for the Microsoft documentation that says the first
argument (int nfds) to select is ignored.

Comment 26

[Wan-Teh Chang]

Doug: sorry, wrong URL.  Here is the URL of the select documentation:
http://msdn.microsoft.com/library/default.asp?url=/library/en-us/winsock/winsock/select_2.asp

Comment 27

[Wan-Teh Chang]

Attachment: Public interface and build system changes (for code review only)

These are the changes to NSPR's exported header files
and build system for 64-bit Windows.  You are welcome
to review the full patch, but in the interest of saving
your time, I'd like you to review the most important
subset of that patch.  If you don't have time to review
this patch, feel free to cancel my review request.

64-bit Windows breaks two assumptions NSPR's code makes:
1. long is large enough to hold a pointer.
2. PRInt32 is large enough to hold an "OS fd" (Unix file
descriptors, Winsock SOCKET, Windows HANDLE, etc.).

So I have to redefine PRUptrdiff, PRWord, and PRUword for
64-bit Windows.  Note that I did not touch the definitions
of these types for the other platforms because I need to
maintain backward compatibility.

I also added a new type called PROsfd for the "OS fd", and
it is defined as __int64 on 64-bit Windows so it can hold
HANDLE (which is a void *), and as PRInt32 on all other
platforms (to be backward compatible).

Thanks.

Comment 28

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

It would be great if this patch could remove the comment for the PRWord type
about that we shouldn't use the type. We desepratly need an integer type that is
the same size as a void*. Currently there are defines all over the place of
different sorts that just begs to get a proper NSPR type.

Additionally, it'd be nice to have official macros to convert between PRWord and
void* and back. Between PRWord and a given pointer type would probably be
usefull too. So something like:

#define NS_VOID_TO_WORD(ptr)  NS_REINTERPRET_CAST(PRWord, (ptr))
#define NS_WORD_TO_VOID(ptr)  NS_REINTERPRET_CAST(void*, (ptr))
#define NS_PTR_TO_WORD(ptr)   NS_REINTERPRET_CAST(PRWord, (ptr))
#define NS_WORD_TO_PTR(type, ptr)  NS_REINTERPRET_CAST((type), (ptr))

Comment 29

[Wan-Teh Chang]

Jonas: PRWord is badly named.  What does "word" mean?
"Word" has many meanings in computer science.  I found
that PRWord originally came from the Java VM because
NSPR's predecessor (NSPR 1) was used to port Sun JVM
to many platforms.  I don't want to use a type whose
name only makes sense in the context of JVM, so I wrote
the comment discouraging people from using PRWord.

I agree an integer type large enough to hold a pointer
is useful.  I can add PRIntptr and PRUintptr (modeled
after C99's intptr_t and uintptr_t) and deprecate PRWord
and PRUword.  How does that sound?

Comment 30

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Sounds good to me, as long as we have the macros and the typedef I don't care
much about the names :)

Comment 31

[Wan-Teh Chang]

Attachment: nsprpub diff @ 2005-10-21

Comment 32

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

This patch doesn't contain the PRIntptr type. Or were you planning on doing that
in a separate patch?

Comment 33

[Wan-Teh Chang]

Doug: I responded to your review comments in comment 24.

Darin: I've checked in the patch "nsprpub diff @2005-10-21",
but I'd still appreciate your review.

Makoto, Michael, and dackz: please pull the tip of the Mozilla
source tree and see if you can build it on 64-bit Windows
without patching mozilla/nsprpub.

Jonas: re: comment 28, I opened bug 313297 for adding PRIntptr
and PRUintptr to NSPR.  The conversion macros probably need to
be defined in XPCOM because they use C++ casts.  We can address
the conversion macros in bug 313297 or a new XPCOM bug.

Comment 34

[Michael Moy]

A build attempt without Makoto's patches died pretty early on. I emailed the
build log to WT Chang. I also tried an autoconf build with the same results.

Comment 35

[Wan-Teh Chang]

Michael: sorry I wasn't clear.  You should remove the
changes to mozilla/nsprpub from Makoto's patch but
still apply the patch to the rest of Mozilla.  The
goal is to see whether you can build without patching
mozilla/nsprpub, but the files in the other directories
still need to be patched.

Comment 36

[Julien Pierre]

*** Bug 318864 has been marked as a duplicate of this bug. ***

Comment 37

[Julien Pierre]

Wan-Teh,

I don't think the NSPR configure script should depend on the value of uname -m being x86_64 . Removing that dependency would be much better fix. No versions of uname.exe for Windows are returning this value currently anyway. One shouldn't have to switch tools and have a wrapper uname.exe specifically for 64-bit in order to go back & forth between 32 and 64 bit builds. This is a release engineering nightmare waiting to happen.

IMO, if NSPR detects an x86 processor, and the --enable-64-bit option is passed in to configure, then that should be sufficient to determine that the target is x86_64 / AMD64 . This would be consistent with other multi-bitness architectures which allow producing a 64-bit build only by setting the --enable-64-bit option, without having two different values for uname -m  .

Comment 38

[Julien Pierre]

Attachment: configure patch that allows 64-bit windows to build without a uname wrapper

Comment 39

[Wan-Teh Chang]

Comment on attachment 205269 [details] [diff] [review]
configure patch that allows 64-bit windows to build without a uname wrapper

This patch is fine.  I will fix the indentation
when I check it in.

Comment 40

[Wan-Teh Chang]

Comment on attachment 205269 [details] [diff] [review]
configure patch that allows 64-bit windows to build without a uname wrapper

Another approach is to invoke "cl" and use the first
line of its output to determine whether its target
CPU is x86 (80x86) or x64 (AMD64).

When Cygwin or MKS is ported to x64, its "uname -m"
should return a value that's different from the value
for x86.  For example, the latest MKS uname man page
says "uname -m" returns "8664" on AMD64/EM64T:
http://www.mkssoftware.com/docs/man1/uname.1.asp

The uname wrapper shell script was intended as an
interim solution before we have true x64 version of
Cygwin or MKS (1.5 years ago, I thought the x64 version
of Cygwin would be available soon).  I think your
patch is fine.  I just wanted to provide the historical
motivation for the uname wrapper shell script.

Comment 41

[Julien Pierre]

Wan-Teh,

When MKS and/or cygwin are ported to 64-bit and their uname returns a different value, there will still be the problem of how to do a 32-bit build. We shouldn't have to use a 32-bit cygwin or MKS to target Win32, and a 64-bit version to target Win64. On all other platforms, the bitness of the tools executables doesn't matter, and in fact we use a mix of many of them. There is no precedent for a USE_32 variable, or --enable-32-bits passed to configure . I think the default target should be 32 bits to be consistent with other architectures that allowed 32 and 64 bits. IMO, we should only have a 64-bit target by default if a 32-bit target is not supported at all by the target OS.

Comment 42

[Edward Lam]

Any chance of these changes committed into the 1.8 branch soon?

Comment 43

[Julien Pierre]

This bug has been fixed. Just set USE_64=1 in your environment and --enable-64-bit in NSPR configure to specify the target and it will build fine. You don't need a 64-bit version of cygwin or MKS, only a 64-bit compiler (VC7.1/8/9).