Summary: | [CLoop] - Fix CLoop on the 32-bit Big-Endians | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Tomas Popela <tpopela> | ||||||
Component: | JavaScriptCore | Assignee: | 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
Tomas Popela
2014-09-23 04:27:26 PDT
Created attachment 238528 [details]
Patch
Proposed patch
Comment on attachment 238528 [details]
Patch
r=me
Comment on attachment 238528 [details] Patch Clearing flags on attachment: 238528 Committed r173886: <http://trac.webkit.org/changeset/173886> All reviewed patches have been landed. Closing bug. *** Bug 103128 has been marked as a duplicate of this bug. *** (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? 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. (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 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. (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. (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? (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! (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. |