Bug 168350

Summary: JSC: missing implementations of MacroAssemblerMIPS::load8SignedExtendTo32()
Product: WebKit Reporter: Guillaume Emont <guijemont>
Component: JavaScriptCoreAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, keith_miller, mark.lam, msaboff, ossy, saam
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Other   
OS: Linux   
Attachments:
Description Flags
Patch
none
Patch none

Description Guillaume Emont 2017-02-14 19:01:02 PST 
Some implementations of MacroAssemblerMIPS::load8SignedExtendTo32() are missing, which prevents compilation on MIPS.
Comment 1 Guillaume Emont 2017-02-14 19:23:53 PST 
Created attachment 301574 [details]
Patch

Proposed patch to add missing implementations.
Comment 2 Guillaume Emont 2017-02-14 19:24:57 PST 
I'm in the process of sending a bunch of patches that make MIPS work again, more to come tomorrow.
Comment 3 Csaba Osztrogonác 2017-02-15 08:42:03 PST 
Just a note, this function is needed 3 months ago - https://trac.webkit.org/changeset/208450. Wouldn't be better to have a MIPS buildbot to notice similar breakages in time?
Comment 4 Guillaume Emont 2017-02-15 08:55:52 PST 
(In reply to comment #3)
> Just a note, this function is needed 3 months ago -
> https://trac.webkit.org/changeset/208450. Wouldn't be better to have a MIPS
> buildbot to notice similar breakages in time?

Absolutely! That's why we are working on that, but first we wanted to get things to a state where they build.
Comment 5 Yusuke Suzuki 2017-02-15 08:58:48 PST 
Comment on attachment 301574 [details]
Patch

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

r=me with a fix.

> Source/JavaScriptCore/assembler/MacroAssemblerMIPS.h:764
> +    }

Looks good.

> Source/JavaScriptCore/assembler/MacroAssemblerMIPS.h:796
> +        load8(address.m_ptr, dest);

Why is it OK? I think we should use load8SignedExtendTo32 here (and delegate to `void load8SignedExtendTo32(const void* address, RegisterID dest)`), correct?

> Source/JavaScriptCore/assembler/MacroAssemblerMIPS.h:806
> +        m_assembler.lb(dest, addrTempRegister, 0);

OK. The difference from load8 is using lb instead of lbu.
Comment 6 Guillaume Emont 2017-02-15 11:45:15 PST 
Created attachment 301639 [details]
Patch

Corrected patch, thanks for the review!
Comment 7 Yusuke Suzuki 2017-02-15 19:59:30 PST 
Comment on attachment 301639 [details]
Patch

r=me
Comment 8 WebKit Commit Bot 2017-02-15 20:25:03 PST 
Comment on attachment 301639 [details]
Patch

Clearing flags on attachment: 301639

Committed r212419: <http://trac.webkit.org/changeset/212419>
Comment 9 WebKit Commit Bot 2017-02-15 20:25:08 PST 
All reviewed patches have been landed.  Closing bug.