Bug 492210 - False positive on x86/amd64 with ZF taken directly from addition

Also adds similar checks for short and char equivalents to the
original int reproducer.

Initial fix provided by
   Alexander Monakov <amonakov@gmail.com>

Two versions of the testcase, one with default options and one with
-expensive-definedness-checks=yes because the byte operations
subb and addb need the flag turned on explicitly.

diff --git a/.gitignore b/.gitignore
index bf477a4fa60e54d953203b4a051cdd4055b1ddf5..c15cf93e0029b1e723cf71e707557f5d2945cfaf 100644 (file)
--- a/.gitignore
+++ b/.gitignore
@@ -1070,6 +1070,7 @@
 /memcheck/tests/amd64/bt_everything
 /memcheck/tests/amd64/bug132146
 /memcheck/tests/amd64/bug279698
+/memcheck/tests/amd64/bug492210
 /memcheck/tests/amd64/defcfaexpr
 /memcheck/tests/amd64/fxsave-amd64
 /memcheck/tests/amd64/int3-amd64

diff --git a/NEWS b/NEWS
index 3906beedf9f10a8733112080af49c8bc59e6a436..b9cb1d5dc25ebc2132a8a9fbe902091eda2a21cf 100644 (file)
--- a/NEWS
+++ b/NEWS
@@ -62,6 +62,7 @@ are not entered into bugzilla tend to get forgotten about or ignored.
 490651  Stop using -flto-partition=one
 491394  (vgModuleLocal_addDiCfSI): Assertion 'di->fsm.have_rx_map &&
         di->fsm.rw_map_count' failed
+492210  False positive on x86/amd64 with ZF taken directly from addition
 492214  statx(fd, NULL, AT_EMPTY_PATH) is supported since Linux 6.11
         but not supported in valgrind
 492663  Valgrind ignores debug info for some binaries

diff --git a/VEX/priv/guest_amd64_helpers.c b/VEX/priv/guest_amd64_helpers.c
index da1cabc3cb2044740027ef5ae31945b0e48dfdf5..9997b4266963d16211b4149cfc5090d09db949a0 100644 (file)
--- a/VEX/priv/guest_amd64_helpers.c
+++ b/VEX/priv/guest_amd64_helpers.c
@@ -1062,6 +1062,7 @@ IRExpr* guest_amd64_spechelper ( const HChar* function_name,
 #  define binop(_op,_a1,_a2) IRExpr_Binop((_op),(_a1),(_a2))
 #  define mkU64(_n) IRExpr_Const(IRConst_U64(_n))
 #  define mkU32(_n) IRExpr_Const(IRConst_U32(_n))
+#  define mkU16(_n) IRExpr_Const(IRConst_U16(_n))
 #  define mkU8(_n)  IRExpr_Const(IRConst_U8(_n))
 
    Int i, arity = 0;
@@ -1140,6 +1141,15 @@ IRExpr* guest_amd64_spechelper ( const HChar* function_name,
 
       }
 
+      /* 4, */
+      if (isU64(cc_op, AMD64G_CC_OP_ADDL) && isU64(cond, AMD64CondZ)) {
+         /* long long add, then Z --> test ((int)(dst+src) == 0) */
+         return unop(Iop_1Uto64,
+                     binop(Iop_CmpEQ32,
+                           unop(Iop_64to32, binop(Iop_Add64, cc_dep1, cc_dep2)),
+                           mkU32(0)));
+      }
+
       /* 8, 9 */
       if (isU64(cc_op, AMD64G_CC_OP_ADDL) && isU64(cond, AMD64CondS)) {
          /* long add, then S (negative)
@@ -1166,6 +1176,29 @@ IRExpr* guest_amd64_spechelper ( const HChar* function_name,
                       mkU64(1));
       }
 
+      /*---------------- ADDW ----------------*/
+
+      /* 4, */
+      if (isU64(cc_op, AMD64G_CC_OP_ADDW) && isU64(cond, AMD64CondZ)) {
+
+         /* long long add, then Z --> test ((short)(dst+src) == 0) */
+         return unop(Iop_1Uto64,
+                     binop(Iop_CmpEQ16,
+                           unop(Iop_64to16, binop(Iop_Add64, cc_dep1, cc_dep2)),
+                           mkU16(0)));
+      }
+
+      /*---------------- ADDB ----------------*/
+
+      /* 4, */
+      if (isU64(cc_op, AMD64G_CC_OP_ADDB) && isU64(cond, AMD64CondZ)) {
+         /* long long add, then Z --> test ((char)(dst+src) == 0) */
+         return unop(Iop_1Uto64,
+                     binop(Iop_CmpEQ8,
+                           unop(Iop_64to8, binop(Iop_Add64, cc_dep1, cc_dep2)),
+                           mkU8(0)));
+      }
+
       /*---------------- SUBQ ----------------*/
 
       /* 0, */
@@ -1307,6 +1340,8 @@ IRExpr* guest_amd64_spechelper ( const HChar* function_name,
                         mkU8(31)),
                   mkU64(1));
       }
+
+      /* 1, */
       if (isU64(cc_op, AMD64G_CC_OP_SUBL) && isU64(cond, AMD64CondNO)) {
          /* No action.  Never yet found a test case. */
       }
@@ -1578,7 +1613,7 @@ IRExpr* guest_amd64_spechelper ( const HChar* function_name,
       if (isU64(cc_op, AMD64G_CC_OP_SUBB) && isU64(cond, AMD64CondZ)) {
          /* byte sub/cmp, then Z --> test dst==src */
          return unop(Iop_1Uto64,
-                     binop(Iop_CmpEQ8, 
+                     binop(Iop_CmpEQ8,
                            unop(Iop_64to8,cc_dep1),
                            unop(Iop_64to8,cc_dep2)));
       }

diff --git a/memcheck/tests/amd64/Makefile.am b/memcheck/tests/amd64/Makefile.am
index 906b8f393b91adc8953b2abf7b66a2ee3025f1d4..c0139bdb20c57eb36ff97477c21bb5985464b26f 100644 (file)
--- a/memcheck/tests/amd64/Makefile.am
+++ b/memcheck/tests/amd64/Makefile.am
@@ -13,6 +13,8 @@ EXTRA_DIST = \
                bt_everything.vgtest \
        bug132146.vgtest bug132146.stderr.exp bug132146.stdout.exp \
        bug279698.vgtest bug279698.stderr.exp bug279698.stdout.exp \
+       bug492210_1.vgtest bug492210_1.stderr.exp \
+       bug492210_2.vgtest bug492210_2.stderr.exp \
        fxsave-amd64.vgtest fxsave-amd64.stdout.exp fxsave-amd64.stderr.exp \
        insn-bsfl.vgtest insn-bsfl.stdout.exp insn-bsfl.stderr.exp \
        insn-pcmpistri.vgtest insn-pcmpistri.stdout.exp insn-pcmpistri.stderr.exp \
@@ -46,6 +48,7 @@ check_PROGRAMS = \
        bt_everything \
        bug132146 \
        bug279698 \
+       bug492210 \
        fxsave-amd64 \
        insn-bsfl \
        insn-pmovmskb \

diff --git a/memcheck/tests/amd64/bug492210.c b/memcheck/tests/amd64/bug492210.c
new file mode 100644 (file)
index 0000000..27e1b17
--- /dev/null
+++ b/memcheck/tests/amd64/bug492210.c
@@ -0,0 +1,106 @@
+/*
+ * Bug 492210 False positive on x86/amd64 with ZF taken directly from addition
+ *
+ * The problem is that the Z flag wasn't being calculated for addl instructions.
+ *
+ * The same problem exists for addw and addb.
+ */
+
+
+unsigned char b;
+unsigned short w;
+unsigned long l;
+unsigned long long q;
+
+extern void test(void);
+
+asm("\n"
+".text\n"
+"test:\n"
+
+"\tmovb b, %al\n"
+"\tmovb $1, %ah\n"
+"\taddb %al, %ah\n"
+"\tje label1\n"
+"\tlabel1:\n"
+
+"\taddb %al, %ah\n"
+"\tjne label2\n"
+"\tlabel2:\n"
+
+"\tsubb %al, %ah\n"
+"\tje label3\n"
+"\tlabel3:\n"
+
+"\tsubb %al, %ah\n"
+"\tjne label4\n"
+"\tlabel4:\n"
+
+"\tmov w, %ax\n"
+"\tmovw $1, %bx\n"
+"\taddw %ax, %bx\n"
+"\tje label5\n"
+"\tlabel5:\n"
+
+"\taddw %ax, %bx\n"
+"\tjne label6\n"
+"\tlabel6:\n"
+
+"\tsubw %ax, %bx\n"
+"\tje label7\n"
+"\tlabel7:\n"
+
+"\tsubw %ax, %bx\n"
+"\tjne label8\n"
+"\tlabel8:\n"
+
+"\tmov l, %eax\n"
+"\tmov $1, %ebx\n"
+"\tadd %eax, %ebx\n"
+"\tje label9\n"
+"\tlabel9:\n"
+
+"\tadd %eax, %ebx\n"
+"\tjne label10\n"
+"\tlabel10:\n"
+
+"\tsub %eax, %ebx\n"
+"\tje label11\n"
+"\tlabel11:\n"
+
+"\tsub %eax, %ebx\n"
+"\tjne label12\n"
+"\tlabel12:\n"
+
+"\tmovq q, %rax\n"
+"\tmovq $1, %rbx\n"
+"\taddq %rax, %rbx\n"
+"\tje label13\n"
+"\tlabel13:\n"
+
+"\taddq %rax, %rbx\n"
+"\tjne label14\n"
+"\tlabel14:\n"
+
+"\tsubq %rax, %rbx\n"
+"\tje label15\n"
+"\tlabel15:\n"
+
+"\tsubq %rax, %rbx\n"
+"\tjne label16\n"
+"\tlabel16:\n"
+
+"\tret\n"
+".previous\n"
+);
+
+int main()
+{
+   unsigned long long uninit;
+   uninit &= 0xfffffffffffffffe;
+   b = uninit;
+   w = uninit;
+   l = uninit;
+   q = uninit;
+   test();
+}

diff --git a/memcheck/tests/amd64/bug492210_1.stderr.exp b/memcheck/tests/amd64/bug492210_1.stderr.exp
new file mode 100644 (file)
index 0000000..afaaad6
--- /dev/null
+++ b/memcheck/tests/amd64/bug492210_1.stderr.exp
@@ -0,0 +1,12 @@
+Conditional jump or move depends on uninitialised value(s)
+   ...
+
+Conditional jump or move depends on uninitialised value(s)
+   ...
+
+Conditional jump or move depends on uninitialised value(s)
+   ...
+
+Conditional jump or move depends on uninitialised value(s)
+   ...
+

diff --git a/memcheck/tests/amd64/bug492210_1.vgtest b/memcheck/tests/amd64/bug492210_1.vgtest
new file mode 100644 (file)
index 0000000..e80be84
--- /dev/null
+++ b/memcheck/tests/amd64/bug492210_1.vgtest
@@ -0,0 +1,4 @@
+prog: bug492210
+vgopts: -q
+
+

diff --git a/memcheck/tests/amd64/bug492210_2.stderr.exp b/memcheck/tests/amd64/bug492210_2.stderr.exp
new file mode 100644 (file)
index 0000000..e69de29

diff --git a/memcheck/tests/amd64/bug492210_2.vgtest b/memcheck/tests/amd64/bug492210_2.vgtest
new file mode 100644 (file)
index 0000000..1c4e361
--- /dev/null
+++ b/memcheck/tests/amd64/bug492210_2.vgtest
@@ -0,0 +1,4 @@
+prog: bug492210
+vgopts: -q --expensive-definedness-checks=yes
+
+