681602 - xptcall implementation for arm-darwin (iOS)

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Comment 1

[(not currently active) Ted Mielczarek]

Attachment: arm-darwin xptcall, based on arm-openbsd

I am a total amateur at ARM assembly, and I don't know anything about Darwin calling conventions, but I was able to copy the arm-openbsd implementation, munge the assembly to make Apple's assembler happy, and it worked. There may be a nicer way to do this, I don't know.

Comment 2

[Jacob Bramley [:jbramley]]

Comment on attachment 555355 [details] [diff] [review]
arm-darwin xptcall, based on arm-openbsd

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

I've not done a proper review, but here's one comment.

::: xpcom/reflect/xptcall/src/md/unix/xptcinvoke_arm_openbsd.cpp
@@ +208,2 @@
>      "addle	sp, sp, r4	\n\t"	/*      and restore stack pointer	*/
> +    "it le\n\t"

The 'IT' instruction can cover up to four subsequent instructions, so in this case you only need two. GT and LE are opposites so you could have "ittee gt" followed by "it le", but since you need two ITs anyway, it might be neater to do this:

itt gt
ldmiagt    @ Condition goes last these days. (Might avoid a warning.)
subgt
ittt le
ldmiale    @ Condition goes last these days. (Might avoid a warning.)
addle
movle

Comment 3

[Jacob Bramley [:jbramley]]

(In reply to Ted Mielczarek [:ted, :luser] from comment #1)

> I am a total amateur at ARM assembly, and I don't know anything about Darwin
> calling conventions [...]

It would probably be a good idea to find a calling convention specification to check it, or something like that, as they can get subtle at times, particularly when floating-point or 64-bit values get involved. I have no idea where you would start looking for something like that, unfortunately.

Comment 4

[(not currently active) Ted Mielczarek]

By inspecting generated assembly, glandium tells me that the !__ARM_EABI__ !__ARM_PCS_VFP codepath of the Linux/ARM code should work just fine. It's sort of a pain to test, since I have to get my patch queue into a state where I can get something built with xptcall, but I will probably try that approach so we don't wind up with another poorly-maintained file.

Comment 5

[(not currently active) Ted Mielczarek]

Attachment: massage xptcstubs_arm.cpp to compile for iOS

This is mostly just assembly fiddling to make the existing assembly work with Apple's assembler. This passes TestXPTCInvoke.cpp on-device for me.

Comment 6

[Mike Hommey [:glandium]]

Comment on attachment 566596 [details] [diff] [review]
massage xptcstubs_arm.cpp to compile for iOS

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

::: xpcom/reflect/xptcall/src/md/unix/Makefile.in
@@ +195,5 @@
> +ifneq (,$(filter arm%,$(OS_TEST)))
> +CPPSRCS		:= xptcinvoke_arm.cpp xptcstubs_arm.cpp
> +CXXFLAGS += -O2
> +endif
> +endif

Any particular reason not to merge with the case for Linux above it?

::: xpcom/reflect/xptcall/src/md/unix/xptcstubs_arm.cpp
@@ +146,5 @@
> +#define STUB_FUNCDEF(size, len)                                      \
> +"	.globl	_ZN14nsXPTCStubBase" #len "Stub" #size "Ev\n"           \
> +"	.type	_ZN14nsXPTCStubBase" #len "Stub" #size "Ev,#function\n" \
> +"_ZN14nsXPTCStubBase" #len "Stub" #size "Ev:\n"
> +#endif

The function name is going to be required once more for bug 695712. I can take it from here, or you can feel free to make a macro for it before landing. That should simplify the above a bit.

Comment 7

[(not currently active) Ted Mielczarek]

Attachment: MozReview Request: bz://681602/ted

/r/8607 - bug 681602 - Implement xptcall for arm iOS.

Pull down this commit:

hg pull -r 873b09f6f7b1f5df52b5bf7fef8e97f54ea3690a https://reviewboard-hg.mozilla.org/gecko/

Comment 8

[Mike Hommey [:glandium]]

https://reviewboard.mozilla.org/r/8607/#review7219

::: xpcom/reflect/xptcall/md/unix/xptcstubs_arm.cpp:157
(Diff revision 1)
> +#endif

I'm puzzled as to why you need to do this duplication when the assembly is the exact same, except for the .save and .fnend that you remove (so you could wrap them with CFI() instead), the .text / .section difference and the additional .thumb_func that you add, which I would argue should be added to the other version when __thumb__ is defined.

::: xpcom/reflect/xptcall/md/unix/xptcstubs_arm.cpp:231
(Diff revision 1)
> +#endif

Here too it's largely the same. I'm sure the .if form could be used in the non ___APPLE__ part with no problem, and you could use a macro for the single or double underscore, we do that in other places iirc.

Comment 10

[(not currently active) Ted Mielczarek]

Attachment: MozReview Request: bug 681602 - Implement xptcall for arm iOS. r?glandium

Comment 11

[(not currently active) Ted Mielczarek]

Comment on attachment 8617983 [details]
MozReview Request: bug 681602 - Implement xptcall for arm iOS. r?glandium

bug 681602 - Implement xptcall for arm iOS. r?glandium

The xptinvoke implementation is taken from the arm_openbsd implementation.
xptcstubs_arm mostly works on iOS but Apple's assembler is ridiculous so
the inline assembly for the SharedStub and the stub methods is just completely
#ifdefed.

Comment 12

[Mike Hommey [:glandium]]

Comment on attachment 8617983 [details]
MozReview Request: bug 681602 - Implement xptcall for arm iOS. r?glandium

https://reviewboard.mozilla.org/r/8607/#review18005

::: xpcom/reflect/xptcall/md/unix/xptcstubs_arm.cpp:11
(Diff revision 2)
> -#if !defined(__arm__) && !(defined(LINUX) || defined(ANDROID))
> +#if !defined(__arm__) && !(defined(LINUX) || defined(ANDROID) || defined(XP_DARWIN))

XP_IOS

::: xpcom/reflect/xptcall/md/unix/xptcstubs_arm.cpp:186
(Diff revision 2)
> -	".section \".text\"\n"					\
> +          GNU(".section \".text\"\n")                           \
> +          APPLE(".section __TEXT,__text\n")                     \
>  "	.align 2\n"						\
> -"	.iflt ("#n" - 10)\n"                                    \
> -"	.globl	_ZN14nsXPTCStubBase5Stub"#n"Ev\n"		\
> -"	.type	_ZN14nsXPTCStubBase5Stub"#n"Ev,#function\n"	\
> -"_ZN14nsXPTCStubBase5Stub"#n"Ev:\n"				\
> +"	.if ("#n" - 10) < 0\n"                                  \
> +"	.globl	" UNDERSCORE "ZN14nsXPTCStubBase5Stub"#n"Ev\n"	\
> +          THUMB_FUNC \
> +          GNU(".type	_ZN14nsXPTCStubBase5Stub"#n"Ev,#function\n") \
> +UNDERSCORE "ZN14nsXPTCStubBase5Stub"#n"Ev:\n"			\
>  "	.else\n"                                                \
> -"	.iflt  ("#n" - 100)\n"                                  \
> -"	.globl	_ZN14nsXPTCStubBase6Stub"#n"Ev\n"		\
> -"	.type	_ZN14nsXPTCStubBase6Stub"#n"Ev,#function\n"	\
> -"_ZN14nsXPTCStubBase6Stub"#n"Ev:\n"				\
> +"	.if  ("#n" - 100) < 0\n"                                \
> +"	.globl	" UNDERSCORE "ZN14nsXPTCStubBase6Stub"#n"Ev\n"	\
> +          THUMB_FUNC \
> +          GNU(".type _ZN14nsXPTCStubBase6Stub"#n"Ev,#function\n") \
> +UNDERSCORE "ZN14nsXPTCStubBase6Stub"#n"Ev:\n"			\
>  "	.else\n"                                                \
> -"	.iflt ("#n" - 1000)\n"                                  \
> -"	.globl	_ZN14nsXPTCStubBase7Stub"#n"Ev\n"		\
> -"	.type	_ZN14nsXPTCStubBase7Stub"#n"Ev,#function\n"	\
> -"_ZN14nsXPTCStubBase7Stub"#n"Ev:\n"				\
> +"	.if ("#n" - 1000) < 0\n"                                \
> +"	.globl	" UNDERSCORE "ZN14nsXPTCStubBase7Stub"#n"Ev\n"	\
> +          THUMB_FUNC \
> +          GNU(".type _ZN14nsXPTCStubBase7Stub"#n"Ev,#function\n") \
> +UNDERSCORE "ZN14nsXPTCStubBase7Stub"#n"Ev:\n"			\

Indentation looks weird here. Maybe it's a combination of mozreview and my broken fontconfig config that uses a non monospace font for monospace, but please double check indentation in this block.

Comment 13

[(not currently active) Ted Mielczarek]

https://reviewboard.mozilla.org/r/8607/#review18005

> Indentation looks weird here. Maybe it's a combination of mozreview and my broken fontconfig config that uses a non monospace font for monospace, but please double check indentation in this block.

It's possible there were tabs here and Emacs swapped them out for spaces. I will fix it.

Comment 14

[(not currently active) Ted Mielczarek]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ebbd463ff0d6971ada8c67a7a31c09874d342e51
bug 681602 - Implement xptcall for arm iOS. r=glandium

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ebbd463ff0d6