|
From: Carl E. L. <ce...@us...> - 2017-05-03 16:38:07
|
On Mon, 2017-05-01 at 12:58 -0700, Carl E. Love wrote:
> On Thu, 2017-04-27 at 17:02 +0200, Julian Seward wrote:
> > The important IR bits are these:
> >
> > 100004bc: 28 4c 20 7d ldbrx r9,0,r9
> > 100004c0: 28 54 40 7d ldbrx r10,0,r10
> > // t143 is r9 after the load
> > // t166 is r10 after the load
> >
> > 100004c4: 51 48 6a 7c subf. r3,r10,r9
> > t69 = Sub64(t143,t166) // r3
> > t167 = 64to8(CmpORD64S(t69,0x0:I64))
> >
> > 100004c8: 1c 00 82 40 bne 100004e4 <main+0x84>
> > if (CmpNE32(Xor32(And32(8Uto32(t167),0x2:I32),0x2:I32),0x0:I32))
> > { PUT(1296) = 0x100004E4:I64; exit-Boring }
> >
> > My best guess is, this is a problem with the instrumentation of the
> > CmpORD64S. What that does is to compare t69 against zero, and produce
> > a 3 bit value like this:
> >
> > 8 if t69 < 0, 4 if t69 > 0, 2 if t69 == 0
> >
> > A bit strange but that's how the Power integer condition codes work.
> > And you can see that the branch condition in the IR tests for 2, meaning
> > equal.
> >
> > This is handled by doCmpORD in mc_translate.c. It special cases the case
> > where the second arg is zero, but only for the < (8) case: that bit is a
> > copy of the most significant bit of t69, so it doesn't matter if all the
> > other bits are undefined. But it doesn't special case the == (2) case, and
> > so that bit is regarded as undefined if any bit in t69 is undefined. But
> > that's too pessimistic. In fact the "is t69 == 0" question is a defined
> > "no" if we can find any bit in t69 which is nonzero and defined. The
> > general logic is explained a bit more in the comment further up, "Accurate
> > interpretation of CmpEQ/CmpNE."
> >
> > Applying that to this problem would fix it, I suspect.
>
Here is my patch with two attempts to fix the issue. I think attempt 1
is actually flawed as I think I was returning the value of the condition
code bits not if they were define. I think fix 2 is the right one, but
I still get the same error plus an additional error about a syscall with
uninitialized data.
Carl Love
>From e7ee8ff9ac7efded61e2288698e09781d37e5520 Mon Sep 17 00:00:00 2001
From: Carl Love <ce...@us...>
Date: Wed, 3 May 2017 11:33:22 -0500
Subject: [PATCH] Attempted fix for compare of uninitialized data.
---
memcheck/mc_translate.c | 148 +++++++++++++++++++++++++++++++++++++++++-------
1 file changed, 128 insertions(+), 20 deletions(-)
diff --git a/memcheck/mc_translate.c b/memcheck/mc_translate.c
index 0b91840..5f08acd 100644
--- a/memcheck/mc_translate.c
+++ b/memcheck/mc_translate.c
@@ -1108,8 +1108,13 @@ static IRAtom* doCmpORD ( MCEnv* mce,
{
Bool m64 = cmp_op == Iop_CmpORD64S || cmp_op == Iop_CmpORD64U;
Bool syned = cmp_op == Iop_CmpORD64S || cmp_op == Iop_CmpORD32S;
+ IROp opNOT = m64 ? Iop_Not64 : Iop_Not32;
IROp opOR = m64 ? Iop_Or64 : Iop_Or32;
+ IROp opXOR = m64 ? Iop_Xor64 : Iop_Xor32;
IROp opAND = m64 ? Iop_And64 : Iop_And32;
+ IROp opEQ = m64 ? Iop_CmpEQ64 : Iop_CmpEQ32;
+ IROp opNE = m64 ? Iop_CmpNE64 : Iop_CmpNE32;
+ IROp opWIDEN = m64 ? Iop_1Uto64 : Iop_1Uto32;
IROp opSHL = m64 ? Iop_Shl64 : Iop_Shl32;
IROp opSHR = m64 ? Iop_Shr64 : Iop_Shr32;
IRType ty = m64 ? Ity_I64 : Ity_I32;
@@ -1119,6 +1124,10 @@ static IRAtom* doCmpORD ( MCEnv* mce,
IRAtom* threeLeft1 = NULL;
IRAtom* sevenLeft1 = NULL;
+ IRAtom* lt_bit = NULL;
+ IRAtom* gt_bit = NULL;
+ IRAtom* eq_bit = NULL;
+ IRAtom* tmp_bit = NULL;
tl_assert(isShadowAtom(mce,xxhash));
tl_assert(isShadowAtom(mce,yyhash));
@@ -1139,26 +1148,125 @@ static IRAtom* doCmpORD ( MCEnv* mce,
/* if yy is zero, then it must be fully defined (zero#). */
tl_assert(isZero(yyhash));
threeLeft1 = m64 ? mkU64(3<<1) : mkU32(3<<1);
- return
- binop(
- opOR,
- assignNew(
- 'V', mce,ty,
- binop(
- opAND,
- mkPCastTo(mce,ty, xxhash),
- threeLeft1
- )),
- assignNew(
- 'V', mce,ty,
- binop(
- opSHL,
- assignNew(
- 'V', mce,ty,
- binop(opSHR, xxhash, mkU8(width-1))),
- mkU8(3)
- ))
- );
+#if 0
+ /* Approach 1. What I did here was calculate the value of the eq and gt
+ bits for the comparison versus zero, i.e. set the condition code
+ result bit. We use the xxhash bits as a mask to get the valid xx bits
+ and then compare that with zero. The returned value for EQ is
+ NOT(NOT(xxhash) AND (xx)) EQ 0) return value should be 0 if the bits
+ are defined and equal to zero. The value for GT is
+ NOT(NOT(xxhash) AND (xx)) NE 0). When comparing to zero NE implies GT.
+
+ THIS APPROACH IS REALLY CALCULATING THE ACTUAL CONDITION CODE VALUES
+ NOT IF BITS ARE VALID
+
+ This approach results in 384 condigional jump/move on uninitialized
+ values instead of just 1 for the test case.
+ */
+ tmp_bit = assignNew('V', mce,ty,
+ binop(opAND,
+ assignNew('V', mce,ty,
+ unop( opNOT,
+ assignNew('V', mce,ty,
+ mkPCastTo(mce,ty, xxhash)))),
+ assignNew('V', mce,ty, xx )));
+
+ /* The comparison gives 1 for EQ which implies value is defined. But "defined" says the shadow bit must be zero so we have
+ to NOT the result.
+ */
+ eq_bit = assignNew('V', mce,ty,
+ unop(opNOT,
+ assignNew('V', mce,ty,
+ unop(opWIDEN,
+ assignNew('V', mce,Ity_I1,
+ binop(opEQ, tmp_bit,
+ (m64 ? mkU64(0) : mkU32(0))))))));
+
+ /* since we are comparing with zero, NE implies xx must be GT zero */
+ gt_bit = assignNew('V', mce,ty,
+ unop(opNOT,
+ assignNew('V', mce,ty,
+ unop(opWIDEN,
+ assignNew('V', mce,Ity_I1,
+ binop(opNE, tmp_bit,
+ (m64 ? mkU64(0) : mkU32(0))))))));
+
+ // old gt calc assignNew('V', mce,ty, binop(opXOR, lt_bit, assignNew('V', mce,ty, unop(opNOT, eq_bit))));
+ lt_bit = assignNew('V', mce,ty, binop(opSHR, xxhash, mkU8(width-1)));
+
+ return binop( opOR,
+ assignNew(
+ 'V', mce,ty,
+ binop(
+ opAND,
+ mkPCastTo(mce,ty, xxhash),
+ threeLeft1
+ )),
+ assignNew(
+ 'V', mce,ty,
+ binop( opOR,
+ assignNew(
+ 'V', mce,ty,
+ binop( opOR,
+ assignNew(
+ 'V', mce,ty,
+ binop(
+ opSHL, lt_bit,
+ mkU8(3))),
+ assignNew(
+ 'V', mce,ty,
+ binop(
+ opSHL, eq_bit,
+ mkU8(2))))),
+ assignNew(
+ 'V', mce,ty,
+ binop(
+ opSHL, gt_bit,
+ mkU8(1))))));
+#else
+ /* Approach 2 is to return 0 for EQ AND GT if there are some xx bits that
+ are defined for use in the comparison. Or in other words if everything
+ is undefined, there are no valid bits that can be compared. EQ and GT
+ shadow bits are set to 1, i.e. undefined */
+ lt_bit = assignNew('V', mce,ty, binop(opSHR, xxhash, mkU8(width-1)));
+
+ /* Comparison is 1 if TRUE (i.e. bits are all undefined).
+
+ * THIS APPROACH STILL RESULTS IN 1 CONDITIONAL JUMP/MOVE DEPENDS ON
+ * UNINITIALIZED VALUE INSTEAD PLUS "Syscall param exit_group
+ * (status) contains uninitialized byte" WHICH DID NOT OCCUR
+ * BEFORE
+ */
+ tmp_bit = assignNew('V', mce,ty,
+ unop(opWIDEN,
+ assignNew('V', mce,Ity_I1,
+ binop( opEQ, (m64 ? mkU64(0xFFFFFFFFFFFFFFFF) : mkU32(0xFFFFFFFF)),
+ assignNew('V', mce,ty,
+ mkPCastTo(mce,ty,
+ xxhash))))));
+ return binop( opOR,
+ assignNew(
+ 'V', mce,ty,
+ binop(
+ opAND,
+ mkPCastTo(mce,ty, xxhash),
+ threeLeft1
+ )),
+ assignNew(
+ 'V', mce,ty,
+ binop( opOR,
+ assignNew(
+ 'V', mce,ty,
+ binop( opAND,
+ tmp_bit,
+ (m64 ? mkU64(0x6) : mkU32(0x6)))),
+ assignNew(
+ 'V', mce,ty,
+ binop(
+ opSHL, lt_bit,
+ mkU8(3))))));
+#endif
+
} else {
/* standard interpretation */
sevenLeft1 = m64 ? mkU64(7<<1) : mkU32(7<<1);
--
2.11.0
|