Bug 30552

Created attachment 41480 [details] first try

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

> +__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".

Created attachment 41561 [details] second try Thanks Gabor for catching this! This patch addresses the comments from Gabor.

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

(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.

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.

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 ?

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

Comment on attachment 41594 [details] Add PRESERVE8 to ctiVMThrowTrampoline Patch needs rework after http://trac.webkit.org/changeset/50109 is landed.

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

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.

Attachment 42077 [details] passed the style-queue

(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) "

(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

Excellent advice, need to try this ASAP.

Comment on attachment 42077 [details] Adapt patch to changes in r50109 Obsolete according to Loki Gabor.

> (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.

(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.

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.

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

Created attachment 45110 [details] Complete solution after r50109 Use a perl script to expand precompiler macros for RVCT.

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

> + * 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.)

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

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

Created attachment 45203 [details] Remove BRANCH macro Remove ARMv4 support (and the BRANCH macro) as suggested by Loki.

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

(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 ?

> 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 on attachment 45203 [details] Remove BRANCH macro Landed as http://trac.webkit.org/changeset/52970. Thanks for the review Gavin.

(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.

> 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.

> 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.