From: Julian Seward Date: Sat, 11 May 2013 13:42:08 +0000 (+0000) Subject: complainIfUndefined: reinstate the 3rd argument (guard) so as to make X-Git-Tag: svn/VALGRIND_3_9_0~298 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=26ec2b06c19f5b77551613786da3671d13a814d2;p=thirdparty%2Fvalgrind.git 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. git-svn-id: svn://svn.valgrind.org/valgrind/trunk@13386 --- diff --git a/memcheck/mc_translate.c b/memcheck/mc_translate.c index 56ea1a003f..66bf8ce63f 100644 --- a/memcheck/mc_translate.c +++ b/memcheck/mc_translate.c @@ -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 @@ static void setHelperAnns ( MCEnv* mce, IRDirty* di ) { 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 @@ static void complainIfUndefined ( MCEnv* mce, IRAtom* atom ) 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 @@ static void complainIfUndefined ( MCEnv* mce, IRAtom* atom ) 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 @@ void do_shadow_PUTI ( MCEnv* mce, IRPutI *puti) 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 @@ IRExpr* shadow_GETI ( MCEnv* mce, 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 @@ IRAtom* expr2vbits_Triop ( MCEnv* mce, /* 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 @@ IRAtom* expr2vbits_Binop ( MCEnv* mce, 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 @@ IRAtom* expr2vbits_Binop ( MCEnv* mce, 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 @@ IRAtom* expr2vbits_Binop ( MCEnv* mce, 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 @@ IRAtom* expr2vbits_Binop ( MCEnv* mce, /* 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 @@ IRAtom* expr2vbits_Binop ( MCEnv* mce, 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 @@ IRAtom* expr2vbits_Binop ( MCEnv* mce, 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 @@ IRAtom* expr2vbits_Binop ( MCEnv* mce, 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 @@ IRAtom* expr2vbits_Binop ( MCEnv* mce, /* 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 @@ IRAtom* expr2vbits_Binop ( MCEnv* mce, /* 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 @@ IRExpr* expr2vbits_Unop ( MCEnv* mce, IROp op, IRAtom* atom ) } -/* 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 @@ IRAtom* expr2vbits_Load_WRK ( MCEnv* mce, /* 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 @@ IRAtom* expr2vbits_Load_WRK ( MCEnv* mce, 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 @@ IRAtom* expr2vbits_Load_WRK ( MCEnv* mce, 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 @@ IRAtom* expr2vbits_Load_WRK ( MCEnv* mce, 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 @@ IRAtom* expr2vbits_Load ( MCEnv* mce, /* 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 @@ IRAtom* expr2vbits_Load_guarded_General ( MCEnv* mce, 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 @@ IRExpr* zwidenToHostWord ( MCEnv* mce, IRAtom* vatom ) /* 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 @@ void do_shadow_Store ( MCEnv* mce, } /* 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 @@ void do_shadow_Dirty ( MCEnv* mce, IRDirty* d ) # 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 @@ void do_shadow_Dirty ( MCEnv* mce, IRDirty* d ) 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 @@ void do_shadow_CAS ( MCEnv* mce, IRCAS* cas ) 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_LLSC ( MCEnv* mce, 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_StoreG ( MCEnv* mce, IRStoreG* sg ) 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 @@ IRSB* MC_(instrument) ( VgCallbackClosure* closure, 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 @@ IRSB* MC_(instrument) ( VgCallbackClosure* closure, 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++) {