Bug 46095 - MacroAssemblerARM generates unaligned loads
Summary: MacroAssemblerARM generates unaligned loads
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other Other
: P2 Normal
Assignee: Nobody
URL: http://acid3.acidtests.org
Keywords:
Depends on:
Blocks:
 
Reported: 2010-09-20 08:37 PDT by David Tapuska
Modified: 2011-02-01 06:40 PST (History)
7 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description David Tapuska 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.
Comment 1 David Tapuska 2010-09-20 10:10:50 PDT
Created attachment 68102 [details]
Fix alignment failures
Comment 2 Zoltan Herczeg 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?
Comment 3 David Tapuska 2010-09-21 06:55:44 PDT
Created attachment 68235 [details]
Add new setTest32 with RegisterID parameter
Comment 4 Oliver Hunt 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?
Comment 5 David Tapuska 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.
Comment 6 Eric Seidel (no email) 2010-12-03 13:33:34 PST
This seems like an easy patch for gbarra to review.
Comment 7 Gavin Barraclough 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.
Comment 8 David Tapuska 2011-01-31 10:29:11 PST
Created attachment 80657 [details]
Re-apply patch to set32Test8 as per comments

Applied patch as per comments.
Comment 9 Gavin Barraclough 2011-01-31 10:36:21 PST
Comment on attachment 80657 [details]
Re-apply patch to set32Test8 as per comments

Looks great David.
Comment 10 WebKit Commit Bot 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>
Comment 11 WebKit Commit Bot 2011-02-01 06:40:21 PST
All reviewed patches have been landed.  Closing bug.