From: Julian Seward <jseward@acm.org>
Date: Fri, 12 Nov 2021 09:40:48 +0000 (+0100)
Subject: Bug 445354 - arm64 backend: incorrect code emitted for doubleword CAS.
X-Git-Tag: VALGRIND_3_19_0~92
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=7dbe2fed72886874f2eaf57dc07929542ae55b58;p=thirdparty%2Fvalgrind.git

Bug 445354 - arm64 backend: incorrect code emitted for doubleword CAS.

The sequence of instructions emitted by the arm64 backend for doubleword
compare-and-swap is incorrect.  This could lead to incorrect simulation of the
AArch8.1 atomic instructions (CASP, at least).  It also causes failures in the
upcoming fix for v8.0 support for LD{,A}XP/ST{,L}XP in bug 444399, at least
when running with the fallback LL/SC implementation
(`--sim-hints=fallback-llsc`, or as autoselected at startup).  In the worst
case it can cause segfaulting in the generated code, because it could jump
backwards unexpectedly far.

The problem is the sequence emitted for ARM64in_CASP:

* the jump offsets are incorrect, both for `bne out` (x 2) and `cbnz w1, loop`.

* using w1 to hold the success indication of the stxp instruction trashes the
  previous value in x1.  But the value in x1 is an output of ARM64in_CASP,
  hence one of the two output registers is corrupted.  That confuses any code
  downstream that want to inspect those values to find out whether or not the
  transaction succeeded.

The fixes are to

* fix the branch offsets

* use a different register to hold the stxp success indication.  w3 is a
  convenient check.
---

diff --git a/NEWS b/NEWS
index c6e9361a14..55ad93c4cb 100644
--- a/NEWS
+++ b/NEWS
@@ -48,6 +48,7 @@ are not entered into bugzilla tend to get forgotten about or ignored.
 444836  PPC, pstq instruction for R=1 is not storing to the correct address.
 445032  valgrind/memcheck crash with SIGSEGV when SIGVTALRM timer used and
         libthr.so associated
+445354  arm64 backend: incorrect code emitted for doubleword CAS
 
 To see details of a given bug, visit
   https://bugs.kde.org/show_bug.cgi?id=XXXXXX
diff --git a/VEX/priv/host_arm64_defs.c b/VEX/priv/host_arm64_defs.c
index 5dccc04954..5657bcab96 100644
--- a/VEX/priv/host_arm64_defs.c
+++ b/VEX/priv/host_arm64_defs.c
@@ -2271,6 +2271,7 @@ void getRegUsage_ARM64Instr ( HRegUsage* u, const ARM64Instr* i, Bool mode64 )
          addHRegUse(u, HRmWrite, hregARM64_X1());
          addHRegUse(u, HRmWrite, hregARM64_X9());
          addHRegUse(u, HRmWrite, hregARM64_X8());
+         addHRegUse(u, HRmWrite, hregARM64_X3());
          break;
       case ARM64in_MFence:
          return;
@@ -4254,16 +4255,16 @@ Int emit_ARM64Instr ( /*MB_MOD*/Bool* is_profInc,
 
               -- always:
               cmp     x0, x8                 // EB08001F
-              bne     out                    // 540000E1 (b.ne #28 <out>)
+              bne     out                    // 540000A1
               cmp     x1, x9                 // EB09003F
-              bne     out                    // 540000A1 (b.ne #20 <out>)
+              bne     out                    // 54000061
 
               -- one of:
-              stxp    w1, x6, x7, [x2]       // C8211C46
-              stxp    w1, w6, w7, [x2]       // 88211C46
+              stxp    w3, x6, x7, [x2]       // C8231C46
+              stxp    w3, w6, w7, [x2]       // 88231C46
 
               -- always:
-              cbnz    w1, loop               // 35FFFE81 (cbnz w1, #-48 <loop>)
+              cbnz    w3, loop               // 35FFFF03
             out:
          */
          switch (i->ARM64in.CASP.szB) {
@@ -4277,15 +4278,15 @@ Int emit_ARM64Instr ( /*MB_MOD*/Bool* is_profInc,
             default: vassert(0);
          }
          *p++ = 0xEB08001F;
-         *p++ = 0x540000E1;
-         *p++ = 0xEB09003F;
          *p++ = 0x540000A1;
+         *p++ = 0xEB09003F;
+         *p++ = 0x54000061;
          switch (i->ARM64in.CASP.szB) {
-            case 8:  *p++ = 0xC8211C46; break;
-            case 4:  *p++ = 0x88211C46; break;
+            case 8:  *p++ = 0xC8231C46; break;
+            case 4:  *p++ = 0x88231C46; break;
             default: vassert(0);
          }
-         *p++ = 0x35FFFE81;
+         *p++ = 0x35FFFF03;
          goto done;
       }
       case ARM64in_MFence: {
diff --git a/VEX/priv/host_arm64_defs.h b/VEX/priv/host_arm64_defs.h
index f0737f2c68..01fb5708e0 100644
--- a/VEX/priv/host_arm64_defs.h
+++ b/VEX/priv/host_arm64_defs.h
@@ -720,6 +720,7 @@ typedef
             Int  szB; /* 1, 2, 4 or 8 */
          } StrEX;
          /* x1 = CAS(x3(addr), x5(expected) -> x7(new)),
+            and trashes x8
             where x1[8*szB-1 : 0] == x5[8*szB-1 : 0] indicates success,
                   x1[8*szB-1 : 0] != x5[8*szB-1 : 0] indicates failure.
             Uses x8 as scratch (but that's not allocatable).
@@ -738,7 +739,7 @@ typedef
             -- if branch taken, failure; x1[[8*szB-1 : 0] holds old value
             -- attempt to store
             stxr    w8, x7, [x3]
-            -- if store successful, x1==0, so the eor is "x1 := x5"
+            -- if store successful, x8==0
             -- if store failed,     branch back and try again.
             cbne    w8, loop
            after:
@@ -746,6 +747,12 @@ typedef
          struct {
             Int szB; /* 1, 2, 4 or 8 */
          } CAS;
+         /* Doubleworld CAS, 2 x 32 bit or 2 x 64 bit
+            x0(oldLSW),x1(oldMSW)
+               = DCAS(x2(addr), x4(expectedLSW),x5(expectedMSW)
+                                -> x6(newLSW),x7(newMSW))
+            and trashes x8, x9 and x3
+         */
          struct {
             Int szB; /* 4 or 8 */
          } CASP;