361415 - PowerPC64 not supported

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[David Woodhouse]

User-Agent:       Mozilla/5.0 (X11; U; Linux ppc64; en-GB; rv:1.8.0.8) Gecko/20061107 Fedora/1.5.0.8-1.fc6 Firefox/1.5.0.8
Build Identifier: Mozilla/5.0 (X11; U; Linux ppc64; en-GB; rv:1.8.0.8) Gecko/20061107 Fedora/1.5.0.8-1.fc6 Firefox/1.5.0.8

The Linux/PPC64 platform is not supported -- the build succeeds but resulting binary is non-functional (see also bug 361413).

Reproducible: Always

Steps to Reproduce:
1. Attempt to build for ppc64


Actual Results:  
Build "succeeds" and generates non-functional firefox.

Expected Results:  
Firefox should work.

Comment 1

[David Woodhouse]

Attachment: PPC64 support

Comment 2

[Benjamin Smedberg]

Comment on attachment 246176 [details] [diff] [review]
PPC64 support

This is a patch against the branch. It will need to be updated to the trunk first (see the XPTCall changes made in bug 349002).

Comment 3

[David Woodhouse]

Attachment: updated patch for trunk

(In reply to comment #2)
> This is a patch against the branch. It will need to be updated to the trunk
> first (see the XPTCall changes made in bug 349002).

OK, those look simple enough -- adjusted patch attached (along with a bug fix for stack misalignment in {NS,XPTC}_InvokeByIndex and some cosmetic cleanups.

Not entirely sure why ppc32 didn't get updated at the same time but I've done at the XPTCall changes, since the changes were so simple -- but I've that too in the attached patch.

Completely untested; my ppc64 build is running now, and then I'll do a ppc32 build.

Comment 4

[David Woodhouse]

(In reply to comment #3)
> Not entirely sure why ppc32 didn't get updated at the same time but I've done
> at the XPTCall changes, since the changes were so simple -- but I've that too
> in the attached patch.

WTF? Sorry, I'll try that sentence again more coherently:

Not entirely sure why ppc32 didn't get updated at the same time as the XPTCall changes, since the changes were so simple -- but I've done that too in the attached patch.

Comment 5

[David Woodhouse]

Attachment: Updated patch

Minor fix; I was overallocating the stack space for the parameters due to a thinko.

@@ -94,7 +94,7 @@ diff -u mozilla/xpcom/reflect/xptcall/sr
 +      // we don't actually need stack space for those. Start by rounding
 +      // down to make sure the stack remains 16-byte aligned.
 +
-+      rldicr  r7,r5,r5,59             // r7 = (r3 << 3) & ~15
++      rldicr  r7,r5,3,59              // r7 = (r5 << 3) & ~15
 +      addi    r7,r7,128+(18*8)        // Standard frame, 7 dwords for GPR,
 +                                      // 13 dwords for FPR, 3 for r29-r31,
 +                                      // less 5 of the extra unused params

Comment 6

[David Woodhouse]

Attachment: Hopefully final version of patch.

This cleans up the PPC64 code a little; especially the stack allocation. It also fixes the PPC build of libxpcom_core.so

I still can't actually build anything else, but that's a separate issue...

pmac /home/dwmw2/working/mozilla/obj-powerpc-unknown-linux-gnu/xpcom/reflect/xptcall/tests $ make
/usr/bin/gmake export
gmake[1]: Entering directory `/home/dwmw2/working/mozilla/obj-powerpc-unknown-linux-gnu/xpcom/reflect/xptcall/tests'
gmake[1]: Leaving directory `/home/dwmw2/working/mozilla/obj-powerpc-unknown-linux-gnu/xpcom/reflect/xptcall/tests'
/usr/bin/gmake libs
gmake[1]: Entering directory `/home/dwmw2/working/mozilla/obj-powerpc-unknown-linux-gnu/xpcom/reflect/xptcall/tests'
c++   -fno-rtti -fno-exceptions -Wall -Wconversion -Wpointer-arith -Wcast-align -Woverloaded-virtual -Wsynth -Wno-ctor-dtor-privacy -Wno-non-virtual-dtor -Wno-long-long -pedantic -fshort-wchar -pthread -pipe  -DNDEBUG -DTRIMMED -O -o TestXPTCInvoke TestXPTCInvoke.o  -lpthread    -L../../../../dist/bin -L../../../../dist/lib -L../../../../dist/bin -lxpcom -lxpcom_core  -L../../../../dist/lib -lplds4 -lplc4 -lnspr4 -lpthread -ldl  -ldl -lm     
TestXPTCInvoke.o: In function `InvokeTestTarget::QueryInterface(nsID const&, void**)':
TestXPTCInvoke.cpp:(.text+0xabc): undefined reference to `NS_TableDrivenQI(void*, QITableEntry const*, nsID const&, void**)'
collect2: ld returned 1 exit status
gmake[1]: *** [TestXPTCInvoke] Error 1
gmake[1]: Leaving directory `/home/dwmw2/working/mozilla/obj-powerpc-unknown-linux-gnu/xpcom/reflect/xptcall/tests'
make: *** [default] Error 2
pmac /home/dwmw2/working/mozilla/obj-powerpc-unknown-linux-gnu/xpcom/reflect/xptcall/tests $ nm -D ../../../../dist/bin/libxpcom_core.so  | grep NS_Tabl | c++filt
00030568 T NS_TableDrivenQI(void*, QITableEntry const*, nsID const&, void**)
pmac /home/dwmw2/working/mozilla/obj-powerpc-unknown-linux-gnu/xpcom/reflect/xptcall/tests $ strace -f -eopen -o foo /usr/bin/c++   -fno-rtti -fno-exceptions -Wall -Wconversion -Wpointer-arith -Wcast-align -Woverloaded-virtual -Wsynth -Wno-ctor-dtor-privacy -Wno-non-virtual-dtor -Wno-long-long -pedantic -fshort-wchar -pthread -pipe  -DNDEBUG -DTRIMMED -O -o TestXPTCInvoke TestXPTCInvoke.o  -lpthread    -L../../../../dist/bin -L../../../../dist/lib -L../../../../dist/bin -lxpcom -lxpcom_core  -L../../../../dist/lib -lplds4 -lplc4 -lnspr4 -lpthread -ldl  -ldl -lm 
TestXPTCInvoke.o: In function `InvokeTestTarget::QueryInterface(nsID const&, void**)':
TestXPTCInvoke.cpp:(.text+0xabc): undefined reference to `NS_TableDrivenQI(void*, QITableEntry const*, nsID const&, void**)'
collect2: ld returned 1 exit status
pmac /home/dwmw2/working/mozilla/obj-powerpc-unknown-linux-gnu/xpcom/reflect/xptcall/tests $ grep xpcom_core foo
31013 open("../../../../dist/bin/libxpcom_core.so", O_RDONLY|O_LARGEFILE) = 12
31013 open("../../../../dist/bin/libxpcom_core.so", O_RDONLY|O_LARGEFILE) = 7

Comment 7

[David Woodhouse]

OK, I disabled the visibility **** (which wasn't very easy -- unless I'm missing something the easiest way to do that is just to edit config.cache).

Now I have successfully tested trunk builds on ppc32 and ppc64 with the latest patch.

Comment 8

[David Woodhouse]

I've finished fi{nd,x}ing bugs in this now, and the Gentoo folks are using it too. Please review and apply.

Comment 9

[Mike Kaply [:mkaply]]

Comment on attachment 246397 [details] [diff] [review]
Hopefully final version of patch.

timeless isn't the right person to review this I believe.

Comment 10

[Mike Kaply [:mkaply]]

So as far as your work shows, this would keep Firefox from working on any Linux PPC platform?

I'm wondering how yellow dog linux - 

http://www.terrasoftsolutions.com/products/ydl/apps.shtml

claims Firefox functionality...

Comment 11

[Benjamin Smedberg]

Comment on attachment 246397 [details] [diff] [review]
Hopefully final version of patch.

timeless is owner of xptcall

Comment 12

[David Woodhouse]

(In reply to comment #10)
> So as far as your work shows, this would keep Firefox from working on any Linux
> PPC platform?

No, it works fine on PPC32 -- at least it did until the recent changes on the trunk. My patch contains a small fix to update the existing PPC32 support so it works again on the trunk, and also provides new support for 64-bit userspace on PPC.

Nobody really uses PPC64 userspace -- it's mostly pointless. 64-bit longs and pointers are just bloat for most use cases, so userspace is all 32-bit. We only ship 64-bit versions of _development_ packages, mostly libraries, just in _case_ someone has an application which actually benefits from 64-bit address space or 64-bit arithmetic. Thus, it's useful for xulrunner to be available in 64-bit mode although the _browser_ should always be 32-bit.

The Gentoo folks don't have a proper biarch environment, so still run everything 64-bit; they'll benefit from proper 64-bit support for the browser --  but most distributions don't need it for the browser itself.

PPC64 support is done for completeness -- but I would strongly recommend that you make sure distributions using the 'firefox' name use the 32-bit version of firefox where their userspace is predominantly 32-bit. Fedora and RHEL currently seem to be shipping 64-bit firefox despite the fact that the _plugins_ they ship are 32-bit, and the fact that the other available plugins (like Java and RealPlayer) are only available in 32-bit. That inconsistency is a very bad thing, IMO.

In fact FC6 was shipped, with an open 'FC6Blocker' bug, with a 64-bit firefox which was _known_ to be totally non-functional -- this was before I wrote this patch. There seems to be resistance to the simple and obvious fix of just making the /usr/bin/firefox shell script favour the 32-bit version in /usr/lib/firefox-$VER/firefox-bin instead of the 64-bit one in /usr/lib64/firefox-$VER/firefox-bin. So Fedora _still_ ships an 'official' firefox which just silently exits when you run it. How compatible is that with the trademark policy?

Comment 13

[o586806]

I can confirm that this patch makes it possible to use Firefox 2.0 on a ppc64 machine with 64bit kernel and 64bit userland.

unfortunately it will not compile on a 64bit kernel with 32bit userland, because `uname -r` returns ppc64 and so 64bit assembler code gets compiled. I'll attach a patch which might fix this (it compiles, but I haven't tried if the resulting binary runs due to a lack of graphical output on that machine running 64bit kernel + 32bit userland)

Comment 14

[David Woodhouse]

We normally build 32-bit firefox in an environment where 'uname -m' returns 'ppc', by using the 'setarch' tool.

Comment 15

[o586806]

Attachment: patch for ppc64 with 32bit userland

this patch does something similar to the Makefile code in mozilla/security/coreconf/Linux.mk (this code was introduced in bug #266123)

Comment 16

[o586806]

(In reply to comment #14)
> We normally build 32-bit firefox in an environment where 'uname -m' returns
> 'ppc', by using the 'setarch' tool.

I see.. so my code is wrong, or just not needed, because a "normal" build env returns 'ppc'? I mean, that pice of check cannot hurt, can it?

Comment 17

[David Woodhouse]

Your patch will make 64-bit builds of Firefox use 32-bit assembly unless USE_64 is defined. That's the wrong condition. On 64-bit hardware, you can build for either 32-bit or 64-bit by default; it depends on how your toolchain was built.

If you want to make the mozilla build adapt automatically, then do it based on BITS_PER_LONG or conditional on whether __powerpc64__ is defined, or something. Certainly not USE_64, which is a Gentoo-specific thing, isn't it?

I don't think it's absolutely necessary to do so anyway -- we've _always_ needed to use 'setarch ppc' to build for ppc32 on ppc64 hardware, because of bug #361413 (in which the build system silently spits out a non-functional binary because 'Linuxppc64' isn't recognised.)

Comment 18

[o586806]

(In reply to comment #17)
> Your patch will make 64-bit builds of Firefox use 32-bit assembly unless USE_64
> is defined. That's the wrong condition. On 64-bit hardware, you can build for
> either 32-bit or 64-bit by default; it depends on how your toolchain was built.
> 
> If you want to make the mozilla build adapt automatically, then do it based on
> BITS_PER_LONG or conditional on whether __powerpc64__ is defined, or something.
> Certainly not USE_64, which is a Gentoo-specific thing, isn't it?


no, USE_64 is not a Gentoo specific thing and I think this is a similar problem as in bug #266123 comment #31 to #33. When I wrote that comment I was sure that USE_64 is not correct, but I was told otherwise, so now I now claimed it should be used and are also told otherwise.. I think I'm not understanding the situation, but this should be handled consistent in every part of mozilla, should it?


> I don't think it's absolutely necessary to do so anyway -- we've _always_
> needed to use 'setarch ppc' to build for ppc32 on ppc64 hardware, because of
> bug #361413 (in which the build system silently spits out a non-functional
> binary because 'Linuxppc64' isn't recognised.)

OK, sounds good, will remember! :-)

Comment 19

[David Woodhouse]

(In reply to comment #18)
> no, USE_64 is not a Gentoo specific thing and I think this is a similar 
> problem as in bug #266123 comment #31 to #33. 

Hm, it's probably OK then.

Comment 20

[David Woodhouse]

Looking closer at the patch in bug 266123, I don't think it's sufficient. If your toolchain on a 64-bit host is configured to output 32-bit by default, you're _also_ going to need the '-melf64ppc' argument to the linker.

This kind of thing bites many packages, which is why when building for 64-bit Fedora/PPC we use a pure 64-bit environment where the compiler defaults to 64-bit -- and for 32-bit Fedora/PPC we use 'setarch ppc'.

I think it's probably best to leave it as it was in my patch -- use the 32-bit stubs if 'uname -m' gives 'ppc', and the 64-bit version if it gives 'ppc64'.

The only better alternative I can see would be to detect the output of the compiler automatically and use the appropriate assembly stubs -- as I said before, you could possibly do that by checking if __powerpc64__ is defined.

Comment 21

[o586806]

(In reply to comment #20)
> Looking closer at the patch in bug 266123, I don't think it's sufficient. If
> your toolchain on a 64-bit host is configured to output 32-bit by default,
> you're _also_ going to need the '-melf64ppc' argument to the linker.

ok. full ack on this. so security/coreconf/Linux.mk and nsprpub/configure.in need fixing, too?

Comment 22

[David Woodhouse]

Attachment: Fix ABI problem.

Fix an ABI problem -- branches need to be followed by a NOP in case the function is called via the PLT.

--- firefox-trunk-ppc64.patch~  2006-11-23 13:44:36.000000000 +0000
+++ firefox-trunk-ppc64.patch   2006-12-06 18:15:46.000000000 +0000
@@ -96,7 +96,7 @@ diff -u -p -r1.7 xptcstubs_ppc_linux.cpp
          delete [] dispatchParams;
 --- /dev/null  2006-11-18 23:38:03.655619302 +0000
 +++ xpcom/reflect/xptcall/src/md/unix/xptcinvoke_asm_ppc64_linux.s     2006-11-22 12:03:56.000000000 +0000
-@@ -0,0 +1,153 @@
+@@ -0,0 +1,154 @@
 +// -*- Mode: Asm -*-
 +//
 +// The contents of this file are subject to the Netscape Public
@@ -195,6 +195,7 @@ diff -u -p -r1.7 xptcstubs_ppc_linux.cpp
 +      subi    r4,r31,(16*8)           // r4 --> FPRS
 +      addi    r7,r1,112               // r7 --> params
 +      bl      invoke_copy_to_stack
++      nop
 +
 +      // Set up to invoke function
 +
@@ -381,7 +382,7 @@ diff -u -p -r1.7 xptcstubs_ppc_linux.cpp
 +
 --- /dev/null  2006-11-18 23:38:03.655619302 +0000
 +++ xpcom/reflect/xptcall/src/md/unix/xptcstubs_asm_ppc64_linux.s      2006-11-22 11:44:54.000000000 +0000
-@@ -0,0 +1,101 @@
+@@ -0,0 +1,102 @@
 +// -*- Mode: Asm -*-
 +//
 +// The contents of this file are subject to the Netscape Public
@@ -472,6 +473,7 @@ diff -u -p -r1.7 xptcstubs_ppc_linux.cpp
 +                              // via r11 in the nsXPTCStubBase::StubXX() call
 +
 +      bl      PrepareAndDispatch
++      nop
 +
 +      ld      1,0(r1)                         // restore stack
 +      ld      r0,16(r1)                       // restore LR

Comment 23

[David Woodhouse]

(In reply to comment #21)
> (In reply to comment #20)
> > Looking closer at the patch in bug 266123, I don't think it's sufficient. If
> > your toolchain on a 64-bit host is configured to output 32-bit by default,
> > you're _also_ going to need the '-melf64ppc' argument to the linker.
> 
> ok. full ack on this. so security/coreconf/Linux.mk and nsprpub/configure.in
> need fixing, too?

No, I think they just need leaving alone -- or at least let them go clutter up another bug of their own rather than this one :)

Comment 24

[Steven Munroe]

The ABI requires a nop following any branch and link so the linker 
can replace the nop with a ld r2,40(r1) in the case the linker 
resolves the branch targer via a PLT call stub.

Comment 25

[Phil Ringnalda (:philor)]

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

Comment 26

[Jory A. Pratt [:anarchy]]

Comment on attachment 247694 [details] [diff] [review]
Fix ABI problem.

This is ready for the 1.8.1 branch, it has been tested and reviewed by myself and others outside the bug.

Comment 27

[Jory A. Pratt [:anarchy]]

(In reply to comment #26)
> (From update of attachment 247694 [details] [diff] [review])
> This is ready for the 1.8.1 branch, it has been tested and reviewed by myself
> and others outside the bug.
> 

I mean MOZILLA_1_8_BRANCH. sorry about that.

Comment 28

[timeless]

Comment on attachment 247694 [details] [diff] [review]
Fix ABI problem.

for reference. patches aren't ready for branches until they're in trunk or unless they're given special dispensation.

patches are ready for review when people say that they are.

some questions and comments in no particular order:

1. are the copyright/attribution lines correct?
especially for new files, are they really copies of other files, or are they really new?
if they're copies, it'd be good for there to be some hint as to which file you're copying, preferably in such a way that i can actually have a cvs copy.

2. you're using //'s in .s files, i expect to see #'s (or ;'s?), is my expectation wrong?
(so much for consistency, the linux ppc files use // other ppc files use other flavors.

3. you're using tabs in your .s/.cpp files, unless there's a specific reason to do so, please don't. they don't work well and we generally only use them as needed (and to my knowledge, that is just one part of makefile glue, not even the part most people touch, since we're variable based, and most people shouldn't be writing rules). i'm relatively ok with seeing them on the leading side of the file where things are fixed, but i'd especially rather not have them on the trailing side where they really will behave randomly (especially before the \s in the macro definitions).

from a quick check, we tend to not use tabs in .s files, not even for leading whitespace. we do tend to use #s for comments in .s files (although in some places we use /* */, or even //).

4. you left some references to XPTC_InvokeByIndex

+invoke_count_words(PRUint32 paramCount, nsXPTCVariant* s)
+  return PRUint32(((paramCount * 2) + 3) & ~3);

doesn't match the indent style you're using (4 space)

+                gpregs[i] = tempu64;
+                *d          = tempu64;

note sure what style this is, but the =s don't align, so it's not a very useful way to add spaces after *d.

+#define PARAM_BUFFER_COUNT     16
+#define GPR_COUNT               7
+#define FPR_COUNT               13

strange alignment.

5. +// caller.  The rest of the parameters are passed in the callers stack

i presume you mean caller's not callers.
but you could mean callers', so please figure out what you mean and fix it.

6. if possible, to aid future readers, if there's a convenient reference to the ABI that specifies the convention, including it in the comment block would be appreciated.

7.
+#define GPR_COUNT               7
+        if (i >= 7)
is this the same 7? if so, could you please use the macro?

Comment 29

[David Woodhouse]

Thanks for the feedback. I'm away from home with **** network access, and won't be back until the end of the month -- I'll provide a revised patch then if nobody's beaten me to it. It's only cosmetic so any interested party should be able to clean it up, if provided with the following answers...

1. The PPC64 files were based on the PPC32 files which already existed. Most of the assembly got rewritten so I suppose I don't really need to preserve original copyrights there, but the C parts are still fairly similar to the files I copied.

2. I don't think the assembler cares. I certainly don't. I inherited this from the ppc32 files, but I'm happy to change it.

3. OK.

4. OK.

5. Must have been inherited from the ppc32 version, so will want fixing there too. I'm far too much of a pedant to have done that myself, unless my brain was melting from overexposure to assembler code. Should be "caller's".

6. Good idea. http://www.freestandards.org/spec/ELF/ppc64/ and in particular http://www.freestandards.org/spec/ELF/ppc64/PPC-elf64abi-1.9.html#FUNC-CALL

7. Er, maybe. That bit was sort of empirical. But probably, so let's change it.

Comment 30

[o586806]

Attachment: cleaned up patch

all done. (hopefully nothing wrong.. I'm not doing such things very often..)

Comment 31

[timeless]

Comment on attachment 251404 [details] [diff] [review]
cleaned up patch

one last sucky lawyer point (sorry). david: if you're claiming the new files as your own, we'd like you to offer them under MPL-trilicense instead of NPL-tri. (a comment by you about this is good enough for me to r+ a subsequent version that someone posts.)

Everything else looks fine to me. but I'd of course appreciate if someone verifies that the code still works.

oh, while I'm reading the code:

> // nsXPTCVariant* and prepared for th native ABI.  For the Linux/PPC

i presume that's "the" native? ...

anyway, fix those two things, get david to verify the license for the "new" files, and have someone vouch this still works, and i'll land the new patches (well, i'll get cvs copies and then land).

Comment 32

[David Woodhouse]

(In reply to comment #31)
> (From update of attachment 251404 [details] [diff] [review])
> one last sucky lawyer point (sorry). david: if you're claiming the new files as
> your own, we'd like you to offer them under MPL-trilicense instead of NPL-tri.
> (a comment by you about this is good enough for me to r+ a subsequent version
> that someone posts.)

That's fine by me although I would be inclined to inspect said "new" files by hand again and double-check that nothing (recognisable) of the original PPC32 files exists there before making such a change. I think we're going solely on my above recollection that "most of the assembly got rewritten" and I'd like to confirm that before actually changing the licence.

I'll take a look, again if nobody else does. I should get home on Saturday and have a slightly better chance of responding more usefully.

Comment 33

[o586806]

Attachment: cleaned up patch v2

I've introduced a little typo (missing space) with my changes: There was this line

".if"#n" < 10 \n\t"

instead of this

".if "#n" < 10 \n\t"

I've corrected the "th" instead of "the" typo, too. The attached patch now applies cleanly to HEAD and compiles.

Comment 34

[timeless]

Comment on attachment 252441 [details] [diff] [review]
cleaned up patch v2

:(

you didn't fix the licenses. 
http://www.mozilla.org/MPL/boilerplate-1.1/mpl-tri-license-c

also, bugzilla and i would appreciate it if people would consistently use cvs diff -upN (use cvsdo add if you don't have cvs write access).

Anyway, not much point in me trying to do anything with this until there's sign off. (it's also best if i can easily compare things, yes, i'm lazy, but i also have hundreds of other things to do).

Comment 35

[Jory A. Pratt [:anarchy]]

(In reply to comment #34)
> (From update of attachment 252441 [details] [diff] [review])
> :(
> 
> you didn't fix the licenses. 
> http://www.mozilla.org/MPL/boilerplate-1.1/mpl-tri-license-c
> 
> also, bugzilla and i would appreciate it if people would consistently use cvs
> diff -upN (use cvsdo add if you don't have cvs write access).
> 
> Anyway, not much point in me trying to do anything with this until there's sign
> off. (it's also best if i can easily compare things, yes, i'm lazy, but i also
> have hundreds of other things to do).
> 

This could not be landed anyways .. it fails to compile.

Comment 36

[David Woodhouse]

(In reply to comment #35)
> This could not be landed anyways .. it fails to compile.

Can you eludicate? It works for me (autocrap breakage aside).

I'm home now and can look at this.

I'd be very much happier if the trademark guidelines were sorted out first though -- I'd like to make sure that distributions with mostly 32-bit userspace are forbidden from shipping with a 64-bit officially-branded firefox™ as default, since that would be inconsistent with both the 32-bit plugin packages in the rest of the same distribution and the externally-available plugins such as Java and RealPlayer. Ideally I shouldn't really have to mention that since it's so obvious, but there's one distribution which _already_ insists on running the 64-bit firefox™ by default, even though it doesn't work at all yet! All attempts to get them to run the 32-bit version by default, in line with the rest of the distribution, have so far failed.

Comment 37

[o586806]

Attachment: cleaned up patch v3

patch with new copyright headers in xptcinvoke_asm_ppc64_linux.s and xptcstubs_asm_ppc64_linux.s.

also created with "cvs diff -upN" (for those files already in cvs, the others didn't diff'ed, because I cannot check them in; cvsdo didn't catched them, too. so these are created normal diff).

Comment 38

[o586806]

(In reply to comment #36)
> I'd be very much happier if the trademark guidelines were sorted out first
> though -- I'd like to make sure that distributions with mostly 32-bit userspace
> are forbidden from shipping with a 64-bit officially-branded firefox™ as
> default, since that would be inconsistent with both the 32-bit plugin packages
> in the rest of the same distribution and the externally-available plugins such
> as Java and RealPlayer. Ideally I shouldn't really have to mention that since
> it's so obvious, but there's one distribution which _already_ insists on
> running the 64-bit firefox™ by default, even though it doesn't work at all
> yet! All attempts to get them to run the 32-bit version by default, in line
> with the rest of the distribution, have so far failed.

could you please elaborate? I think you don't mean Gentoo/PPC64, because we have full 64bit userspace.

This situation with this one distribution shouldn't hold the patch back I think. It's a problem of the distribution and *not* a problem with the patch, is it?

Comment 39

[David Woodhouse]

No, I mean Fedora. There was an FC6Blocker bug filed before the release of Fedora Core 6 and it _still_ shipped with a firefox™ which just exited silently because it was totally broken. And they're still shipping broken updates.

Comment 40

[Jory A. Pratt [:anarchy]]

(In reply to comment #38)
> could you please elaborate? I think you don't mean Gentoo/PPC64, because we
> have full 64bit userspace.
> 
> This situation with this one distribution shouldn't hold the patch back I
> think. It's a problem of the distribution and *not* a problem with the patch,
> is it?
> 

Gentoo also does not use a CVS version ... the patch has to work on TRUNK before it will be commited on any of the branches.

Comment 41

[o586806]

(In reply to comment #40)
> Gentoo also does not use a CVS version ... the patch has to work on TRUNK
> before it will be commited on any of the branches.

it does work on TRUNK: http://www.unixforces.net/~markus/screenshots/screenshot-2007-01-29.png

could you please post the error you get. please note that you have to export CFLAGS="-mminimal-toc" and CXXFLAGS="-mminimal-toc" (something is broken in our toolchain...)

Comment 42

[o586806]

(In reply to comment #32)
> That's fine by me although I would be inclined to inspect said "new" files by
> hand again and double-check that nothing (recognisable) of the original PPC32
> files exists there before making such a change. I think we're going solely on
> my above recollection that "most of the assembly got rewritten" and I'd like to
> confirm that before actually changing the licence.

well.. I want this patch to be committed *soon* ;-)

So I decided to compare those .s files by hand to make sure they are "new". As I don't know asm, all I can tell is that there are a few lines which are the same, but that seem to be some generic instructions. for example those ".set r0,0; ..." instructions are equal. Conclusion: I would declare those "new" files as new.

Could someone confirm this? Then this patch can be applied, can it? Or are there any other issues left?

Comment 43

[timeless]

i'm willing to take your word on the comparison. i'll deal w/ the cvs bits.

although if someone could remind me if there are any cvs copies intended. i believe there aren't and that it's just a simple commit.

sorry, it's 7am and i'm short a sleep cycle. next week's an interesting week, there are all sorts of holy/holi-days floating around, hopefully there will be time for me to get such things addressed.

Comment 44

[David Woodhouse]

Just a simple commit, yes.

Thanks.

Comment 45

[o586806]

still not in cvs :-/

Comment 46

[o586806]

I hate this kind of postings, but "still noch in CVS :-/"

and by the way: why is this bug assigned to me? I do neither am a Mozilla Developer nor have I CVS access.

Comment 47

[o586806]

Attachment: cleaned up patch v4

parts of the patch (namely those for linux/ppc) have already been applied. patch now applies cleanly to HEAD again.

Comment 48

[o586806]

this is still just a simple commit. and I would like to see this in firefox 3 :-)

Comment 49

[David Woodhouse]

If/when you finally get round to committing this, pass it through s/NS_InvokeByIndex/NS_InvokeByIndex_P/ first.

cf. bug #375449.

Note also that for xulrunner on Fedora rawhide I also seem to see $(OS_ARCH) being set to 'powerpc64' and not 'ppc64' so the makefile needs adjusting accordingly (and likewise for ppc/powerpc). I'm not sure if that's due to a change in Mozilla's configure scripts or some other strangeness in my build environment. 

Comment 50

[o586806]

Attachment: another update

another updated patch, which now includes changes from previous comment

we (gentoo) have been using Linuxpowerpc(64) instead of Linuxppc(64) for quite some time now. This is on ppc, 32bit ppc64 and 64bit ppc64 machines.

Comment 51

[Raúl Porcel]

Attachment: ppc64.patch

The previous patch doesn't apply anymore since the powerpc fix was fixed in another bug.

I've changed that part and i did the patch as a cvs diff :)

Comment 52

[timeless]

Comment on attachment 290716 [details] [diff] [review]
ppc64.patch

i had this open earlier, which presumably meant i intended to comment about /something/ unfortunately i force quit my browser, so I'm not sure what I meant to say :(.

the only thing that jumps out is the lack of a space after == in each of these lines (it'd be nice if it were included)
+            else if (type ==nsXPTType::T_I8)

Comment 53

[o586806]

Attachment: added whitespace after "type =="

Comment 54

[Reed Loden [:reed]]

Comment on attachment 293483 [details] [diff] [review]
added whitespace after "type =="

Support PPC64.

Comment 55

[Reed Loden [:reed]]

Checking in xpcom/reflect/xptcall/src/md/unix/Makefile.in;
/cvsroot/mozilla/xpcom/reflect/xptcall/src/md/unix/Makefile.in,v  <--  Makefile.in
new revision: 1.99; previous revision: 1.98
done
RCS file: /cvsroot/mozilla/xpcom/reflect/xptcall/src/md/unix/xptcinvoke_asm_ppc64_linux.s,v
done
Checking in xpcom/reflect/xptcall/src/md/unix/xptcinvoke_asm_ppc64_linux.s;
/cvsroot/mozilla/xpcom/reflect/xptcall/src/md/unix/xptcinvoke_asm_ppc64_linux.s,v  <--  xptcinvoke_asm_ppc64_linux.s
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/xpcom/reflect/xptcall/src/md/unix/xptcinvoke_ppc64_linux.cpp,v
done
Checking in xpcom/reflect/xptcall/src/md/unix/xptcinvoke_ppc64_linux.cpp;
/cvsroot/mozilla/xpcom/reflect/xptcall/src/md/unix/xptcinvoke_ppc64_linux.cpp,v  <--  xptcinvoke_ppc64_linux.cpp
initial revision: 1.1
done
Checking in xpcom/reflect/xptcall/src/md/unix/xptcinvoke_ppc_netbsd.cpp;
/cvsroot/mozilla/xpcom/reflect/xptcall/src/md/unix/xptcinvoke_ppc_netbsd.cpp,v  <--  xptcinvoke_ppc_netbsd.cpp
new revision: 1.4; previous revision: 1.3
done
RCS file: /cvsroot/mozilla/xpcom/reflect/xptcall/src/md/unix/xptcstubs_asm_ppc64_linux.s,v
done
Checking in xpcom/reflect/xptcall/src/md/unix/xptcstubs_asm_ppc64_linux.s;
/cvsroot/mozilla/xpcom/reflect/xptcall/src/md/unix/xptcstubs_asm_ppc64_linux.s,v  <--  xptcstubs_asm_ppc64_linux.s
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/xpcom/reflect/xptcall/src/md/unix/xptcstubs_ppc64_linux.cpp,v
done
Checking in xpcom/reflect/xptcall/src/md/unix/xptcstubs_ppc64_linux.cpp;
/cvsroot/mozilla/xpcom/reflect/xptcall/src/md/unix/xptcstubs_ppc64_linux.cpp,v  <--  xptcstubs_ppc64_linux.cpp
initial revision: 1.1
done