From: Andreas Arnez <arnez@linux.ibm.com>
Date: Tue, 16 May 2023 18:29:33 +0000 (+0200)
Subject: s390x: Fix memcheck false positives with certain comparisons
X-Git-Tag: VALGRIND_3_22_0~19
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=7cf98163adb3f0d9dad28ad952325e91955811a4;p=thirdparty%2Fvalgrind.git

s390x: Fix memcheck false positives with certain comparisons

Consider this structure definition:

  struct s {
    unsigned b : 1;
    unsigned c : 1;
  } x;

Then certain compiler optimizations for a big-endian system may transform
the test

  if (x.b || x.c)
     ...

into a comparison `> 0x3f' of the structure's first byte.  Indeed, the
result of this comparison only depends on the two highest bits of the
byte.  Thus, even if the lower bits are undefined, memcheck shouldn't
complain, but it does.

For certain cases this can be fixed.  Do this by detecting comparisons
like this in the condition code helper for S390_CC_OP_UNSIGNED_COMPARE and
transforming them to a test for the selected bits instead.  Add a small
test to verify the fix.
---

diff --git a/VEX/priv/guest_s390_helpers.c b/VEX/priv/guest_s390_helpers.c
index df565ffa7f..693e96bf6f 100644
--- a/VEX/priv/guest_s390_helpers.c
+++ b/VEX/priv/guest_s390_helpers.c
@@ -2043,6 +2043,29 @@ guest_s390x_spechelper(const HChar *function_name, IRExpr **args,
             Because cc == 3 cannot occur the rightmost bit of cond is
             a don't care.
          */
+         if (isC64(cc_dep2)) {
+            /* Avoid memcheck false positives for comparisons like `<= 0x1f',
+               `> 0x1f', `< 0x20', or `>= 0x20', where the lower bits don't
+               matter.  Some compiler optimizations yield such comparisons when
+               testing if any (or none) of the upper bits are set. */
+
+            ULong mask = cc_dep2->Iex.Const.con->Ico.U64;
+            ULong c    = cond & (8 + 4 + 2);
+
+            if ((mask & (mask - 1)) == 0) {
+               /* Transform `<  0x20' to `<= 0x1f' and
+                            `>= 0x20' to `>  0x1f' */
+               mask -= 1;
+               c ^= 8;
+            }
+            if (mask != 0 && (mask + 1) != 0 && (mask & (mask + 1)) == 0 &&
+                (c == 8 + 4 || c == 2)) {
+               IROp cmp = c == 8 + 4 ? Iop_CmpEQ64 : Iop_CmpNE64;
+               return unop(Iop_1Uto32,
+                           binop(cmp, binop(Iop_And64, cc_dep1, mkU64(~mask)),
+                                 mkU64(0)));
+            }
+         }
          if (cond == 8 || cond == 8 + 1) {
             return unop(Iop_1Uto32, binop(Iop_CmpEQ64, cc_dep1, cc_dep2));
          }
diff --git a/memcheck/tests/s390x/Makefile.am b/memcheck/tests/s390x/Makefile.am
index 668fd9933c..095c07c7f3 100644
--- a/memcheck/tests/s390x/Makefile.am
+++ b/memcheck/tests/s390x/Makefile.am
@@ -2,7 +2,7 @@ include $(top_srcdir)/Makefile.tool-tests.am
 
 dist_noinst_SCRIPTS = filter_stderr
 
-INSN_TESTS = cdsg cu21 cu42 ltgjhe vstrc vfae vistr vstrs
+INSN_TESTS = cdsg cli cu21 cu42 ltgjhe vstrc vfae vistr vstrs
 
 check_PROGRAMS = $(INSN_TESTS) 
 
diff --git a/memcheck/tests/s390x/cli.c b/memcheck/tests/s390x/cli.c
new file mode 100644
index 0000000000..479ad89420
--- /dev/null
+++ b/memcheck/tests/s390x/cli.c
@@ -0,0 +1,41 @@
+#include <stdlib.h>
+
+volatile unsigned long tmp;
+static const char      use_idx[] = "01234567890abcdefghijklmnopqrstuvwxyz";
+
+static void depend_on(int val) { tmp = use_idx[val]; }
+
+static void pretend_write(unsigned long* val) { __asm__("" : "=m"(*val) : :); }
+
+static void do_compares(unsigned long value)
+{
+   unsigned char val1 = value & 0xff;
+   int result = 0;
+
+   __asm__("cli  %[val],0x20\n\t"
+           "jl   1f\n\t"
+           "lghi %[res],1\n\t"
+           "1:   nopr 0"
+           : [res] "+d"(result)
+           : [val] "Q"(val1)
+           :);
+   depend_on(result);
+
+   __asm__("cli  %[val],0x40\n\t"
+           "jnl  1f\n\t"
+           "lghi %[res],2\n\t"
+           "1:   nopr 0"
+           : [res] "+d"(result)
+           : [val] "Q"(val1)
+           :);
+   depend_on(result);
+}
+
+int main()
+{
+   unsigned long* buf = malloc(sizeof(unsigned long));
+   pretend_write(buf);
+   do_compares(*buf & 0x3f);
+   free(buf);
+   return 0;
+}
diff --git a/memcheck/tests/s390x/cli.stderr.exp b/memcheck/tests/s390x/cli.stderr.exp
new file mode 100644
index 0000000000..54a7a4c4df
--- /dev/null
+++ b/memcheck/tests/s390x/cli.stderr.exp
@@ -0,0 +1,4 @@
+Conditional jump or move depends on uninitialised value(s)
+   at 0x........: do_compares (cli.c:15)
+   by 0x........: main (cli.c:38)
+
diff --git a/memcheck/tests/s390x/cli.stdout.exp b/memcheck/tests/s390x/cli.stdout.exp
new file mode 100644
index 0000000000..e69de29bb2
diff --git a/memcheck/tests/s390x/cli.vgtest b/memcheck/tests/s390x/cli.vgtest
new file mode 100644
index 0000000000..89db99e7f3
--- /dev/null
+++ b/memcheck/tests/s390x/cli.vgtest
@@ -0,0 +1,2 @@
+prog: cli
+vgopts: -q