Bug 137020

Summary: [CLoop] - Fix CLoop on the 32-bit Big-Endians
Product: WebKit Reporter: Tomas Popela <tpopela>
Component: JavaScriptCoreAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: berto, commit-queue, fabien.coeurjoly, fpizlo, mark.lam, mhahnenb, tobias.netzel
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch against 538.15, which was the base for the 2.4 branch of WebkitGTK none

Description Tomas Popela 2014-09-23 04:27:26 PDT
The LLInt CLoop crashes on various places on 32-bit Big-Endians.
Comment 1 Tomas Popela 2014-09-23 04:30:47 PDT
Created attachment 238528 [details]
Patch

Proposed patch
Comment 2 Mark Lam 2014-09-23 12:45:40 PDT
Comment on attachment 238528 [details]
Patch

r=me
Comment 3 WebKit Commit Bot 2014-09-23 13:21:58 PDT
Comment on attachment 238528 [details]
Patch

Clearing flags on attachment: 238528

Committed r173886: <http://trac.webkit.org/changeset/173886>
Comment 4 WebKit Commit Bot 2014-09-23 13:22:01 PDT
All reviewed patches have been landed.  Closing bug.
Comment 5 Tobias Netzel 2015-03-10 15:10:49 PDT
*** Bug 103128 has been marked as a duplicate of this bug. ***
Comment 6 Alberto Garcia 2015-03-22 06:36:49 PDT
(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?
Comment 7 Tobias Netzel 2015-03-22 09:19:07 PDT
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.
Comment 8 Tomas Popela 2015-03-23 00:20:35 PDT
(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/
Comment 9 Mark Lam 2015-03-23 21:29:44 PDT
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.
Comment 10 Filip Pizlo 2015-03-23 21:35:01 PDT
(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.
Comment 11 Tobias Netzel 2015-03-24 06:35:21 PDT
(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.
Comment 12 fabien.coeurjoly 2015-05-13 02:21:04 PDT
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.
Comment 13 fabien.coeurjoly 2015-05-19 00:13:15 PDT
Can this ticket be reopened or should a new one be created?
Comment 14 Mark Lam 2015-05-19 00:24:55 PDT
(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.
Comment 15 fabien.coeurjoly 2015-05-19 01:12:31 PDT
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!
Comment 16 Mark Lam 2015-05-20 18:15:18 PDT
(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.