From: Andreas Arnez Date: Mon, 19 Aug 2024 13:22:40 +0000 (+0200) Subject: s390x: Fix PC calculations with EX/EXRL X-Git-Tag: VALGRIND_3_24_0~82 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=1375f1d0b3f1f50feb2315b00517126f27f9776e;p=thirdparty%2Fvalgrind.git 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 69e804cce2..a64d563ec9 100644 --- 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 69a7c7d061..94d0a242db 100644 --- 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 c237a96558..1d8fb72cc5 100644 --- 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 439246eba9..a25087bcb3 100644 --- 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 1271b58253..c898774d34 100644 --- 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 e669e484fe..927476e819 100644 --- 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 30dcde8295..2d81007ff0 100644 --- 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|