Bug 34953 – [WINCE] Implement DEFINE_STUB_FUNCTION for WinCE

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

[Patrick R. Gansterer]

see patch

[Patrick R. Gansterer]

Created attachment 48765 [details]
The patch

[Laszlo Gombos]

In general, change looks good to me, but I'd like to understand how the generated GeneratedJITStubs_MSVC.asm gets included in the build system ? 

For RVCT "GeneratedJITStubs_RVCT.h" gets included in explicitly in JITStubs.cpp.

[Patrick R. Gansterer]

(In reply to comment #2)
> In general, change looks good to me, but I'd like to understand how the
> generated GeneratedJITStubs_MSVC.asm gets included in the build system ? 
> 
> For RVCT "GeneratedJITStubs_RVCT.h" gets included in explicitly in
> JITStubs.cpp.

The MS assembler (armasm) must compile this file.

One possibility is to compile this file as it is and only link the generated object file.
The other way is to include it into the (yet missing) assembler file for the trampolines (ctiTrampoline, ctiVMThrowTrampoline, ctiOpThrowNotCaught).

[Laszlo Gombos]

I'd like to see how this file gets incorporated into (at least one) WinCE port as part of this patch.

[Patrick R. Gansterer]

Created attachment 50582 [details]
The patch

ATTENTION: depends on bug 36050

[WebKit Review Bot]

Attachment 50582 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
JavaScriptCore/jit/JITStubs.cpp:1101:  One or more unexpected \r (^M) found; better to use only a \n  [whitespace/carriage_return] [1]
Suppressing further [whitespace/carriage_return] reports for this file.
Total errors found: 30 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Patrick R. Gansterer]

(In reply to comment #4)
> I'd like to see how this file gets incorporated into (at least one) WinCE port
> as part of this patch.
I'm not sure if it is ok now. The remainig part is to call armasm.exe and link
the object file. But i don't know what is the prefered way to do this in the Qt
port. (How do i dedect the correct platform? When is JIT enabled on WinCE?)

[Patrick R. Gansterer]

Created attachment 50583 [details]
The patch (corrected line endings)

[Eric Seidel (no email)]

CCing the JIT-masters.

[Eric Seidel (no email)]

Ping?

[Oliver Hunt]

Comment on attachment 50583 [details]
The patch (corrected line endings)

Given the goal of this appears to be to code that can be stripped out and turned into an asm file, why don't we just have an asm file?

[Patrick R. Gansterer]

(In reply to comment #11)
> (From update of attachment 50583 [details])
> Given the goal of this appears to be to code that can be stripped out and
> turned into an asm file, why don't we just have an asm file?

Because the asm file from "cti_#op#" functions need a "header" for at least the AREA, and because the JSVALUE32_64 and !JSVALUE32_64 code is nearly the same. If you prefere a seperate asm file it's also ok: How to name it? JITTrampoline_ARM32.asm and JITTrampoline_ARM64.asm. (The differ only in two lines)

[Zoltan Herczeg]

Seeing the growth of these compiler specific generated assembly files, might be a good idea to move all of them to a seperated file, from which a (python?) script would generate a target specific assembly file. Basically we need the compiler type, begin / end assembly sequence for the compiler, and the stub specific code:

Might be something:

# comments (only at line start)
# indentation is not expected but suggested
# hopefully @ is not used by those assemblers
#     (gnu-as use them on some platforms)
#     if so it can be replaced to something else

@compiler: RVCT

   @begin:
      assembly code

   @body:
      assembly code for each stub function #op# replaced to something

   @end:
      assembly code

@compiler: MSVC

...

[Patrick R. Gansterer]

If you look at the ctiTrampoline function the body is nearly the same for all compilers:

----------------------------------------
GCC:
----------------------------------------
asm volatile (
".text\n"
".globl " SYMBOL_STRING(ctiTrampoline) "\n"
HIDE_SYMBOL(ctiTrampoline) "\n"
SYMBOL_STRING(ctiTrampoline) ":" "\n"
    "stmdb sp!, {r1-r3}" "\n"
    "stmdb sp!, {r4-r8, lr}" "\n"
    "sub sp, sp, #36" "\n"
    "mov r4, r2" "\n"
    "mov r5, #512" "\n"
    "mov lr, pc" "\n"
    "mov pc, r0" "\n"
    "add sp, sp, #36" "\n"
    "ldmia sp!, {r4-r8, lr}" "\n"
    "add sp, sp, #12" "\n"
    "mov pc, lr" "\n"
}
----------------------------------------
RVCT:
----------------------------------------
__asm EncodedJSValue ctiTrampoline(...)
{
    ARM
    stmdb sp!, {r1-r3}
    stmdb sp!, {r4-r8, lr}
    sub sp, sp, #36
    mov r4, r2
    mov r5, #512
    mov lr, pc
    bx r0
    add sp, sp, #36
    ldmia sp!, {r4-r8, lr}
    add sp, sp, #12
    bx lr
}
----------------------------------------
MSVC:
----------------------------------------
MSVC_BEGIN(ctiTrampoline PROC)
MSVC_BEGIN(    stmdb sp!, {r1-r3})
MSVC_BEGIN(    stmdb sp!, {r4-r8, lr})
MSVC_BEGIN(    sub sp, sp, ##offset#+4)
MSVC_BEGIN(    mov r4, r2)
MSVC_BEGIN(    mov r5, #512)
MSVC_BEGIN(    mov lr, pc)
MSVC_BEGIN(    bx r0)
MSVC_BEGIN(    add sp, sp, ##offset#+4)
MSVC_BEGIN(    ldmia sp!, {r4-r8, lr})
MSVC_BEGIN(    add sp, sp, #12)
MSVC_BEGIN(    bx lr)
MSVC_BEGIN(ctiTrampoline ENDP)
----------------------------------------

Additionally the difference between en-/disabled JSVALUE32_64 is only the size for the stackpointer.

Maybe we can generate also the trampoline functions all out of one source file?

[Gabor Loki]

The patch looks good to me.
I have only one question. Why the JITSTUBS_OFFSET is hard-coded in the pro file? Could not we use a better approach? like parsing it from JITStups.cpp or Platform.h with the same script?

[Patrick R. Gansterer]

Created attachment 60889 [details]
Patch (current trunk)

(In reply to comment #15)
> Why the JITSTUBS_OFFSET is hard-coded in the pro file? Could not we use a better approach? like parsing it from JITStups.cpp or Platform.h with the same script?
It depends on USE(JSVALUE32_64) and and to get the correct one we need to pass it to a preprecessor with all the defines.
I don't like it there too. Maybe you have a good idea to get rid of it?

[Patrick R. Gansterer]

Created attachment 60895 [details]
alternative Patch (only JITStubs.cpp, no qmake changes)

(In reply to comment #15)
> The patch looks good to me.
> I have only one question. Why the JITSTUBS_OFFSET is hard-coded in the pro file? Could not we use a better approach? like parsing it from JITStups.cpp or Platform.h with the same script?
Maybe we can commit only the changes in JITStubs.cpp in the meantime?

As i know Nokia ships the sourcecode with already generated files. This means that DerivedSources.pro should generate GeneratedJITStubs_RVCT.h, 
GeneratedJITStubs_MSVC32.asm and GeneratedJITStubs_MSVC64.asm at the same time.

[Ismail Donmez]

(In reply to comment #17)
> Created an attachment (id=60895) [details]
> alternative Patch (only JITStubs.cpp, no qmake changes)
> 
> (In reply to comment #15)
> > The patch looks good to me.
> > I have only one question. Why the JITSTUBS_OFFSET is hard-coded in the pro file? Could not we use a better approach? like parsing it from JITStups.cpp or Platform.h with the same script?
> Maybe we can commit only the changes in JITStubs.cpp in the meantime?
> 
> As i know Nokia ships the sourcecode with already generated files. This means that DerivedSources.pro should generate GeneratedJITStubs_RVCT.h, 
> GeneratedJITStubs_MSVC32.asm and GeneratedJITStubs_MSVC64.asm at the same time.

Parsing the offset value from Platform.h would not be so hard, I can try a fix for that if its desired.

[Patrick R. Gansterer]

(In reply to comment #18)
> Parsing the offset value from Platform.h would not be so hard, I can try a fix for that if its desired.
I think we need to genereate all different types of GeneratedJITStubs for Qt.
Which of them to uses must be decided in the JavaScriptCore.pro/pri. GeneratedJITStubs_RVCT.h will be included directly by the JITStubs.cpp. The asm files need an addition buildstep for generating the objectfile. (like PaintHooks.asm in http://trac.webkit.org/browser/trunk/WebCore/WebCore.pro#L3124)

[Ismail Donmez]

Ok then the buildsystem part can wait.

[WebKit Commit Bot]

Comment on attachment 60895 [details]
alternative Patch (only JITStubs.cpp, no qmake changes)

Clearing flags on attachment: 60895

Committed r64618: <http://trac.webkit.org/changeset/64618>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.