s390x: Fix PC calculations with EX/EXRL

When executing under EX or EXRL, some instructions yield wrong results
under Valgrind.  This affects

* PC-relative instructions such as LARL or BRC

* instructions that set a link register, such as BASR

The issue is caused by confusions about the various instruction addresses
involved.  When executing an instruction under EX or EXRL, the following
addresses are relevant:

(1) The address of the execute instruction (guest_IA_curr_instr).  This is
needed when restarting the instruction or iterating over it.

(2) The address following the execute instruction (guest_IA_next_instr).
This is what a link register needs to be set to.

(3) The address of the target instruction.  This is the base for relative
addressing.

The latter isn't handled at all when translating for EX/EXRL.  And the
instructions that set a link register don't use guest_IA_next_instr, but
add their own instruction length to guest_IA_curr_instr.  This is wrong
whenever the target instruction and the EX/EXRL instruction have different
lengths.

Fix all this and enhance the test cases accordingly.  The updated test
cases fail before this patch and succeed afterwards.

diff --git a/VEX/priv/guest_s390_defs.h b/VEX/priv/guest_s390_defs.h
index 69e804cce2c8de196f0c30ad0f3fff620883a0f0..a64d563ec9e69a4fe9789db155e860e29fb1850c 100644 (file)
--- a/VEX/priv/guest_s390_defs.h
+++ b/VEX/priv/guest_s390_defs.h
@@ -69,7 +69,7 @@ extern VexGuestLayout s390xGuest_layout;
 /*------------------------------------------------------------*/
 /*--- Helper functions.                                    ---*/
 /*------------------------------------------------------------*/
-void s390x_dirtyhelper_EX(ULong torun);
+void s390x_dirtyhelper_EX(ULong torun, Addr64 addr);
 ULong s390x_dirtyhelper_STCK(ULong *addr);
 ULong s390x_dirtyhelper_STCKF(ULong *addr);
 ULong s390x_dirtyhelper_STCKE(ULong *addr);
@@ -253,6 +253,9 @@ UInt s390_calculate_cond(ULong mask, ULong op, ULong dep1, ULong dep2,
 /* Last target instruction for the EX helper */
 extern ULong last_execute_target;
 
+/* Base for relative addressing while processing EX */
+extern Addr64 guest_IA_rel_base;
+
 /*------------------------------------------------------------*/
 /*--- Vector helpers.                                      ---*/
 /*------------------------------------------------------------*/

diff --git a/VEX/priv/guest_s390_helpers.c b/VEX/priv/guest_s390_helpers.c
index 69a7c7d061bb29cdad10cb76dcd536d0f6e36a51..94d0a242db7e01738ac40782e4eab5f9d3b86c1b 100644 (file)
--- a/VEX/priv/guest_s390_helpers.c
+++ b/VEX/priv/guest_s390_helpers.c
@@ -260,9 +260,10 @@ VexGuestLayout s390xGuest_layout = {
 /*--- Dirty helper for EXecute                             ---*/
 /*------------------------------------------------------------*/
 void
-s390x_dirtyhelper_EX(ULong torun)
+s390x_dirtyhelper_EX(ULong torun, Addr64 addr)
 {
    last_execute_target = torun;
+   guest_IA_rel_base = addr;
 }
 
 

diff --git a/VEX/priv/guest_s390_toIR.c b/VEX/priv/guest_s390_toIR.c
index c237a965582d36d649e67af14769c8958c7a0fa3..1d8fb72cc548804603d8aee2191fae39c4261d34 100644 (file)
--- a/VEX/priv/guest_s390_toIR.c
+++ b/VEX/priv/guest_s390_toIR.c
@@ -75,6 +75,9 @@ static Bool sigill_diag;
 enum { Invalid_execute_target = 1 };
 ULong last_execute_target = Invalid_execute_target;
 
+/* The guest address to be used as the base for relative addresses. */
+Addr64 guest_IA_rel_base;
+
 /* The possible outcomes of a decoding operation */
 typedef enum {
    S390_DECODE_OK,
@@ -405,7 +408,7 @@ mkF64i(ULong value)
 static __inline__ Addr64
 addr_rel_long(UInt offset)
 {
-   return guest_IA_curr_instr + ((Addr64)(Long)(Int)offset << 1);
+   return guest_IA_rel_base + ((Addr64)(Long)(Int)offset << 1);
 }
 
 /* Return the 64-bit address with the given 16-bit "relative" offset from the
@@ -413,7 +416,7 @@ addr_rel_long(UInt offset)
 static __inline__ Addr64
 addr_relative(UShort offset)
 {
-   return guest_IA_curr_instr + ((Addr64)(Long)(Short)offset << 1);
+   return guest_IA_rel_base + ((Addr64)(Long)(Short)offset << 1);
 }
 
 /* Little helper function for my sanity. ITE = if-then-else */
@@ -5375,14 +5378,14 @@ s390_irgen_BASR(UChar r1, UChar r2)
    IRTemp target = newTemp(Ity_I64);
 
    if (r2 == 0) {
-      put_gpr_dw0(r1, mkU64(guest_IA_curr_instr + 2ULL));
+      put_gpr_dw0(r1, mkU64(guest_IA_next_instr));
    } else {
       if (r1 != r2) {
-         put_gpr_dw0(r1, mkU64(guest_IA_curr_instr + 2ULL));
+         put_gpr_dw0(r1, mkU64(guest_IA_next_instr));
          call_function(get_gpr_dw0(r2));
       } else {
          assign(target, get_gpr_dw0(r2));
-         put_gpr_dw0(r1, mkU64(guest_IA_curr_instr + 2ULL));
+         put_gpr_dw0(r1, mkU64(guest_IA_next_instr));
          call_function(mkexpr(target));
       }
    }
@@ -5395,7 +5398,7 @@ s390_irgen_BAS(UChar r1, IRTemp op2addr)
 {
    IRTemp target = newTemp(Ity_I64);
 
-   put_gpr_dw0(r1, mkU64(guest_IA_curr_instr + 4ULL));
+   put_gpr_dw0(r1, mkU64(guest_IA_next_instr));
    assign(target, mkexpr(op2addr));
    call_function(mkexpr(target));
 
@@ -5565,7 +5568,7 @@ s390_irgen_BXLEG(UChar r1, UChar r3, IRTemp op2addr)
 static const HChar *
 s390_irgen_BRAS(UChar r1, UShort i2)
 {
-   put_gpr_dw0(r1, mkU64(guest_IA_curr_instr + 4ULL));
+   put_gpr_dw0(r1, mkU64(guest_IA_next_instr));
    call_function_and_chase(addr_relative(i2));
 
    return "bras";
@@ -5574,7 +5577,7 @@ s390_irgen_BRAS(UChar r1, UShort i2)
 static const HChar *
 s390_irgen_BRASL(UChar r1, UInt i2)
 {
-   put_gpr_dw0(r1, mkU64(guest_IA_curr_instr + 6ULL));
+   put_gpr_dw0(r1, mkU64(guest_IA_next_instr));
    call_function_and_chase(addr_rel_long(i2));
 
    return "brasl";
@@ -13186,7 +13189,7 @@ s390_irgen_EX_SS(UChar r, IRTemp addr2, IRTemp torun,
    assign(cond, binop(Iop_CmpNE64, mkexpr(torun), mkU64(last_execute_target)));
    /* If not, save the new value */
    d = unsafeIRDirty_0_N (0, "s390x_dirtyhelper_EX", &s390x_dirtyhelper_EX,
-                          mkIRExprVec_1(mkexpr(torun)));
+                          mkIRExprVec_2(mkexpr(torun), mkexpr(addr2)));
    d->guard = mkexpr(cond);
    stmt(IRStmt_Dirty(d));
 
@@ -13235,7 +13238,7 @@ s390_irgen_EX(UChar r1, IRTemp addr2)
 
       /* so safe the code... */
       d = unsafeIRDirty_0_N (0, "s390x_dirtyhelper_EX", &s390x_dirtyhelper_EX,
-                             mkIRExprVec_1(mkexpr(unmodified_insn)));
+                             mkIRExprVec_2(mkexpr(unmodified_insn), mkexpr(addr2)));
       stmt(IRStmt_Dirty(d));
       /* and restart */
       stmt(IRStmt_Put(S390X_GUEST_OFFSET(guest_CMSTART),
@@ -13308,12 +13311,16 @@ s390_irgen_EX(UChar r1, IRTemp addr2)
       assign(torun, binop(Iop_Or64, mkexpr(unmodified_insn),
                           binop(Iop_Shl64, mkexpr(orperand), mkU8(48))));
 
-      /* Start with a check that saved code is still correct */
-      assign(cond, binop(Iop_CmpNE64, mkexpr(torun),
-             mkU64(last_execute_target)));
-      /* If not, save the new value */
+      /* Start with a check that saved code is still correct. Compare the target
+       * address as well, since it may be relevant to relative addressing. */
+      assign(
+         cond,
+         binop(Iop_Or1,
+               binop(Iop_CmpNE64, mkexpr(torun), mkU64(last_execute_target)),
+               binop(Iop_CmpNE64, mkexpr(addr2), mkU64(guest_IA_rel_base))));
+      /* If not, save the new values */
       d = unsafeIRDirty_0_N (0, "s390x_dirtyhelper_EX", &s390x_dirtyhelper_EX,
-                             mkIRExprVec_1(mkexpr(torun)));
+                             mkIRExprVec_2(mkexpr(torun), mkexpr(addr2)));
       d->guard = mkexpr(cond);
       stmt(IRStmt_Dirty(d));
 
@@ -13339,16 +13346,20 @@ static const HChar *
 s390_irgen_EXRL(UChar r1, UInt offset)
 {
    IRTemp addr = newTemp(Ity_I64);
-   Addr64 bytes_addr = addr_rel_long(offset);
-   UChar *bytes = (UChar *)(HWord)bytes_addr;
+   Addr64 bytes_addr;
+   UChar *bytes;
    /* we might save one round trip because we know the target */
    if (last_execute_target == Invalid_execute_target) {
+      bytes_addr = addr_rel_long(offset);
+      bytes = (UChar *)(HWord)bytes_addr;
       last_execute_target = ((ULong)bytes[0] << 56) | ((ULong)bytes[1] << 48);
       if (bytes[0] >= 0x40)
          last_execute_target |= ((ULong)bytes[2] << 40) | ((ULong)bytes[3] << 32);
       if (bytes[0] >= 0xc0)
          last_execute_target |= ((ULong)bytes[4] << 24) | ((ULong)bytes[5] << 16);
-   }
+      guest_IA_rel_base = bytes_addr;
+   } else
+      bytes_addr = guest_IA_rel_base;
    assign(addr, mkU64(bytes_addr));
    s390_irgen_EX(r1, addr);
    return "exrl";
@@ -22994,6 +23005,8 @@ disInstr_S390(IRSB        *irsb_IN,
 
    /* Set globals (see top of this file) */
    guest_IA_curr_instr = guest_IP;
+   if (last_execute_target == Invalid_execute_target)
+      guest_IA_rel_base = guest_IA_curr_instr;
    irsb = irsb_IN;
    sigill_diag = sigill_diag_IN;
 

diff --git a/none/tests/s390x/ex.c b/none/tests/s390x/ex.c
index 439246eba9dec2a0bfef39c276fa66221c8d8826..a25087bcb3a2811add0d4e57294b62bcf600e635 100644 (file)
--- a/none/tests/s390x/ex.c
+++ b/none/tests/s390x/ex.c
@@ -5,6 +5,8 @@ char target[] ="XXXXXXXXXXXXXXXX";
 
 int main(void)
 {
+   unsigned long offset;
+
    setbuf(stdout, NULL);
 
    printf("------- Copy 10+1 bytes from buffer to target\n");
@@ -57,6 +59,30 @@ int main(void)
    printf("|\n");
    printf("\n");
 
+   printf("------- EX targeting a PC-relative instruction\n");
+   asm volatile( "1:\n\t"
+                 "larl 1,1b\n\t"
+                 "lgr  2,1\n\t"
+                 "ex   0, 0(2)\n\t"
+                 "sgrk %0,1,2\n\t"
+                 : "=d" (offset) :
+                 : "1", "2");
+   printf("        offset = |%016lx|\n", offset);
+   printf("\n");
+
+   printf("------- EX targeting a branch-and-link instruction\n");
+   asm volatile( "larl  1,1f\n\t"
+                 "ex    0, 0(1)\n\t"
+                 ".insn e,0x0000\n\t"
+                 "1:\n\t"
+                 "brasl 2,2f\n\t"
+                 "2:\n\t"
+                 "sgrk %0,1,2\n\t"
+                 : "=&d" (offset) :
+                 : "1", "2");
+   printf("        offset = |%016lx|\n", offset);
+   printf("\n");
+
    return 0;
 }
 

diff --git a/none/tests/s390x/ex.stdout.exp b/none/tests/s390x/ex.stdout.exp
index 1271b5825346ce2a43ff7f70dadd1d087ac46c5c..c898774d3401de46429bfe2d05e1b7b2f17700b1 100644 (file)
--- a/none/tests/s390x/ex.stdout.exp
+++ b/none/tests/s390x/ex.stdout.exp
@@ -11,3 +11,9 @@ after:  target = |0123456789aXXXXX|
 ------- EX to OR in the syscall number (writes out target)
         target = |0123456789aXXXXX|
 
+------- EX targeting a PC-relative instruction
+        offset = |0000000000000000|
+
+------- EX targeting a branch-and-link instruction
+        offset = |0000000000000002|
+

diff --git a/none/tests/s390x/exrl.c b/none/tests/s390x/exrl.c
index e669e484fe1ae863ff45e2d9dca611af7af59fe2..927476e819d9f6a853cb4b805dedc0dbd0bb213b 100644 (file)
--- a/none/tests/s390x/exrl.c
+++ b/none/tests/s390x/exrl.c
@@ -5,6 +5,8 @@ char target[] ="XXXXXXXXXXXXXXXX";
 
 int main(void)
 {
+   unsigned long offset;
+
    setbuf(stdout, NULL);
 
    printf("------- Copy 10+1 bytes from buffer to target\n");
@@ -64,6 +66,30 @@ int main(void)
                  : : "a" (target)
                  : "1", "2", "3", "4");
    printf("        target = |%s|\n", target);
+   printf("\n");
+
+   printf("------- EXRL targeting a PC-relative instruction\n");
+   asm volatile( "basr 1,0\n\t"
+                 "j    2f\n\t"
+                 "1:\n\t"
+                 "larl 2,1b\n\t"
+                 "2:\n\t"
+                 ".insn ril,0xc60000000000,0,1b\n\t" // exrl 0, 1b
+                 "sgrk %0,2,1\n\t"
+                 : "=d" (offset) :
+                 : "1", "2");
+   printf("        offset = |%016lx|\n", offset);
+   printf("\n");
+
+   printf("------- EXRL targeting a branch-and-link instruction\n");
+   asm volatile( "1:\n\t"
+                 "basr 1,0\n\t"
+                 "lgr  2,1\n\t"
+                 ".insn ril,0xc60000000000,0,1b\n\t" // exrl 0, 1b
+                 "sgrk %0,1,2\n\t"
+                 : "=&d" (offset) :
+                 : "1", "2");
+   printf("        offset = |%016lx|\n", offset);
 
    return 0;
 }

diff --git a/none/tests/s390x/exrl.stdout.exp b/none/tests/s390x/exrl.stdout.exp
index 30dcde82955caaf4702c833e0f15d971f9ef1ffe..2d81007ff051d68d236559e6e2d56c29c78781b3 100644 (file)
--- a/none/tests/s390x/exrl.stdout.exp
+++ b/none/tests/s390x/exrl.stdout.exp
@@ -13,3 +13,9 @@ after:  target = |0123456789aXXXXX|
 
 ------- EXRL with negative offset
         target = |01010101010XXXXX|
+
+------- EXRL targeting a PC-relative instruction
+        offset = |0000000000000004|
+
+------- EXRL targeting a branch-and-link instruction
+        offset = |000000000000000a|