Bug 195181

Summary: cloop.rb shift mask should depend on the word size being shifted.
Product: WebKit Reporter: Mark Lam <mark.lam>
Component: JavaScriptCoreAssignee: Mark Lam <mark.lam>
Status: RESOLVED FIXED    
Severity: Normal CC: ews-watchlist, keith_miller, mitya57, msaboff, ryanhaddad, saam, webkit-bug-importer, ysuzuki
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
proposed patch. ysuzuki: review+

Mark Lam
Reported 2019-02-28 12:29:41 PST
Previously, we're always masking the shift amount with 0x1f. This only works for 32-bit words. For 64-bit words, the mask should be 0x3f. For intptr_t shifts, the mask depends on sizeof(uintptr_t).
Attachments
proposed patch. (2.06 KB, patch)
2019-02-28 12:35 PST, Mark Lam 		ysuzuki: review+
DetailsFormatted DiffDiff
View All Add attachment proposed patch, testcase, etc.
Radar WebKit Bug Importer
Comment 1 2019-02-28 12:30:06 PST
<rdar://problem/48484164>
Mark Lam
Comment 2 2019-02-28 12:35:11 PST
Created attachment 363250 [details] proposed patch.
Yusuke Suzuki
Comment 3 2019-02-28 12:45:10 PST
Comment on attachment 363250 [details] proposed patch. View in context: https://bugs.webkit.org/attachment.cgi?id=363250&action=review r=me > Source/JavaScriptCore/offlineasm/cloop.rb:429 > + shiftMask = "((sizeof(uintptr_t) == 8) ? 0x3f : 0x1f)" if type == :int || type == :uint Discussed with Mark. The type ":int" and ":uint" actually means "intptr_t" and "uintptr_t". Then this is correct. It is nice if we have either of 1. Add a comment about it here now, and rename ":int" => ":intPtr" or something like that in a subsequent patch. 2. Rename them in this patch too.
Mark Lam
Comment 4 2019-02-28 12:49:35 PST
Thanks for the review. I added the FIXME. Landed in r242215: <http://trac.webkit.org/r242215>.
Mark Lam
Comment 5 2019-02-28 12:51:16 PST
*** Bug 195175 has been marked as a duplicate of this bug. ***
Alberto Garcia
Comment 6 2019-07-17 05:58:56 PDT
*** Bug 199684 has been marked as a duplicate of this bug. ***
Note You need to log in before you can comment on or make changes to this bug.