Bug 46095 – MacroAssemblerARM generates unaligned loads

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[David Tapuska]

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.

[David Tapuska]

Created attachment 68102 [details]
Fix alignment failures

[Zoltan Herczeg]

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

[David Tapuska]

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

[Oliver Hunt]

Comment on attachment 68235 [details]
Add new setTest32 with RegisterID parameter

Have you tested perf impact of this on non-armv5?

[David Tapuska]

(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)]

This seems like an easy patch for gbarra to review.

[Gavin Barraclough]

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]

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

Applied patch as per comments.

[Gavin Barraclough]

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

Looks great David.

[WebKit Commit Bot]

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]

All reviewed patches have been landed.  Closing bug.