|
From: <sv...@va...> - 2013-05-11 13:42:24
|
sewardj 2013-05-11 14:42:08 +0100 (Sat, 11 May 2013)
New Revision: 13386
Log:
complainIfUndefined: reinstate the 3rd argument (guard) so as to make
the definedness check and possible shadow temp set-to-defined be
optional. Use this to properly instrument IRLoadG and IRStoreG, so
that if the load/store does not happen, not only is the validity of
the address not checked, neither is the definedness.
This fixes a regression introduced by the COMEM branch on ARM, in
which conditional loads/stores with addresses which are undefined at
runtime and with guards which are false, would generate false errors.
Also extensively re-checked the check-generation machinery and updated
a bunch of comments.
Modified files:
trunk/memcheck/mc_translate.c
Modified: trunk/memcheck/mc_translate.c (+121 -115)
===================================================================
--- trunk/memcheck/mc_translate.c 2013-05-10 14:14:54 +01:00 (rev 13385)
+++ trunk/memcheck/mc_translate.c 2013-05-11 14:42:08 +01:00 (rev 13386)
@@ -43,56 +43,6 @@
#include "mc_include.h"
-/* Comments re guarded loads and stores, and conditional dirty calls
- that access memory, JRS 2012-Dec-14, for branches/COMEM, r13180.
-
- Currently Memcheck generates code that checks the definedness of
- addresses in such cases, regardless of the what the guard value is
- (at runtime). This could potentially lead to false positives if we
- ever construct IR in which a guarded memory access happens, and the
- address is undefined when the guard is false. However, at the
- moment I am not aware of any situations where such IR is generated.
-
- The obvious thing to do is generate conditionalised checking code
- in such cases. However:
-
- * it's more complex to verify
-
- * it is cheaper to always do the check -- basically a check if
- the shadow value is nonzero, and conditional call to report
- an error if so -- than it is to conditionalise the check.
-
- * currently the implementation is incomplete. complainIfUndefined
- can correctly conditionalise the check and complaint as per its
- third argument. However, the part of it that then sets the
- shadow to 'defined' (see comments at the top of said fn) ignores
- the guard.
-
- Therefore, removing this functionality in r13181 until we know we
- need it. To reinstate, do the following:
-
- * back out r13181 (which also adds this comment)
-
- * undo (== reinstate the non-NULL 3rd args) in the following two
- chunks, which were removed in r13142. These are the only two
- places where complainIfUndefined is actually used with a guard.
-
- // First, emit a definedness test for the address. This also sets
- // the address (shadow) to 'defined' following the test.
- - complainIfUndefined( mce, addr, guard );
- + complainIfUndefined( mce, addr, NULL );
-
- and
-
- IRType tyAddr;
- tl_assert(d->mAddr);
- - complainIfUndefined(mce, d->mAddr, d->guard);
- + complainIfUndefined(mce, d->mAddr, NULL);
-
- * fix complainIfUndefined to conditionalise setting the shadow temp
- to 'defined', as described above.
-*/
-
/* FIXMEs JRS 2011-June-16.
Check the interpretation for vector narrowing and widening ops,
@@ -169,6 +119,11 @@
In practice, 1 and 2 account for the vast majority of cases.
*/
+/* Generation of addr-definedness, addr-validity and
+ guard-definedness checks pertaining to loads and stores (Iex_Load,
+ Ist_Store, IRLoadG, IRStoreG, LLSC, CAS and Dirty memory
+ loads/stores) was re-checked 11 May 2013. */
+
/*------------------------------------------------------------*/
/*--- Forward decls ---*/
/*------------------------------------------------------------*/
@@ -1158,11 +1113,17 @@
original->tmp mapping accordingly; we cannot simply assign a new
value to an existing shadow tmp as this breaks SSAness.
- It may be that any resulting complaint should only be emitted
- conditionally, as defined by |guard|. If |guard| is NULL then it
- is assumed to be always-true.
+ The checks are performed, any resulting complaint emitted, and
+ |atom|'s shadow temp set to 'defined', ONLY in the case that
+ |guard| evaluates to True at run-time. If it evaluates to False
+ then no action is performed. If |guard| is NULL (the usual case)
+ then it is assumed to be always-true, and hence these actions are
+ performed unconditionally.
+
+ This routine does not generate code to check the definedness of
+ |guard|. The caller is assumed to have taken care of that already.
*/
-static void complainIfUndefined ( MCEnv* mce, IRAtom* atom )
+static void complainIfUndefined ( MCEnv* mce, IRAtom* atom, IRExpr *guard )
{
IRAtom* vatom;
IRType ty;
@@ -1179,6 +1140,9 @@
if (MC_(clo_mc_level) == 1)
return;
+ if (guard)
+ tl_assert(isOriginalAtom(mce, guard));
+
/* Since the original expression is atomic, there's no duplicated
work generated by making multiple V-expressions for it. So we
don't really care about the possibility that someone else may
@@ -1293,21 +1257,46 @@
di = unsafeIRDirty_0_N( nargs/*regparms*/, nm,
VG_(fnptr_to_fnentry)( fn ), args );
- di->guard = cond;
+ di->guard = cond; // and cond is PCast-to-1(atom#)
+ /* If the complaint is to be issued under a guard condition, AND
+ that into the guard condition for the helper call. */
+ if (guard) {
+ IRAtom *g1 = assignNew('V', mce, Ity_I32, unop(Iop_1Uto32, di->guard));
+ IRAtom *g2 = assignNew('V', mce, Ity_I32, unop(Iop_1Uto32, guard));
+ IRAtom *e = assignNew('V', mce, Ity_I32, binop(Iop_And32, g1, g2));
+ di->guard = assignNew('V', mce, Ity_I1, unop(Iop_32to1, e));
+ }
+
setHelperAnns( mce, di );
stmt( 'V', mce, IRStmt_Dirty(di));
- /* Set the shadow tmp to be defined. First, update the
- orig->shadow tmp mapping to reflect the fact that this shadow is
- getting a new value. */
+ /* If |atom| is shadowed by an IRTemp, set the shadow tmp to be
+ defined -- but only in the case where the guard evaluates to
+ True at run-time. Do the update by setting the orig->shadow
+ mapping for tmp to reflect the fact that this shadow is getting
+ a new value. */
tl_assert(isIRAtom(vatom));
/* sameKindedAtoms ... */
if (vatom->tag == Iex_RdTmp) {
tl_assert(atom->tag == Iex_RdTmp);
- newShadowTmpV(mce, atom->Iex.RdTmp.tmp);
- assign('V', mce, findShadowTmpV(mce, atom->Iex.RdTmp.tmp),
- definedOfType(ty));
+ if (guard == NULL) {
+ // guard is 'always True', hence update unconditionally
+ newShadowTmpV(mce, atom->Iex.RdTmp.tmp);
+ assign('V', mce, findShadowTmpV(mce, atom->Iex.RdTmp.tmp),
+ definedOfType(ty));
+ } else {
+ // update the temp only conditionally. Do this by copying
+ // its old value when the guard is False.
+ // The old value ..
+ IRTemp old_tmpV = findShadowTmpV(mce, atom->Iex.RdTmp.tmp);
+ newShadowTmpV(mce, atom->Iex.RdTmp.tmp);
+ IRAtom* new_tmpV
+ = assignNew('V', mce, shadowTypeV(ty),
+ IRExpr_ITE(guard, definedOfType(ty),
+ mkexpr(old_tmpV)));
+ assign('V', mce, findShadowTmpV(mce, atom->Iex.RdTmp.tmp), new_tmpV);
+ }
}
}
@@ -1426,7 +1415,7 @@
arrSize = descr->nElems * sizeofIRType(ty);
tl_assert(ty != Ity_I1);
tl_assert(isOriginalAtom(mce,ix));
- complainIfUndefined(mce, ix);
+ complainIfUndefined(mce, ix, NULL);
if (isAlwaysDefd(mce, descr->base, arrSize)) {
/* later: no ... */
/* emit code to emit a complaint if any of the vbits are 1. */
@@ -1475,7 +1464,7 @@
Int arrSize = descr->nElems * sizeofIRType(ty);
tl_assert(ty != Ity_I1);
tl_assert(isOriginalAtom(mce,ix));
- complainIfUndefined(mce, ix);
+ complainIfUndefined(mce, ix, NULL);
if (isAlwaysDefd(mce, descr->base, arrSize)) {
/* Always defined, return all zeroes of the relevant type */
return definedOfType(tyS);
@@ -2706,15 +2695,15 @@
/* IRRoundingModeDFP(I32) x I8 x D128 -> D128 */
return mkLazy3(mce, Ity_I128, vatom1, vatom2, vatom3);
case Iop_ExtractV128:
- complainIfUndefined(mce, atom3);
+ complainIfUndefined(mce, atom3, NULL);
return assignNew('V', mce, Ity_V128, triop(op, vatom1, vatom2, atom3));
case Iop_Extract64:
- complainIfUndefined(mce, atom3);
+ complainIfUndefined(mce, atom3, NULL);
return assignNew('V', mce, Ity_I64, triop(op, vatom1, vatom2, atom3));
case Iop_SetElem8x8:
case Iop_SetElem16x4:
case Iop_SetElem32x2:
- complainIfUndefined(mce, atom2);
+ complainIfUndefined(mce, atom2, NULL);
return assignNew('V', mce, Ity_I64, triop(op, vatom1, atom2, vatom3));
default:
ppIROp(op);
@@ -2781,7 +2770,7 @@
case Iop_ShlN32x2:
case Iop_ShlN8x8:
/* Same scheme as with all other shifts. */
- complainIfUndefined(mce, atom2);
+ complainIfUndefined(mce, atom2, NULL);
return assignNew('V', mce, Ity_I64, binop(op, vatom1, atom2));
case Iop_QNarrowBin32Sto16Sx4:
@@ -2864,25 +2853,25 @@
case Iop_QShlN8Sx8:
case Iop_QShlN8x8:
case Iop_QSalN8x8:
- complainIfUndefined(mce, atom2);
+ complainIfUndefined(mce, atom2, NULL);
return mkPCast8x8(mce, vatom1);
case Iop_QShlN16Sx4:
case Iop_QShlN16x4:
case Iop_QSalN16x4:
- complainIfUndefined(mce, atom2);
+ complainIfUndefined(mce, atom2, NULL);
return mkPCast16x4(mce, vatom1);
case Iop_QShlN32Sx2:
case Iop_QShlN32x2:
case Iop_QSalN32x2:
- complainIfUndefined(mce, atom2);
+ complainIfUndefined(mce, atom2, NULL);
return mkPCast32x2(mce, vatom1);
case Iop_QShlN64Sx1:
case Iop_QShlN64x1:
case Iop_QSalN64x1:
- complainIfUndefined(mce, atom2);
+ complainIfUndefined(mce, atom2, NULL);
return mkPCast32x2(mce, vatom1);
case Iop_PwMax32Sx2:
@@ -2979,13 +2968,13 @@
return assignNew('V', mce, Ity_I64, binop(op, vatom1, vatom2));
case Iop_GetElem8x8:
- complainIfUndefined(mce, atom2);
+ complainIfUndefined(mce, atom2, NULL);
return assignNew('V', mce, Ity_I8, binop(op, vatom1, atom2));
case Iop_GetElem16x4:
- complainIfUndefined(mce, atom2);
+ complainIfUndefined(mce, atom2, NULL);
return assignNew('V', mce, Ity_I16, binop(op, vatom1, atom2));
case Iop_GetElem32x2:
- complainIfUndefined(mce, atom2);
+ complainIfUndefined(mce, atom2, NULL);
return assignNew('V', mce, Ity_I32, binop(op, vatom1, atom2));
/* Perm8x8: rearrange values in left arg using steering values
@@ -3015,7 +3004,7 @@
/* Same scheme as with all other shifts. Note: 22 Oct 05:
this is wrong now, scalar shifts are done properly lazily.
Vector shifts should be fixed too. */
- complainIfUndefined(mce, atom2);
+ complainIfUndefined(mce, atom2, NULL);
return assignNew('V', mce, Ity_V128, binop(op, vatom1, atom2));
/* V x V shifts/rotates are done using the standard lazy scheme. */
@@ -3062,14 +3051,14 @@
case Iop_F32ToFixed32Sx4_RZ:
case Iop_Fixed32UToF32x4_RN:
case Iop_Fixed32SToF32x4_RN:
- complainIfUndefined(mce, atom2);
+ complainIfUndefined(mce, atom2, NULL);
return mkPCast32x4(mce, vatom1);
case Iop_F32ToFixed32Ux2_RZ:
case Iop_F32ToFixed32Sx2_RZ:
case Iop_Fixed32UToF32x2_RN:
case Iop_Fixed32SToF32x2_RN:
- complainIfUndefined(mce, atom2);
+ complainIfUndefined(mce, atom2, NULL);
return mkPCast32x2(mce, vatom1);
case Iop_QSub8Ux16:
@@ -3226,25 +3215,25 @@
case Iop_QShlN8Sx16:
case Iop_QShlN8x16:
case Iop_QSalN8x16:
- complainIfUndefined(mce, atom2);
+ complainIfUndefined(mce, atom2, NULL);
return mkPCast8x16(mce, vatom1);
case Iop_QShlN16Sx8:
case Iop_QShlN16x8:
case Iop_QSalN16x8:
- complainIfUndefined(mce, atom2);
+ complainIfUndefined(mce, atom2, NULL);
return mkPCast16x8(mce, vatom1);
case Iop_QShlN32Sx4:
case Iop_QShlN32x4:
case Iop_QSalN32x4:
- complainIfUndefined(mce, atom2);
+ complainIfUndefined(mce, atom2, NULL);
return mkPCast32x4(mce, vatom1);
case Iop_QShlN64Sx2:
case Iop_QShlN64x2:
case Iop_QSalN64x2:
- complainIfUndefined(mce, atom2);
+ complainIfUndefined(mce, atom2, NULL);
return mkPCast32x4(mce, vatom1);
case Iop_Mull32Sx2:
@@ -3307,16 +3296,16 @@
return assignNew('V', mce, Ity_V128, binop(op, vatom1, vatom2));
case Iop_GetElem8x16:
- complainIfUndefined(mce, atom2);
+ complainIfUndefined(mce, atom2, NULL);
return assignNew('V', mce, Ity_I8, binop(op, vatom1, atom2));
case Iop_GetElem16x8:
- complainIfUndefined(mce, atom2);
+ complainIfUndefined(mce, atom2, NULL);
return assignNew('V', mce, Ity_I16, binop(op, vatom1, atom2));
case Iop_GetElem32x4:
- complainIfUndefined(mce, atom2);
+ complainIfUndefined(mce, atom2, NULL);
return assignNew('V', mce, Ity_I32, binop(op, vatom1, atom2));
case Iop_GetElem64x2:
- complainIfUndefined(mce, atom2);
+ complainIfUndefined(mce, atom2, NULL);
return assignNew('V', mce, Ity_I64, binop(op, vatom1, atom2));
/* Perm8x16: rearrange values in left arg using steering values
@@ -3375,7 +3364,7 @@
/* Same scheme as with all other shifts. Note: 10 Nov 05:
this is wrong now, scalar shifts are done properly lazily.
Vector shifts should be fixed too. */
- complainIfUndefined(mce, atom2);
+ complainIfUndefined(mce, atom2, NULL);
return assignNew('V', mce, Ity_V128, binop(op, vatom1, atom2));
/* I128-bit data-steering */
@@ -3784,7 +3773,7 @@
/* Same scheme as with all other shifts. Note: 22 Oct 05:
this is wrong now, scalar shifts are done properly lazily.
Vector shifts should be fixed too. */
- complainIfUndefined(mce, atom2);
+ complainIfUndefined(mce, atom2, NULL);
return assignNew('V', mce, Ity_V256, binop(op, vatom1, atom2));
case Iop_QSub8Ux32:
@@ -4168,9 +4157,20 @@
}
-/* Worker function; do not call directly. See comments on
- expr2vbits_Load for the meaning of 'guard'. If 'guard' evaluates
- to False at run time, the returned value is all-ones. */
+/* Worker function -- do not call directly. See comments on
+ expr2vbits_Load for the meaning of |guard|.
+
+ Generates IR to (1) perform a definedness test of |addr|, (2)
+ perform a validity test of |addr|, and (3) return the Vbits for the
+ location indicated by |addr|. All of this only happens when
+ |guard| is NULL or |guard| evaluates to True at run time.
+
+ If |guard| evaluates to False at run time, the returned value is
+ the IR-mandated 0x55..55 value, and no checks nor shadow loads are
+ performed.
+
+ The definedness of |guard| itself is not checked. That is assumed
+ to have been done before this point, by the caller. */
static
IRAtom* expr2vbits_Load_WRK ( MCEnv* mce,
IREndness end, IRType ty,
@@ -4187,7 +4187,7 @@
/* First, emit a definedness test for the address. This also sets
the address (shadow) to 'defined' following the test. */
- complainIfUndefined( mce, addr );
+ complainIfUndefined( mce, addr, guard );
/* Now cook up a call to the relevant helper function, to read the
data V bits from shadow memory. */
@@ -4208,7 +4208,7 @@
hname = "MC_(helperc_LOADV8)";
break;
default: ppIRType(ty);
- VG_(tool_panic)("memcheck:do_shadow_Load(LE)");
+ VG_(tool_panic)("memcheck:expr2vbits_Load_WRK(LE)");
}
} else {
switch (ty) {
@@ -4225,7 +4225,7 @@
hname = "MC_(helperc_LOADV8)";
break;
default: ppIRType(ty);
- VG_(tool_panic)("memcheck:do_shadow_Load(BE)");
+ VG_(tool_panic)("memcheck:expr2vbits_Load_WRK(BE)");
}
}
@@ -4268,10 +4268,13 @@
the validity of the address and return the V bits for that address.
This can optionally be controlled by a guard, which is assumed to
be True if NULL. In the case where the guard is False at runtime,
- the helper will return the didn't-do-the-call value of all-ones.
- Since all ones means "completely undefined result", the caller of
+ the helper will return the didn't-do-the-call value of 0x55..55.
+ Since that means "completely undefined result", the caller of
this function will need to fix up the result somehow in that
case.
+
+ Caller of this function is also expected to have checked the
+ definedness of |guard| before this point.
*/
static
IRAtom* expr2vbits_Load ( MCEnv* mce,
@@ -4324,8 +4327,9 @@
/* The most general handler for guarded loads. Assumes the
- definedness of GUARD and ADDR have already been checked by the
- caller. A GUARD of NULL is assumed to mean "always True".
+ definedness of GUARD has already been checked by the caller. A
+ GUARD of NULL is assumed to mean "always True". Generates code to
+ check the definedness and validity of ADDR.
Generate IR to do a shadow load from ADDR and return the V bits.
The loaded type is TY. The loaded data is then (shadow) widened by
@@ -4390,8 +4394,8 @@
conversion operation, and the default V bit return (when the guard
evaluates to False at runtime) is "all defined". If there is no
guard expression or the guard is always TRUE this function behaves
- like expr2vbits_Load. It is assumed that definedness of GUARD and
- ADDR has already been checked at the call site. */
+ like expr2vbits_Load. It is assumed that definedness of GUARD has
+ already been checked at the call site. */
static
IRAtom* expr2vbits_Load_guarded_Simple ( MCEnv* mce,
IREndness end, IRType ty,
@@ -4553,8 +4557,8 @@
/* Generate a shadow store. |addr| is always the original address
atom. You can pass in either originals or V-bits for the data
atom, but obviously not both. This function generates a check for
- the definedness of |addr|. That check is performed regardless of
- whether |guard| is true or not.
+ the definedness and (indirectly) the validity of |addr|, but only
+ when |guard| evaluates to True at run time (or is NULL).
|guard| :: Ity_I1 controls whether the store really happens; NULL
means it unconditionally does. Note that |guard| itself is not
@@ -4617,8 +4621,9 @@
}
/* First, emit a definedness test for the address. This also sets
- the address (shadow) to 'defined' following the test. */
- complainIfUndefined( mce, addr );
+ the address (shadow) to 'defined' following the test. Both of
+ those actions are gated on |guard|. */
+ complainIfUndefined( mce, addr, guard );
/* Now decide which helper function to call to write the data V
bits into shadow memory. */
@@ -4845,7 +4850,7 @@
# endif
/* First check the guard. */
- complainIfUndefined(mce, d->guard);
+ complainIfUndefined(mce, d->guard, NULL);
/* Now round up all inputs and PCast over them. */
curr = definedOfType(Ity_I32);
@@ -4919,7 +4924,7 @@
should remove all but this test. */
IRType tyAddr;
tl_assert(d->mAddr);
- complainIfUndefined(mce, d->mAddr);
+ complainIfUndefined(mce, d->mAddr, d->guard);
tyAddr = typeOfIRExpr(mce->sb->tyenv, d->mAddr);
tl_assert(tyAddr == Ity_I32 || tyAddr == Ity_I64);
@@ -5194,7 +5199,7 @@
Note two things. Firstly, in the sequence above, we compute
"expected == old", but we don't check definedness of it. Why
not? Also, the x86 and amd64 front ends use
- Iop_CmpCas{EQ,NE}{8,16,32,64} comparisons to make the equivalent
+ Iop_CasCmp{EQ,NE}{8,16,32,64} comparisons to make the equivalent
determination (expected == old ?) for themselves, and we also
don't check definedness for those primops; we just say that the
result is defined. Why? Details follow.
@@ -5591,9 +5596,10 @@
static void do_shadow_StoreG ( MCEnv* mce, IRStoreG* sg )
{
- if (0) VG_(printf)("XXXX StoreG\n");
- complainIfUndefined(mce, sg->guard);
- /* do_shadow_Store will check the definedness of sg->addr. */
+ complainIfUndefined(mce, sg->guard, NULL);
+ /* do_shadow_Store will generate code to check the definedness and
+ validity of sg->addr, in the case where sg->guard evaluates to
+ True at run-time. */
do_shadow_Store( mce, sg->end,
sg->addr, 0/* addr bias */,
sg->data,
@@ -5603,10 +5609,10 @@
static void do_shadow_LoadG ( MCEnv* mce, IRLoadG* lg )
{
- if (0) VG_(printf)("XXXX LoadG\n");
- complainIfUndefined(mce, lg->guard);
- /* expr2vbits_Load_guarded_General will check the definedness of
- lg->addr. */
+ complainIfUndefined(mce, lg->guard, NULL);
+ /* expr2vbits_Load_guarded_General will generate code to check the
+ definedness and validity of lg->addr, in the case where
+ lg->guard evaluates to True at run-time. */
/* Look at the LoadG's built-in conversion operation, to determine
the source (actual loaded data) type, and the equivalent IROp.
@@ -5997,7 +6003,7 @@
break;
case Ist_Exit:
- complainIfUndefined( &mce, st->Ist.Exit.guard );
+ complainIfUndefined( &mce, st->Ist.Exit.guard, NULL );
break;
case Ist_IMark:
@@ -6068,7 +6074,7 @@
VG_(printf)("\n\n");
}
- complainIfUndefined( &mce, sb_in->next );
+ complainIfUndefined( &mce, sb_in->next, NULL );
if (0 && verboze) {
for (j = first_stmt; j < sb_out->stmts_used; j++) {
|