Bug 46095

Summary:

MacroAssemblerARM generates unaligned loads

Product:

WebKit

Reporter:

David Tapuska <dave+webkit>

Component:

JavaScriptCore

Assignee:

Nobody <webkit-unassigned>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

barraclough, commit-queue, eric, loki, staikos, thomas, zherczeg

Priority:

Version:

528+ (Nightly build)

Hardware:

Other

OS:

Other

URL:

http://acid3.acidtests.org

Attachments:

Description	Flags
Fix alignment failures	none
Add new setTest32 with RegisterID parameter	barraclough: review-, barraclough: commit-queue-
Re-apply patch to set32Test8 as per comments	none

David Tapuska

Reported 2010-09-20 08:37:28 PDT

While running acid3.acidtests.org on a ARMv5 processor a ldr r8,[r1, #7] will be generated This instruction should be ldrb r8,[r1, #7] The problem code is the set8 and setTest8 implementations call load32 where they should be calling load8.

Attachments
Fix alignment failures (2.78 KB, patch) 2010-09-20 10:10 PDT, David Tapuska	no flags	Details Formatted Diff Diff
Add new setTest32 with RegisterID parameter (3.51 KB, patch) 2010-09-21 06:55 PDT, David Tapuska	barraclough: review- barraclough: commit-queue-	Details Formatted Diff Diff
Re-apply patch to set32Test8 as per comments (3.34 KB, patch) 2011-01-31 10:29 PST, David Tapuska	no flags	Details Formatted Diff Diff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.

David Tapuska

Comment 1 2010-09-20 10:10:50 PDT

Created attachment 68102 [details] Fix alignment failures

Zoltan Herczeg

Comment 2 2010-09-20 11:53:56 PDT

Good fix. Could you add a common function for the common code of setTest8 and setTest32 to avoid code duplication?

David Tapuska

Comment 3 2010-09-21 06:55:44 PDT

Created attachment 68235 [details] Add new setTest32 with RegisterID parameter

Oliver Hunt

Comment 4 2010-10-27 13:25:26 PDT

Comment on attachment 68235 [details] Add new setTest32 with RegisterID parameter Have you tested perf impact of this on non-armv5?

David Tapuska

Comment 5 2010-10-27 13:46:27 PDT

(In reply to comment #4) > (From update of attachment 68235 [details]) > Have you tested perf impact of this on non-armv5? No I didn't measure a pref impact. The old code was just wrong; as the fill on the register would be wrong because it would be using data around the address instead of letting the core fill in zeros into the register in top 3 bytes. So one could expect an incorrect branch occasionally and possible data exposure.

Eric Seidel (no email)

Comment 6 2010-12-03 13:33:34 PST

This seems like an easy patch for gbarra to review.

Gavin Barraclough

Comment 7 2010-12-04 16:04:53 PST

Comment on attachment 68235 [details] Add new setTest32 with RegisterID parameter Hi David, These methods had been rather poorly named, and it was rather unclear what they were doing. I've hopefully just cleared this up a little: https://bugs.webkit.org/show_bug.cgi?id=50509 Looking at your patch, I think your changes to (the method previously known as) setTest8 are correct, but the change to set8 is incorrect, and should be reverted. Please update & check your change. cheers, G.

David Tapuska

Comment 8 2011-01-31 10:29:11 PST

Created attachment 80657 [details] Re-apply patch to set32Test8 as per comments Applied patch as per comments.

Gavin Barraclough

Comment 9 2011-01-31 10:36:21 PST

Comment on attachment 80657 [details] Re-apply patch to set32Test8 as per comments Looks great David.

WebKit Commit Bot

Comment 10 2011-02-01 06:40:15 PST

Comment on attachment 80657 [details] Re-apply patch to set32Test8 as per comments Clearing flags on attachment: 80657 Committed r77248: <http://trac.webkit.org/changeset/77248>

WebKit Commit Bot

Comment 11 2011-02-01 06:40:21 PST

All reviewed patches have been landed. Closing bug.

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