Bug 131146 - ARMv7 compare32() should not use TST to do CMP's job
Summary: ARMv7 compare32() should not use TST to do CMP's job
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mark Lam
Keywords: InRadar
Depends on:
Blocks: 108645
  Show dependency treegraph
Reported: 2014-04-02 21:00 PDT by Mark Lam
Modified: 2014-04-03 07:39 PDT (History)
8 users (show)

See Also:

the patch. (2.34 KB, patch)
2014-04-02 21:09 PDT, Mark Lam
ggaren: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mark Lam 2014-04-02 21:00:38 PDT
The ARMv7 implementation of "compare32(RegisterID left, TrustedImm32 right)" was being cute and was using "tst reg, reg" to implement "cmp reg, #0".  Unfortunately, the tst instruction doesn't set the Overflow (V) flag and this results in random results depending on whether there was a preceeding instruction that did set the Overflow (V) flag.  This results in seemingly random manifestations of wrongful behavior that are hard to diagnose.

Here's the specific instruction stream I was anaylzing:

       // initial values:
       // r3 = 0xa1a1a1a1
       // r4 = 0x786bcc7a
       // r10 = 0xa1a1a1a1

       0x66d1b5bc:    mov    r0, r4             // r0 = 0x786bcc7a
       0x66d1b5be:    subs   r0, r0, r3         // r0 = 0x786bcc7a - 0xa1a1a1a1 = 0xd6ca2ad9, and sets the V flag

       0x66d1b5c0:    mov    r2, r10           // r2 = 0xa1a1a1a1
       // Check if r2 < 0:
       0x66d1b5c2:    tst    r2, r2               // Sets the N flag and leaves the V flag
       0x66d1b5c4:    blt    0x66d1b7dc     // "lt" checks if "N != V".  Hence, no branch because both N and V are set.

       0x66d1b5c8:    ldr    r9, [r7, #-8]
       0x66d1b5cc:    mov    r8, r9             // r8 = 0x12015037
       // Check if r8 < 0:
       0x66d1b5ce:    tst    r8, r8               // Clears the N flag but leaves the V flag set.
       0x66d1b5d2:    blt    0x66d1b7fe     // "lt" checks if "N != V".  N is 0.  V is 1.  Hence, we branch.

That second blt is where I see the branch taken when I don't expect it to.  The value in r8 (0x12015037) is obviously not less than 0, and the branch should not be taken.

The first blt is also wrong.  It didn't take the branch when it should have.  The value in r2 (0xa1a1a1a1) is obviously less than 0, and the branch should have been taken.

The fix is to use "cmp reg, #0" instead of "tst reg, reg" when we actually want to compare to 0.
Comment 1 Mark Lam 2014-04-02 21:01:11 PDT
Comment 2 Mark Lam 2014-04-02 21:09:26 PDT
Created attachment 228459 [details]
the patch.
Comment 3 Geoffrey Garen 2014-04-02 23:06:17 PDT
Comment on attachment 228459 [details]
the patch.


You should mention the test that covers this in your ChangeLog.
Comment 4 Mark Lam 2014-04-03 07:39:15 PDT
Thanks for the review.  Landed in r166716: <http://trac.webkit.org/r166716>.