382214 - XPTCall SH4 port completed!

Status: RESOLVED INCOMPLETE

(Core :: XPCOM, defect)

Description

[M. Onur Okyay]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.0.11) Gecko/20070312 Firefox/1.5.0.11
Build Identifier: firefox 2.0.0.1

Dear All,

I just completed the port of xptcall to sh4 architecture. I tested it on an ST7109 platform and it works fine. I would like you to review and check this in. 



Reproducible: Always

Steps to Reproduce:
1. Copy the attached files to mozilla/xpcom/reflect/xptcall/src/md/unix
2. Modify the makefile accordingly
3. Disable optimize by adding -O0 to compile flags in makefile.
Actual Results:  
A working firefox on SH4 

Expected Results:  
A working firefox on SH4

Comment 1

[M. Onur Okyay]

Attachment: xptcinvoke for SH4

Comment 2

[M. Onur Okyay]

Attachment: xptcstubs for SH4

Comment 3

[timeless]

Comment on attachment 266346 [details]
xptcinvoke for SH4

is this file essentially new? If so it shouldn't list Netscape and it shouldn't be (c) 1998.

please don't use tabs.

int *freg32;
int* target;

please be consistent about where the * goes. I can check to see which way we favor (I sure hope we favor one).

		// It doesn't matter to return a local pointer here, as it will only be used by XPTC_InvokeByIndex

This worries me (and is poor English).

struct InvokeLocals

that's not really how we name structs

http://mxr.mozilla.org/seamonkey/search?string=struct&find=xptcal&findi=&filter=%5B%5Ef%21%5D+struct+

I think given that you're using it the same way as the others use my_params_struct, it'd be good to share the name. (Yes, I can see the properties aren't identical.)

	pLocals->n = (int)invoke_count_words(paramCount, params) * 4 ;
	pLocals->copyFunc = (int)invoke_copy_to_stack;

please no space before ;.


	"mov r9, r1 \n\t"
	"mov r10, r2 \n\t"
	"mov r11, r3  \n\t"

strange alignment of \s

	//Prepare call frame for method
	"mov.l 	@r8, r4\n\t" //get that
	"mov.l   @r4, r4\n\t"  //get vtable

strange indentation of @r8/@r4

the simple casts using (int) for InvokeLocals kinda worries me, I suppose given that this is going to have to be redone for any other system it's not a big deal.

Comment 4

[timeless]

Comment on attachment 266347 [details]
xptcstubs for SH4

#error "This code is for Linux SH4 only. Please check if it works for you, too.\nDepends strongly on gcc behaviour."

it'd be nice if you indicated which versions [just min/max] of gcc will / won't work (2.94/2.95/3.0/3.1/3.2/3.3/3.4).

You can just do that in this bug.

    NS_ASSERTION(dispatchParams,"no place for params");

i know some of our code does this, but it's wrong. that alloc can fail. oh well.

my comments from the other file apply to this file as well.

Comment 5

[M. Onur Okyay]

Attachment: XPTCInvoke for SH4 processor(Modified)

I reviewed your comments and modified the file accordingly. Hope you like it :)

Comment 6

[M. Onur Okyay]

Attachment: XPTCStubs for SH4 processor(Modified)

I reviewed your comments and modified the file accordingly. Hope you like it :)

Comment 7

[M. Onur Okyay]

I modified both files according to your requests. However,

* I didn't change the file header. It still lists Netscape. Although this is a new file, the core design still belongs to Netscape. So I didn't touch that. Also all the porting files have the same header. But I am not sure whether Netscape wrote them all.
* To comment#3: Returning a local pointer is not a problem in this case as this is a processor specific code. Don't worry, I know what the compiler generates :) That function is only called internally in this file and the returned buffer is used immediately.

Thanks for your attention.

Comment 8

[timeless]

Comment on attachment 268057 [details]
XPTCInvoke for SH4 processor(Modified)

sorry for the delay, I've had a bad year.

/* -*- Mode: C; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 4 -*- */

invoke_count_words(PRUint32 paramCount, nsXPTCVariant* s)
{
   PRUint32 result = 0;

you're using 3 space indentation. please don't, the file claims 4, pick 2 or 4, make sure you update the header to match. same applies to both files

(you can carry forward this review when you attach replacement files).

      "jsr     @r0            \n\t" //Call invoke_copy_to_stack
      "jsr	   @r0            \n\t" //Call method

overidented?

Comment 9

[timeless]

Comment on attachment 268058 [details]
XPTCStubs for SH4 processor(Modified)

please add the null check for this case:
   NS_ASSERTION(dispatchParams,"no place for params");
(by now most platforms have it).

your \'s in:
#define STUB_ENTRY(n)                                       \

don't align very well

re comment 7, you've fixed the English I referenced in comment 3, which is what I wanted. And the license answer is also quite satisfactory.

Comment 10

[M. Onur Okyay]

Attachment: XPTCInvoke for SH4 processor(In reply to comment 8)

- Fixed indentations to 4 spaces as you requested.

Comment 11

[M. Onur Okyay]

Attachment: XPTCStubs for SH4 processor(In reply to comment #9)

- Fixed indentations & alignments as you requested
- Added null check after NS_ASSERTION(dispatchParams,"no place for params");

I hope we can commit this to the sourcebase this time :)

Thanks.

Comment 12

[timeless]

Comment on attachment 326257 [details]
XPTCStubs for SH4 processor(In reply to comment #9)

there are a few tabs in this, someone should on checkin fix the alignment of those \'s

Comment 13

[Mike Hommey [:glandium]]

Attachment: Different patch

I received this patch a few weeks ago from Nobuhiro Iwamatsu who got it from http://www.stlinux.com/.
It is slightly different from the patches included here already, and includes the necessary additions to the Makefile.

Comment 14

[M. Onur Okyay]

Hi All,

At the time I implemented porting for SH4 there was no other port available and this was almost three years ago.

Now you say that there is another port from Stlinux.com. That's fine. Go ahead and use that one. I am really tired of this issue.

Regards,

Onur

Comment 15

[Benjamin Smedberg]

Comment on attachment 426956 [details] [diff] [review]
Different patch

I'll delegate to timeless, I don't know SH4 at all.

Comment 16

[Nobuhiro Iwamatsu]

Hi, 

How about the progress of this problem?

Nobuhiro

Comment 17

[timeless]

1. please include an architecture reference, preferably the one you're using.

http://documentation.renesas.com/eng/products/mpumcu/rej09b0318_sh_4sm.pdf

is one i found. unlike arm it was easy to find today, but i shouldn't have to search for it, the source should point me to it.

2. don't use tabs in c(pp)/h code, only in makefiles

3. please try to align \'s. registers, etc.

4. this isn't how we write out contributors lines:
+ * Contributor(s):
+ *  - Copyright (C) 2008-2009 STMicroelectronics

5. please end sentences with periods :)
+	 * stack. So it is simpler to just write the whole thing in assembler anyway

6. please use a consistent style for multiline comments, the previous block used a * on each line
+	/* Because the SH processor passes the first few parameters in registers
+	   it is a bit tricky setting things up right.  To make things easier,

Comment 18

[timeless]

Comment on attachment 426956 [details] [diff] [review]
Different patch

Review of attachment 426956 [details] [diff] [review]:
-----------------------------------------------------------------

Someone should just land this. Preferably addressing my comments. But whatever.

::: xpcom/reflect/xptcall/src/md/unix/Makefile.in
@@ +455,5 @@
> +#
> +# Currently, tested on sh4 and sh4a (non sh4al and sh4al-dsp..)
> +ifeq ($(OS_ARCH),Linux)
> +ifneq (,$(filter sh4 sh4a,$(OS_TEST)))
> +CPPSRCS		:= xptcinvoke_linux_sh.cpp xptcstubs_linux_sh.cpp

I think we normally encourage:

CPPSRCS		:= \
 xptcinvoke_linux_sh.cpp \
 xptcstubs_linux_sh.cpp \
 $(NULL)

::: xpcom/reflect/xptcall/src/md/unix/xptcinvoke_linux_sh.cpp
@@ +51,5 @@
> +copy_to_stack(PRUint32 **that,PRUint32 methodIndex,PRUint32 paramCount, nsXPTCVariant* s,PRUint32* data)
> +{
> +	int intCount = 1; // Because of that
> +	int floatCount = 0;
> +	PRUint32 *intRegParams=data+1 ;

i think we normally encourage spaces around assignment.

Comment 19

[Mike Hommey [:glandium]]

(In reply to timeless from comment #18)
> Comment on attachment 426956 [details] [diff] [review]
> Different patch
> 
> Review of attachment 426956 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Someone should just land this. Preferably addressing my comments. But
> whatever.
> 
> ::: xpcom/reflect/xptcall/src/md/unix/Makefile.in
> @@ +455,5 @@
> > +#
> > +# Currently, tested on sh4 and sh4a (non sh4al and sh4al-dsp..)
> > +ifeq ($(OS_ARCH),Linux)
> > +ifneq (,$(filter sh4 sh4a,$(OS_TEST)))
> > +CPPSRCS		:= xptcinvoke_linux_sh.cpp xptcstubs_linux_sh.cpp
> 
> I think we normally encourage:
> 
> CPPSRCS		:= \
>  xptcinvoke_linux_sh.cpp \
>  xptcstubs_linux_sh.cpp \
>  $(NULL)

We don't define those in Makefile.in anymore :)

Comment 20

[Mike Hommey [:glandium]]

Iwamatsu-san, would you mind refreshing the patch following comments 17 and 18? (Don't worry about the Makefile.in part, I already have an up-to-date patch for that in Debian)

Comment 21

[John Paul Adrian Glaubitz]

Mike, I'm currently updating the patch in the progress of fixing Firefox on all Linux targets where it currently doesn't build. I will follow up with a patch shortly.

Comment 22

[John Paul Adrian Glaubitz]

Attachment: 0001-Bug-382214-xpcom-Add-xptcall-support-for-sh4.patch

Comment 23

[John Paul Adrian Glaubitz]

Attachment: 0002-Bug-382214-xpcom-Rename-NS_InvokeByIndex_P-to-NS_Inv.patch

Comment 24

[John Paul Adrian Glaubitz]

Attachment: 0003-Bug-382214-xpcom-Make-sh4-xpctcall-inline-assembly-p.patch

Comment 25

[John Paul Adrian Glaubitz]

Attaching an updated set of patches. The original xptcall code was first extracted from the Firefox Debian package with fixed paths for xptcstubs_linux_sh.cpp and xptcinvoke_linux_sh.cpp. I then renamed NS_InvokeByIndex_P to NS_InvokeByIndex and, finally, Michael Karcher fixed the inline assembly to be position-independent.

Comment 26

[Martin Husemann]

Comment on attachment 8822378 [details] [diff] [review]
0001-Bug-382214-xpcom-Add-xptcall-support-for-sh4.patch

Sorry, I don't know anything about SH cpus

Comment 27

[John Paul Adrian Glaubitz]

Attachment: 0001-Bug-382214-xpcom-Add-xptcall-support-for-Linux-SH.patch

Comment 28

[John Paul Adrian Glaubitz]

Attachment: 0002-Bug-382214-xpcom-Rename-NS_InvokeByIndex_P-to-NS_Inv.patch

Comment 29

[John Paul Adrian Glaubitz]

Attachment: 0003-Bug-382214-xpcom-Make-SH-xpctcall-inline-assembly-po.patch

Comment 30

[John Paul Adrian Glaubitz]

Michael Karcher found some bugs in his patch with the inline assembly which are now fixed, so I have updated his patch. I updated the other two patches as well with improved commit messages.

With the three patches applied, xpcshell works correctly on a SH7785LCR board.

Comment 31

[Mike Hommey [:glandium]]

Comment on attachment 8822827 [details] [diff] [review]
0001-Bug-382214-xpcom-Add-xptcall-support-for-Linux-SH.patch

Review of attachment 8822827 [details] [diff] [review]:
-----------------------------------------------------------------

The attribution in this patch is wrong. This is essentially the same patch as attachment 426956 [details] [diff] [review], which was submitted by Nobuhiro Iwamatsu who got it from http://www.stlinux.com/ (see comment 13)

Comments 17 and 19 should also be addressed.

Comment 32

[Mike Hommey [:glandium]]

Comment on attachment 8822828 [details] [diff] [review]
0002-Bug-382214-xpcom-Rename-NS_InvokeByIndex_P-to-NS_Inv.patch

Review of attachment 8822828 [details] [diff] [review]:
-----------------------------------------------------------------

::: xpcom/reflect/xptcall/md/unix/xptcinvoke_linux_sh.cpp
@@ +156,5 @@
>      */
>  	".section .text\n"
>  	".balign 4\n"
> +	".global NS_InvokeByIndex\n"
> +	"NS_InvokeByIndex:\n"

This should just be squashed into the previous patch.

Comment 33

[Mike Hommey [:glandium]]

(In reply to Mike Hommey [:glandium] from comment #31)
> Comment on attachment 8822827 [details] [diff] [review]
> 0001-Bug-382214-xpcom-Add-xptcall-support-for-Linux-SH.patch
> 
> Review of attachment 8822827 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> The attribution in this patch is wrong. This is essentially the same patch
> as attachment 426956 [details] [diff] [review], which was submitted by
> Nobuhiro Iwamatsu who got it from http://www.stlinux.com/ (see comment 13)
> 
> Comments 17 and 19 should also be addressed.

Oh, and the license should be MPL2...
See question 6 in https://www.mozilla.org/en-US/MPL/2.0/Revision-FAQ/

Comment 34

[John Paul Adrian Glaubitz]

(In reply to Mike Hommey [:glandium] from comment #31)
> The attribution in this patch is wrong. This is essentially the same patch
> as attachment 426956 [details] [diff] [review], which was submitted by
> Nobuhiro Iwamatsu who got it from http://www.stlinux.com/ (see comment 13)

I kept Mehmet Onur Okyay as the original author. Isn't that correct?

> Comments 17 and 19 should also be addressed.

Comment 19 isn't relevant anymore. I just used the original two .cpp files from Mehmet, but added them in mozbuild. Then Michael Karcher made some fixes to the assembler code to make the code actually work.

I can address the comments from 17 in an additional commit. I don't want to modify Mehmet's commit to heavily as to keep the changes he made attributed to him.

Comment 35

[Mike Hommey [:glandium]]

Comment on attachment 8822829 [details] [diff] [review]
0003-Bug-382214-xpcom-Make-SH-xpctcall-inline-assembly-po.patch

Review of attachment 8822829 [details] [diff] [review]:
-----------------------------------------------------------------

Same as comment 15, I'll delegate to timeless.

Note that while this makes the review easier as an interdiff, this should be squashed into the first patch.

Comment 36

[John Paul Adrian Glaubitz]

(In reply to Mike Hommey [:glandium] from comment #32)
> Comment on attachment 8822828 [details] [diff] [review]
> 0002-Bug-382214-xpcom-Rename-NS_InvokeByIndex_P-to-NS_Inv.patch
> 
> Review of attachment 8822828 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: xpcom/reflect/xptcall/md/unix/xptcinvoke_linux_sh.cpp
> @@ +156,5 @@
> >      */
> >  	".section .text\n"
> >  	".balign 4\n"
> > +	".global NS_InvokeByIndex\n"
> > +	"NS_InvokeByIndex:\n"
> 
> This should just be squashed into the previous patch.

But that's a change from me while the original patch comes from Mehmet. I kept these things as separate commits to keep authorship in the revision history. But I can change that if you wish.

And requesting from review from Nobuhiro Iwamatsu won't get you any results, he isn't active anymore. I have been dealing with this situation in Debian for the sh4 port for a long time now. He hasn't answered mail for over a year now.

Comment 37

[Mike Hommey [:glandium]]

(In reply to John Paul Adrian Glaubitz from comment #36)
> (In reply to Mike Hommey [:glandium] from comment #32)
> > Comment on attachment 8822828 [details] [diff] [review]
> > 0002-Bug-382214-xpcom-Rename-NS_InvokeByIndex_P-to-NS_Inv.patch
> > 
> > Review of attachment 8822828 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: xpcom/reflect/xptcall/md/unix/xptcinvoke_linux_sh.cpp
> > @@ +156,5 @@
> > >      */
> > >  	".section .text\n"
> > >  	".balign 4\n"
> > > +	".global NS_InvokeByIndex\n"
> > > +	"NS_InvokeByIndex:\n"
> > 
> > This should just be squashed into the previous patch.
> 
> But that's a change from me while the original patch comes from Mehmet. I
> kept these things as separate commits to keep authorship in the revision
> history. But I can change that if you wish.

Just keep the authorship to the original author.

> And requesting from review from Nobuhiro Iwamatsu won't get you any results,
> he isn't active anymore. I have been dealing with this situation in Debian
> for the sh4 port for a long time now. He hasn't answered mail for over a
> year now.

He's not the target of the review.

Comment 38

[John Paul Adrian Glaubitz]

(In reply to Mike Hommey [:glandium] from comment #37)
> > But that's a change from me while the original patch comes from Mehmet. I
> > kept these things as separate commits to keep authorship in the revision
> > history. But I can change that if you wish.
> 
> Just keep the authorship to the original author.

Ok, will do.

> > And requesting from review from Nobuhiro Iwamatsu won't get you any results,
> > he isn't active anymore. I have been dealing with this situation in Debian
> > for the sh4 port for a long time now. He hasn't answered mail for over a
> > year now.
> 
> He's not the target of the review.

Odd. He's listed at the bottom here (with Needinfo requested from [e-mail]) https://bugzilla.mozilla.org/attachment.cgi?id=8822828&action=edit, but maybe I'm reading this wrong.

Comment 39

[Mike Hommey [:glandium]]

(In reply to John Paul Adrian Glaubitz from comment #34)
> Comment 19 isn't relevant anymore. I just used the original two .cpp files
> from Mehmet, but added them in mozbuild. Then Michael Karcher made some
> fixes to the assembler code to make the code actually work.

It doesn't look like that's what you did. Because https://bugzilla.mozilla.org/attachment.cgi?id=326255 is vastly different from the xptcinvoke file in your patch.

To me, your patch looks like the patch I attached in comment 13, that came from StMicro through Nobuhiro Iwamatsu.

Comment 40

[Mike Hommey [:glandium]]

Same for https://bugzilla.mozilla.org/attachment.cgi?id=326257 and xptcstubs from your patch.

Comment 41

[John Paul Adrian Glaubitz]

(In reply to Mike Hommey [:glandium] from comment #40)
> Same for https://bugzilla.mozilla.org/attachment.cgi?id=326257 and xptcstubs
> from your patch.

I took the patch from the current Debian package. I assumed it was the same as in the bug tracker.

In any case, what do you suggest to do?

Should I squash all three patches into one (the patch from ST/Nobuhiro, Michael Karcher's, mine) and just attribute the authors in the source code itself?

Plus, of course address all the issues mentioned in Comment #17.

Just let me know what needs to be changed and I will follow up with an updated patch :).

Comment 42

[John Paul Adrian Glaubitz]

Hi!

(In reply to Mike Hommey [:glandium] from comment #13)
> I received this patch a few weeks ago from Nobuhiro Iwamatsu who got it from
> http://www.stlinux.com/.
> It is slightly different from the patches included here already, and
> includes the necessary additions to the Makefile.

I have done some research and it seems it's not possible to determine the original author for the patch from STLinux.

The patch can be found here:

> http://archive.stlinux.com/stlinux/2.3/updates/RPMS/sh4/stlinux23-sh4-firefox-3.0.1-3.sh4.rpm
> http://archive.stlinux.com/stlinux/2.3/updates/SRPMS/stlinux23-target-firefox-3.0.1-4.src.rpm
> http://archive.stlinux.com/stlinux/2.3/updates/SRPMS/stlinux23-target-firefox-3.0.1-5.src.rpm
> http://archive.stlinux.com/stlinux/2.3/updates/SRPMS/stlinux23-target-firefox-3.0.1-6.src.rpm
> http://archive.stlinux.com/stlinux/2.3/updates/SRPMS/stlinux23-target-firefox-3.0b5-1.src.rpm

The only statement regarding the origin of the code that I could find is:

> Adds support for SuperH to firefox, main work is in the xpcom stuff. This work was based on the SuperH work done for Neutrono. [sic]

With "Neutrono" they mean "Neutrino" which used to be a very popular Linux distribution for settop boxes in the 90ies which also used SuperH-based SoCs.

I think the only proper way to go would be to take Onur's patch and clean it up to work on a current Debian/sh4 environment.

Comment 43

[John Paul Adrian Glaubitz]

Hi!

(In reply to timeless from comment #17)
> 4. this isn't how we write out contributors lines:
> + * Contributor(s):
> + *  - Copyright (C) 2008-2009 STMicroelectronics

I'm a bit stuck here. I have done some research and it seems the code actually comes from STMicroElectronics. It was first written for QNX Neutrino, then ported to Linux by STMicroelectronics. So, I think the copyright notice here is correct.

Do you need the names of the individual authors? I can try to get into touch with STM and ask them.

(In reply to glaubitz from comment #42)
> With "Neutrono" they mean "Neutrino" which used to be a very popular Linux distribution for settop boxes in the 90ies which also used SuperH-based SoCs.

Ok, I think this Neutrino actually refers to "QNX Neutrino" [1] for which STM wrote the initial xptcall patch for Firefox.

> I think the only proper way to go would be to take Onur's patch and clean it up to work on a current Debian/sh4 environment.

I have tried building with Onur's patch but that failed. The patch from STLinux.com actually works with the modifications we made, so I think we should get this one merged.

Adrian

> [1] https://en.wikipedia.org/wiki/QNX#Description

Comment 44

[timeless]

From memory, contributors is about people not copyright. I'm not even sure Mozilla still maintains those lines, I think they may rely on VCS history instead (but someone would have to check).

fwiw, I'm quite familiar w/ QNX Neutrino. I used it in 2002 and then later worked for RIM/BlackBerry after they acquired QNX (for use in the PlayBook/BlackBerry 10 platforms, as well as QNX Car).

I left r+ marks on all of the patches a long time ago, my comments were not intended to stop people from moving forward.

Comment 45

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee didn't login in Bugzilla in the last 7 months.
:nika, could you have a look please?
For more information, please visit auto_nag documentation.