Bug 38280 - Fix spanning branch instruction on Cortex-A8 with Thumb-2 JIT
Summary: Fix spanning branch instruction on Cortex-A8 with Thumb-2 JIT
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-04-28 13:32 PDT by Gabor Loki
Modified: 2010-05-08 12:51 PDT (History)
2 users (show)

See Also:


Attachments
Fix spanning branch instruction on Cortex-A8 with Thumb-2 JIT (2.78 KB, patch)
2010-04-28 13:34 PDT, Gabor Loki
barraclough: review-
Details | Formatted Diff | Diff
Fix spanning branch instruction on Cortex-A8 with Thumb-2 JIT (v2) (2.86 KB, patch)
2010-04-29 01:31 PDT, Gabor Loki
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Gabor Loki 2010-04-28 13:32:21 PDT
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.
Comment 1 Gabor Loki 2010-04-28 13:34:14 PDT
Created attachment 54615 [details]
Fix spanning branch instruction on Cortex-A8 with Thumb-2 JIT
Comment 2 Gavin Barraclough 2010-04-28 18:28:55 PDT
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!
Comment 3 Gabor Loki 2010-04-29 00:57:48 PDT
> 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.
Comment 4 Gabor Loki 2010-04-29 01:31:35 PDT
Created attachment 54682 [details]
Fix spanning branch instruction on Cortex-A8 with Thumb-2 JIT  (v2)
Comment 5 WebKit Commit Bot 2010-05-08 12:51:32 PDT
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>
Comment 6 WebKit Commit Bot 2010-05-08 12:51:38 PDT
All reviewed patches have been landed.  Closing bug.