Last Comment Bug 287150 - parisc-linux support for xpcom
: parisc-linux support for xpcom
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: XPCOM (show other bugs)
: Trunk
: Other Linux
: -- major (vote)
: mozilla1.9beta3
Assigned To: Raúl Porcel
:
Mentors:
: 365744 (view as bug list)
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2005-03-21 18:43 PST by Randolph Chung
Modified: 2007-12-27 18:58 PST (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch (7.82 KB, patch)
2005-03-21 18:44 PST, Randolph Chung
darin.moz: review-
Details | Diff | Splinter Review
cleaned up patch (7.19 KB, patch)
2007-01-03 10:23 PST, Jory A. Pratt
timeless: review-
Details | Diff | Splinter Review
hppa.patch (8.87 KB, patch)
2007-11-27 12:45 PST, Raúl Porcel
no flags Details | Diff | Splinter Review
hppa.patch (12.69 KB, patch)
2007-11-27 13:53 PST, Raúl Porcel
no flags Details | Diff | Splinter Review
hppa.patch (13.94 KB, patch)
2007-11-28 07:24 PST, Raúl Porcel
benjamin: superreview+
Details | Diff | Splinter Review
hppa.patch (13.94 KB, patch)
2007-12-05 04:17 PST, Raúl Porcel
no flags Details | Diff | Splinter Review
hppa.patch (13.84 KB, patch)
2007-12-05 13:53 PST, Raúl Porcel
timeless: review+
benjamin: superreview+
Details | Diff | Splinter Review
hppa.patch (13.84 KB, patch)
2007-12-27 02:51 PST, Raúl Porcel
mbeltzner: approval1.9+
Details | Diff | Splinter Review

Description Randolph Chung 2005-03-21 18:43:46 PST
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 2005-03-21 18:44:53 PST
Created attachment 178194 [details] [diff] [review]
Patch
Comment 2 Randolph Chung 2005-03-21 18:50:01 PST
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 [:bz] 2005-04-13 00:08:47 PDT
Rudolph, is that patch at a point where you'd want it reviewed and maybe checked in?
Comment 4 Randolph Chung 2005-04-13 01:56:21 PDT
Randolph, not Rudolph! :)

Yes, review and check in will be appreciated.
Comment 5 Boris Zbarsky [:bz] 2005-04-13 08:29:12 PDT
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 2005-04-14 16:52:12 PDT
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) 2005-04-14 17:42:45 PDT
ifdef GNU_CC
...
endif
Comment 8 Gervase Markham [:gerv] 2005-09-27 02:02:53 PDT
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: gavin@gavinsharp.com] 2006-09-11 23:06:18 PDT
Randolph, are you still interested in having this patch committed to the Mozilla tree?
Comment 10 timeless 2007-01-03 07:31:22 PST
*** Bug 365744 has been marked as a duplicate of this bug. ***
Comment 11 Jory A. Pratt 2007-01-03 10:23:36 PST
Created attachment 250360 [details] [diff] [review]
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 2007-01-03 10:35:18 PST
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 2007-01-13 14:53:27 PST
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 2007-11-27 12:45:49 PST
Created attachment 290445 [details] [diff] [review]
hppa.patch

Just an updated patch for 1.9.
Comment 15 Raúl Porcel 2007-11-27 13:53:16 PST
Created attachment 290452 [details] [diff] [review]
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 2007-11-27 14:49:21 PST
(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 2007-11-28 06:36:53 PST
(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 2007-11-28 07:24:35 PST
Created attachment 290533 [details] [diff] [review]
hppa.patch

Patch done with cvs diff :)
Comment 19 Benjamin Smedberg [:bsmedberg] 2007-11-28 13:45:02 PST
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 2007-12-05 04:17:37 PST
Created attachment 291645 [details] [diff] [review]
hppa.patch

Added the GNU_CXX check :)
Comment 21 Reed Loden [:reed] (use needinfo?) 2007-12-05 04:22:07 PST
Comment on attachment 291645 [details] [diff] [review]
hppa.patch

Still need review from timeless.
Comment 22 timeless 2007-12-05 10:41:45 PST
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 2007-12-05 11:43:37 PST
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 2007-12-05 13:53:53 PST
Created attachment 291740 [details] [diff] [review]
hppa.patch

I hope this is enough :)
Comment 25 timeless 2007-12-26 14:25:09 PST
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] (use needinfo?) 2007-12-26 14:27:13 PST
(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 2007-12-27 02:51:13 PST
Created attachment 294644 [details] [diff] [review]
hppa.patch

There we go
Comment 28 Raúl Porcel 2007-12-27 10:27:51 PST
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] 2007-12-27 12:22:20 PST
Comment on attachment 294644 [details] [diff] [review]
hppa.patch

a=beltzner for 1.9
Comment 30 Reed Loden [:reed] (use needinfo?) 2007-12-27 18:58:24 PST
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

Note You need to log in before you can comment on or make changes to this bug.