[webkit-reviews] review denied: [Bug 38280] Fix spanning branch instruction on Cortex-A8 with Thumb-2 JIT : [Attachment 54615] Fix spanning branch instruction on Cortex-A8 with Thumb-2 JIT

[bugzilla-daemon at webkit.org]

Gavin Barraclough <barraclough at apple.com> has denied Gabor Loki
<loki at webkit.org>'s request for review:
Bug 38280: Fix spanning branch instruction on Cortex-A8 with Thumb-2 JIT
https://bugs.webkit.org/show_bug.cgi?id=38280

Attachment 54615: Fix spanning branch instruction on Cortex-A8 with Thumb-2 JIT
https://bugs.webkit.org/attachment.cgi?id=54615&action=review

------- Additional Comments from Gavin Barraclough <barraclough at apple.com>
> +			     && (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!