232742 - Mozilla incl. firebird does not build on Amd64 cpu

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Henrik Lynggaard Hansen]

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

Comment 1

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

also see bug 163013

Comment 2

[Henrik Lynggaard Hansen]

Attachment: This patch makes mozilla build on amd64 under gentoo linux

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

Comment 3

[Henrik Lynggaard Hansen]

Requesting that this blocks 1.7a..

Comment 4

[Andrew Schultz]

FWIW, the same patch is included with Blizzard's SRPMS (except that the attached
patch includes changes to TestXPTCInvoke.cpp)

Comment 5

[Roland Mainz]

Can someone update the patch for mozilla1.7a trunk and request review for it,
please ?

Comment 6

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 7

[Roland Mainz]

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 8

[Jungshik Shin]

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 9

[Wan-Teh Chang]

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 10

[Wan-Teh Chang]

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 11

[Wan-Teh Chang]

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 12

[Brian Ryner (not reading)]

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)

Comment 13

[Darin Fisher]

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

Comment 14

[Wolfgang Rosenauer [:wolfiR]]

as you might know an incompatible patch for the same thing has been checked in
with bug #236599. Please solve this.

Comment 15

[Roland Mainz]

(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.

Comment 16

[tor]

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).

Comment 17

[Wolfgang Rosenauer [:wolfiR]]

Attachment: patch for current tree

This is the patch I use currently to build on AMD64.
It fits into the current tree and should do it.

Comment 18

[Darin Fisher]

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.

Comment 19

[Johnny Stenback (:jst)]

I went ahead and corrected the comment, and added the lisence blocks to the new
files and checked this in. Marking FIXED.

Comment 20

[Johnny Stenback (:jst)]

Attachment: Same thing for the 1.7 branch.

Comment 21

[Johnny Stenback (:jst)]

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 22

[Brendan Eich [:brendan]]

Comment on attachment 146629 [details] [diff] [review]
Same thing for the 1.7 branch.

a=[e-mail] for 1.7 final.

/be

Comment 23

[Johnny Stenback (:jst)]

Fix checked in (and tested on the branch too).

Comment 24

[Wolfgang Rosenauer [:wolfiR]]

hmm, the patch didn't appear in the 1.7 branch. Can you please check?

Comment 25

[Johnny Stenback (:jst)]

Hmm, not sure what happened there. Now it's in, for sure!