From: Julian Seward <jseward@acm.org>
Date: Wed, 19 Nov 2008 16:35:15 +0000 (+0000)
Subject: Don't put raced-on locations in an (E)rror state; instead leave them
X-Git-Tag: svn/VALGRIND_3_4_0~106
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=9398b50c4a939873e8ba7f419aa6bfa37ebee93d;p=thirdparty%2Fvalgrind.git

Don't put raced-on locations in an (E)rror state; instead leave them
in a (C)onstraint state.  The former approach can cause races to be
missed.  Also, update state machine slightly following re-analysis
thereof.



git-svn-id: svn://svn.valgrind.org/valgrind/trunk@8788
---

diff --git a/helgrind/libhb_core.c b/helgrind/libhb_core.c
index 63f5b32f56..09ff6c92a6 100644
--- a/helgrind/libhb_core.c
+++ b/helgrind/libhb_core.c
@@ -3184,12 +3184,33 @@ static void event_map_maybe_GC ( void )
 //                                                     //
 /////////////////////////////////////////////////////////
 
-#define MSM_CONFACC 1
+/* Logic in msm_read/msm_write updated/verified after re-analysis,
+   19 Nov 08. */
 
-#define MSM_RACE2ERR 1
+#define MSM_CONFACC 1
 
 #define MSM_CHECK 0
 
+/* 19 Nov 08: it seems that MSM_RACE2ERR == 1 is a bad idea.  When
+   nonzero, the effect is that when a race is detected for a location,
+   that location is put into a special 'error' state and no further
+   checking of it is done until it returns to a 'normal' state, which
+   requires it to be deallocated and reallocated.
+
+   This is a bad idea, because of the interaction with suppressions.
+   Suppose there is a race on the location, but the error is
+   suppressed.  The location now is marked as in-error.  Now any
+   subsequent race -- including ones we want to see -- will never be
+   detected until the location is deallocated and reallocated.
+
+   Hence set MSM_CHECK to zero.  This causes raced-on locations to
+   remain in the normal 'C' (constrained) state, but places on them
+   the constraint that the next accesses happen-after both the
+   existing constraint and the relevant vector clock of the thread
+   doing the racing access. 
+*/
+#define MSM_RACE2ERR 0
+
 static ULong stats__msm_read         = 0;
 static ULong stats__msm_read_change  = 0;
 static ULong stats__msm_write        = 0;
@@ -3262,9 +3283,13 @@ static inline SVal msm_read ( SVal svOld,
          svNew = SVal__mkC( rmini, VtsID__join2(wmini, tviW) );
          goto out;
       } else {
+         /* assert on sanity of constraints. */
+         POrd  ordxx = VtsID__getOrdering(rmini,wmini);
+         tl_assert(ordxx == POrd_EQ || ordxx == POrd_LT);
          svNew = MSM_RACE2ERR
                     ? SVal__mkE()
-                    : SVal__mkC( rmini, VtsID__join2(wmini,tviR) );
+                    : SVal__mkC( VtsID__join2(wmini,tviR),
+                                 VtsID__join2(wmini,tviW) );
          record_race_info( acc_thr, acc_addr, szB, False/*!isWrite*/,
                            svOld, svNew );
          goto out;
@@ -3326,10 +3351,14 @@ static inline SVal msm_write ( SVal svOld,
          svNew = SVal__mkC( tviW, tviW );
          goto out;
       } else {
+         VtsID tviR  = acc_thr->viR;
          VtsID rmini = SVal__unC_Rmin(svOld);
+         /* assert on sanity of constraints. */
+         POrd  ordxx = VtsID__getOrdering(rmini,wmini);
+         tl_assert(ordxx == POrd_EQ || ordxx == POrd_LT);
          svNew = MSM_RACE2ERR
                     ? SVal__mkE()
-                    : SVal__mkC( VtsID__join2(rmini,tviW),
+                    : SVal__mkC( VtsID__join2(wmini,tviR),
                                  VtsID__join2(wmini,tviW) );
          record_race_info( acc_thr, acc_addr, szB, True/*isWrite*/,
                            svOld, svNew );