Bug 137020 – [CLoop] - Fix CLoop on the 32-bit Big-Endians

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

[Tomas Popela]

The LLInt CLoop crashes on various places on 32-bit Big-Endians.

[Tomas Popela]

Created attachment 238528 [details]
Patch

Proposed patch

[Mark Lam]

Comment on attachment 238528 [details]
Patch

r=me

[WebKit Commit Bot]

Comment on attachment 238528 [details]
Patch

Clearing flags on attachment: 238528

Committed r173886: <http://trac.webkit.org/changeset/173886>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Tobias Netzel]

*** Bug 103128 has been marked as a duplicate of this bug. ***

[Alberto Garcia]

(In reply to comment #0)
> The LLInt CLoop crashes on various places on 32-bit Big-Endians.

Hey Tomas, this seems to affect the 2.4 branch of WebkitGTK, however the patch
does not apply cleanly.

In particular these changes:

http://trac.webkit.org/changeset/173886/trunk/Source/JavaScriptCore/llint/LowLevelInterpreter32_64.asm

They don't apply because the code in the 2.4 branch does not include the patch written for bug 136894 ( http://trac.webkit.org/changeset/173706 ).

I'm not familiar with that code, do you know if it's possible to port the patch easily?

[Tobias Netzel]

Created attachment 249198 [details]
Patch against 538.15, which was the base for the 2.4 branch of WebkitGTK

(In reply to comment #6)
This attached patch was what I had in place when 538.15 was current, which was the base for the WebkitGTK 2.4 branch.
You'll only need the FOUR_BYTE_BOOL changes if your target OS ABI specifies 32 bits for bools.
The YarrJIT big endian changes are only useful in case you've got a MacroAssembler implementation as well.

I'm not sure whether this patch made it work 100%; you should test and in case you've got further difficulties you could send me an e-mail.
I've made big endian C Loop patches against most Safari version tags until 600.5.x as part of working on a PowerPC OS X fork.

[Tomas Popela]

(In reply to comment #6)
> (In reply to comment #0)
> > The LLInt CLoop crashes on various places on 32-bit Big-Endians.
> 
> Hey Tomas, this seems to affect the 2.4 branch of WebkitGTK, however the
> patch
> does not apply cleanly.
> 
> In particular these changes:
> 
> http://trac.webkit.org/changeset/173886/trunk/Source/JavaScriptCore/llint/
> LowLevelInterpreter32_64.asm
> 
> They don't apply because the code in the 2.4 branch does not include the
> patch written for bug 136894 ( http://trac.webkit.org/changeset/173706 ).
> 
> I'm not familiar with that code, do you know if it's possible to port the
> patch easily?

Hi Berto,
please look at our Fedora webkitgtk3 package[0]. With patches that are there the CLoop is working on PPC(64)(LE) and S390(X).

[0] - http://pkgs.fedoraproject.org/cgit/webkitgtk3.git/tree/

[Mark Lam]

Comment on attachment 238528 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=238528&action=review

> Source/JavaScriptCore/llint/LowLevelInterpreter.asm:540
> -    loadp Callee[cfr], targetRegister
> +    if JSVALUE64
> +        loadp Callee[cfr], targetRegister
> +    else
> +        loadp Callee + PayloadOffset[cfr], targetRegister
> +    end

I’m thinking that for 64-bit, PayloadOffset will always be 0 anyway.  Hence, this change should not be needed.  Is that not the case?

> Source/JavaScriptCore/llint/LowLevelInterpreter.asm:550
> -    loadp Callee[cfr], targetRegister
> +    if JSVALUE64
> +        loadp Callee[cfr], targetRegister
> +    else
> +        loadp Callee + PayloadOffset[cfr], targetRegister
> +    end

Ditto.

[Filip Pizlo]

(In reply to comment #9)
> Comment on attachment 238528 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=238528&action=review
> 
> > Source/JavaScriptCore/llint/LowLevelInterpreter.asm:540
> > -    loadp Callee[cfr], targetRegister
> > +    if JSVALUE64
> > +        loadp Callee[cfr], targetRegister
> > +    else
> > +        loadp Callee + PayloadOffset[cfr], targetRegister
> > +    end
> 
> I’m thinking that for 64-bit, PayloadOffset will always be 0 anyway.  Hence,
> this change should not be needed.  Is that not the case?

Nope, PayloadOffset is non-zero in 64-bit.  We use it to mean the following in 64-bit: if you stored a JSValue containing an int32, then what is the offset of the int32?

However, this change is really dirty and I object to the approach.  I recommend having a CellOffset defined, whose meaning is: PayloadOffset on 32-bit and 0 on 64-bit.  I believe that I've made a similar comment on a related bug.

> 
> > Source/JavaScriptCore/llint/LowLevelInterpreter.asm:550
> > -    loadp Callee[cfr], targetRegister
> > +    if JSVALUE64
> > +        loadp Callee[cfr], targetRegister
> > +    else
> > +        loadp Callee + PayloadOffset[cfr], targetRegister
> > +    end
> 
> Ditto.

Ditto from my end.  Instead of adding "if"'s everywhere, just abstract what you're really after: the CellOffset.

[Tobias Netzel]

(In reply to comment #10)
> However, this change is really dirty and I object to the approach.  I
> recommend having a CellOffset defined, whose meaning is: PayloadOffset on
> 32-bit and 0 on 64-bit.  I believe that I've made a similar comment on a
> related bug.

Yes, that was bug 103128 which I marked as a duplicate of this one.

CLoop seems to be broken again in current trunk on 32bits bigendian architectures (at least it is in r183644).

I didn't have time to track WebKit changes during the last months, but i notice there was a related fix in r173886, though i don't know if it had been tested. In any case, either that fix wasn't enough, or something broke it since.

Tobias, any idea? I know you're already quite busy and somewhat stucked with 600.x branch.

Can this ticket be reopened or should a new one be created?

[Mark Lam]

(In reply to comment #13)
> Can this ticket be reopened or should a new one be created?

If it’s new issue, open a new bug.  If the fix for this bug was ineffective, then reopen this one.

The fix might have been effective in r173xxx, but it's not anymore. Cloop is broken again on 32 bits bigendian platforms.

Since I don't have permissions to reopen this bug, can someone do it for me? Thanks!

[Mark Lam]

(In reply to comment #15)
> The fix might have been effective in r173xxx, but it's not anymore. Cloop is
> broken again on 32 bits bigendian platforms.
> 
> Since I don't have permissions to reopen this bug, can someone do it for me?
> Thanks!

It sounds like there’s a new issue though it may pertain to endianness.  Please go ahead and open a new bug.  Please cc me so that I can keep an eye on it as well.  Thanks.