Bug 103128 – LLInt C Loop backend should work on big endian platforms both 32 and 64 bit

• Component: JavaScriptCore
• Status: RESOLVED DUPLICATE

[Tobias Netzel]

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

[Dan Horák]

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

[Tobias Netzel]

Created attachment 180007 [details]
Implementation of missing function Double2Ints()

Sure, here it comes.

[Dan Horák]

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

[Tobias Netzel]

Created attachment 183151 [details]
make LLInt C Loop backend work on 32 bit platforms, little and big endian

[Tobias Netzel]

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.

[Gustavo Luiz Duarte]

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

[Filip Pizlo]

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?

[Filip Pizlo]

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.

[Tobias Netzel]

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.

[Landry Breuil]

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

[Landry Breuil]

Vaguely related, this seems to have been backported to fedora/qtwebkit, with PayloadOffset : http://pkgs.fedoraproject.org/cgit/qtwebkit.git/commit/?id=10c1a5330b3c82bebae8a3093ca6dbd95639917d

[Filip Pizlo]

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

[Filip Pizlo]

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.

[Landry Breuil]

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

[Allan Sandfeld Jensen]

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.

[Tobias Netzel]

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

[Tobias Netzel]

I'm sorry but I can't work on this until mid/end of May.

[Tobias Netzel]

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

[Tobias Netzel]

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

[Tobias Netzel]

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.

[Tobias Netzel]

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

[Tobias Netzel]

Created attachment 202072 [details]
make LLInt C Loop backend work on 32 bit big endian platforms

[Tobias Netzel]

Created attachment 202074 [details]
make LLInt C Loop backend work on 32 bit big endian platforms

last try?

[Tobias Netzel]

Created attachment 202075 [details]
make LLInt C Loop backend work on 32 bit big endian platforms

poor build bots and poor people being spammed

[Tobias Netzel]

Created attachment 202076 [details]
make LLInt C Loop backend work on 32 bit big endian platforms

The price indeed has to be paid...

[Tobias Netzel]

32 bit little endian platforms should work now, after http://trac.webkit.org/changeset/145551 , so change the bug title

[Tobias Netzel]

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.

[Tobias Netzel]

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

[Build Bot]

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

[Early Warning System Bot]

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

[Early Warning System Bot]

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

[EFL EWS Bot]

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

[EFL EWS Bot]

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

[Build Bot]

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

[kov's GTK+ EWS bot]

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

[Tobias Netzel]

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.

[Tobias Netzel]

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.

[Landry Breuil]

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 ?

[Tobias Netzel]

Created attachment 213511 [details]
make LLInt C Loop backend work on 32/64 bit big endian platforms

This patch integrates all former ones

[Tobias Netzel]

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.

[Tobias Netzel]

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

[Pacho Ramos]

Can the patches be applied unconditionally or they will break if applied in other arches than ppc?

[Tobias Netzel]

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.

[Allan Sandfeld Jensen]

I didn't know this was still unfixed upstream. We have a similar patch in QtWebKit to fix big-endian builds.

[Tobias Netzel]

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

[Allan Sandfeld Jensen]

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

[Tobias Netzel]

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

[Tobias Netzel]

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