Menu
▾
▴
[Valgrind-developers] [valgrind] Bug 502324 - s390x: Fix memcheck false positives with TMxx
|
From: Andreas A. <ar...@so...> - 2025-04-02 17:57:53
|
https://sourceware.org/git/gitweb.cgi?p=valgrind.git;h=a95ba0d6e549764d5a68536efc6eb1545b537c09 commit a95ba0d6e549764d5a68536efc6eb1545b537c09 Author: Andreas Arnez <ar...@li...> Date: Wed Apr 2 19:52:26 2025 +0200 Bug 502324 - s390x: Fix memcheck false positives with TMxx The 16-bit "test under mask" instructions TMLL, TMLH, TMHL, and TMHH can yield memcheck false positives when: * some of the operand bits in the tested 16-bit chunk (but outside the mask) are undefined * the resulting condition code is used in a different block In this case the condition code is computed by the helper s390_call_calculate_cond, with the full 16-bit chunk and the given mask as arguments. Since the 16-bit chunk is not fully defined, memcheck complains. To fix this, AND the operand with the given mask before storing it for use as a helper argument. Also, optimize the frequent case of testing a single bit. For this purpose, add S390_CC_OP_BITWISE2 as a new way of computing the condition code. Diff: --- VEX/priv/guest_s390_defs.h | 3 +- VEX/priv/guest_s390_helpers.c | 146 +++++++++++++++++++++--------------------- VEX/priv/guest_s390_toIR.c | 63 ++++++++---------- 3 files changed, 104 insertions(+), 108 deletions(-) diff --git a/VEX/priv/guest_s390_defs.h b/VEX/priv/guest_s390_defs.h index 0039f5e784..0ec242a99b 100644 --- a/VEX/priv/guest_s390_defs.h +++ b/VEX/priv/guest_s390_defs.h @@ -155,7 +155,8 @@ enum { S390_CC_OP_PFPO_64 = 59, S390_CC_OP_PFPO_128 = 60, S390_CC_OP_MUL_32 = 61, - S390_CC_OP_MUL_64 = 62 + S390_CC_OP_MUL_64 = 62, + S390_CC_OP_BITWISE2 = 63 }; /*------------------------------------------------------------*/ diff --git a/VEX/priv/guest_s390_helpers.c b/VEX/priv/guest_s390_helpers.c index bbba22b478..8f3f1a172f 100644 --- a/VEX/priv/guest_s390_helpers.c +++ b/VEX/priv/guest_s390_helpers.c @@ -1454,7 +1454,10 @@ s390_calculate_cc(ULong cc_op, ULong cc_dep1, ULong cc_dep2, ULong cc_ndep) switch (cc_op) { case S390_CC_OP_BITWISE: - return S390_CC_FOR_BINARY("ogr", cc_dep1, (ULong)0); + return cc_dep1 == 0 ? 0 : 1; + + case S390_CC_OP_BITWISE2: + return cc_dep1 == 0 ? 0 : 3; case S390_CC_OP_SIGNED_COMPARE: return S390_CC_FOR_BINARY("cgr", cc_dep1, cc_dep2); @@ -2045,6 +2048,21 @@ guest_s390x_spechelper(const HChar *function_name, IRExpr **args, return mkU32(0); } + /* S390_CC_OP_BITWISE2 + Like S390_CC_OP_BITWISE, but yielding cc = 3 for nonzero result */ + if (cc_op == S390_CC_OP_BITWISE2) { + if ((cond & (8 + 1)) == 8 + 1) { + return mkU32(1); + } + if (cond & 8) { + return unop(Iop_1Uto32, binop(Iop_CmpEQ64, cc_dep1, mkU64(0))); + } + if (cond & 1) { + return unop(Iop_1Uto32, binop(Iop_CmpNE64, cc_dep1, mkU64(0))); + } + return mkU32(0); + } + /* S390_CC_OP_INSERT_CHAR_MASK_32 Since the mask comes from an immediate field in the opcode, we expect the mask to be a constant here. That simplifies matters. */ @@ -2163,110 +2181,94 @@ guest_s390x_spechelper(const HChar *function_name, IRExpr **args, } /* S390_CC_OP_TEST_UNDER_MASK_16 - Since the mask comes from an immediate field in the opcode, we - expect the mask to be a constant here. That simplifies matters. */ + cc_dep1 = the value to be tested, ANDed with the mask + cc_dep2 = a 16-bit mask; expected to be a constant here */ if (cc_op == S390_CC_OP_TEST_UNDER_MASK_16) { - ULong mask16; - UInt msb; + IRExpr* val = cc_dep1; + ULong mask; + ULong msb; if (! isC64(cc_dep2)) goto missed; - mask16 = cc_dep2->Iex.Const.con->Ico.U64; + mask = cc_dep2->Iex.Const.con->Ico.U64; - /* Get rid of the mask16 == 0 case first. Some of the simplifications - below (e.g. for OVFL) only hold if mask16 == 0. */ - if (mask16 == 0) { /* cc == 0 */ + if (mask == 0) { /* cc == 0 */ if (cond & 0x8) return mkU32(1); return mkU32(0); } + /* Find MSB in mask */ + msb = 0x8000; + while (msb > mask) + msb >>= 1; + + /* If cc_dep1 results from a shift, avoid the shift operation */ + if (val->tag == Iex_Binop && val->Iex.Binop.op == Iop_Shr64 && + val->Iex.Binop.arg2->tag == Iex_Const && + val->Iex.Binop.arg2->Iex.Const.con->tag == Ico_U8) { + UInt n_bits = val->Iex.Binop.arg2->Iex.Const.con->Ico.U8; + mask <<= n_bits; + msb <<= n_bits; + val = val->Iex.Binop.arg1; + } + if (cond == 15) return mkU32(1); - if (cond == 8) { - return unop(Iop_1Uto32, binop(Iop_CmpEQ64, - binop(Iop_And64, cc_dep1, cc_dep2), - mkU64(0))); + if (cond == 8) { /* all bits zero */ + return unop(Iop_1Uto32, binop(Iop_CmpEQ64, val, mkU64(0))); } - if (cond == 7) { - return unop(Iop_1Uto32, binop(Iop_CmpNE64, - binop(Iop_And64, cc_dep1, cc_dep2), - mkU64(0))); + if (cond == 7) { /* not all bits zero */ + return unop(Iop_1Uto32, binop(Iop_CmpNE64, val, mkU64(0))); } - if (cond == 1) { - return unop(Iop_1Uto32, binop(Iop_CmpEQ64, - binop(Iop_And64, cc_dep1, cc_dep2), - mkU64(mask16))); + if (cond == 1) { /* all bits set */ + return unop(Iop_1Uto32, binop(Iop_CmpEQ64, val, mkU64(mask))); } - if (cond == 14) { /* ! OVFL */ - return unop(Iop_1Uto32, binop(Iop_CmpNE64, - binop(Iop_And64, cc_dep1, cc_dep2), - mkU64(mask16))); + if (cond == 14) { /* not all bits set */ + return unop(Iop_1Uto32, binop(Iop_CmpNE64, val, mkU64(mask))); } - /* Find MSB in mask */ - msb = 0x8000; - while (msb > mask16) - msb >>= 1; + IRExpr *masked_msb = binop(Iop_And64, val, mkU64(msb)); if (cond == 2) { /* cc == 2 */ - IRExpr *c1, *c2; - - /* (cc_dep & msb) != 0 && (cc_dep & mask16) != mask16 */ - c1 = binop(Iop_CmpNE64, - binop(Iop_And64, cc_dep1, mkU64(msb)), mkU64(0)); - c2 = binop(Iop_CmpNE64, - binop(Iop_And64, cc_dep1, cc_dep2), - mkU64(mask16)); - return binop(Iop_And32, unop(Iop_1Uto32, c1), - unop(Iop_1Uto32, c2)); + /* mixed, and leftmost bit set */ + return unop(Iop_1Uto32, + binop(Iop_And1, + binop(Iop_CmpNE64, masked_msb, mkU64(0)), + binop(Iop_CmpNE64, val, mkU64(mask)))); } if (cond == 4) { /* cc == 1 */ - IRExpr *c1, *c2; - - /* (cc_dep & msb) == 0 && (cc_dep & mask16) != 0 */ - c1 = binop(Iop_CmpEQ64, - binop(Iop_And64, cc_dep1, mkU64(msb)), mkU64(0)); - c2 = binop(Iop_CmpNE64, - binop(Iop_And64, cc_dep1, cc_dep2), - mkU64(0)); - return binop(Iop_And32, unop(Iop_1Uto32, c1), - unop(Iop_1Uto32, c2)); + /* mixed, and leftmost bit zero */ + return unop(Iop_1Uto32, + binop(Iop_And1, + binop(Iop_CmpEQ64, masked_msb, mkU64(0)), + binop(Iop_CmpNE64, val, mkU64(0)))); } if (cond == 11) { /* cc == 0,2,3 */ - IRExpr *c1, *c2; - - c1 = binop(Iop_CmpNE64, - binop(Iop_And64, cc_dep1, mkU64(msb)), mkU64(0)); - c2 = binop(Iop_CmpEQ64, - binop(Iop_And64, cc_dep1, cc_dep2), - mkU64(0)); - return binop(Iop_Or32, unop(Iop_1Uto32, c1), - unop(Iop_1Uto32, c2)); + /* leftmost bit set, or all bits zero */ + return unop(Iop_1Uto32, + binop(Iop_Or1, + binop(Iop_CmpNE64, masked_msb, mkU64(0)), + binop(Iop_CmpEQ64, val, mkU64(0)))); } if (cond == 3) { /* cc == 2 || cc == 3 */ + /* leftmost bit set, rest don't care */ return unop(Iop_1Uto32, - binop(Iop_CmpNE64, - binop(Iop_And64, cc_dep1, mkU64(msb)), - mkU64(0))); + binop(Iop_CmpNE64, masked_msb, mkU64(0))); } if (cond == 12) { /* cc == 0 || cc == 1 */ + /* leftmost bit zero, rest don't care */ return unop(Iop_1Uto32, - binop(Iop_CmpEQ64, - binop(Iop_And64, cc_dep1, mkU64(msb)), - mkU64(0))); + binop(Iop_CmpEQ64, masked_msb, mkU64(0))); } if (cond == 13) { /* cc == 0 || cc == 1 || cc == 3 */ - IRExpr *c01, *c3; - - c01 = binop(Iop_CmpEQ64, binop(Iop_And64, cc_dep1, mkU64(msb)), - mkU64(0)); - c3 = binop(Iop_CmpEQ64, binop(Iop_And64, cc_dep1, cc_dep2), - mkU64(mask16)); - return binop(Iop_Or32, unop(Iop_1Uto32, c01), - unop(Iop_1Uto32, c3)); + /* leftmost bit zero, or all bits set */ + return unop(Iop_1Uto32, + binop(Iop_Or1, + binop(Iop_CmpEQ64, masked_msb, mkU64(0)), + binop(Iop_CmpEQ64, val, mkU64(mask)))); } // fixs390: handle cond = 5,6,9,10 (the missing cases) // vex_printf("TUM mask = 0x%llx\n", mask16); diff --git a/VEX/priv/guest_s390_toIR.c b/VEX/priv/guest_s390_toIR.c index 5b16002103..e95f28bfda 100644 --- a/VEX/priv/guest_s390_toIR.c +++ b/VEX/priv/guest_s390_toIR.c @@ -11094,59 +11094,52 @@ s390_irgen_TMY(UChar i2, IRTemp op1addr) } static const HChar * -s390_irgen_TMHH(UChar r1, UShort i2) +s390_irgen_TMxx(const HChar *mnem, UChar r1, UShort mask, UChar offs) { - UShort mask; - IRTemp value = newTemp(Ity_I16); + if (mask == 0) { + s390_cc_set_val(0); + return mnem; + } - mask = i2; - assign(value, get_gpr_hw0(r1)); - s390_cc_thunk_putZZ(S390_CC_OP_TEST_UNDER_MASK_16, value, mktemp(Ity_I16, - mkU16(mask))); + IRExpr* masked; + masked = binop(Iop_And64, get_gpr_dw0(r1), mkU64((ULong)mask << offs)); - return "tmhh"; + if ((mask & (mask - 1)) == 0) { + /* Single-bit mask */ + s390_cc_thunk_put1(S390_CC_OP_BITWISE2, mktemp(Ity_I64, masked), False); + } else { + if (offs) { + masked = binop(Iop_Shr64, masked, mkU8(offs)); + } + s390_cc_thunk_putZZ(S390_CC_OP_TEST_UNDER_MASK_16, + mktemp(Ity_I64, masked), + mktemp(Ity_I64, mkU64(mask))); + } + return mnem; } static const HChar * -s390_irgen_TMHL(UChar r1, UShort i2) +s390_irgen_TMHH(UChar r1, UShort i2) { - UShort mask; - IRTemp value = newTemp(Ity_I16); - - mask = i2; - assign(value, get_gpr_hw1(r1)); - s390_cc_thunk_putZZ(S390_CC_OP_TEST_UNDER_MASK_16, value, mktemp(Ity_I16, - mkU16(mask))); + return s390_irgen_TMxx("tmhh", r1, i2, 48); +} - return "tmhl"; +static const HChar * +s390_irgen_TMHL(UChar r1, UShort i2) +{ + return s390_irgen_TMxx("tmhl", r1, i2, 32); } static const HChar * s390_irgen_TMLH(UChar r1, UShort i2) { - UShort mask; - IRTemp value = newTemp(Ity_I16); - - mask = i2; - assign(value, get_gpr_hw2(r1)); - s390_cc_thunk_putZZ(S390_CC_OP_TEST_UNDER_MASK_16, value, mktemp(Ity_I16, - mkU16(mask))); - - return "tmlh"; + return s390_irgen_TMxx("tmlh", r1, i2, 16); } static const HChar * s390_irgen_TMLL(UChar r1, UShort i2) { - UShort mask; - IRTemp value = newTemp(Ity_I16); - - mask = i2; - assign(value, get_gpr_hw3(r1)); - s390_cc_thunk_putZZ(S390_CC_OP_TEST_UNDER_MASK_16, value, mktemp(Ity_I16, - mkU16(mask))); - - return "tmll"; + return s390_irgen_TMxx("tmll", r1, i2, 0); } static const HChar * |
