The C loop backend isn't (any more?) fully implemented when building with JSVALUE32_64. An implementation of Double2Ints is missing. After providing an implemented for that running the tests results in crashes when running the most basic API tests. As I'm using a big endian platform (ppc) that might also be related to that. Calls to java script functions don't work; the arguments aren't passed correctly (i.e. what is expected in t1 ends up in t3) and (after copying the contents of t3 to t1 using gdb) return values aren't passed correctly as well. I'd be grateful for any hints on where to proceed debugging! For information: On a 64 bit big endian platform (ppc64) the C loop backend builds and mostly works. At least one of the more complex API tests fails (the API tests abort after one fail) but only around 50 of the java script tests fail. (Maybe because of some endianness issue(s)?)
(In reply to comment #0) > The C loop backend isn't (any more?) fully implemented when building with JSVALUE32_64. > An implementation of Double2Ints is missing. > > After providing an implemented for that running the tests results in crashes Tobias, can you attach the implementation here?
Created attachment 180007 [details] Implementation of missing function Double2Ints() Sure, here it comes.
(In reply to comment #2) > Created an attachment (id=180007) [details] > Implementation of missing function Double2Ints() > > Sure, here it comes. Thanks, I think you can follow http://www.webkit.org/coding/contributing.html and get the patch included officially.
Created attachment 183151 [details] make LLInt C Loop backend work on 32 bit platforms, little and big endian
Please confirm/accept this bug (status change), please, so the patch can be reviewed and landed. Anybody who can confirm this bug, please state that here!
I can confirm the latest patch from Tobias fixes JavascriptCore LLInt on another operating system as well: MorphOS (32 bits PPC platform). It would be really nice to integrate this patch in official WebKit repository since currently, all 32bits bigendian platforms are broken.
I could not add this as a patch review, so adding as a comment. This breaks big-endian 64 bits platforms. JSValue pointers are 64 bits on 64 bits platforms [1] so PayloadOffset should only be used on 32 bits platforms, probably using "if JSVALUE64". [1] http://trac.webkit.org/browser/trunk/Source/JavaScriptCore/runtime/JSCJSValue.h#L310
Comment on attachment 183151 [details] make LLInt C Loop backend work on 32 bit platforms, little and big endian Have you tested this on both big and little endian 64-bit platforms?
Comment on attachment 183151 [details] make LLInt C Loop backend work on 32 bit platforms, little and big endian View in context: https://bugs.webkit.org/attachment.cgi?id=183151&action=review I recommend adding to LowLevelInterpreter.asm the following: if JSVALUE64 const CellOffset = 0 else const CellOffset = PayloadOffset end And then use CellOffset instead of PayloadOffset in the places that I marked. > Source/JavaScriptCore/llint/LowLevelInterpreter.asm:266 > + loadp Callee + PayloadOffset[cfr], targetRegister This will break 64-bit big endian. > Source/JavaScriptCore/llint/LowLevelInterpreter.asm:272 > + loadp Callee + PayloadOffset[cfr], targetRegister This will break 64-bit big endian. > Source/JavaScriptCore/llint/LowLevelInterpreter.asm:844 > + loadp ScopeChain + PayloadOffset[cfr], t3 This will break 64-bit big endian.
Thanks for the review! No, I didn't test 64-bit big endian yet. However, bug 111497 broke things again (crash in _llint_op_get_scoped_var, line 1828: https://bugs.webkit.org/attachment.cgi?id=191809&action=diff#a/Source/JavaScriptCore/llint/LowLevelInterpreter32_64.asm_sec1). It seems that deBruijinIndexOperand needs an offset, since t2 doesn't contain a valid pointer when it crashes. I added the usual offset for testing and while it doesn't crash any longer I get assertions, although I don't know if they are related to this problem. What do you think? And what about the other places in LowLevelInterpreter32_64.asm where an immediate is loaded without offset? _llint_op_get_by_pname: ... loadi [cfr, t0, 8], t0 ... _llint_op_switch_imm: ... loadi [t3, t0, 4], t1 ... _llint_op_switch_char: ... loadi [t2, t0, 4], t1 ... Giving those an offset doesn't seem to change behaviour (maybe that code isn't even used when running the CLoop backend?), but I wonder whether an offset would be correct.
Fwiw i'm interested in this too. The Double2Ints part has been commited in bug #112141 / r145551. webkitgtk 2.0.0 blows its stack on ppc at startup now, see bug #113638. I'm currently testing the following diff on OpenBSD/ppc (on top of the other things). Should the PayloadOffset in LowLevelInterpreter32_64.asm use the same CellOffset construct for 64 bits ? $OpenBSD$ https://bugs.webkit.org/show_bug.cgi?id=103128 --- Source/JavaScriptCore/llint/LowLevelInterpreter32_64.asm.orig Sun Mar 31 11:51:25 2013 +++ Source/JavaScriptCore/llint/LowLevelInterpreter32_64.asm Sun Mar 31 11:52:29 2013 @@ -1729,7 +1729,7 @@ _llint_op_next_pname: loadi 20[PC], t2 loadi PayloadOffset[cfr, t2, 8], t2 loadp JSPropertyNameIterator::m_jsStrings[t2], t3 - loadi [t3, t0, 8], t3 + loadi PayloadOffset[t3, t0, 8], t3 addi 1, t0 storei t0, PayloadOffset[cfr, t1, 8] loadi 4[PC], t1 $OpenBSD$ https://bugs.webkit.org/show_bug.cgi?id=103128 --- Source/JavaScriptCore/llint/LowLevelInterpreter.asm.orig Sun Mar 31 11:48:40 2013 +++ Source/JavaScriptCore/llint/LowLevelInterpreter.asm Sun Mar 31 19:00:09 2013 @@ -87,6 +87,12 @@ else const PayloadOffset = 0 end +if JSVALUE64 + const CellOffset = 0 +else + const CellOffset = PayloadOffset +end + # Constant for reasoning about butterflies. const IsArray = 1 const IndexingShapeMask = 30 @@ -263,13 +269,13 @@ macro assertNotConstant(index) end macro functionForCallCodeBlockGetter(targetRegister) - loadp Callee[cfr], targetRegister + loadp Callee + CellOffset[cfr], targetRegister loadp JSFunction::m_executable[targetRegister], targetRegister loadp FunctionExecutable::m_codeBlockForCall[targetRegister], targetRegister end macro functionForConstructCodeBlockGetter(targetRegister) - loadp Callee[cfr], targetRegister + loadp Callee + CellOffset[cfr], targetRegister loadp JSFunction::m_executable[targetRegister], targetRegister loadp FunctionExecutable::m_codeBlockForConstruct[targetRegister], targetRegister end @@ -824,7 +830,7 @@ macro interpretResolveWithBase(opcodeLength, slowPath) getResolveOperation(4, t0) btpz t0, .slowPath - loadp ScopeChain[cfr], t3 + loadp ScopeChain + CellOffset[cfr], t3 # Get the base loadis ResolveOperation::m_operation[t0], t2
Vaguely related, this seems to have been backported to fedora/qtwebkit, with PayloadOffset : http://pkgs.fedoraproject.org/cgit/qtwebkit.git/commit/?id=10c1a5330b3c82bebae8a3093ca6dbd95639917d
(In reply to comment #10) > Thanks for the review! > > No, I didn't test 64-bit big endian yet. > > However, bug 111497 broke things again (crash in _llint_op_get_scoped_var, line 1828: https://bugs.webkit.org/attachment.cgi?id=191809&action=diff#a/Source/JavaScriptCore/llint/LowLevelInterpreter32_64.asm_sec1). > It seems that deBruijinIndexOperand needs an offset, since t2 doesn't contain a valid pointer when it crashes. I added the usual offset for testing and while it doesn't crash any longer I get assertions, although I don't know if they are related to this problem. What do you think? What do you mean by "the usual offset"? > > And what about the other places in LowLevelInterpreter32_64.asm where an immediate is loaded without offset? > _llint_op_get_by_pname: > ... > loadi [cfr, t0, 8], t0 > ... > > _llint_op_switch_imm: > ... > loadi [t3, t0, 4], t1 > ... > > _llint_op_switch_char: > ... > loadi [t2, t0, 4], t1 > ... > Giving those an offset doesn't seem to change behaviour (maybe that code isn't even used when running the CLoop backend?), but I wonder whether an offset would be correct. It depends. Is the thing being loaded typed as int, or typed as JSValue? If it's typed as int, then union rules should put it in low addresses rather than low bits. The get_by_pname case needs PayloadOffset, since it's loading from the call frame, where everything is a JSValue. The op_switch_imm case is loading from an int array, so it should not have an offset. Same for op_switch_char.
Can you post another patch based on my review comments? The point here is not to fix all bugs in one go. It's better to fix them one at a time. I believe your patch is correct modulo my review comments.
Fwiw, the patchset as detailed in comment #11 allows webkitgtk 2.0.0 to build on OpenBSD/ppc, but runtime still seems broken : $jsc-1 > 2+5 Exception: Gmail loads in GtkLauncher, but is utterly slow. Maps.google.fr sorta loads, but doesnt work. Midori doesnt crash, but shows the same symtoms as gtklauncher. I'll know shortly how it fares on sparc64 but i dont have much hope, given how sparc64 has been broken (SIGBUS) at runtime for a while..
Comment on attachment 183151 [details] make LLInt C Loop backend work on 32 bit platforms, little and big endian View in context: https://bugs.webkit.org/attachment.cgi?id=183151&action=review > Source/JavaScriptCore/ChangeLog:8 > + This makes the C Loop backend work on 32 bit platforms (again) Again? What version did it break in? This information is vital when determining if patches like this should be merged to release branches.
(In reply to comment #16) > (From update of attachment 183151 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=183151&action=review > > > Source/JavaScriptCore/ChangeLog:8 > > + This makes the C Loop backend work on 32 bit platforms (again) > > Again? What version did it break in? > > This information is vital when determining if patches like this should be merged to release branches. It should have been "(again?)" indeed, indicating that I don't know if it ever worked in an unreleased version.
I'm sorry but I can't work on this until mid/end of May.
Created attachment 202064 [details] make LLInt C Loop backend work on 32 bit big endian platforms The first step. I still have a problem when using JavaScript in a web browser, see comment 10. I now found that the problem is caused by JSVariableObject::m_registers not containing a valid pointer (it was 0xa here).
Created attachment 202069 [details] make LLInt C Loop backend work on 32 bit big endian platforms the same one as before but this time should apply
With the introduction of the watchdog timer (http://trac.webkit.org/changeset/148639) there appeared a new problem; the size of bool. Currently the code assumes that bool is always 8 bit sized, while this isn't the case for all platforms/compilers. I think little endian platforms should be able to read a 32/64 bit sized bool correctly using a 8 bit load instruction, but big endian platforms do definitely have a problem here as you have to read using the correct instruction in order to get the correct value. As this is a compiler property I guess detection for size of bool has to go into <wtf/Compiler.h>? Source/JavaScriptCore/llint/LowLevelInterpreter.asm: _llint_op_loop_hint: traceExecution() loadp JITStackFrame::vm[sp], t1 - loadb VM::watchdog+Watchdog::m_timerDidFire[t1], t0 + if C_LOOP + loadi VM::watchdog+Watchdog::m_timerDidFire[t1], t0 + else + loadb VM::watchdog+Watchdog::m_timerDidFire[t1], t0 + end btbnz t0, .handleWatchdogTimer .afterWatchdogTimerCheck: checkSwitchToJITForLoop() Obviously my patch is very ugly but at least it makes it work correctly on 32 and 64 bit big endian platforms and helps to illustrate the problem.
Created attachment 202071 [details] make LLInt C Loop backend work on 32 bit big endian platforms the price of messing around manually has to be paid
Created attachment 202072 [details] make LLInt C Loop backend work on 32 bit big endian platforms
Created attachment 202074 [details] make LLInt C Loop backend work on 32 bit big endian platforms last try?
Created attachment 202075 [details] make LLInt C Loop backend work on 32 bit big endian platforms poor build bots and poor people being spammed
Created attachment 202076 [details] make LLInt C Loop backend work on 32 bit big endian platforms The price indeed has to be paid...
32 bit little endian platforms should work now, after http://trac.webkit.org/changeset/145551 , so change the bug title
Created attachment 202088 [details] make LLInt C Loop backend work on 64 bit big endian platforms Here the needed changes for 64 bit big endian platforms.
(In reply to comment #28) > Created an attachment (id=202088) [details] > make LLInt C Loop backend work on 64 bit big endian platforms > > Here the needed changes for 64 bit big endian platforms. Obviously the patch for 64 bit big endian needs to have 32 bit patch applied first, in order to have CellOffset defined.
Comment on attachment 202088 [details] make LLInt C Loop backend work on 64 bit big endian platforms Attachment 202088 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/484703
Comment on attachment 202088 [details] make LLInt C Loop backend work on 64 bit big endian platforms Attachment 202088 [details] did not pass qt-ews (qt): Output: http://webkit-queues.appspot.com/results/481916
Comment on attachment 202088 [details] make LLInt C Loop backend work on 64 bit big endian platforms Attachment 202088 [details] did not pass qt-wk2-ews (qt-wk2): Output: http://webkit-queues.appspot.com/results/488385
Comment on attachment 202088 [details] make LLInt C Loop backend work on 64 bit big endian platforms Attachment 202088 [details] did not pass efl-wk2-ews (efl-wk2): Output: http://webkit-queues.appspot.com/results/495298
Comment on attachment 202088 [details] make LLInt C Loop backend work on 64 bit big endian platforms Attachment 202088 [details] did not pass efl-ews (efl): Output: http://webkit-queues.appspot.com/results/488391
Comment on attachment 202088 [details] make LLInt C Loop backend work on 64 bit big endian platforms Attachment 202088 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/493305
Comment on attachment 202088 [details] make LLInt C Loop backend work on 64 bit big endian platforms Attachment 202088 [details] did not pass gtk-ews (gtk): Output: http://webkit-queues.appspot.com/results/493343
Running a 64 bit web browser shows a maybe similar problem as the 32 bit version (see comment 10): The macro putToBaseVariableBody (LowLevelInterpreter.asm:551) crashes when executing: loadp JSVariableObject::m_registers[scratch1], scratch1 // line 554 When I change line 553 loadp PayloadOffset[cfr, scratch1, 8], scratch1 // line 553 to loadp CellOffset[cfr, scratch1, 8], scratch1 it doesn't crash anymore in the same line but nevertheless the value it reads is not a valid pointer. It does then crash in line 555, loadisFromInstruction(3, scratch2) which should be completely unrelated. In fact it seems that the arguments passed to macro aren't valid. Unlike the problem in comment 10 this one does always happen.
Created attachment 202651 [details] the size of bool is platform dependent Turned out the problems I had running the browser were also because of 4 byte bools. I post this patch just in case you consider it relevant.
What is the status of this patchset ? I was applying https://bugs.webkit.org/attachment.cgi?id=202076 on top of webkitgtk 2.0.4 to fix runtime on openbsd/powerpc, but with 2.2.0 it seems not enough anymore, it now blows in llint::execute() again. Tobias, does your analysis from comment #21 onwards fixes that too re bool size, and patch from https://bugs.webkit.org/attachment.cgi?id=202076 should also be applied on top, and not only for MacOS but for all powerpcs ?
Created attachment 213511 [details] make LLInt C Loop backend work on 32/64 bit big endian platforms This patch integrates all former ones
Created attachment 213512 [details] llint cloop patches for WebKit 537 series This is the patch needed to get the C Loop backend work on 32/64 bit big endian platforms.
(In reply to comment #39) > What is the status of this patchset ? I was applying https://bugs.webkit.org/attachment.cgi?id=202076 on top of webkitgtk 2.0.4 to fix runtime on openbsd/powerpc, but with 2.2.0 it seems not enough anymore, it now blows in llint::execute() again. Tobias, does your analysis from comment #21 onwards fixes that too re bool size, and patch from https://bugs.webkit.org/attachment.cgi?id=202076 should also be applied on top, and not only for MacOS but for all powerpcs ? I posted my two current versions of the needed patches, one for the stable 537 series and one for the current development 538 series of the sources. I couldn't find any information about 4 byte bools being the default on PowerPC platforms other than OS X. I'm not entirely sure whether the 64 bit version in the 538 series is actually working but I think it did at some point.
Can the patches be applied unconditionally or they will break if applied in other arches than ppc?
Created attachment 248357 [details] make LLInt C Loop backend work on 32/64 bit big endian platforms (plus YARR JIT big endian fixes) This patch is against the 600.5 branch. It should be compatible with any other OS and/or CPU architecture.
I didn't know this was still unfixed upstream. We have a similar patch in QtWebKit to fix big-endian builds.
(In reply to comment #45) > I didn't know this was still unfixed upstream. We have a similar patch in > QtWebKit to fix big-endian builds. I just now found bug 137020. That bug got committed the same day it was filed - and this one was waiting here for years. My patch solves quite some more issues that apparently don't exist anymore in the current trunk - but they do in the safari-600.5-branch that I'm currently stuck at. If I ever catch up with trunk again I'll see what's still necessary and file some new bugs for that - at least I'll need compatibility with 32 bit bools. There still are potentially interesting pieces for you like the change to Source/JavaScriptCore/llint/LLIntSlowPaths.cpp which might be needed when using CLoop only (https://bugs.webkit.org/attachment.cgi?id=248357&action=diff#a/tags/Safari-600.5.10/Source/JavaScriptCore/llint/LLIntSlowPaths.cpp_sec1). In debug builds I got occasional assertions without this. Or the changes I made to Source/JavaScriptCore/runtime/DataView.h in order to make the DataView object behave more endian independent (https://bugs.webkit.org/attachment.cgi?id=248357&action=diff#a/tags/Safari-600.5.10/Source/JavaScriptCore/runtime/DataView.h_sec1). Regarding my change to Source/JavaScriptCore/runtime/Arguments.h I hope it isn't necessary anymore but in case of yet more occasional assertions in JavaScriptCore it might help (https://bugs.webkit.org/attachment.cgi?id=248357&action=diff#a/tags/Safari-600.5.10/Source/JavaScriptCore/runtime/Arguments.h_sec1). Again in debug builds I got occasional assertions without this. *** This bug has been marked as a duplicate of bug 137020 ***
https://bugs.webkit.org/show_bug.cgi?id=137020 did address 64bit nor YARR. Then again even powerpc is usually little-endian in 64bit distros (at least it is in Debian).
(In reply to comment #47) > https://bugs.webkit.org/show_bug.cgi?id=137020 did address 64bit nor YARR. > Then again even powerpc is usually little-endian in 64bit distros (at least > it is in Debian). Last time I tested (which was a long while ago) there weren't any changes needed any more for 64 bit big endian powerpc - apart from 32 bit bool compatibility. In case you've got a MacroAssembler implementation for some big endian architecture, you'd need the YARR JIT changes in order to make YARR JIT work correctly. I do have one for PPC OS X (ported from TenFourFox - Firefox port for PPC OS X) but I'll probably never be able to make it ready for getting committed. By the way I've also got the CSS selector JIT working with that MacroAssembler on PPC OS X.
(In reply to comment #48) > Last time I tested (which was a long while ago) there weren't any changes > needed any more for 64 bit big endian powerpc - apart from 32 bit bool > compatibility. And now I realize that 32 bit bool compatibility isn't even used nor needed for 64 bit PPC OS X - I added the changes to LowLevelInterpreter64.asm for completeness only.