Bug 29695 – Unaligned data access in YARR_JIT on ARMv5 and below

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

[Gabor Loki]

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.

[Gabor Loki]

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.

[Gabor Loki]

Opps, I forgot to paste the bug number into the ChangeLog entry.

[Gavin Barraclough]

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?

[Ricardo Salveti de Araujo]

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

[Gabor Loki]

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

[Gabor Loki]

Created attachment 40106 [details]
Fix unaligned data access in YARR_JIT on ARMv5 and below (v2)

Updated considering Gavin's suggestions from comment #3.

[Eric Seidel (no email)]

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.

[WebKit Commit Bot]

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>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.