Bug 30552 – [Symbian] Port ARM traditional JIT Trampolines to RVCT

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Laszlo Gombos]

ARM traditional JIT Trampolines are only available for the GCC compiler syntax at the moment. Symbian and other embedded platforms might use the RVCT (ARM) compiler which has a slightly different syntax fro ARM assembly. This bug is to port over the GCC compiler syntax assmebly to RVCT.

[Laszlo Gombos]

Created attachment 41480 [details]
first try

[Laszlo Gombos]

CCd Gavin and Geoffrey as the JIT experts with review rights hoping that one of them could review this relatively simple porting patch.

[Gabor Loki]

> +__asm void ctiVMThrowTrampoline() {    
> +    ARM
> +    IMPORT cti_vm_throw;
> +    mov r0, sp
> +    mov lr, r6
> +    add r8, pc, #4
> +    str r8, [sp, #-4]!
> +    b cti_vm_throw
> +}      

Not this way! This code is a tricky one. :) It is working by luck of the draw. The cti_vm_throw should return to the code of ctiOpThrowNotCaught.

The "add r8, pc, #4 \n str r8, [sp, #-4]!" pair set the return address of cti_vm_throw to the next instruction of "b cti_vm_throw".

It wasn't an accident to group those two function body in ARM trampolines. ;)

Fix these by adding the body of ctiOpThrowNotCaught after "b cti_vm_throw".

[Laszlo Gombos]

Created attachment 41561 [details]
second try

Thanks Gabor for catching this! This patch addresses the comments from Gabor.

[Holger Freyther]

In general. Is there any way that RVCT can use the GNU syntax?

[Laszlo Gombos]

(In reply to comment #5)
> In general. Is there any way that RVCT can use the GNU syntax?

with some macro trickery the RVCT and GCC code path can be shared, but that would require changes for the GCC code path as well and would make the code hard to read.

I would propose to take sharing the two codepath up as a separate patch.

[Laszlo Gombos]

Created attachment 41594 [details]
Add PRESERVE8 to ctiVMThrowTrampoline

RVCT needs an explicit "PRESERVE8" for ctiVMThrowTrampoline otherwise linking might fail with the following error:

Error: L6238E: QtWebKit.in(.emb_text) contains invalid call from '~PRES8' function to 'REQ8' function cti_vm_throw.

See http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.faqs/ka4127.html for the relevant RVCT documentation from ARM.

[Janne Koskinen]

I was talking with Iain about this. I guess you have been in contact with him.
He was worried about PRESERVE8 and str r8, [sp, #-4]! in the same function.
Did you run it with --diag_warning 1546 ?

[Gabor Loki]

I've removed the dependency flag on bug 30782. We should manage the changes separately.

[Laszlo Gombos]

Comment on attachment 41594 [details]
Add PRESERVE8 to ctiVMThrowTrampoline

Patch needs rework after http://trac.webkit.org/changeset/50109 is landed.

[Laszlo Gombos]

Created attachment 42077 [details]
Adapt patch to changes in r50109

[Janne Koskinen]

I tried running sunspider testsuite with the trampolines and got heap corruption. Don't know if the issue is the trampoline itself or some regression it causes with different codepath.
Callstack and register dump of the run: http://pastebin.ca/1656908
Search for ">>>>" to find the current stack pointer.

[Adam Barth]

Attachment 42077 [details] passed the style-queue

[yzkuang]

(In reply to comment #12)
> I tried running sunspider testsuite with the trampolines and got heap
> corruption. Don't know if the issue is the trampoline itself or some regression
> it causes with different codepath.
> Callstack and register dump of the run: http://pastebin.ca/1656908
> Search for ">>>>" to find the current stack pointer.

 You use the RVCT (ARM) compiler ?
 The macro " DEFINE_STUB_FUNCTION" must be redefined for RVCT and ARM!
 Like this:

"DEFINE_STUB_FUNCTION(EncodedJSValue, op_convert_this)"

  ====>>>>

"
extern "C" {
EncodedJSValue JITStubThunked_op_convert_this(STUB_ARGS_DECLARATION); 
	}; 
__asm EncodedJSValue JIT_STUB cti_op_convert_this(STUB_ARGS_DECLARATION)
{
	ARM
	str lr,[sp,#32]
	import JITStubThunked_op_convert_this
	bl JITStubThunked_op_convert_this
	ldr lr,[sp,#32]
	bx lr	
}
EncodedJSValue JITStubThunked_op_convert_this(STUB_ARGS_DECLARATION)
"

[yzkuang]

(In reply to comment #12)
> I tried running sunspider testsuite with the trampolines and got heap
> corruption. Don't know if the issue is the trampoline itself or some regression
> it causes with different codepath.
> Callstack and register dump of the run: http://pastebin.ca/1656908
> Search for ">>>>" to find the current stack pointer.

expand the macro "DEFINE_STUB_FUNCTION" one by one !  Because I have no other good idea.

You can open JITStubs.cpp with EditPlus, then replace
  DEFINE_STUB_FUNCTION\(([a-zA-Z_*]+), ([a-zA-Z_]+)\)\n
with
 extern "C" {
\1 JITStubThunked_\2(STUB_ARGS_DECLARATION); 
	}; 
__asm \1 JIT_STUB cti_\2(STUB_ARGS_DECLARATION)
{
	ARM
	str lr,[sp,#32]
	import JITStubThunked_\2
	bl JITStubThunked_\2
	ldr lr,[sp,#32]
	bx lr	
}
\1 JITStubThunked_\2(STUB_ARGS_DECLARATION)\n

[Janne Koskinen]

Excellent advice, need to try this ASAP.

[Kenneth Rohde Christiansen]

Comment on attachment 42077 [details]
Adapt patch to changes in r50109

Obsolete according to Loki Gabor.

[Gabor Loki]

> (From update of attachment 42077 [details])
> Obsolete according to Loki, Gabor.

Unfortunately, this patch is not enough in itself. It was useful for Qt4.6.0, but for trunk it is just a part of final solution. We should add a tricky approach for DEFINE_STUB_FUNCTION, because rvct is not able to handle any kind of assembly in macro.

Laszlo and I have a long discussion about it some weeks ago, and we think a generator can be used to produce those small functions. We have preliminary patch for it, but it was delayed because of Qt release.

[Janne Koskinen]

(In reply to comment #18)
> > (From update of attachment 42077 [details] [details])
> > Obsolete according to Loki, Gabor.
> 
> Unfortunately, this patch is not enough in itself. It was useful for Qt4.6.0,
> but for trunk it is just a part of final solution. We should add a tricky
> approach for DEFINE_STUB_FUNCTION, because rvct is not able to handle any kind
> of assembly in macro.
> 
> Laszlo and I have a long discussion about it some weeks ago, and we think a
> generator can be used to produce those small functions. We have preliminary
> patch for it, but it was delayed because of Qt release.

Right I remember this kind of discussion as well. Solution was to have code generator (perl based) to create those functions.

[Laszlo Gombos]

The patch was not obsolete per say, but it certainly was not complete.

As noted, the code generation step is needed for the full solution, which means that the solution will be port/build system specific. This is why I wanted to keep the generic patch separate from the build system specific work.

Either way I will post the complete solution for review soon later this week.

[Laszlo Gombos]

yzkuang, are you using Qt port of WebKit or some other ports with RVCT ?

[Laszlo Gombos]

Created attachment 45110 [details]
Complete solution after r50109

Use a perl script to expand precompiler macros for RVCT.

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 45110 [details] without any errors.

[Gabor Loki]

> +        * JavaScriptCore.pri: Extra step to generate RVCT stubs. The 
> +        script generation intentionally executed all the time not just
> +        for RVCT targets.

Is the generation necessary for all platform? Why can not we do the followings?

+symbian: {
+RVCT_STUB_FILES += \
+    jit/JITStubs.cpp
+}
...
+symbian: {
+# GENERATOR 3: JIT Stub functions for RVCT
+rvctstubs.output = $${GENERATED_SOURCES_DIR}$${QMAKE_DIR_SEP}Generated${QMAKE_FILE_BASE}_RVCT.h
+rvctstubs.commands = perl $$PWD/create_rvct_stubs ${QMAKE_FILE_NAME} -i > ${QMAKE_FILE_OUT}
+rvctstubs.depend = ${QMAKE_FILE_NAME}
+rvctstubs.input = RVCT_STUB_FILES
+rvctstubs.CONFIG += no_link
+addExtraCompiler(rvctstubs)
+}

It would be much better if an rvct symbol can be used instead of symbian.


+#if PLATFORM(ARM_TRADITIONAL)
+#if PLATFORM_ARM_ARCH(5)
+#define BRANCH(reg) bx reg
+#else
+#define BRANCH(reg) mov pc, reg
+#endif
+#endif

Do you really want to support ARMv4 JIT on Symbian? I think I have asked this question earlier, but I can not recall you answer.
If you want to support it, guard it with an additional COMPILER(RVCT) macro or move to the next PLATFORM(ARM_TRADITIONAL) && COMPILER(RVCT) section.
(Btw, we should give a better name for #define BRANCH.)

[yzkuang]

(In reply to comment #21)
> yzkuang, are you using Qt port of WebKit or some other ports with RVCT ?


Symbian S60 v3 + RVCT

[Simon Hausmann]

The build part of the patch looks good to me (it's the only part I understand ;)

[Laszlo Gombos]

Created attachment 45203 [details]
Remove BRANCH macro

Remove ARMv4 support (and the BRANCH macro) as suggested by Loki.

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 45203 [details] without any errors.

[Laszlo Gombos]

(In reply to comment #24)
> > +        * JavaScriptCore.pri: Extra step to generate RVCT stubs. The 
> > +        script generation intentionally executed all the time not just
> > +        for RVCT targets.
> 
> Is the generation necessary for all platform? 

Loki, the easiest/safest way to create a Symbian build is to create the generated files on Linux and than use the Symbian environment only for compiling/linking. This assumes that generation is turned on not just for RVCT/Symbian builds, but for all Qt builds. I do not think this is a big overhead for non-RVCT builds.

Can you give us your feedback on the latest uploaded patch ? I think your input would help reviewers. Does the patch looks good to you now ?

[Gabor Loki]

> Loki, the easiest/safest way to create a Symbian build is to create the
> generated files on Linux and than use the Symbian environment only for
> compiling/linking. 

I see now. So the generation part of the build runs on Linux, and Symbian environment is responsible only for compiling and linking. All right, then.

> Can you give us your feedback on the latest uploaded patch ? I think your input
> would help reviewers. Does the patch looks good to you now ?

:) Well, the patch looks good to me, and it is definitely needed for Symbian.

[Laszlo Gombos]

Comment on attachment 45203 [details]
Remove BRANCH macro

Landed as http://trac.webkit.org/changeset/52970. Thanks for the review Gavin.

[Pushparajan]

(In reply to comment #29)
> (In reply to comment #24)
> > > +        * JavaScriptCore.pri: Extra step to generate RVCT stubs. The 
> > > +        script generation intentionally executed all the time not just
> > > +        for RVCT targets.
> > 
> > Is the generation necessary for all platform? 
> 
> Loki, the easiest/safest way to create a Symbian build is to create the
> generated files on Linux and than use the Symbian environment only for
> compiling/linking. This assumes that generation is turned on not just for
> RVCT/Symbian builds, but for all Qt builds. I do not think this is a big
> overhead for non-RVCT builds.
> 
> Can you give us your feedback on the latest uploaded patch ? I think your input
> would help reviewers. Does the patch looks good to you now ?

using GNU m4 instead of custom perl scripts would be a good option.. I too faced a similar issue and did something like this for RVCT compiler, 

>>>>>>>>>>>>>>>>>>>>>>>>>

m4_define(`DEFINE_STUB_FUNCTION', `
extern "C" { 
      	$1 JITStubThunked_$2(STUB_ARGS_DECLARATION); 
}; 

__asm $1 JIT_STUB cti_$2(STUB_ARGS_DECLARATION) { 
	str lr, [sp, #0x1c]; 
	bl __cpp(JITStubThunked_$2);
	ldr lr, [sp, #0x1c];
	bx lr;
}

$1 JITStubThunked_$2(STUB_ARGS_DECLARATION)')

>>>>>>>>>>>>>>>>>>>>>>

After that, just ran m4 tool over the source code to get automatically expanded macros. 

Just sharing here so since this fix looked simpler and effective.

[Gabor Loki]

> using GNU m4 instead of custom perl scripts would be a good option.

Well, this could be also an option, but as far as I known Symbian does not have GNU m4.

[Laszlo Gombos]

> using GNU m4 instead of custom perl scripts would be a good option.. I too
> faced a similar issue and did something like this for RVCT compiler, 

> Just sharing here so since this fix looked simpler and effective.

WebKit build process is already dependent on perl in a few places, but not on m4. m4 would create an additional tool dependency to build WebKit. The choice of tool has been made to minimize tool dependencies.