]> git.ipfire.org Git - thirdparty/valgrind.git/commitdiff
complainIfUndefined: reinstate the 3rd argument (guard) so as to make
authorJulian Seward <jseward@acm.org>
Sat, 11 May 2013 13:42:08 +0000 (13:42 +0000)
committerJulian Seward <jseward@acm.org>
Sat, 11 May 2013 13:42:08 +0000 (13:42 +0000)
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

memcheck/mc_translate.c

index 56ea1a003f00f579b58e35bf8b44bf7769bd9fb2..66bf8ce63f1d8ae0359e96d54f7c9a9b80257105 100644 (file)
 #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,
    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++) {