The default bug view has changed. See this FAQ.

parisc-linux support for xpcom

RESOLVED FIXED in mozilla1.9beta3

Status

()

Core
XPCOM
--
major
RESOLVED FIXED
12 years ago
9 years ago

People

(Reporter: Randolph Chung, Assigned: Raúl Porcel)

Tracking

Trunk
mozilla1.9beta3
Other
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 6 obsolete attachments)

(Reporter)

Description

12 years ago
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 :-)
(Reporter)

Comment 1

12 years ago
Created attachment 178194 [details] [diff] [review]
Patch
(Reporter)

Comment 2

12 years ago
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.
Rudolph, is that patch at a point where you'd want it reviewed and maybe checked in?
(Reporter)

Comment 4

12 years ago
Randolph, not Rudolph! :)

Yes, review and check in will be appreciated.
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...
Attachment #178194 - Flags: superreview?(shaver)
Attachment #178194 - Flags: review?(darin)

Comment 6

12 years ago
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.
Attachment #178194 - Flags: review?(darin) → review-
ifdef GNU_CC
...
endif
Attachment #178194 - Flags: superreview?(shaver)
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/
Status: UNCONFIRMED → NEW
Ever confirmed: true
Assignee: dougt → nobody
QA Contact: xpcom
Randolph, are you still interested in having this patch committed to the Mozilla tree?

Updated

10 years ago
Duplicate of this bug: 365744

Comment 11

10 years ago
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

10 years ago
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.
Attachment #250360 - Flags: review?(timeless)

Comment 13

10 years ago
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.
Attachment #250360 - Flags: review?(timeless) → review-
(Assignee)

Comment 14

10 years ago
Created attachment 290445 [details] [diff] [review]
hppa.patch

Just an updated patch for 1.9.
(Assignee)

Comment 15

10 years ago
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?
Attachment #290445 - Attachment is obsolete: true

Comment 16

10 years ago
(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.
> 

(Assignee)

Comment 17

10 years ago
(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.

Attachment #290452 - Flags: superreview?(benjamin)
Attachment #290452 - Flags: review?(timeless)
Attachment #290452 - Flags: review?(benjamin)
Assignee: nobody → armin76
Attachment #250360 - Attachment is obsolete: true
Attachment #178194 - Attachment is obsolete: true
(Assignee)

Comment 18

10 years ago
Created attachment 290533 [details] [diff] [review]
hppa.patch

Patch done with cvs diff :)
Attachment #290452 - Attachment is obsolete: true
Attachment #290452 - Flags: superreview?(benjamin)
Attachment #290452 - Flags: review?(timeless)
Attachment #290452 - Flags: review?(benjamin)
Attachment #290533 - Flags: superreview?(benjamin)
Attachment #290533 - Flags: review?(timeless)
Attachment #290533 - Flags: review?(benjamin)
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
Attachment #290533 - Flags: superreview?(benjamin)
Attachment #290533 - Flags: superreview+
Attachment #290533 - Flags: review?(benjamin)
(Assignee)

Comment 20

9 years ago
Created attachment 291645 [details] [diff] [review]
hppa.patch

Added the GNU_CXX check :)
Attachment #290533 - Attachment is obsolete: true
Attachment #291645 - Flags: superreview?(benjamin)
Attachment #290533 - Flags: review?(timeless)
Comment on attachment 291645 [details] [diff] [review]
hppa.patch

Still need review from timeless.
Attachment #291645 - Flags: review?(timeless)

Comment 22

9 years ago
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
(Reporter)

Comment 23

9 years ago
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
(Assignee)

Comment 24

9 years ago
Created attachment 291740 [details] [diff] [review]
hppa.patch

I hope this is enough :)
Attachment #291645 - Attachment is obsolete: true
Attachment #291740 - Flags: superreview?(benjamin)
Attachment #291740 - Flags: review?(timeless)
Attachment #291645 - Flags: superreview?(benjamin)
Attachment #291645 - Flags: review?(timeless)
Attachment #291740 - Flags: superreview?(benjamin) → superreview+

Comment 25

9 years ago
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.
Attachment #291740 - Flags: review?(timeless) → review+
(In reply to comment #25)
> someone should just land your next patch.

Once the new patch gets approved, of course.
(Assignee)

Comment 27

9 years ago
Created attachment 294644 [details] [diff] [review]
hppa.patch

There we go
(Assignee)

Updated

9 years ago
Attachment #294644 - Flags: approval1.9?
(Assignee)

Comment 28

9 years ago
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 on attachment 294644 [details] [diff] [review]
hppa.patch

a=beltzner for 1.9
Attachment #294644 - Flags: approval1.9? → approval1.9+
(Assignee)

Updated

9 years ago
Keywords: checkin-needed
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
Status: NEW → RESOLVED
Last Resolved: 9 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9 M11
You need to log in before you can comment on or make changes to this bug.