Bug 168350 - JSC: missing implementations of MacroAssemblerMIPS::load8SignedExtendTo32()
Summary: JSC: missing implementations of MacroAssemblerMIPS::load8SignedExtendTo32()
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Other Linux
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-02-14 19:01 PST by Guillaume Emont
Modified: 2017-02-15 20:25 PST (History)
6 users (show)

See Also:


Attachments
Patch (2.69 KB, patch)
2017-02-14 19:23 PST, Guillaume Emont
no flags Details | Formatted Diff | Diff
Patch (2.70 KB, patch)
2017-02-15 11:45 PST, Guillaume Emont
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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.