12.92 KB, patch
|Details | Diff | Splinter Review|
62.24 KB, patch
Darin Fisher: review+
|Details | Diff | Splinter Review|
1.68 KB, patch
Wan-Teh Chang: review+
|Details | Diff | Splinter Review|
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.
Created attachment 136000 [details] [diff] [review] mozilla/nsprpub new diff @ 2003-11-21 I re-wrote several code by bug #226094.
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 22.214.171.124 >diff -u -r126.96.36.199 prtypes.h >--- pr/include/prtypes.h 16 Sep 2003 20:30:38 -0000 188.8.131.52 >+++ 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...)
> 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.
Created attachment 140341 [details] [diff] [review] nsprpub diff @ 2004-02-01 I tested to build on RedHat Fedora Core1 and Win32.
not going to happen for 1.7. Maybe 1.8 or a 1.7.x release can incorporate these changes.
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.
Created attachment 147952 [details] [diff] [review] 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 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.
Althouth new patch is OK, I cannot edit this bug due to "You are not authorised to edit attachment 147952 [details] [diff] [review]" error.
*** Bug 293580 has been marked as a duplicate of this bug. ***
Created attachment 183172 [details] [diff] [review] 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.
(In reply to comment #13) > Created an attachment (id=183172)  > 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
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)   > > 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
(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.
(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.
(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.
(In reply to comment #13) Wan, looks OK.
Created attachment 199213 [details] [diff] [review] 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.
Created attachment 199234 [details] [diff] [review] 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 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?
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.
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.
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
Created attachment 199357 [details] [diff] [review] 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.
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))
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?
Sounds good to me, as long as we have the macros and the typedef I don't care much about the names :)
Created attachment 200365 [details] [diff] [review] nsprpub diff @ 2005-10-21
This patch doesn't contain the PRIntptr type. Or were you planning on doing that in a separate patch?
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.
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.
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.
*** Bug 318864 has been marked as a duplicate of this bug. ***
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 .
Created attachment 205269 [details] [diff] [review] configure patch that allows 64-bit windows to build without a uname wrapper
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 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.
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.
Any chance of these changes committed into the 1.8 branch soon?
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).