Bug 38280 – Fix spanning branch instruction on Cortex-A8 with Thumb-2 JIT

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

[Gabor Loki]

Cortex-A8 errata says the following:
If the 32-bit Thumb-2 branch instruction spans two 4KiB regions and the target of the branch falls within the first region it is possible for the processor to incorrectly determine the branch instruction, and it is also possible in some cases for the processor to enter a deadlock state.

[Gabor Loki]

Created attachment 54615 [details]
Fix spanning branch instruction on Cortex-A8 with Thumb-2 JIT

[Gavin Barraclough]

Comment on attachment 54615 [details]
Fix spanning branch instruction on Cortex-A8 with Thumb-2 JIT

> +                          && (instruction - 2 - reinterpret_cast<uint16_t*>(target) >= 0)
> +                          && (instruction - 2 - reinterpret_cast<uint16_t*>(target) <= 0xffe);

Hey loki,

I think there is a bug in this calculation.

The result of a subtraction of two uint16_t* pointers is a difference in shorts, not in bytes – so checking this against 0xffe is checking a range of ~8KiB, not ~4KiB.

Rather than performing the check in terms of a difference in shorts, it is probably clearer to check the difference in terms of bytes (which I'm guessing is what you were intending to do).  'relative' already measures in the difference in bytes, so you can probably use this.

Also, the name of the variable is a little confusing, since it only describes part of the check - the first line checks if the instruction is spanning two pages, the second and third lines are checking if the target is in the first page.

    // The instruction is spanning two pages if it ends at an address ending 0x002.
    bool spansTwo4K = (reinterpret_cast<intptr_t>(instruction) & 0xfff) == 0x002;
    // The target is in the first page if the jump branch back back by [3..0x1002] bytes.  (only if spansTwo4K is true)
    bool targetInFirstPage = (relative >= -0x1002) && (relative < -2);
    bool wouldTriggerA8Errata = spansTwo4K && targetInFirstPage;

^^ I think something like this would check more accurately?

r-, but nice catch on spotting this erratum!

[Gabor Loki]

> I think there is a bug in this calculation.

Ops, you are right. I am going to fix this.

>     // The instruction is spanning two pages if it ends at an address ending
> 0x002.
>     bool spansTwo4K = (reinterpret_cast<intptr_t>(instruction) & 0xfff) ==
> 0x002;
>     // The target is in the first page if the jump branch back back by
> [3..0x1002] bytes.  (only if spansTwo4K is true)
>     bool targetInFirstPage = (relative >= -0x1002) && (relative < -2);
>     bool wouldTriggerA8Errata = spansTwo4K && targetInFirstPage;

This looks better. Thanks.

[Gabor Loki]

Created attachment 54682 [details]
Fix spanning branch instruction on Cortex-A8 with Thumb-2 JIT  (v2)

[WebKit Commit Bot]

Comment on attachment 54682 [details]
Fix spanning branch instruction on Cortex-A8 with Thumb-2 JIT  (v2)

Clearing flags on attachment: 54682

Committed r59037: <http://trac.webkit.org/changeset/59037>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.