287150 - parisc-linux support for xpcom

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Randolph Chung]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.6) Gecko/20050225 Firefox/1.0.1
Build Identifier: Firefox/1.0.1 (applies to all versions of mozilla/firefox)

Currently xpcom does not support parisc-linux. The attached patch, contributed
by Ivar with some additional modifications by myself, adds support for
parisc-linux. This makes firefox work on this platform.

Reproducible: Always

Steps to Reproduce:
1. Run firefox


Actual Results:  
Application starts up but none of the buttons/menus work


Expected Results:  
It should work :-)

Comment 1

[Randolph Chung]

Attachment: Patch

Comment 2

[Randolph Chung]

This patch is adapted from the existing hpux support. parisc-linux doesn't
support multiple spaces, so we've had to change the code a bit.

parisc support is compiled with -O0 because the code relies on the compiler not
making certain optimizations. More work is ongoing to clean up this patch further.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Rudolph, is that patch at a point where you'd want it reviewed and maybe checked in?

Comment 4

[Randolph Chung]

Randolph, not Rudolph! :)

Yes, review and check in will be appreciated.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 178194 [details] [diff] [review]
Patch

That's what I get for looking at bugs that late at night.  :(  Sorry for
getting the name wrong, Randolph.

Darin, shaver, could you check this out?  bsmedberg says this sort of thing
would generally need your overall ok; if you're fine with it I can land it...

Comment 6

[Darin Fisher]

Comment on attachment 178194 [details] [diff] [review]
Patch

mozilla-firefox-1.0.1-new/xpcom/reflect/xptcall/src/md/unix/Makefile.in
...
>+ifeq ($(OS_ARCH),Linux)
>+ifneq (,$(filter parisc parisc64,$(OS_TEST)))
>+ifeq ($(CC),gcc)

This doesn't seem like the right way to test for GCC, but ok.


>+CPPSRCS		:= xptcinvoke_pa32.cpp xptcstubs_pa32.cpp
>+ASFILES		:= xptcstubs_asm_parisc_linux.s xptcinvoke_asm_parisc_linux.s
>+CXXFLAGS	+= -O0

Might be nice to make these line up columnwise.


mozilla-firefox-1.0.1-new/xpcom/reflect/xptcall/src/md/unix/xptcinvoke_asm_pari
sc_linux.s
...
>+
>+;	B,L     .+8,%r2
>+;	ADDIL   L'invoke_count_bytes-$PIC_pcrel$1+4,%r2,%r1
>+;    ;    LDO     R'invoke_count_bytes-$PIC_pcrel$2+8(%r1),%r1
>+;$PIC_pcrel$1
>+;        LDSID   (%r1),%r31
>+;$PIC_pcrel$2
>+;        MTSP    %r31,%sr0

Should this commented out code maybe just be removed?  I'm
not a big fan of leaving commented out code in the source.
At least add a comment explaining why this remains.

The same thing appears elsewhere:

>+;        B,L     .+8,%r2
>+;        ADDIL   L'invoke_copy_to_stack-$PIC_pcrel$3+4,%r2,%r1
>+;        LDO     R'invoke_copy_to_stack-$PIC_pcrel$4+8(%r1),%r1
>+;$PIC_pcrel$3
>+;        LDSID   (%r1),%r31
>+;$PIC_pcrel$4
>+;        MTSP    %r31,%sr0

>+;        LDO     0(%r20),%r20   ; offset vtable by 16 bytes (g++: 8, aCC: 16)
...
>+;        B,L     .+8,%r2
>+;        ADDIL   L'$$dyncall_external-$PIC_pcrel$5+4,%r2,%r1
>+;        LDO     R'$$dyncall_external-$PIC_pcrel$6+8(%r1),%r1
>+;$PIC_pcrel$5
>+;        LDSID   (%r1),%r31
>+;$PIC_pcrel$6
>+;        MTSP    %r31,%sr0

mozilla-firefox-1.0.1-new/xpcom/reflect/xptcall/src/md/unix/xptcstubs_asm_paris
c_linux.s
...
>+;       BL      .+8,%r2
>+;       ADDIL   L'PrepareAndDispatch-$PIC_pcrel$0+4,%r2
>+;        LDO     R'PrepareAndDispatch-$PIC_pcrel$1+8(%r1),%r1
>+;$PIC_pcrel$0
>+;        LDSID   (%r1),%r31
>+;$PIC_pcrel$1
>+;        MTSP    %r31,%sr0


I did not review the code for accuracy.  I assume that it either
works or it doesn't ;-)

Please clean up the code as requested, and then I think it will
be good to go, assuming shaver agrees.

Comment 7

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

ifdef GNU_CC
...
endif

Comment 8

[Gervase Markham [:gerv]]

This is an automated message, with ID "auto-resolve01".

This bug has had no comments for a long time. Statistically, we have found that
bug reports that have not been confirmed by a second user after three months are
highly unlikely to be the source of a fix to the code.

While your input is very important to us, our resources are limited and so we
are asking for your help in focussing our efforts. If you can still reproduce
this problem in the latest version of the product (see below for how to obtain a
copy) or, for feature requests, if it's not present in the latest version and
you still believe we should implement it, please visit the URL of this bug
(given at the top of this mail) and add a comment to that effect, giving more
reproduction information if you have it.

If it is not a problem any longer, you need take no action. If this bug is not
changed in any way in the next two weeks, it will be automatically resolved.
Thank you for your help in this matter.

The latest beta releases can be obtained from:
Firefox:     http://www.mozilla.org/projects/firefox/
Thunderbird: http://www.mozilla.org/products/thunderbird/releases/1.5beta1.html
Seamonkey:   http://www.mozilla.org/projects/seamonkey/

Comment 9

[:Gavin Sharp [email: [e-mail]]]

Randolph, are you still interested in having this patch committed to the Mozilla tree?

Comment 11

[Jory A. Pratt [:anarchy]]

Attachment: cleaned up patch

I believe I have addressed all the issue only one I am not sure on it the gcc check maybe someone with more experience in that department can handle that.

Comment 12

[Jory A. Pratt [:anarchy]]

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

Well here it is with all changes other then the GCC check which I am still a bit fuzzy on.

Comment 13

[timeless]

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

+XPTC_InvokeByIndex:

Wrong name?

+	.CALL   ARGW0=GR,ARGW1=GR,ARGW2=GR ;in=24,25,26;out=28
+        .CALL   ARGW0=GR,ARGW1=GR,ARGW2=GR ;in=24,25,26

not consistently indented - AHA, it's tabs!
please don't use tabs, they're clearly not required by the file. mozilla style says No to tabs unless they're absolutely necessary for this very reason.

the asm files lack a header block, i believe we want one (especially fill in the original author field with the correct information)

+        ; but register 25-23 are not because of the arguments mismatch

i presume you mean "argument mismatch" (no s).

I don't understand what that means though :(. The comment seems to expect it's obvious. This comment here is my indication that it is *not*.

http://mxr-test.landfill.bugzilla.org/seamonkey/search?string=ifdef+GNU_CC&find=%2FMakefile&findi=&filter=&tree=seamonkey

shows the correct test. note that xptcall seems to be very bad about using it. I'll file a bug to clean it up, but i'll probably fix my end before you fix my review comments, so you should write the correct test for the next patch (obviously you should verify that it works, at least for the gcc case).

Note that without a reference to the ABI you're supporting, I really can't do much else as code reviews go.

I'll land this with those implemented. I'd of course appreciate a url to a reference for this ABI (preferably embedded in one of the .s files) so that anyone interested in verifying the correctness, or just further reading will have an easy access.

Comment 14

[Raúl Porcel]

Attachment: hppa.patch

Just an updated patch for 1.9.

Comment 15

[Raúl Porcel]

Attachment: hppa.patch

Okay, here goes a fixed patch with the requests from comment #13.

Regarding the gcc check...according to the comment it's the only compiler on hppa under linux, i guess the check is unneded?
And regarding the ABI, i have no idea either, i didn't do the patch.

Did i do the license block okay?

Comment 16

[Jory A. Pratt [:anarchy]]

(In reply to comment #15)
> Created an attachment (id=290452) [details]
> hppa.patch
> 
> Okay, here goes a fixed patch with the requests from comment #13.
> 
> Regarding the gcc check...according to the comment it's the only compiler on
> hppa under linux, i guess the check is unneded?
> And regarding the ABI, i have no idea either, i didn't do the patch.
> 
> Did i do the license block okay?

Patch needs to be against CVS, before it can be reviewed.
> 

Comment 17

[Raúl Porcel]

(In reply to comment #16)
> Patch needs to be against CVS, before it can be reviewed.

It works against CVS, that's what i updated.

Comment 18

[Raúl Porcel]

Attachment: hppa.patch

Patch done with cvs diff :)

Comment 19

[Benjamin Smedberg]

Comment on attachment 290533 [details] [diff] [review]
hppa.patch

>Index: xpcom/reflect/xptcall/src/md/unix/Makefile.in

>+#
>+# Linux/HPPA/gcc
>+#
>+ifeq ($(OS_ARCH),Linux)
>+ifneq (,$(filter hppa2.0 hppa1.1,$(OS_TEST)))
>+#ifeq ($(CC),gcc)  # Do not check for gcc since there is only this compiler on linux for hppa
>+CPPSRCS		:= xptcinvoke_pa32.cpp xptcstubs_pa32.cpp
>+ASFILES		:= xptcstubs_asm_parisc_linux.s xptcinvoke_asm_parisc_linux.s
>+#endif
>+endif
>+endif

How about, instead of commenting out the gcc check, do something like this?

ifndef GNU_CXX
$(error Unknown C++ compiler, xptcall assembly will probably be incorrect.)

sr=me with that change

Comment 20

[Raúl Porcel]

Attachment: hppa.patch

Added the GNU_CXX check :)

Comment 21

[Reed Loden [:reed]]

Comment on attachment 291645 [details] [diff] [review]
hppa.patch

Still need review from timeless.

Comment 22

[timeless]

Comment on attachment 291645 [details] [diff] [review]
hppa.patch

+++ xpcom/reflect/xptcall/src/md/unix/xptcinvoke_asm_parisc_linux.s	5 Dec 2007 12:15:13 -0000
+       .LEVEL 1.1
+       .PROC
+       .CALLINFO FRAME=72, CALLER,SAVE_RP, SAVE_SP, ENTRY_GR=3
+       .ENTRY
+        STW          %rp,-20(%sp)
+        .CALL   ARGW0=GR,ARGW1=GR,ARGW2=GR ;in=24,25,26;out=28
+        LDWM       -128(%sp),%r3
+       BV,N             (%rp)
+       NOP
+   .EXIT
+  .PROCEND  ;in=23,24,25,26;

is there some meaning to these different indentations?
(.LEVEL v. .CALL differ by one space)
(LDWM and NOP differ by one space)
(.EXIT/.PROCEND do not align with .PROC and .ENTRY)

+++ xpcom/reflect/xptcall/src/md/unix/xptcstubs_asm_parisc_linux.s	5 Dec 2007 12:15:14 -0000
+       .LEVEL  1.1
+        .EQU 128
+       .EQU 64
+        .PROC
+  .ENTRY
+        STW     %rp,-20(%sp)
+        .CALL   ARGW0=GR,ARGW1=GR,ARGW2=GR,ARGW3=GR,RTNVAL=GR ;in=23-26;out=28;
+        BL     PrepareAndDispatch, %r31
+       BV,N     (%rp)
+       NOP
+        .EXIT
+        .PROCEND        ;in=26;out=28;
+       .SIZE SharedStub, .-SharedStub

Similar questions. Ideally there'd be a single style for all the files you're adding.
+ ; frame marker takes 48 bytes,
+; SharedStub has stack size of 128 bytes

Comment 23

[Randolph Chung]

There is no reason for the space inconsistency. The code was ported from an old patch that had some funny spacing and mixing of tabs and spaces. As the patch got edited more it seems to have gotten more inconsistent.

Raul, could you please update the patch? I'm sorry that I haven't had time to work on this more to get it submitted properly.

randolph

Comment 24

[Raúl Porcel]

Attachment: hppa.patch

I hope this is enough :)

Comment 25

[timeless]

Comment on attachment 291740 [details] [diff] [review]
hppa.patch

thanks, much better 

+        BL    invoke_count_bytes,%r31
+        LDW          -148(%sp),%rp
+        LDWM       -128(%sp),%r3
+        BL     PrepareAndDispatch, %r31
+        LDW     -32(%r30),%r19

no space after !:
+  if (! info)
two space indentation:
+      return NS_ERROR_UNEXPECTED;

   if(paramCount > PARAM_BUFFER_COUNT)

+  if (! dispatchParams)
+      return NS_ERROR_OUT_OF_MEMORY;
 
   for(i = 0; i < paramCount; ++i, --args)
   {
     const nsXPTParamInfo& param = info->GetParam(i);
     if(param.IsOut() || !type.IsArithmetic())

someone should just land your next patch.

Comment 26

[Reed Loden [:reed]]

(In reply to comment #25)
> someone should just land your next patch.

Once the new patch gets approved, of course.

Comment 27

[Raúl Porcel]

Attachment: hppa.patch

There we go

Comment 28

[Raúl Porcel]

This patch just adds support for HPPA under Linux. There's no risk since this was unsupported before, so you couldn't build it with HPPA under Linux, with the patch everything works as it should.

Comment 29

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 294644 [details] [diff] [review]
hppa.patch

a=beltzner for 1.9

Comment 30

[Reed Loden [:reed]]

Thanks for the patch!

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.98; previous revision: 1.97
done
RCS file: /cvsroot/mozilla/xpcom/reflect/xptcall/src/md/unix/xptcinvoke_asm_parisc_linux.s,v
done
Checking in xpcom/reflect/xptcall/src/md/unix/xptcinvoke_asm_parisc_linux.s;
/cvsroot/mozilla/xpcom/reflect/xptcall/src/md/unix/xptcinvoke_asm_parisc_linux.s,v  <--  xptcinvoke_asm_parisc_linux.s
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/xpcom/reflect/xptcall/src/md/unix/xptcstubs_asm_parisc_linux.s,v
done
Checking in xpcom/reflect/xptcall/src/md/unix/xptcstubs_asm_parisc_linux.s;
/cvsroot/mozilla/xpcom/reflect/xptcall/src/md/unix/xptcstubs_asm_parisc_linux.s,v  <--  xptcstubs_asm_parisc_linux.s
initial revision: 1.1
done
Checking in xpcom/reflect/xptcall/src/md/unix/xptcstubs_pa32.cpp;
/cvsroot/mozilla/xpcom/reflect/xptcall/src/md/unix/xptcstubs_pa32.cpp,v  <--  xptcstubs_pa32.cpp
new revision: 1.4; previous revision: 1.3
done