[Valgrind-developers] [valgrind] Bug 502324 - s390x: Fix memcheck false positives with TM/TMY

[Andreas A. <ar...@so...>]

https://sourceware.org/git/gitweb.cgi?p=valgrind.git;h=29f8a0ad4ed17d9cdf4a5d6661a51d25db6fc472

commit 29f8a0ad4ed17d9cdf4a5d6661a51d25db6fc472
Author: Andreas Arnez <ar...@li...>
Date:   Wed Apr 2 19:52:26 2025 +0200

    Bug 502324 - s390x: Fix memcheck false positives with TM/TMY
    
    If the condition code of TM/TMY is generated in a different block than it
    is used, memcheck can yield false positives for a partially initialized
    value even if the checked bits are all defined.
    
    Fix this by storing the operand ANDed with the mask in the flags thunk,
    instead of the unmodified operand.  This enables memcheck to track the
    defined bits correctly.

Diff:
---
 VEX/priv/guest_s390_helpers.c | 30 ++++++++++--------------------
 VEX/priv/guest_s390_toIR.c    | 27 +++++++++++----------------
 2 files changed, 21 insertions(+), 36 deletions(-)

diff --git a/VEX/priv/guest_s390_helpers.c b/VEX/priv/guest_s390_helpers.c
index 8f3f1a172f..b99d71a5af 100644
--- a/VEX/priv/guest_s390_helpers.c
+++ b/VEX/priv/guest_s390_helpers.c
@@ -2140,8 +2140,8 @@ guest_s390x_spechelper(const HChar *function_name, IRExpr **args,
       }
 
       /* S390_CC_OP_TEST_UNDER_MASK_8
-         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 = an 8-bit mask; expected to be a constant here */
       if (cc_op == S390_CC_OP_TEST_UNDER_MASK_8) {
          ULong mask16;
 
@@ -2149,33 +2149,23 @@ guest_s390x_spechelper(const HChar *function_name, IRExpr **args,
 
          mask16 = 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 (cond & 0x8) return mkU32(1);
             return mkU32(0);
          }
 
          /* cc == 2 is a don't care */
-         if (cond == 8 || cond == 8 + 2) {
-            return unop(Iop_1Uto32, binop(Iop_CmpEQ64,
-                                          binop(Iop_And64, cc_dep1, cc_dep2),
-                                          mkU64(0)));
+         if (cond == 8 || cond == 8 + 2) { /* all bits zero */
+            return unop(Iop_1Uto32, binop(Iop_CmpEQ64, cc_dep1, mkU64(0)));
          }
-         if (cond == 7 || cond == 7 - 2) {
-            return unop(Iop_1Uto32, binop(Iop_CmpNE64,
-                                          binop(Iop_And64, cc_dep1, cc_dep2),
-                                          mkU64(0)));
+         if (cond == 7 || cond == 7 - 2) { /* not all bits zero */
+            return unop(Iop_1Uto32, binop(Iop_CmpNE64, cc_dep1, mkU64(0)));
          }
-         if (cond == 1 || cond == 1 + 2) {
-            return unop(Iop_1Uto32, binop(Iop_CmpEQ64,
-                                          binop(Iop_And64, cc_dep1, cc_dep2),
-                                          cc_dep2));
+         if (cond == 1 || cond == 1 + 2) { /* all bits set */
+            return unop(Iop_1Uto32, binop(Iop_CmpEQ64, cc_dep1, cc_dep2));
          }
-         if (cond == 14 || cond == 14 - 2) {  /* ! OVFL */
-            return unop(Iop_1Uto32, binop(Iop_CmpNE64,
-                                          binop(Iop_And64, cc_dep1, cc_dep2),
-                                          cc_dep2));
+         if (cond == 14 || cond == 14 - 2) { /* not all bits set */
+            return unop(Iop_1Uto32, binop(Iop_CmpNE64, cc_dep1, cc_dep2));
          }
          goto missed;
       }
diff --git a/VEX/priv/guest_s390_toIR.c b/VEX/priv/guest_s390_toIR.c
index e95f28bfda..a5b1fcae6a 100644
--- a/VEX/priv/guest_s390_toIR.c
+++ b/VEX/priv/guest_s390_toIR.c
@@ -11066,31 +11066,26 @@ s390_irgen_SVC(UChar i)
 }
 
 static const HChar *
-s390_irgen_TM(UChar i2, IRTemp op1addr)
+s390_irgen_TMx(const HChar *mnem, UChar mask, IRTemp op1addr)
 {
-   UChar mask;
-   IRTemp value = newTemp(Ity_I8);
+   IRTemp masked = newTemp(Ity_I8);
 
-   mask = i2;
-   assign(value, load(Ity_I8, mkexpr(op1addr)));
-   s390_cc_thunk_putZZ(S390_CC_OP_TEST_UNDER_MASK_8, value, mktemp(Ity_I8,
+   assign(masked, binop(Iop_And8, load(Ity_I8, mkexpr(op1addr)), mkU8(mask)));
+   s390_cc_thunk_putZZ(S390_CC_OP_TEST_UNDER_MASK_8, masked, mktemp(Ity_I8,
                        mkU8(mask)));
+   return mnem;
+}
 
-   return "tm";
+static const HChar *
+s390_irgen_TM(UChar i2, IRTemp op1addr)
+{
+   return s390_irgen_TMx("tm", i2, op1addr);
 }
 
 static const HChar *
 s390_irgen_TMY(UChar i2, IRTemp op1addr)
 {
-   UChar mask;
-   IRTemp value = newTemp(Ity_I8);
-
-   mask = i2;
-   assign(value, load(Ity_I8, mkexpr(op1addr)));
-   s390_cc_thunk_putZZ(S390_CC_OP_TEST_UNDER_MASK_8, value, mktemp(Ity_I8,
-                       mkU8(mask)));
-
-   return "tmy";
+   return s390_irgen_TMx("tmy", i2, op1addr);
 }
 
 static const HChar *