34.23 KB, patch
|Details | Diff | Splinter Review|
24.74 KB, patch
|Details | Diff | Splinter Review|
33.28 KB, patch
|Details | Diff | Splinter Review|
I have just got a new amd64 bit system and naturally wanted to build mozilla on it. I'm running gentoo linux in 64 bit mode, and It comes with mozilla so I knew it should work. However my own builds from cvs failed. The gentoo ebuilds has some amd64 patches it applies to the mozilla source before building it, so I tried to manually apply the same patch to my cvs tree, and it worked for firebird (currently testing the suite) I will attach the patch to this bugreport
also see bug 163013
This patch is taken from gentoo linux's portage and it makes the gentoo mozilla build, along with my cvs tree build (pulled today) both firebird and the suite. ps. I set severity to blocker, since this bug blocks the building and testing of mozilla apps. pps. I choose xpcom since i believe the patch touches some xpcom files (but Im not sure). afaik it patches: henrik@workstation MozillaBuild $ patch -p1 </usr/portage/net-www/mozilla/files/mozilla-1.4-amd64.patch patching file config/mkdepend/imakemdep.h patching file directory/c-sdk/ldap/libraries/liblber/lber-int.h patching file nsprpub/configure Hunk #1 succeeded at 3736 (offset 7 lines). patching file nsprpub/configure.in Hunk #1 succeeded at 1222 (offset 4 lines). patching file nsprpub/pr/include/md/_linux.cfg patching file nsprpub/pr/include/md/_linux.h Hunk #2 succeeded at 111 (offset 2 lines). patching file nsprpub/pr/src/io/prprf.c patching file nsprpub/pr/src/md/unix/os_Linux_x86_64.s patching file security/coreconf/Linux.mk Hunk #2 succeeded at 116 (offset 5 lines). patching file xpcom/reflect/xptcall/src/md/unix/Makefile.in patching file xpcom/reflect/xptcall/src/md/unix/xptcinvoke_x86_64_linux.cpp patching file xpcom/reflect/xptcall/src/md/unix/xptcstubs_x86_64_linux.cpp patching file xpcom/reflect/xptcall/tests/TestXPTCInvoke.cpp
Requesting that this blocks 1.7a..
FWIW, the same patch is included with Blizzard's SRPMS (except that the attached patch includes changes to TestXPTCInvoke.cpp)
Can someone update the patch for mozilla1.7a trunk and request review for it, please ?
We're not going to hold the 1.7a release because porting fixes aren't in. Trying to get porting fixes in is a good thing, but blocking flags aren't the way to do it.
Flags: blocking1.7a? → blocking1.7a-
Comment on attachment 140309 [details] [diff] [review] This patch makes mozilla build on amd64 under gentoo linux Requesting r= from wtc for the NSPR and NSS changes per bryner's suggestion...
Comment on attachment 140309 [details] [diff] [review] This patch makes mozilla build on amd64 under gentoo linux >diff -ruN mozilla.old/config/mkdepend/imakemdep.h >@@ -236,23 +236,22 @@ >-#elif !defined(__alpha) || defined(VMS) >+#elif !defined(__x86_64__) && (!defined(__alpha) || defined(VMS)) > > #define LBER_HTONL( l ) htonl( l ) > #define LBER_NTOHL( l ) ntohl( l ) > > #else /* __alpha */ nit: comment here should be updated to include '__x86_64__' >- * htonl and ntohl on the DEC Alpha under OSF 1 seem to only swap the >- * lower-order 32-bits of a (64-bit) long, so we define correct versions >- * here. >+ * htonl and ntohl on the 64-bit UNIX platforms only swap the lower-order >+ * 32-bits of a (64-bit) long, so we define correct versions here. nit: How about 64bit version of AIX, Solaris and Irix? If they don't need this, the comment shouldn't say '64-bit UNIX platforms....'. Instead, list platforms that are affected (Alpha and AMD64).
Comment on attachment 140309 [details] [diff] [review] This patch makes mozilla build on amd64 under gentoo linux The NSPR and NSS changes in this patch look good. I can't really review the x86-64 assembly code in mozilla/nsprpub/pr/src/md/unix/os_Linux_x86_64.s, but it looks close enough to the x86 assembly code in mozilla/nsprpub/pr/src/md/unix/os_Linux_x86.s. I suggest that you run the following NSPR test: % cd nsprpub/pr/tests % make atomic % ./atomic -d If someone who knows x86-64 assembly language can review os_Linux_x86_64.s, I'd appreciate it. Question: can we add the following assembler directive at the end of os_Linux_x86_64.s? / Magic indicating no need for an executable stack .section .note.GNU-stack, "", @progbits ; .previous os_Linux_x86.s has it.
Comment on attachment 140309 [details] [diff] [review] This patch makes mozilla build on amd64 under gentoo linux I forgot to mention that I've already checked in the only NSS change in this patch (mozilla/security/coreconf/Linux.mk) on the NSS trunk (NSS 3.10), NSS_3_9_BRANCH (NSS 3.9.1), and NSS_CLIENT_TAG (Mozilla 1.7a). I will take care of the NSPR changes when I have the NSPR "atomic" test result and the answer to my question about the non-executable stack assembler directive.
Comment on attachment 140309 [details] [diff] [review] This patch makes mozilla build on amd64 under gentoo linux I've checked in the NSPR changes in this patch (after minor editing) on the NSPR trunk (NSPR 4.5) and NSPRPUB_PRE_4_2_CLIENT_BRANCH (Mozilla 1.7 beta).
Comment on attachment 140309 [details] [diff] [review] This patch makes mozilla build on amd64 under gentoo linux xptcinvoke_x86_64_linux.cpp and xptcstubs_x86_64_linux.cpp are missing license headers. sr=bryner with that fixed. (note that I can't vouch for the corectness of the xptcall assembly code)
Attachment #140309 - Flags: superreview?(bryner) → superreview+
Comment on attachment 140309 [details] [diff] [review] This patch makes mozilla build on amd64 under gentoo linux >diff -ruN mozilla.old/directory/c-sdk/ldap/libraries/liblber/lber-int.h >-#elif !defined(__alpha) || defined(VMS) >+#elif !defined(__x86_64__) && (!defined(__alpha) || defined(VMS)) > > #define LBER_HTONL( l ) htonl( l ) > #define LBER_NTOHL( l ) ntohl( l ) > > #else /* __alpha */ > /* >- * htonl and ntohl on the DEC Alpha under OSF 1 seem to only swap the ... >+ * htonl and ntohl on the 64-bit UNIX platforms only swap the lower-order how do you know this is true of all 64-bit UNIX platforms? what about 64-bit Solaris? should you remove the "/* __alpha */" comment? >+#endif /* __alpha || __x86_64__ */ this comment isn't really correct. >diff -ruN mozilla.old/nsprpub/pr/src/md/unix/os_Linux_x86_64.s >+/ The Initial Developer of the Original Code is Netscape is Netscape really the initial developer? >diff -ruN mozilla.old/xpcom/reflect/xptcall/src/md/unix/xptcinvoke_x86_64_linux.cpp >+// 6 integral parameters are passed in registers >+const PRUint32 GPR_COUNT = 6; >+ >+// 8 floating point parameters are passed in SSE registers >+const PRUint32 FPR_COUNT = 8; nit: declare these static >+ typedef PRUint32 (*Method)(PRUint64, PRUint64, PRUint64, PRUint64, PRUint64, PRUint64); >+ PRUint32 result = ((Method)methodAddress)(a0, a1, a2, a3, a4, a5); uhm.. does this mean that this code will only work with methods that take 6 or fewer arguments? (i admit that i don't know much about xtpcall stuff) dbradley should probably review this code. >diff -ruN mozilla.old/xpcom/reflect/xptcall/src/md/unix/xptcstubs_x86_64_linux.cpp >+const PRUint32 PARAM_BUFFER_COUNT = 16; >+const PRUint32 GPR_COUNT = 6; >+const PRUint32 FPR_COUNT = 8; declare static >+ NS_ASSERTION(dispatchParams,"no place for params"); >+ if (! dispatchParams) >+ return NS_ERROR_OUT_OF_MEMORY; nit: use NS_ERROR instead >+ NS_ASSERTION(0, "bad type"); >+ break; NS_NOTREACHED >+ NS_ASSERTION(0,"nsXPTCStubBase::Sentinel called"); \ NS_NOTREACHED
Attachment #140309 - Flags: review?(wchang0222) → review+
as you might know an incompatible patch for the same thing has been checked in with bug #236599. Please solve this.
(In reply to comment #2) > Created an attachment (id=140309) > This patch makes mozilla build on amd64 under gentoo linux Can you please make a new patch _ASAP_ incl.the corrections requested by comment #13 (comment #14 is invalid AFAIK, that patch was not checked-in yet) ? There is a small chanche that this patch can be shipped with mozilla1.7final and I'd like to grab it.
Assignee: dougt → admin
Comment on attachment 140309 [details] [diff] [review] This patch makes mozilla build on amd64 under gentoo linux Please ask for approval again when there is a version of the patch that matches the current tree (comment #14).
Attachment #140309 - Flags: approval1.7? → approval1.7-
This is the patch I use currently to build on AMD64. It fits into the current tree and should do it.
Comment on attachment 146251 [details] [diff] [review] patch for current tree seems fine to me, although i haven't carefully reviewed the assembly code. since x86-64 is the only platform affected by that code, i'm going to take your word for it that it does the right thing. this error seems badly worded: +#error "can't find a compiler to use" perhaps it should say something like your compiler is not supported instead.
I went ahead and corrected the comment, and added the lisence blocks to the new files and checked this in. Marking FIXED.
Status: NEW → RESOLVED
Last Resolved: 15 years ago
Resolution: --- → FIXED
Comment on attachment 146629 [details] [diff] [review] Same thing for the 1.7 branch. Requesting approval for landing this Linux/x86-64 only change on the 1.7 branch.
Comment on attachment 146629 [details] [diff] [review] Same thing for the 1.7 branch. firstname.lastname@example.org for 1.7 final. /be
Attachment #146629 - Flags: approval1.7? → approval1.7+
Fix checked in (and tested on the branch too).
hmm, the patch didn't appear in the 1.7 branch. Can you please check?
Hmm, not sure what happened there. Now it's in, for sure!
You need to log in before you can comment on or make changes to this bug.