Bug 29695

Summary: Unaligned data access in YARR_JIT on ARMv5 and below
Product: WebKit Reporter: Gabor Loki <loki>
Component: JavaScriptCoreAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, kenneth, ricardo.salveti
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Other   
OS: Linux   
Attachments:
Description Flags
Fix unaligned data access in YARR_JIT on ARMv5 and below.
none
Fix unaligned data access in YARR_JIT on ARMv5 and below (v2) none

Description Gabor Loki 2009-09-23 14:18:59 PDT
On ARMv5 and below all data access should be naturally aligned.

In the YARR_JIT there is a case when character pairs are loaded from the input string, but this data access is not naturally aligned.
Comment 1 Gabor Loki 2009-09-23 14:20:33 PDT
Created attachment 40018 [details]
Fix unaligned data access in YARR_JIT on ARMv5 and below.

This fix introduces load32WithUnalignedHalfWords and branch32WithUnalignedHalfWords functions which contain naturally aligned memory loads - half word loads - instead of simple load32 on ARMv5 and below.
Comment 2 Gabor Loki 2009-09-23 14:22:50 PDT
Opps, I forgot to paste the bug number into the ChangeLog entry.
Comment 3 Gavin Barraclough 2009-09-23 16:37:41 PDT
Comment on attachment 40018 [details]
Fix unaligned data access in YARR_JIT on ARMv5 and below.

So, the code here all looks fine, but I don't think that the ARMAssembler is really the right layer for this.  The idea of the Assembler classes is that they provide the same interface as the machine instruction set, and that the MacroAssembler provides a higher level set of primitives above this.  In similar cases where an operation must be constructed using multiple machine instructions we try to do so in the MacroAssembler.  There are a couple of cases where this is not so, particularly where a load or branch planted over multiple instructions must be repatched, but we've started refactoring the relinking so we can hopefully we can also move these up to the MacroAssembler.

So I'd prefer to see the logic in baseIndexTransfer32WithUnalignedHalfWords shift up to the MacroAssembler.  Does this sound reasonable?
Comment 4 Ricardo Salveti de Araujo 2009-09-23 18:02:54 PDT
(In reply to comment #1)
> Created an attachment (id=40018) [details]
> Fix unaligned data access in YARR_JIT on ARMv5 and below.
> 
> This fix introduces load32WithUnalignedHalfWords and
> branch32WithUnalignedHalfWords functions which contain naturally aligned memory
> loads - half word loads - instead of simple load32 on ARMv5 and below.

I confirm that this patch fix the alignment trap I was having with ARMv5.

Now I can successfully open google maps and other complex websites (but still with a low performance, but better than before).
Comment 5 Gabor Loki 2009-09-23 21:41:00 PDT
(In reply to comment #3)
> So I'd prefer to see the logic in baseIndexTransfer32WithUnalignedHalfWords
> shift up to the MacroAssembler.  Does this sound reasonable?

Ok, I will refactor the patch.

What do you think about the name of those function? Is it fine for you? Or do you have other suggestion(s)?
Comment 6 Gabor Loki 2009-09-25 05:52:27 PDT
Created attachment 40106 [details]
Fix unaligned data access in YARR_JIT on ARMv5 and below (v2)

Updated considering Gavin's suggestions from comment #3.
Comment 7 Eric Seidel (no email) 2009-09-25 14:12:49 PDT
Comment on attachment 40106 [details]
Fix unaligned data access in YARR_JIT on ARMv5 and below (v2)

As far as I can tell from http://trac.webkit.org/browser/trunk/WebKitTools/Scripts/modules/committers.py Gabor is not a committer.  So I'm going to assume he wants this committed by someone else.  Setting cq+ to let the commit bot do so.
Comment 8 WebKit Commit Bot 2009-09-25 19:27:09 PDT
Comment on attachment 40106 [details]
Fix unaligned data access in YARR_JIT on ARMv5 and below (v2)

Clearing flags on attachment: 40106

Committed r48782: <http://trac.webkit.org/changeset/48782>
Comment 9 WebKit Commit Bot 2009-09-25 19:27:13 PDT
All reviewed patches have been landed.  Closing bug.