Bug 152833 - ARMv7 or32(TrustedImm32, AbsoluteAddress) may have a bug with its use of dataTempRegister.
Summary: ARMv7 or32(TrustedImm32, AbsoluteAddress) may have a bug with its use of data...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mark Lam
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-01-07 09:12 PST by Mark Lam
Modified: 2016-01-07 12:22 PST (History)
6 users (show)

See Also:


Attachments
proposed fix. (5.02 KB, patch)
2016-01-07 09:34 PST, Mark Lam
msaboff: review-
Details | Formatted Diff | Diff
follow up fix for ARM64. (1.89 KB, patch)
2016-01-07 12:17 PST, Mark Lam
msaboff: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mark Lam 2016-01-07 09:12:01 PST
Details in the coming patch.
Comment 1 Mark Lam 2016-01-07 09:34:53 PST
Created attachment 268457 [details]
proposed fix.
Comment 2 Mark Lam 2016-01-07 11:25:25 PST
Comment on attachment 268457 [details]
proposed fix.

This patch has passed the JSC tests on ARM64 and ARMv7.  It's ready for a review.
Comment 3 Mark Lam 2016-01-07 11:43:41 PST
Thanks for the review.  Landed in r194707: <http://trac.webkit.org/r194707>.
Comment 4 Michael Saboff 2016-01-07 11:52:29 PST
Comment on attachment 268457 [details]
proposed fix.

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

r-

> Source/JavaScriptCore/assembler/MacroAssemblerARM64.h:548
> +            or32(imm, memoryTempRegister, memoryTempRegister);
> +            store32(memoryTempRegister, address.m_ptr);

You can't store32 with a source of memoryTempRegister as it will be needed to materialize the address.

You could change this to:
    or32(imm, memoryTempRegister, getCachedDataTempRegisterIDAndInvalidate());
    store32(dataTempRegister, address.m_ptr);

Also, please add an ASSERT() to template<int datasize> store() that the src register cannot be memoryTempRegister.
Comment 5 Mark Lam 2016-01-07 12:17:33 PST
Created attachment 268473 [details]
follow up fix for ARM64.
Comment 6 Michael Saboff 2016-01-07 12:19:03 PST
Comment on attachment 268473 [details]
follow up fix for ARM64.

r=me
Comment 7 Mark Lam 2016-01-07 12:22:24 PST
Thanks for the review.  Follow up fix landed in r194711: <http://trac.webkit.org/r194711>.