Bug 30552 - [Symbian] Port ARM traditional JIT Trampolines to RVCT
Summary: [Symbian] Port ARM traditional JIT Trampolines to RVCT
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC S60 3rd edition
: P2 Normal
Assignee: Nobody
URL:
Keywords: Qt
Depends on:
Blocks: 27065
  Show dependency treegraph
 
Reported: 2009-10-19 21:10 PDT by Laszlo Gombos
Modified: 2010-01-12 05:14 PST (History)
11 users (show)

See Also:


Attachments
first try (2.26 KB, patch)
2009-10-19 21:29 PDT, Laszlo Gombos
no flags Details | Formatted Diff | Diff
second try (2.37 KB, patch)
2009-10-21 05:32 PDT, Laszlo Gombos
no flags Details | Formatted Diff | Diff
Add PRESERVE8 to ctiVMThrowTrampoline (2.38 KB, patch)
2009-10-21 11:40 PDT, Laszlo Gombos
no flags Details | Formatted Diff | Diff
Adapt patch to changes in r50109 (2.26 KB, patch)
2009-10-28 18:42 PDT, Laszlo Gombos
kenneth: review-
Details | Formatted Diff | Diff
Complete solution after r50109 (6.23 KB, patch)
2009-12-17 16:31 PST, Laszlo Gombos
no flags Details | Formatted Diff | Diff
Remove BRANCH macro (5.94 KB, patch)
2009-12-18 16:26 PST, Laszlo Gombos
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Laszlo Gombos 2009-10-19 21:10:27 PDT
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.
Comment 1 Laszlo Gombos 2009-10-19 21:29:30 PDT
Created attachment 41480 [details]
first try
Comment 2 Laszlo Gombos 2009-10-20 10:16:40 PDT
CCd Gavin and Geoffrey as the JIT experts with review rights hoping that one of them could review this relatively simple porting patch.
Comment 3 Gabor Loki 2009-10-21 00:45:48 PDT
> +__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".
Comment 4 Laszlo Gombos 2009-10-21 05:32:16 PDT
Created attachment 41561 [details]
second try

Thanks Gabor for catching this! This patch addresses the comments from Gabor.
Comment 5 Holger Freyther 2009-10-21 05:58:28 PDT
In general. Is there any way that RVCT can use the GNU syntax?
Comment 6 Laszlo Gombos 2009-10-21 09:00:21 PDT
(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.
Comment 7 Laszlo Gombos 2009-10-21 11:40:53 PDT
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.
Comment 8 Janne Koskinen 2009-10-26 04:07:01 PDT
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 ?
Comment 9 Gabor Loki 2009-10-26 12:11:26 PDT
I've removed the dependency flag on bug 30782. We should manage the changes separately.
Comment 10 Laszlo Gombos 2009-10-26 16:23:41 PDT
Comment on attachment 41594 [details]
Add PRESERVE8 to ctiVMThrowTrampoline

Patch needs rework after http://trac.webkit.org/changeset/50109 is landed.
Comment 11 Laszlo Gombos 2009-10-28 18:42:41 PDT
Created attachment 42077 [details]
Adapt patch to changes in r50109
Comment 12 Janne Koskinen 2009-11-04 23:59:27 PST
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.
Comment 13 Adam Barth 2009-11-30 12:21:34 PST
Attachment 42077 [details] passed the style-queue
Comment 14 yzkuang 2009-12-08 08:18:48 PST
(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)
"
Comment 15 yzkuang 2009-12-08 08:37:25 PST
(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
Comment 16 Janne Koskinen 2009-12-14 03:28:21 PST
Excellent advice, need to try this ASAP.
Comment 17 Kenneth Rohde Christiansen 2009-12-14 05:25:25 PST
Comment on attachment 42077 [details]
Adapt patch to changes in r50109

Obsolete according to Loki Gabor.
Comment 18 Gabor Loki 2009-12-14 05:41:55 PST
> (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.
Comment 19 Janne Koskinen 2009-12-14 05:54:21 PST
(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.
Comment 20 Laszlo Gombos 2009-12-14 05:59:59 PST
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.
Comment 21 Laszlo Gombos 2009-12-17 07:30:56 PST
yzkuang, are you using Qt port of WebKit or some other ports with RVCT ?
Comment 22 Laszlo Gombos 2009-12-17 16:31:51 PST
Created attachment 45110 [details]
Complete solution after r50109

Use a perl script to expand precompiler macros for RVCT.
Comment 23 WebKit Review Bot 2009-12-17 16:35:12 PST
style-queue ran check-webkit-style on attachment 45110 [details] without any errors.
Comment 24 Gabor Loki 2009-12-18 02:14:58 PST
> +        * 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.)
Comment 25 yzkuang 2009-12-18 03:52:19 PST
(In reply to comment #21)
> yzkuang, are you using Qt port of WebKit or some other ports with RVCT ?


Symbian S60 v3 + RVCT
Comment 26 Simon Hausmann 2009-12-18 06:30:47 PST
The build part of the patch looks good to me (it's the only part I understand ;)
Comment 27 Laszlo Gombos 2009-12-18 16:26:56 PST
Created attachment 45203 [details]
Remove BRANCH macro

Remove ARMv4 support (and the BRANCH macro) as suggested by Loki.
Comment 28 WebKit Review Bot 2009-12-18 16:28:01 PST
style-queue ran check-webkit-style on attachment 45203 [details] without any errors.
Comment 29 Laszlo Gombos 2009-12-18 16:36:39 PST
(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 ?
Comment 30 Gabor Loki 2009-12-18 23:32:47 PST
> 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.
Comment 31 Laszlo Gombos 2010-01-07 21:57:45 PST
Comment on attachment 45203 [details]
Remove BRANCH macro

Landed as http://trac.webkit.org/changeset/52970. Thanks for the review Gavin.
Comment 32 Pushparajan 2010-01-12 01:56:12 PST
(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.
Comment 33 Gabor Loki 2010-01-12 02:38:01 PST
> 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.
Comment 34 Laszlo Gombos 2010-01-12 05:14:52 PST
> 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.