Bug 151514

Summary:

[ARMv7] stress/op_rshift.js and stress/op_urshift.js are failing.

Product:

WebKit

Reporter:

Mark Lam <mark.lam>

Component:

JavaScriptCore

Assignee:

Mark Lam <mark.lam>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

benjamin, fpizlo, ggaren, keith_miller, msaboff, ossy, saam, webkit-bug-importer

Priority:

Keywords:

InRadar

Version:

WebKit Local Build

Hardware:

Unspecified

OS:

Unspecified

Bug Depends on:

Bug Blocks:

108645

Attachments:

Description	Flags
proposed patch.	fpizlo: review+

Mark Lam

Reported 2015-11-20 14:13:18 PST

These newly added tests shows that we have a latent bug somewhere in the 32-bit ARM JITs.

Attachments
proposed patch. (4.91 KB, patch) 2016-02-15 12:10 PST, Mark Lam	fpizlo: review+	Details Formatted Diff Diff
View All Add attachment proposed patch, testcase, etc.

Radar WebKit Bug Importer

Comment 1 2015-11-20 14:14:36 PST

<rdar://problem/23636019>

Mark Lam

Comment 2 2015-11-20 15:47:03 PST

The tests have been temporarily skipped in r192708: <http://trac.webkit.org/r192708>.

Mark Lam

Comment 3 2016-02-15 11:57:18 PST

The issue turns out to be trivial: on ARMv7 (and traditional ARM too), arithmetic shift right (ASR) and logical shift right (LSR) takes an immediate shift amount from 1-32. See http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.dui0204j/Cjacbgca.html. An immediate shift amount of 0 is interpreted as a shift of 32 bits. Meanwhile, our assembler is expecting the immediate shift value to be between 0-31. As a result, a shift amount of 0 is being wrongly encoded with 0 bits which means shift right by 32 bits. The fix is to check if the shift amount is 0, and if so, emit a move. Else, emit the right shift as usual. This issue does not affect left shifts, as the immediate shift amount for left shifts is between 0-31 as our JIT expects.

Mark Lam

Comment 4 2016-02-15 12:10:42 PST

Created attachment 271354 [details] proposed patch.

Filip Pizlo

Comment 5 2016-02-15 12:11:45 PST

Comment on attachment 271354 [details] proposed patch. View in context: https://bugs.webkit.org/attachment.cgi?id=271354&action=review > Source/JavaScriptCore/assembler/MacroAssemblerARM.h:278 > + move(src, dest); This is wrong. Please use a zero-extending move. > Source/JavaScriptCore/assembler/MacroAssemblerARM.h:304 > + move(src, dest); Ditto. > Source/JavaScriptCore/assembler/MacroAssemblerARMv7.h:404 > + move(src, dest); Ditto. > Source/JavaScriptCore/assembler/MacroAssemblerARMv7.h:432 > + move(src, dest); Ditto.

Mark Lam

Comment 6 2016-02-15 12:14:16 PST

These changes are in MacroAssemblerARM.h and MacroAssemblerARMv7.h. These operations are 32-bit only. Why do we need a zero extending move? (In reply to comment #5) > Comment on attachment 271354 [details] > proposed patch. > > View in context: > https://bugs.webkit.org/attachment.cgi?id=271354&action=review > > > Source/JavaScriptCore/assembler/MacroAssemblerARM.h:278 > > + move(src, dest); > > This is wrong. Please use a zero-extending move. > > > Source/JavaScriptCore/assembler/MacroAssemblerARM.h:304 > > + move(src, dest); > > Ditto. > > > Source/JavaScriptCore/assembler/MacroAssemblerARMv7.h:404 > > + move(src, dest); > > Ditto. > > > Source/JavaScriptCore/assembler/MacroAssemblerARMv7.h:432 > > + move(src, dest); > > Ditto.

Filip Pizlo

Comment 7 2016-02-15 12:21:22 PST

(In reply to comment #6) > These changes are in MacroAssemblerARM.h and MacroAssemblerARMv7.h. These > operations are 32-bit only. Why do we need a zero extending move? > > (In reply to comment #5) > > Comment on attachment 271354 [details] > > proposed patch. > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=271354&action=review > > > > > Source/JavaScriptCore/assembler/MacroAssemblerARM.h:278 > > > + move(src, dest); > > > > This is wrong. Please use a zero-extending move. > > > > > Source/JavaScriptCore/assembler/MacroAssemblerARM.h:304 > > > + move(src, dest); > > > > Ditto. > > > > > Source/JavaScriptCore/assembler/MacroAssemblerARMv7.h:404 > > > + move(src, dest); > > > > Ditto. > > > > > Source/JavaScriptCore/assembler/MacroAssemblerARMv7.h:432 > > > + move(src, dest); > > > > Ditto. OMG you're right. Please ignore me.

Mark Lam

Comment 8 2016-02-15 13:09:46 PST

Thanks for the review. Landed in r196591: <http://trac.webkit.org/r196591>.

Note You need to log in before you can comment on or make changes to this bug.