|
From: <sv...@va...> - 2011-03-20 20:51:04
|
Author: sewardj
Date: 2011-03-20 20:50:54 +0000 (Sun, 20 Mar 2011)
New Revision: 11659
Log:
Add the ability to show the set of locks shown by each thread in a
race, which makes diagnosing races a lot easier. As a side effect,
change the way errors are printed to more clearly delimit the
boundaries of errors.
Modified:
branches/HGDEV2/helgrind/hg_errors.c
branches/HGDEV2/helgrind/hg_lock_n_thread.h
branches/HGDEV2/helgrind/hg_main.c
branches/HGDEV2/helgrind/libhb.h
branches/HGDEV2/helgrind/libhb_core.c
Modified: branches/HGDEV2/helgrind/hg_errors.c
===================================================================
--- branches/HGDEV2/helgrind/hg_errors.c 2011-03-20 12:50:48 UTC (rev 11658)
+++ branches/HGDEV2/helgrind/hg_errors.c 2011-03-20 20:50:54 UTC (rev 11659)
@@ -110,11 +110,56 @@
return 0;
}
-static Lock* mk_LockP_from_LockN ( Lock* lkn )
+/* Given a normal Lock (LockN), convert it to a persistent Lock
+ (LockP). In some cases the LockN could be invalid (if it's been
+ freed), so we enquire, in hg_main.c's admin_locks list, whether it
+ is in fact valid. If allowed_to_be_invalid is True, then it's OK
+ for the LockN to be invalid, in which case Lock_INVALID is
+ returned. In all other cases, we insist that the LockN is a valid
+ lock, and return its corresponding LockP.
+
+ Why can LockNs sometimes be invalid? Because they are harvested
+ from locksets that are attached to the OldRef info for conflicting
+ threads. By the time we detect a race, the some of the elements of
+ the lockset may have been destroyed by the client, in which case
+ the corresponding Lock structures we maintain will have been freed.
+
+ So we check that each LockN is a member of the admin_locks double
+ linked list of all Lock structures. That stops us prodding around
+ in potentially freed-up Lock structures. However, it's not quite a
+ proper check: if a new Lock has been reallocated at the same
+ address as one which was previously freed, we'll wind up copying
+ the new one as the basis for the LockP, which is completely bogus
+ because it is unrelated to the previous Lock that lived there.
+ Let's hope that doesn't happen too often.
+*/
+static Lock* mk_LockP_from_LockN ( Lock* lkn,
+ Bool allowed_to_be_invalid )
{
Lock* lkp = NULL;
HG_(stats__LockN_to_P_queries)++;
+
+ /* First off, let's do some sanity checks. If
+ allowed_to_be_invalid is False, we _must_ be able to find 'lkn'
+ in admin_locks; else we must assert. If it is True, it's OK for
+ it not to be findable, but in that case we must return
+ Lock_INVALID right away. */
+ Lock* lock_list = HG_(get_admin_locks)();
+ while (lock_list) {
+ if (lock_list == lkn)
+ break;
+ lock_list = lock_list->admin_next;
+ }
+ if (lock_list == NULL) {
+ /* We didn't find it. That possibility has to be OK'd by the
+ caller. */
+ tl_assert(allowed_to_be_invalid);
+ return Lock_INVALID;
+ }
+
+ /* So we must be looking at a valid LockN. */
tl_assert( HG_(is_sane_LockN)(lkn) );
+
if (!map_LockN_to_P) {
map_LockN_to_P = VG_(newFM)( HG_(zalloc), "hg.mLPfLN.1",
HG_(free), lock_unique_cmp );
@@ -139,6 +184,75 @@
return lkp;
}
+/* Expand a WordSet of LockN*'s into a NULL-terminated vector of
+ LockP*'s. Any LockN's that can't be converted into a LockP
+ (because they have been freed, see comment on mk_LockP_from_LockN)
+ are converted instead into the value Lock_INVALID. Hence the
+ returned vector is a sequence: zero or more (valid LockP* or
+ LockN_INVALID), terminated by a NULL. */
+static
+Lock** enumerate_WordSet_into_LockP_vector( WordSetU* univ_lsets,
+ WordSetID lockset,
+ Bool allowed_to_be_invalid )
+{
+ tl_assert(univ_lsets);
+ tl_assert( HG_(plausibleWS)(univ_lsets, lockset) );
+ UWord nLocks = HG_(cardinalityWS)(univ_lsets, lockset);
+ Lock** lockPs = HG_(zalloc)( "hg.eWSiLPa",
+ (nLocks+1) * sizeof(Lock*) );
+ tl_assert(lockPs);
+ tl_assert(lockPs[nLocks] == NULL); /* pre-NULL terminated */
+ UWord* lockNs = NULL;
+ UWord nLockNs = 0;
+ if (nLocks > 0) {
+ /* HG_(getPayloadWS) doesn't assign non-NULL to &lockNs if the
+ lockset is empty; hence the guarding "if". Sigh. */
+ HG_(getPayloadWS)( &lockNs, &nLockNs, univ_lsets, lockset );
+ tl_assert(lockNs);
+ }
+ UWord i;
+ /* Convert to LockPs. */
+ for (i = 0; i < nLockNs; i++) {
+ lockPs[i] = mk_LockP_from_LockN( (Lock*)lockNs[i],
+ allowed_to_be_invalid );
+ }
+ return lockPs;
+}
+
+/* Get the number of useful elements in a vector created by
+ enumerate_WordSet_into_LockP_vector. Returns both the total number
+ of elements (not including the terminating NULL) and the number of
+ non-Lock_INVALID elements. */
+static void count_LockP_vector ( /*OUT*/UWord* nLocks,
+ /*OUT*/UWord* nLocksValid,
+ Lock** vec )
+{
+ tl_assert(vec);
+ *nLocks = *nLocksValid = 0;
+ UWord n = 0;
+ while (vec[n]) {
+ (*nLocks)++;
+ if (vec[n] != Lock_INVALID)
+ (*nLocksValid)++;
+ n++;
+ }
+}
+
+/* Find out whether 'lk' is in 'vec'. */
+static Bool elem_LockP_vector ( Lock** vec, Lock* lk )
+{
+ tl_assert(vec);
+ tl_assert(lk);
+ UWord n = 0;
+ while (vec[n]) {
+ if (vec[n] == lk)
+ return True;
+ n++;
+ }
+ return False;
+}
+
+
/* Errors:
race: program counter
@@ -179,6 +293,7 @@
Int szB;
Bool isWrite;
Thread* thr;
+ Lock** locksHeldW;
/* descr1/2 provide a description of stack/global locs */
XArray* descr1; /* XArray* of HChar */
XArray* descr2; /* XArray* of HChar */
@@ -195,6 +310,7 @@
ExeContext* h2_ct_accEC;
Int h2_ct_accSzB;
Bool h2_ct_accIsW;
+ Lock** h2_ct_locksHeldW;
} Race;
struct {
Thread* thr; /* doing the unlocking */
@@ -264,6 +380,18 @@
if (xe->tag == XE_Race) {
+ /* Note the set of locks that the thread is (w-)holding.
+ Convert the WordSetID of LockN*'s into a NULL-terminated
+ vector of LockP*'s. We don't expect to encounter any invalid
+ LockNs in this conversion. */
+ tl_assert(xe->XE.Race.thr);
+ xe->XE.Race.locksHeldW
+ = enumerate_WordSet_into_LockP_vector(
+ HG_(get_univ_lsets)(),
+ xe->XE.Race.thr->locksetW,
+ False/*!allowed_to_be_invalid*/
+ );
+
/* See if we can come up with a source level description of the
raced-upon address. This is potentially expensive, which is
why it's only done at the update_extra point, not when the
@@ -325,18 +453,19 @@
can rustle up a plausible-looking conflicting memory access
to show. */
if (HG_(clo_history_level) >= 2) {
- Thr* thrp = NULL;
- ExeContext* wherep = NULL;
- Addr acc_addr = xe->XE.Race.data_addr;
- Int acc_szB = xe->XE.Race.szB;
- Thr* acc_thr = xe->XE.Race.thr->hbthr;
- Bool acc_isW = xe->XE.Race.isWrite;
- SizeT conf_szB = 0;
- Bool conf_isW = False;
+ Thr* thrp = NULL;
+ ExeContext* wherep = NULL;
+ Addr acc_addr = xe->XE.Race.data_addr;
+ Int acc_szB = xe->XE.Race.szB;
+ Thr* acc_thr = xe->XE.Race.thr->hbthr;
+ Bool acc_isW = xe->XE.Race.isWrite;
+ SizeT conf_szB = 0;
+ Bool conf_isW = False;
+ WordSetID conf_locksHeldW = 0;
tl_assert(!xe->XE.Race.h2_ct_accEC);
tl_assert(!xe->XE.Race.h2_ct);
if (libhb_event_map_lookup(
- &wherep, &thrp, &conf_szB, &conf_isW,
+ &wherep, &thrp, &conf_szB, &conf_isW, &conf_locksHeldW,
acc_thr, acc_addr, acc_szB, acc_isW )) {
Thread* threadp;
tl_assert(wherep);
@@ -347,6 +476,12 @@
xe->XE.Race.h2_ct = threadp;
xe->XE.Race.h2_ct_accSzB = (Int)conf_szB;
xe->XE.Race.h2_ct_accIsW = conf_isW;
+ xe->XE.Race.h2_ct_locksHeldW
+ = enumerate_WordSet_into_LockP_vector(
+ HG_(get_univ_lsets)(),
+ conf_locksHeldW,
+ True/*allowed_to_be_invalid*/
+ );
}
}
@@ -424,8 +559,10 @@
tl_assert( HG_(is_sane_LockN)(lk) );
init_XError(&xe);
xe.tag = XE_UnlockUnlocked;
- xe.XE.UnlockUnlocked.thr = thr;
- xe.XE.UnlockUnlocked.lock = mk_LockP_from_LockN(lk);
+ xe.XE.UnlockUnlocked.thr
+ = thr;
+ xe.XE.UnlockUnlocked.lock
+ = mk_LockP_from_LockN(lk, False/*!allowed_to_be_invalid*/);
// FIXME: tid vs thr
tl_assert( HG_(is_sane_ThreadId)(thr->coretid) );
tl_assert( thr->coretid != VG_INVALID_THREADID );
@@ -444,7 +581,8 @@
xe.tag = XE_UnlockForeign;
xe.XE.UnlockForeign.thr = thr;
xe.XE.UnlockForeign.owner = owner;
- xe.XE.UnlockForeign.lock = mk_LockP_from_LockN(lk);
+ xe.XE.UnlockForeign.lock
+ = mk_LockP_from_LockN(lk, False/*!allowed_to_be_invalid*/);
// FIXME: tid vs thr
tl_assert( HG_(is_sane_ThreadId)(thr->coretid) );
tl_assert( thr->coretid != VG_INVALID_THREADID );
@@ -639,6 +777,10 @@
} else {
+ VG_(umsg)("---Thread-Announcement----------"
+ "--------------------------------" "\n");
+ VG_(umsg)("\n");
+
if (thr->errmsg_index == 1) {
tl_assert(thr->created_at == NULL);
VG_(message)(Vg_UserMsg,
@@ -659,6 +801,76 @@
}
+/* Announce 'lk'. */
+static void announce_LockP ( Lock* lk )
+{
+ tl_assert(lk);
+ if (lk == Lock_INVALID)
+ return; /* Can't be announced -- we know nothing about it. */
+ tl_assert(lk->magic == LockP_MAGIC);
+ if (!lk->appeared_at)
+ return; /* There's nothing we can show */
+
+ if (VG_(clo_xml)) {
+ /* fixme: add announcement */
+ } else {
+ VG_(umsg)( "Lock at %p was first observed\n",
+ (void*)lk->guestaddr );
+ VG_(pp_ExeContext)( lk->appeared_at );
+ VG_(umsg)("\n");
+ }
+}
+
+/* Announce (that is, print point-of-first-observation) for the
+ locks in 'lockvec' and, if non-NULL, 'lockvec2'. */
+static void announce_combined_LockP_vecs ( Lock** lockvec,
+ Lock** lockvec2 )
+{
+ UWord i;
+ tl_assert(lockvec);
+ for (i = 0; lockvec[i]; i++) {
+ announce_LockP(lockvec[i]);
+ }
+ if (lockvec2) {
+ for (i = 0; lockvec2[i]; i++) {
+ Lock* lk = lockvec2[i];
+ if (!elem_LockP_vector(lockvec, lk))
+ announce_LockP(lk);
+ }
+ }
+}
+
+
+static void show_LockP_summary_textmode ( Lock** locks, HChar* pre )
+{
+ tl_assert(locks);
+ UWord i;
+ UWord nLocks = 0, nLocksValid = 0;
+ count_LockP_vector(&nLocks, &nLocksValid, locks);
+ tl_assert(nLocksValid <= nLocks);
+
+ if (nLocks == 0) {
+ VG_(umsg)( "%sLocks held: none", pre );
+ } else {
+ VG_(umsg)( "%sLocks held: %lu, at address%s ",
+ pre, nLocks, nLocksValid == 1 ? "" : "es" );
+ }
+
+ if (nLocks > 0) {
+ for (i = 0; i < nLocks; i++) {
+ if (locks[i] == Lock_INVALID)
+ continue;
+ VG_(umsg)( "%p", (void*)locks[i]->guestaddr);
+ if (locks[i+1] != NULL)
+ VG_(umsg)(" ");
+ }
+ if (nLocksValid < nLocks)
+ VG_(umsg)(" (and %lu that can't be shown)", nLocks - nLocksValid);
+ }
+ VG_(umsg)("\n");
+}
+
+
/* This is the "this error is due to be printed shortly; so have a
look at it any print any preamble you want" function. We use it to
announce any previously un-announced threads in the upcoming error
@@ -707,6 +919,12 @@
{
const Bool xml = VG_(clo_xml); /* a shorthand, that's all */
+ if (!xml) {
+ VG_(umsg)("--------------------------------"
+ "--------------------------------" "\n");
+ VG_(umsg)("\n");
+ }
+
XError *xe = (XError*)VG_(get_error_extra)(err);
tl_assert(xe);
@@ -1005,18 +1223,27 @@
} else {
/* ------ Text ------ */
+ announce_combined_LockP_vecs( xe->XE.Race.locksHeldW,
+ xe->XE.Race.h2_ct_locksHeldW );
+
emit( "Possible data race during %s of size %d "
"at %#lx by thread #%d\n",
what, szB, err_ga, (Int)xe->XE.Race.thr->errmsg_index );
+
+ tl_assert(xe->XE.Race.locksHeldW);
+ show_LockP_summary_textmode( xe->XE.Race.locksHeldW, "" );
VG_(pp_ExeContext)( VG_(get_error_where)(err) );
if (xe->XE.Race.h2_ct) {
tl_assert(xe->XE.Race.h2_ct_accEC); // assured by update_extra
- emit( " This conflicts with a previous %s of size %d "
+ tl_assert(xe->XE.Race.h2_ct_locksHeldW);
+ emit( "\n" );
+ emit( "This conflicts with a previous %s of size %d "
"by thread #%d\n",
xe->XE.Race.h2_ct_accIsW ? "write" : "read",
xe->XE.Race.h2_ct_accSzB,
xe->XE.Race.h2_ct->errmsg_index );
+ show_LockP_summary_textmode( xe->XE.Race.h2_ct_locksHeldW, "" );
VG_(pp_ExeContext)( xe->XE.Race.h2_ct_accEC );
}
@@ -1049,7 +1276,8 @@
xe->XE.Race.hszB);
VG_(pp_ExeContext)( xe->XE.Race.hctxt );
} else {
- emit(" Address %#lx is %ld bytes inside a block "
+ emit("\n");
+ emit("Address %#lx is %ld bytes inside a block "
"of size %ld alloc'd\n", err_ga, delta,
xe->XE.Race.hszB);
VG_(pp_ExeContext)( xe->XE.Race.hctxt );
@@ -1060,12 +1288,23 @@
Note that in XML mode, it will already by nicely wrapped up
in tags, either <auxwhat> or <xauxwhat>, so we can just emit
it verbatim. */
- if (xe->XE.Race.descr1)
- emit( "%s%s\n", xml ? " " : " ",
- (HChar*)VG_(indexXA)( xe->XE.Race.descr1, 0 ) );
- if (xe->XE.Race.descr2)
- emit( "%s%s\n", xml ? " " : " ",
- (HChar*)VG_(indexXA)( xe->XE.Race.descr2, 0 ) );
+ if (xml) {
+ if (xe->XE.Race.descr1)
+ emit( " %s\n",
+ (HChar*)VG_(indexXA)( xe->XE.Race.descr1, 0 ) );
+ if (xe->XE.Race.descr2)
+ emit( " %s\n",
+ (HChar*)VG_(indexXA)( xe->XE.Race.descr2, 0 ) );
+ } else {
+ if (xe->XE.Race.descr1 || xe->XE.Race.descr2)
+ emit("\n");
+ if (xe->XE.Race.descr1)
+ emit( "%s\n",
+ (HChar*)VG_(indexXA)( xe->XE.Race.descr1, 0 ) );
+ if (xe->XE.Race.descr2)
+ emit( "%s\n",
+ (HChar*)VG_(indexXA)( xe->XE.Race.descr2, 0 ) );
+ }
break; /* case XE_Race */
} /* case XE_Race */
Modified: branches/HGDEV2/helgrind/hg_lock_n_thread.h
===================================================================
--- branches/HGDEV2/helgrind/hg_lock_n_thread.h 2011-03-20 12:50:48 UTC (rev 11658)
+++ branches/HGDEV2/helgrind/hg_lock_n_thread.h 2011-03-20 20:50:54 UTC (rev 11659)
@@ -45,9 +45,9 @@
#define LockP_MAGIC 0x755b5456 /* persistent (copied) locks */
-/* These are handles for Word sets. CONSTRAINTS: must be (very) small
- ints numbered from zero, since < 30-bit versions of them are used to
- encode thread-sets and lock-sets in 32-bit shadow words. */
+/* These are handles for Word sets. CONSTRAINTS: must be small ints
+ numbered from zero, since 32-bit versions of them are used to
+ encode lock-sets in libhb's history records (Thr_n_RCEC). */
typedef WordSet WordSetID;
@@ -156,6 +156,8 @@
}
Lock;
+#define Lock_INVALID ((Lock*)1UL)
+
/*----------------------------------------------------------------*/
/*--- Sanity checking ---*/
/*----------------------------------------------------------------*/
Modified: branches/HGDEV2/helgrind/hg_main.c
===================================================================
--- branches/HGDEV2/helgrind/hg_main.c 2011-03-20 12:50:48 UTC (rev 11658)
+++ branches/HGDEV2/helgrind/hg_main.c 2011-03-20 20:50:54 UTC (rev 11659)
@@ -137,7 +137,15 @@
static WordSetU* univ_lsets = NULL; /* sets of Lock* */
static WordSetU* univ_laog = NULL; /* sets of Lock*, for LAOG */
+/* Allow libhb to get at the universe of locksets stored
+ here. Sigh. */
+WordSetU* HG_(get_univ_lsets) ( void ) { return univ_lsets; }
+/* Allow libhb to get at the list of locks stored here. Ditto
+ sigh. */
+Lock* HG_(get_admin_locks) ( void ) { return admin_locks; }
+
+
/*----------------------------------------------------------------*/
/*--- Simple helpers for the data structures ---*/
/*----------------------------------------------------------------*/
@@ -538,6 +546,7 @@
static void initialise_data_structures ( Thr* hbthr_root )
{
Thread* thr;
+ WordSetID wsid;
/* Get everything initialised and zeroed. */
tl_assert(admin_threads == NULL);
@@ -559,6 +568,11 @@
univ_lsets = HG_(newWordSetU)( HG_(zalloc), "hg.ids.4", HG_(free),
8/*cacheSize*/ );
tl_assert(univ_lsets != NULL);
+ /* Ensure that univ_lsets is non-empty, with lockset zero being the
+ empty lockset. hg_errors.c relies on the assumption that
+ lockset number zero in univ_lsets is always valid. */
+ wsid = HG_(emptyWS)(univ_lsets);
+ tl_assert(wsid == 0);
tl_assert(univ_laog == NULL);
if (HG_(clo_track_lockorders)) {
Modified: branches/HGDEV2/helgrind/libhb.h
===================================================================
--- branches/HGDEV2/helgrind/libhb.h 2011-03-20 12:50:48 UTC (rev 11658)
+++ branches/HGDEV2/helgrind/libhb.h 2011-03-20 20:50:54 UTC (rev 11659)
@@ -146,11 +146,22 @@
/* Extract info from the conflicting-access machinery. */
Bool libhb_event_map_lookup ( /*OUT*/ExeContext** resEC,
- /*OUT*/Thr** resThr,
- /*OUT*/SizeT* resSzB,
- /*OUT*/Bool* resIsW,
+ /*OUT*/Thr** resThr,
+ /*OUT*/SizeT* resSzB,
+ /*OUT*/Bool* resIsW,
+ /*OUT*/WordSetID* locksHeldW,
Thr* thr, Addr a, SizeT szB, Bool isW );
+/* ------ Exported from hg_main.c ------ */
+/* Yes, this is a horrible tangle. Sigh. */
+
+/* Get the univ_lset (universe for locksets) from hg_main.c. Sigh. */
+WordSetU* HG_(get_univ_lsets) ( void );
+
+/* Get the the header pointer for the double linked list of locks
+ (admin_locks). */
+Lock* HG_(get_admin_locks) ( void );
+
#endif /* __LIBHB_H */
/*--------------------------------------------------------------------*/
Modified: branches/HGDEV2/helgrind/libhb_core.c
===================================================================
--- branches/HGDEV2/helgrind/libhb_core.c 2011-03-20 12:50:48 UTC (rev 11658)
+++ branches/HGDEV2/helgrind/libhb_core.c 2011-03-20 20:50:54 UTC (rev 11659)
@@ -152,11 +152,11 @@
at a clock rate of 5 GHz is 162.9 days. And that's doing nothing
but VTS ticks, which isn't realistic.
- NB1: SCALARTS_N_THRBITS must be 21 or lower. The obvious limit is
- 32 since a ThrID is a UInt. 21 comes from the fact that
+ NB1: SCALARTS_N_THRBITS must be 29 or lower. The obvious limit is
+ 32 since a ThrID is a UInt. 29 comes from the fact that
'Thr_n_RCEC', which records information about old accesses, packs
- not only a ThrID but also 2+1+8 other bits in a UInt, hence
- limiting size to 32-(2+1+8) == 21.
+ not only a ThrID but also 2+1 other bits (access size and
+ writeness) in a UInt, hence limiting size to 32-(2+1) == 29.
NB2: thrid values are issued upwards from 1024, and values less
than that aren't valid. This isn't per se necessary (any order
@@ -182,7 +182,8 @@
ThrID == 0 to denote an empty Thr_n_RCEC record. So ThrID == 0
must never be a valid ThrID. Given NB2 that's OK.
*/
-#define SCALARTS_N_THRBITS 18 /* valid range: 11 to 21 inclusive */
+#define SCALARTS_N_THRBITS 18 /* valid range: 11 to 29 inclusive */
+
#define SCALARTS_N_TYMBITS (64 - SCALARTS_N_THRBITS)
typedef
struct {
@@ -4175,13 +4176,19 @@
// (UInt) `echo "Old Reference Information" | md5sum`
#define OldRef_MAGIC 0x30b1f075UL
-/* Records an access: a thread and a context. The size
- (1,2,4,8) and read-or-writeness are also encoded as
- follows: bottom bit of .thr is 1 if write, 0 if read
- bottom 2 bits of .rcec are encode size:
- 00 = 1, 01 = 2, 10 = 4, 11 = 8
+/* Records an access: a thread, a context (size & writeness) and the
+ number of held locks. The size (1,2,4,8) is encoded as 00 = 1, 01 =
+ 2, 10 = 4, 11 = 8.
*/
-typedef struct { Thr* thr; RCEC* rcec; } Thr_n_RCEC;
+typedef
+ struct {
+ RCEC* rcec;
+ WordSetID locksHeldW;
+ UInt thrid : SCALARTS_N_THRBITS;
+ UInt szLg2B : 2;
+ UInt isW : 1;
+ }
+ Thr_n_RCEC;
#define N_OLDREF_ACCS 5
@@ -4190,7 +4197,7 @@
UWord magic; /* sanity check only */
UWord gen; /* when most recently accessed */
/* or free list when not in use */
- /* unused slots in this array have .thr == NULL */
+ /* unused slots in this array have .thrid == 0, which is invalid */
Thr_n_RCEC accs[N_OLDREF_ACCS];
}
OldRef;
@@ -4215,13 +4222,6 @@
static UWord oldrefTreeN = 0; /* # elems in oldrefTree */
static UWord oldrefGenIncAt = 0; /* inc gen # when size hits this */
-inline static void* ptr_or_UWord ( void* p, UWord w ) {
- return (void*)( ((UWord)p) | ((UWord)w) );
-}
-inline static void* ptr_and_UWord ( void* p, UWord w ) {
- return (void*)( ((UWord)p) & ((UWord)w) );
-}
-
inline static UInt min_UInt ( UInt a, UInt b ) {
return a < b ? a : b;
}
@@ -4253,50 +4253,51 @@
UWord keyW, valW;
Bool b;
+ tl_assert(thr);
+ ThrID thrid = thr->thrid;
+ tl_assert(thrid != 0); /* zero is used to denote an empty slot. */
+
+ WordSetID locksHeldW = thr->hgthread->locksetW;
+
rcec = get_RCEC( thr );
ctxt__rcinc(rcec);
- /* encode the size and writeness of the transaction in the bottom
- two bits of thr and rcec. */
- thr = ptr_or_UWord(thr, isW ? 1 : 0);
+ UInt szLg2B = 0;
switch (szB) {
/* This doesn't look particularly branch-predictor friendly. */
- case 1: rcec = ptr_or_UWord(rcec, 0); break;
- case 2: rcec = ptr_or_UWord(rcec, 1); break;
- case 4: rcec = ptr_or_UWord(rcec, 2); break;
- case 8: rcec = ptr_or_UWord(rcec, 3); break;
+ case 1: szLg2B = 0; break;
+ case 2: szLg2B = 1; break;
+ case 4: szLg2B = 2; break;
+ case 8: szLg2B = 3; break;
default: tl_assert(0);
}
- /* Look in the map to see if we already have this. */
+ /* Look in the map to see if we already have a record for this
+ address. */
b = VG_(lookupSWA)( oldrefTree, &keyW, &valW, a );
if (b) {
/* We already have a record for this address. We now need to
- see if we have a stack trace pertaining to this (thread, R/W,
+ see if we have a stack trace pertaining to this (thrid, R/W,
size) triple. */
tl_assert(keyW == a);
ref = (OldRef*)valW;
tl_assert(ref->magic == OldRef_MAGIC);
- tl_assert(thr);
for (i = 0; i < N_OLDREF_ACCS; i++) {
- if (ref->accs[i].thr != thr)
+ if (ref->accs[i].thrid != thrid)
continue;
- /* since .thr encodes both the accessing thread and the
- read/writeness, we know now that at least those features
- of the access match this entry. So we just need to check
- the size indication. Do this by inspecting the lowest 2 bits of
- .rcec, which contain the encoded size info. */
- if (ptr_and_UWord(ref->accs[i].rcec,3) != ptr_and_UWord(rcec,3))
+ if (ref->accs[i].szLg2B != szLg2B)
continue;
+ if (ref->accs[i].isW != (UInt)(isW & 1))
+ continue;
/* else we have a match, so stop looking. */
break;
}
if (i < N_OLDREF_ACCS) {
- /* thread 'thr' has an entry at index 'i'. Update it. */
+ /* thread 'thr' has an entry at index 'i'. Update its RCEC. */
if (i > 0) {
Thr_n_RCEC tmp = ref->accs[i-1];
ref->accs[i-1] = ref->accs[i];
@@ -4305,31 +4306,36 @@
}
if (rcec == ref->accs[i].rcec) stats__ctxt_rcdec1_eq++;
stats__ctxt_rcdec1++;
- ctxt__rcdec( ptr_and_UWord(ref->accs[i].rcec, ~3) );
- ref->accs[i].rcec = rcec;
- tl_assert(ref->accs[i].thr == thr);
+ ctxt__rcdec( ref->accs[i].rcec );
+ tl_assert(ref->accs[i].thrid == thrid);
+ /* Update the RCEC and the W-held lockset. */
+ ref->accs[i].rcec = rcec;
+ ref->accs[i].locksHeldW = locksHeldW;
} else {
- /* No entry for this (thread, R/W, size) triple. Shuffle all
- of them down one slot, and put the new entry at the start
- of the array. */
- if (ref->accs[N_OLDREF_ACCS-1].thr) {
+ /* No entry for this (thread, R/W, size, nWHeld) quad.
+ Shuffle all of them down one slot, and put the new entry
+ at the start of the array. */
+ if (ref->accs[N_OLDREF_ACCS-1].thrid != 0) {
/* the last slot is in use. We must dec the rc on the
associated rcec. */
tl_assert(ref->accs[N_OLDREF_ACCS-1].rcec);
stats__ctxt_rcdec2++;
if (0 && 0 == (stats__ctxt_rcdec2 & 0xFFF))
VG_(printf)("QQQQ %lu overflows\n",stats__ctxt_rcdec2);
- ctxt__rcdec( ptr_and_UWord(ref->accs[N_OLDREF_ACCS-1].rcec, ~3) );
+ ctxt__rcdec( ref->accs[N_OLDREF_ACCS-1].rcec );
} else {
tl_assert(!ref->accs[N_OLDREF_ACCS-1].rcec);
}
for (j = N_OLDREF_ACCS-1; j >= 1; j--)
ref->accs[j] = ref->accs[j-1];
- ref->accs[0].thr = thr;
- ref->accs[0].rcec = rcec;
- /* thr==NULL is used to signify an empty slot, so we can't
- add a NULL thr. */
- tl_assert(ptr_and_UWord(thr, ~3) != 0);
+ ref->accs[0].thrid = thrid;
+ ref->accs[0].szLg2B = szLg2B;
+ ref->accs[0].isW = (UInt)(isW & 1);
+ ref->accs[0].locksHeldW = locksHeldW;
+ ref->accs[0].rcec = rcec;
+ /* thrid==0 is used to signify an empty slot, so we can't
+ add zero thrid (such a ThrID is invalid anyway). */
+ /* tl_assert(thrid != 0); */ /* There's a dominating assert above. */
}
ref->gen = oldrefGen;
@@ -4346,15 +4352,24 @@
ref = alloc_OldRef();
ref->magic = OldRef_MAGIC;
- ref->gen = oldrefGen;
- ref->accs[0].rcec = rcec;
- ref->accs[0].thr = thr;
- /* thr==NULL is used to signify an empty slot, so we can't add a
- NULL thr. */
- tl_assert(ptr_and_UWord(thr, ~3) != 0);
+ ref->gen = oldrefGen;
+ ref->accs[0].thrid = thrid;
+ ref->accs[0].szLg2B = szLg2B;
+ ref->accs[0].isW = (UInt)(isW & 1);
+ ref->accs[0].locksHeldW = locksHeldW;
+ ref->accs[0].rcec = rcec;
+
+ /* thrid==0 is used to signify an empty slot, so we can't
+ add zero thrid (such a ThrID is invalid anyway). */
+ /* tl_assert(thrid != 0); */ /* There's a dominating assert above. */
+
+ /* Clear out the rest of the entries */
for (j = 1; j < N_OLDREF_ACCS; j++) {
- ref->accs[j].thr = NULL;
- ref->accs[j].rcec = NULL;
+ ref->accs[j].rcec = NULL;
+ ref->accs[j].thrid = 0;
+ ref->accs[j].szLg2B = 0;
+ ref->accs[j].isW = 0;
+ ref->accs[j].locksHeldW = 0;
}
VG_(addToSWA)( oldrefTree, a, (UWord)ref );
oldrefTreeN++;
@@ -4363,10 +4378,12 @@
}
+/* Extract info from the conflicting-access machinery. */
Bool libhb_event_map_lookup ( /*OUT*/ExeContext** resEC,
- /*OUT*/Thr** resThr,
- /*OUT*/SizeT* resSzB,
- /*OUT*/Bool* resIsW,
+ /*OUT*/Thr** resThr,
+ /*OUT*/SizeT* resSzB,
+ /*OUT*/Bool* resIsW,
+ /*OUT*/WordSetID* locksHeldW,
Thr* thr, Addr a, SizeT szB, Bool isW )
{
Word i, j;
@@ -4374,11 +4391,12 @@
UWord keyW, valW;
Bool b;
- Thr* cand_thr;
- RCEC* cand_rcec;
- Bool cand_isW;
- SizeT cand_szB;
- Addr cand_a;
+ ThrID cand_thrid;
+ RCEC* cand_rcec;
+ Bool cand_isW;
+ SizeT cand_szB;
+ WordSetID cand_locksHeldW;
+ Addr cand_a;
Addr toCheck[15];
Int nToCheck = 0;
@@ -4386,6 +4404,8 @@
tl_assert(thr);
tl_assert(szB == 8 || szB == 4 || szB == 2 || szB == 1);
+ ThrID thrid = thr->thrid;
+
toCheck[nToCheck++] = a;
for (i = -7; i < (Word)szB; i++) {
if (i != 0)
@@ -4407,33 +4427,27 @@
ref = (OldRef*)valW;
tl_assert(keyW == cand_a);
tl_assert(ref->magic == OldRef_MAGIC);
- tl_assert(ref->accs[0].thr); /* first slot must always be used */
+ tl_assert(ref->accs[0].thrid != 0); /* first slot must always be used */
- cand_thr = NULL;
- cand_rcec = NULL;
- cand_isW = False;
- cand_szB = 0;
+ cand_thrid = 0; /* invalid; see comments in event_map_bind */
+ cand_rcec = NULL;
+ cand_isW = False;
+ cand_szB = 0;
+ cand_locksHeldW = 0; /* always valid; see initialise_data_structures() */
for (i = 0; i < N_OLDREF_ACCS; i++) {
Thr_n_RCEC* cand = &ref->accs[i];
- cand_thr = ptr_and_UWord(cand->thr, ~3);
- cand_rcec = ptr_and_UWord(cand->rcec, ~3);
- /* Decode the writeness from the bottom bit of .thr. */
- cand_isW = 1 == (UWord)ptr_and_UWord(cand->thr, 1);
- /* Decode the size from the bottom two bits of .rcec. */
- switch ((UWord)ptr_and_UWord(cand->rcec, 3)) {
- case 0: cand_szB = 1; break;
- case 1: cand_szB = 2; break;
- case 2: cand_szB = 4; break;
- case 3: cand_szB = 8; break;
- default: tl_assert(0);
- }
+ cand_rcec = cand->rcec;
+ cand_thrid = cand->thrid;
+ cand_isW = (Bool)cand->isW;
+ cand_szB = 1 << cand->szLg2B;
+ cand_locksHeldW = cand->locksHeldW;
- if (cand_thr == NULL)
+ if (cand_thrid == 0)
/* This slot isn't in use. Ignore it. */
continue;
- if (cand_thr == thr)
+ if (cand_thrid == thrid)
/* This is an access by the same thread, but we're only
interested in accesses from other threads. Ignore. */
continue;
@@ -4456,7 +4470,7 @@
if (i < N_OLDREF_ACCS) {
Int n, maxNFrames;
/* return with success */
- tl_assert(cand_thr);
+ tl_assert(cand_thrid);
tl_assert(cand_rcec);
tl_assert(cand_rcec->magic == RCEC_MAGIC);
tl_assert(cand_szB >= 1);
@@ -4465,10 +4479,12 @@
for (n = 0; n < maxNFrames; n++) {
if (0 == cand_rcec->frames[n]) break;
}
- *resEC = VG_(make_ExeContext_from_StackTrace)(cand_rcec->frames, n);
- *resThr = cand_thr;
- *resSzB = cand_szB;
- *resIsW = cand_isW;
+ *resEC = VG_(make_ExeContext_from_StackTrace)
+ (cand_rcec->frames, n);
+ *resThr = Thr__from_ThrID(cand_thrid);
+ *resSzB = cand_szB;
+ *resIsW = cand_isW;
+ *locksHeldW = cand_locksHeldW;
return True;
}
@@ -4555,9 +4571,9 @@
oldref = (OldRef*)valW;
tl_assert(oldref->magic == OldRef_MAGIC);
for (i = 0; i < N_OLDREF_ACCS; i++) {
- Thr* aThr = ptr_and_UWord(oldref->accs[i].thr, ~3);
- RCEC* aRef = ptr_and_UWord(oldref->accs[i].rcec, ~3);
- if (aThr) {
+ ThrID aThrID = oldref->accs[i].thrid;
+ RCEC* aRef = oldref->accs[i].rcec;
+ if (aThrID != 0) {
tl_assert(aRef);
tl_assert(aRef->magic == RCEC_MAGIC);
aRef->rcX++;
@@ -4798,14 +4814,14 @@
tl_assert(keyW == ga2del);
oldref = (OldRef*)valW;
for (j = 0; j < N_OLDREF_ACCS; j++) {
- Thr* aThr = ptr_and_UWord(oldref->accs[j].thr, ~3);
- RCEC* aRef = ptr_and_UWord(oldref->accs[j].rcec, ~3);
+ ThrID aThrID = oldref->accs[j].thrid;
+ RCEC* aRef = oldref->accs[j].rcec;
if (aRef) {
- tl_assert(aThr);
+ tl_assert(aThrID != 0);
stats__ctxt_rcdec3++;
ctxt__rcdec( aRef );
} else {
- tl_assert(!aThr);
+ tl_assert(aThrID == 0);
}
}
@@ -6158,9 +6174,27 @@
// We will have to have to store a large number of these,
// so make sure they're the size we expect them to be.
tl_assert(sizeof(ScalarTS) == 8);
- tl_assert(SCALARTS_N_THRBITS >= 11); /* because first 1024 unusable */
- tl_assert(SCALARTS_N_THRBITS <= 32); /* so as to fit in a UInt */
+ /* because first 1024 unusable */
+ tl_assert(SCALARTS_N_THRBITS >= 11);
+ /* so as to fit in a UInt w/ 3 bits to spare (see defn of
+ Thr_n_RCEC). */
+ tl_assert(SCALARTS_N_THRBITS <= 29);
+
+ /* Need to be sure that Thr_n_RCEC is 2 words (64-bit) or 3 words
+ (32-bit). It's not correctness-critical, but there are a lot of
+ them, so it's important from a space viewpoint. Unfortunately
+ we simply can't pack it into 2 words on a 32-bit target. */
+ if (sizeof(UWord) == 8) {
+ tl_assert(sizeof(Thr_n_RCEC) == 16);
+ } else {
+ tl_assert(sizeof(Thr_n_RCEC) == 12);
+ }
+
+ /* Word sets really are 32 bits. Even on a 64 bit target. */
+ tl_assert(sizeof(WordSetID) == 4);
+ tl_assert(sizeof(WordSet) == sizeof(WordSetID));
+
tl_assert(get_stacktrace);
tl_assert(get_EC);
main_get_stacktrace = get_stacktrace;
|
|
From: Konstantin S. <kon...@gm...> - 2011-03-21 12:10:27
|
Cool!
Is this for free, or it adds a non-zero overhead?
--kcc
On Sun, Mar 20, 2011 at 11:50 PM, <sv...@va...> wrote:
> Author: sewardj
> Date: 2011-03-20 20:50:54 +0000 (Sun, 20 Mar 2011)
> New Revision: 11659
>
> Log:
> Add the ability to show the set of locks shown by each thread in a
> race, which makes diagnosing races a lot easier. As a side effect,
> change the way errors are printed to more clearly delimit the
> boundaries of errors.
>
>
>
> Modified:
> branches/HGDEV2/helgrind/hg_errors.c
> branches/HGDEV2/helgrind/hg_lock_n_thread.h
> branches/HGDEV2/helgrind/hg_main.c
> branches/HGDEV2/helgrind/libhb.h
> branches/HGDEV2/helgrind/libhb_core.c
>
>
> Modified: branches/HGDEV2/helgrind/hg_errors.c
> ===================================================================
> --- branches/HGDEV2/helgrind/hg_errors.c 2011-03-20 12:50:48 UTC (rev 11658)
> +++ branches/HGDEV2/helgrind/hg_errors.c 2011-03-20 20:50:54 UTC (rev 11659)
> @@ -110,11 +110,56 @@
> return 0;
> }
>
> -static Lock* mk_LockP_from_LockN ( Lock* lkn )
> +/* Given a normal Lock (LockN), convert it to a persistent Lock
> + (LockP). In some cases the LockN could be invalid (if it's been
> + freed), so we enquire, in hg_main.c's admin_locks list, whether it
> + is in fact valid. If allowed_to_be_invalid is True, then it's OK
> + for the LockN to be invalid, in which case Lock_INVALID is
> + returned. In all other cases, we insist that the LockN is a valid
> + lock, and return its corresponding LockP.
> +
> + Why can LockNs sometimes be invalid? Because they are harvested
> + from locksets that are attached to the OldRef info for conflicting
> + threads. By the time we detect a race, the some of the elements of
> + the lockset may have been destroyed by the client, in which case
> + the corresponding Lock structures we maintain will have been freed.
> +
> + So we check that each LockN is a member of the admin_locks double
> + linked list of all Lock structures. That stops us prodding around
> + in potentially freed-up Lock structures. However, it's not quite a
> + proper check: if a new Lock has been reallocated at the same
> + address as one which was previously freed, we'll wind up copying
> + the new one as the basis for the LockP, which is completely bogus
> + because it is unrelated to the previous Lock that lived there.
> + Let's hope that doesn't happen too often.
> +*/
> +static Lock* mk_LockP_from_LockN ( Lock* lkn,
> + Bool allowed_to_be_invalid )
> {
> Lock* lkp = NULL;
> HG_(stats__LockN_to_P_queries)++;
> +
> + /* First off, let's do some sanity checks. If
> + allowed_to_be_invalid is False, we _must_ be able to find 'lkn'
> + in admin_locks; else we must assert. If it is True, it's OK for
> + it not to be findable, but in that case we must return
> + Lock_INVALID right away. */
> + Lock* lock_list = HG_(get_admin_locks)();
> + while (lock_list) {
> + if (lock_list == lkn)
> + break;
> + lock_list = lock_list->admin_next;
> + }
> + if (lock_list == NULL) {
> + /* We didn't find it. That possibility has to be OK'd by the
> + caller. */
> + tl_assert(allowed_to_be_invalid);
> + return Lock_INVALID;
> + }
> +
> + /* So we must be looking at a valid LockN. */
> tl_assert( HG_(is_sane_LockN)(lkn) );
> +
> if (!map_LockN_to_P) {
> map_LockN_to_P = VG_(newFM)( HG_(zalloc), "hg.mLPfLN.1",
> HG_(free), lock_unique_cmp );
> @@ -139,6 +184,75 @@
> return lkp;
> }
>
> +/* Expand a WordSet of LockN*'s into a NULL-terminated vector of
> + LockP*'s. Any LockN's that can't be converted into a LockP
> + (because they have been freed, see comment on mk_LockP_from_LockN)
> + are converted instead into the value Lock_INVALID. Hence the
> + returned vector is a sequence: zero or more (valid LockP* or
> + LockN_INVALID), terminated by a NULL. */
> +static
> +Lock** enumerate_WordSet_into_LockP_vector( WordSetU* univ_lsets,
> + WordSetID lockset,
> + Bool allowed_to_be_invalid )
> +{
> + tl_assert(univ_lsets);
> + tl_assert( HG_(plausibleWS)(univ_lsets, lockset) );
> + UWord nLocks = HG_(cardinalityWS)(univ_lsets, lockset);
> + Lock** lockPs = HG_(zalloc)( "hg.eWSiLPa",
> + (nLocks+1) * sizeof(Lock*) );
> + tl_assert(lockPs);
> + tl_assert(lockPs[nLocks] == NULL); /* pre-NULL terminated */
> + UWord* lockNs = NULL;
> + UWord nLockNs = 0;
> + if (nLocks > 0) {
> + /* HG_(getPayloadWS) doesn't assign non-NULL to &lockNs if the
> + lockset is empty; hence the guarding "if". Sigh. */
> + HG_(getPayloadWS)( &lockNs, &nLockNs, univ_lsets, lockset );
> + tl_assert(lockNs);
> + }
> + UWord i;
> + /* Convert to LockPs. */
> + for (i = 0; i < nLockNs; i++) {
> + lockPs[i] = mk_LockP_from_LockN( (Lock*)lockNs[i],
> + allowed_to_be_invalid );
> + }
> + return lockPs;
> +}
> +
> +/* Get the number of useful elements in a vector created by
> + enumerate_WordSet_into_LockP_vector. Returns both the total number
> + of elements (not including the terminating NULL) and the number of
> + non-Lock_INVALID elements. */
> +static void count_LockP_vector ( /*OUT*/UWord* nLocks,
> + /*OUT*/UWord* nLocksValid,
> + Lock** vec )
> +{
> + tl_assert(vec);
> + *nLocks = *nLocksValid = 0;
> + UWord n = 0;
> + while (vec[n]) {
> + (*nLocks)++;
> + if (vec[n] != Lock_INVALID)
> + (*nLocksValid)++;
> + n++;
> + }
> +}
> +
> +/* Find out whether 'lk' is in 'vec'. */
> +static Bool elem_LockP_vector ( Lock** vec, Lock* lk )
> +{
> + tl_assert(vec);
> + tl_assert(lk);
> + UWord n = 0;
> + while (vec[n]) {
> + if (vec[n] == lk)
> + return True;
> + n++;
> + }
> + return False;
> +}
> +
> +
> /* Errors:
>
> race: program counter
> @@ -179,6 +293,7 @@
> Int szB;
> Bool isWrite;
> Thread* thr;
> + Lock** locksHeldW;
> /* descr1/2 provide a description of stack/global locs */
> XArray* descr1; /* XArray* of HChar */
> XArray* descr2; /* XArray* of HChar */
> @@ -195,6 +310,7 @@
> ExeContext* h2_ct_accEC;
> Int h2_ct_accSzB;
> Bool h2_ct_accIsW;
> + Lock** h2_ct_locksHeldW;
> } Race;
> struct {
> Thread* thr; /* doing the unlocking */
> @@ -264,6 +380,18 @@
>
> if (xe->tag == XE_Race) {
>
> + /* Note the set of locks that the thread is (w-)holding.
> + Convert the WordSetID of LockN*'s into a NULL-terminated
> + vector of LockP*'s. We don't expect to encounter any invalid
> + LockNs in this conversion. */
> + tl_assert(xe->XE.Race.thr);
> + xe->XE.Race.locksHeldW
> + = enumerate_WordSet_into_LockP_vector(
> + HG_(get_univ_lsets)(),
> + xe->XE.Race.thr->locksetW,
> + False/*!allowed_to_be_invalid*/
> + );
> +
> /* See if we can come up with a source level description of the
> raced-upon address. This is potentially expensive, which is
> why it's only done at the update_extra point, not when the
> @@ -325,18 +453,19 @@
> can rustle up a plausible-looking conflicting memory access
> to show. */
> if (HG_(clo_history_level) >= 2) {
> - Thr* thrp = NULL;
> - ExeContext* wherep = NULL;
> - Addr acc_addr = xe->XE.Race.data_addr;
> - Int acc_szB = xe->XE.Race.szB;
> - Thr* acc_thr = xe->XE.Race.thr->hbthr;
> - Bool acc_isW = xe->XE.Race.isWrite;
> - SizeT conf_szB = 0;
> - Bool conf_isW = False;
> + Thr* thrp = NULL;
> + ExeContext* wherep = NULL;
> + Addr acc_addr = xe->XE.Race.data_addr;
> + Int acc_szB = xe->XE.Race.szB;
> + Thr* acc_thr = xe->XE.Race.thr->hbthr;
> + Bool acc_isW = xe->XE.Race.isWrite;
> + SizeT conf_szB = 0;
> + Bool conf_isW = False;
> + WordSetID conf_locksHeldW = 0;
> tl_assert(!xe->XE.Race.h2_ct_accEC);
> tl_assert(!xe->XE.Race.h2_ct);
> if (libhb_event_map_lookup(
> - &wherep, &thrp, &conf_szB, &conf_isW,
> + &wherep, &thrp, &conf_szB, &conf_isW, &conf_locksHeldW,
> acc_thr, acc_addr, acc_szB, acc_isW )) {
> Thread* threadp;
> tl_assert(wherep);
> @@ -347,6 +476,12 @@
> xe->XE.Race.h2_ct = threadp;
> xe->XE.Race.h2_ct_accSzB = (Int)conf_szB;
> xe->XE.Race.h2_ct_accIsW = conf_isW;
> + xe->XE.Race.h2_ct_locksHeldW
> + = enumerate_WordSet_into_LockP_vector(
> + HG_(get_univ_lsets)(),
> + conf_locksHeldW,
> + True/*allowed_to_be_invalid*/
> + );
> }
> }
>
> @@ -424,8 +559,10 @@
> tl_assert( HG_(is_sane_LockN)(lk) );
> init_XError(&xe);
> xe.tag = XE_UnlockUnlocked;
> - xe.XE.UnlockUnlocked.thr = thr;
> - xe.XE.UnlockUnlocked.lock = mk_LockP_from_LockN(lk);
> + xe.XE.UnlockUnlocked.thr
> + = thr;
> + xe.XE.UnlockUnlocked.lock
> + = mk_LockP_from_LockN(lk, False/*!allowed_to_be_invalid*/);
> // FIXME: tid vs thr
> tl_assert( HG_(is_sane_ThreadId)(thr->coretid) );
> tl_assert( thr->coretid != VG_INVALID_THREADID );
> @@ -444,7 +581,8 @@
> xe.tag = XE_UnlockForeign;
> xe.XE.UnlockForeign.thr = thr;
> xe.XE.UnlockForeign.owner = owner;
> - xe.XE.UnlockForeign.lock = mk_LockP_from_LockN(lk);
> + xe.XE.UnlockForeign.lock
> + = mk_LockP_from_LockN(lk, False/*!allowed_to_be_invalid*/);
> // FIXME: tid vs thr
> tl_assert( HG_(is_sane_ThreadId)(thr->coretid) );
> tl_assert( thr->coretid != VG_INVALID_THREADID );
> @@ -639,6 +777,10 @@
>
> } else {
>
> + VG_(umsg)("---Thread-Announcement----------"
> + "--------------------------------" "\n");
> + VG_(umsg)("\n");
> +
> if (thr->errmsg_index == 1) {
> tl_assert(thr->created_at == NULL);
> VG_(message)(Vg_UserMsg,
> @@ -659,6 +801,76 @@
> }
>
>
> +/* Announce 'lk'. */
> +static void announce_LockP ( Lock* lk )
> +{
> + tl_assert(lk);
> + if (lk == Lock_INVALID)
> + return; /* Can't be announced -- we know nothing about it. */
> + tl_assert(lk->magic == LockP_MAGIC);
> + if (!lk->appeared_at)
> + return; /* There's nothing we can show */
> +
> + if (VG_(clo_xml)) {
> + /* fixme: add announcement */
> + } else {
> + VG_(umsg)( "Lock at %p was first observed\n",
> + (void*)lk->guestaddr );
> + VG_(pp_ExeContext)( lk->appeared_at );
> + VG_(umsg)("\n");
> + }
> +}
> +
> +/* Announce (that is, print point-of-first-observation) for the
> + locks in 'lockvec' and, if non-NULL, 'lockvec2'. */
> +static void announce_combined_LockP_vecs ( Lock** lockvec,
> + Lock** lockvec2 )
> +{
> + UWord i;
> + tl_assert(lockvec);
> + for (i = 0; lockvec[i]; i++) {
> + announce_LockP(lockvec[i]);
> + }
> + if (lockvec2) {
> + for (i = 0; lockvec2[i]; i++) {
> + Lock* lk = lockvec2[i];
> + if (!elem_LockP_vector(lockvec, lk))
> + announce_LockP(lk);
> + }
> + }
> +}
> +
> +
> +static void show_LockP_summary_textmode ( Lock** locks, HChar* pre )
> +{
> + tl_assert(locks);
> + UWord i;
> + UWord nLocks = 0, nLocksValid = 0;
> + count_LockP_vector(&nLocks, &nLocksValid, locks);
> + tl_assert(nLocksValid <= nLocks);
> +
> + if (nLocks == 0) {
> + VG_(umsg)( "%sLocks held: none", pre );
> + } else {
> + VG_(umsg)( "%sLocks held: %lu, at address%s ",
> + pre, nLocks, nLocksValid == 1 ? "" : "es" );
> + }
> +
> + if (nLocks > 0) {
> + for (i = 0; i < nLocks; i++) {
> + if (locks[i] == Lock_INVALID)
> + continue;
> + VG_(umsg)( "%p", (void*)locks[i]->guestaddr);
> + if (locks[i+1] != NULL)
> + VG_(umsg)(" ");
> + }
> + if (nLocksValid < nLocks)
> + VG_(umsg)(" (and %lu that can't be shown)", nLocks - nLocksValid);
> + }
> + VG_(umsg)("\n");
> +}
> +
> +
> /* This is the "this error is due to be printed shortly; so have a
> look at it any print any preamble you want" function. We use it to
> announce any previously un-announced threads in the upcoming error
> @@ -707,6 +919,12 @@
> {
> const Bool xml = VG_(clo_xml); /* a shorthand, that's all */
>
> + if (!xml) {
> + VG_(umsg)("--------------------------------"
> + "--------------------------------" "\n");
> + VG_(umsg)("\n");
> + }
> +
> XError *xe = (XError*)VG_(get_error_extra)(err);
> tl_assert(xe);
>
> @@ -1005,18 +1223,27 @@
> } else {
>
> /* ------ Text ------ */
> + announce_combined_LockP_vecs( xe->XE.Race.locksHeldW,
> + xe->XE.Race.h2_ct_locksHeldW );
> +
> emit( "Possible data race during %s of size %d "
> "at %#lx by thread #%d\n",
> what, szB, err_ga, (Int)xe->XE.Race.thr->errmsg_index );
> +
> + tl_assert(xe->XE.Race.locksHeldW);
> + show_LockP_summary_textmode( xe->XE.Race.locksHeldW, "" );
> VG_(pp_ExeContext)( VG_(get_error_where)(err) );
>
> if (xe->XE.Race.h2_ct) {
> tl_assert(xe->XE.Race.h2_ct_accEC); // assured by update_extra
> - emit( " This conflicts with a previous %s of size %d "
> + tl_assert(xe->XE.Race.h2_ct_locksHeldW);
> + emit( "\n" );
> + emit( "This conflicts with a previous %s of size %d "
> "by thread #%d\n",
> xe->XE.Race.h2_ct_accIsW ? "write" : "read",
> xe->XE.Race.h2_ct_accSzB,
> xe->XE.Race.h2_ct->errmsg_index );
> + show_LockP_summary_textmode( xe->XE.Race.h2_ct_locksHeldW, "" );
> VG_(pp_ExeContext)( xe->XE.Race.h2_ct_accEC );
> }
>
> @@ -1049,7 +1276,8 @@
> xe->XE.Race.hszB);
> VG_(pp_ExeContext)( xe->XE.Race.hctxt );
> } else {
> - emit(" Address %#lx is %ld bytes inside a block "
> + emit("\n");
> + emit("Address %#lx is %ld bytes inside a block "
> "of size %ld alloc'd\n", err_ga, delta,
> xe->XE.Race.hszB);
> VG_(pp_ExeContext)( xe->XE.Race.hctxt );
> @@ -1060,12 +1288,23 @@
> Note that in XML mode, it will already by nicely wrapped up
> in tags, either <auxwhat> or <xauxwhat>, so we can just emit
> it verbatim. */
> - if (xe->XE.Race.descr1)
> - emit( "%s%s\n", xml ? " " : " ",
> - (HChar*)VG_(indexXA)( xe->XE.Race.descr1, 0 ) );
> - if (xe->XE.Race.descr2)
> - emit( "%s%s\n", xml ? " " : " ",
> - (HChar*)VG_(indexXA)( xe->XE.Race.descr2, 0 ) );
> + if (xml) {
> + if (xe->XE.Race.descr1)
> + emit( " %s\n",
> + (HChar*)VG_(indexXA)( xe->XE.Race.descr1, 0 ) );
> + if (xe->XE.Race.descr2)
> + emit( " %s\n",
> + (HChar*)VG_(indexXA)( xe->XE.Race.descr2, 0 ) );
> + } else {
> + if (xe->XE.Race.descr1 || xe->XE.Race.descr2)
> + emit("\n");
> + if (xe->XE.Race.descr1)
> + emit( "%s\n",
> + (HChar*)VG_(indexXA)( xe->XE.Race.descr1, 0 ) );
> + if (xe->XE.Race.descr2)
> + emit( "%s\n",
> + (HChar*)VG_(indexXA)( xe->XE.Race.descr2, 0 ) );
> + }
>
> break; /* case XE_Race */
> } /* case XE_Race */
>
> Modified: branches/HGDEV2/helgrind/hg_lock_n_thread.h
> ===================================================================
> --- branches/HGDEV2/helgrind/hg_lock_n_thread.h 2011-03-20 12:50:48 UTC (rev 11658)
> +++ branches/HGDEV2/helgrind/hg_lock_n_thread.h 2011-03-20 20:50:54 UTC (rev 11659)
> @@ -45,9 +45,9 @@
> #define LockP_MAGIC 0x755b5456 /* persistent (copied) locks */
>
>
> -/* These are handles for Word sets. CONSTRAINTS: must be (very) small
> - ints numbered from zero, since < 30-bit versions of them are used to
> - encode thread-sets and lock-sets in 32-bit shadow words. */
> +/* These are handles for Word sets. CONSTRAINTS: must be small ints
> + numbered from zero, since 32-bit versions of them are used to
> + encode lock-sets in libhb's history records (Thr_n_RCEC). */
> typedef WordSet WordSetID;
>
>
> @@ -156,6 +156,8 @@
> }
> Lock;
>
> +#define Lock_INVALID ((Lock*)1UL)
> +
> /*----------------------------------------------------------------*/
> /*--- Sanity checking ---*/
> /*----------------------------------------------------------------*/
>
> Modified: branches/HGDEV2/helgrind/hg_main.c
> ===================================================================
> --- branches/HGDEV2/helgrind/hg_main.c 2011-03-20 12:50:48 UTC (rev 11658)
> +++ branches/HGDEV2/helgrind/hg_main.c 2011-03-20 20:50:54 UTC (rev 11659)
> @@ -137,7 +137,15 @@
> static WordSetU* univ_lsets = NULL; /* sets of Lock* */
> static WordSetU* univ_laog = NULL; /* sets of Lock*, for LAOG */
>
> +/* Allow libhb to get at the universe of locksets stored
> + here. Sigh. */
> +WordSetU* HG_(get_univ_lsets) ( void ) { return univ_lsets; }
>
> +/* Allow libhb to get at the list of locks stored here. Ditto
> + sigh. */
> +Lock* HG_(get_admin_locks) ( void ) { return admin_locks; }
> +
> +
> /*----------------------------------------------------------------*/
> /*--- Simple helpers for the data structures ---*/
> /*----------------------------------------------------------------*/
> @@ -538,6 +546,7 @@
> static void initialise_data_structures ( Thr* hbthr_root )
> {
> Thread* thr;
> + WordSetID wsid;
>
> /* Get everything initialised and zeroed. */
> tl_assert(admin_threads == NULL);
> @@ -559,6 +568,11 @@
> univ_lsets = HG_(newWordSetU)( HG_(zalloc), "hg.ids.4", HG_(free),
> 8/*cacheSize*/ );
> tl_assert(univ_lsets != NULL);
> + /* Ensure that univ_lsets is non-empty, with lockset zero being the
> + empty lockset. hg_errors.c relies on the assumption that
> + lockset number zero in univ_lsets is always valid. */
> + wsid = HG_(emptyWS)(univ_lsets);
> + tl_assert(wsid == 0);
>
> tl_assert(univ_laog == NULL);
> if (HG_(clo_track_lockorders)) {
>
> Modified: branches/HGDEV2/helgrind/libhb.h
> ===================================================================
> --- branches/HGDEV2/helgrind/libhb.h 2011-03-20 12:50:48 UTC (rev 11658)
> +++ branches/HGDEV2/helgrind/libhb.h 2011-03-20 20:50:54 UTC (rev 11659)
> @@ -146,11 +146,22 @@
>
> /* Extract info from the conflicting-access machinery. */
> Bool libhb_event_map_lookup ( /*OUT*/ExeContext** resEC,
> - /*OUT*/Thr** resThr,
> - /*OUT*/SizeT* resSzB,
> - /*OUT*/Bool* resIsW,
> + /*OUT*/Thr** resThr,
> + /*OUT*/SizeT* resSzB,
> + /*OUT*/Bool* resIsW,
> + /*OUT*/WordSetID* locksHeldW,
> Thr* thr, Addr a, SizeT szB, Bool isW );
>
> +/* ------ Exported from hg_main.c ------ */
> +/* Yes, this is a horrible tangle. Sigh. */
> +
> +/* Get the univ_lset (universe for locksets) from hg_main.c. Sigh. */
> +WordSetU* HG_(get_univ_lsets) ( void );
> +
> +/* Get the the header pointer for the double linked list of locks
> + (admin_locks). */
> +Lock* HG_(get_admin_locks) ( void );
> +
> #endif /* __LIBHB_H */
>
> /*--------------------------------------------------------------------*/
>
> Modified: branches/HGDEV2/helgrind/libhb_core.c
> ===================================================================
> --- branches/HGDEV2/helgrind/libhb_core.c 2011-03-20 12:50:48 UTC (rev 11658)
> +++ branches/HGDEV2/helgrind/libhb_core.c 2011-03-20 20:50:54 UTC (rev 11659)
> @@ -152,11 +152,11 @@
> at a clock rate of 5 GHz is 162.9 days. And that's doing nothing
> but VTS ticks, which isn't realistic.
>
> - NB1: SCALARTS_N_THRBITS must be 21 or lower. The obvious limit is
> - 32 since a ThrID is a UInt. 21 comes from the fact that
> + NB1: SCALARTS_N_THRBITS must be 29 or lower. The obvious limit is
> + 32 since a ThrID is a UInt. 29 comes from the fact that
> 'Thr_n_RCEC', which records information about old accesses, packs
> - not only a ThrID but also 2+1+8 other bits in a UInt, hence
> - limiting size to 32-(2+1+8) == 21.
> + not only a ThrID but also 2+1 other bits (access size and
> + writeness) in a UInt, hence limiting size to 32-(2+1) == 29.
>
> NB2: thrid values are issued upwards from 1024, and values less
> than that aren't valid. This isn't per se necessary (any order
> @@ -182,7 +182,8 @@
> ThrID == 0 to denote an empty Thr_n_RCEC record. So ThrID == 0
> must never be a valid ThrID. Given NB2 that's OK.
> */
> -#define SCALARTS_N_THRBITS 18 /* valid range: 11 to 21 inclusive */
> +#define SCALARTS_N_THRBITS 18 /* valid range: 11 to 29 inclusive */
> +
> #define SCALARTS_N_TYMBITS (64 - SCALARTS_N_THRBITS)
> typedef
> struct {
> @@ -4175,13 +4176,19 @@
> // (UInt) `echo "Old Reference Information" | md5sum`
> #define OldRef_MAGIC 0x30b1f075UL
>
> -/* Records an access: a thread and a context. The size
> - (1,2,4,8) and read-or-writeness are also encoded as
> - follows: bottom bit of .thr is 1 if write, 0 if read
> - bottom 2 bits of .rcec are encode size:
> - 00 = 1, 01 = 2, 10 = 4, 11 = 8
> +/* Records an access: a thread, a context (size & writeness) and the
> + number of held locks. The size (1,2,4,8) is encoded as 00 = 1, 01 =
> + 2, 10 = 4, 11 = 8.
> */
> -typedef struct { Thr* thr; RCEC* rcec; } Thr_n_RCEC;
> +typedef
> + struct {
> + RCEC* rcec;
> + WordSetID locksHeldW;
> + UInt thrid : SCALARTS_N_THRBITS;
> + UInt szLg2B : 2;
> + UInt isW : 1;
> + }
> + Thr_n_RCEC;
>
> #define N_OLDREF_ACCS 5
>
> @@ -4190,7 +4197,7 @@
> UWord magic; /* sanity check only */
> UWord gen; /* when most recently accessed */
> /* or free list when not in use */
> - /* unused slots in this array have .thr == NULL */
> + /* unused slots in this array have .thrid == 0, which is invalid */
> Thr_n_RCEC accs[N_OLDREF_ACCS];
> }
> OldRef;
> @@ -4215,13 +4222,6 @@
> static UWord oldrefTreeN = 0; /* # elems in oldrefTree */
> static UWord oldrefGenIncAt = 0; /* inc gen # when size hits this */
>
> -inline static void* ptr_or_UWord ( void* p, UWord w ) {
> - return (void*)( ((UWord)p) | ((UWord)w) );
> -}
> -inline static void* ptr_and_UWord ( void* p, UWord w ) {
> - return (void*)( ((UWord)p) & ((UWord)w) );
> -}
> -
> inline static UInt min_UInt ( UInt a, UInt b ) {
> return a < b ? a : b;
> }
> @@ -4253,50 +4253,51 @@
> UWord keyW, valW;
> Bool b;
>
> + tl_assert(thr);
> + ThrID thrid = thr->thrid;
> + tl_assert(thrid != 0); /* zero is used to denote an empty slot. */
> +
> + WordSetID locksHeldW = thr->hgthread->locksetW;
> +
> rcec = get_RCEC( thr );
> ctxt__rcinc(rcec);
>
> - /* encode the size and writeness of the transaction in the bottom
> - two bits of thr and rcec. */
> - thr = ptr_or_UWord(thr, isW ? 1 : 0);
> + UInt szLg2B = 0;
> switch (szB) {
> /* This doesn't look particularly branch-predictor friendly. */
> - case 1: rcec = ptr_or_UWord(rcec, 0); break;
> - case 2: rcec = ptr_or_UWord(rcec, 1); break;
> - case 4: rcec = ptr_or_UWord(rcec, 2); break;
> - case 8: rcec = ptr_or_UWord(rcec, 3); break;
> + case 1: szLg2B = 0; break;
> + case 2: szLg2B = 1; break;
> + case 4: szLg2B = 2; break;
> + case 8: szLg2B = 3; break;
> default: tl_assert(0);
> }
>
> - /* Look in the map to see if we already have this. */
> + /* Look in the map to see if we already have a record for this
> + address. */
> b = VG_(lookupSWA)( oldrefTree, &keyW, &valW, a );
>
> if (b) {
>
> /* We already have a record for this address. We now need to
> - see if we have a stack trace pertaining to this (thread, R/W,
> + see if we have a stack trace pertaining to this (thrid, R/W,
> size) triple. */
> tl_assert(keyW == a);
> ref = (OldRef*)valW;
> tl_assert(ref->magic == OldRef_MAGIC);
>
> - tl_assert(thr);
> for (i = 0; i < N_OLDREF_ACCS; i++) {
> - if (ref->accs[i].thr != thr)
> + if (ref->accs[i].thrid != thrid)
> continue;
> - /* since .thr encodes both the accessing thread and the
> - read/writeness, we know now that at least those features
> - of the access match this entry. So we just need to check
> - the size indication. Do this by inspecting the lowest 2 bits of
> - .rcec, which contain the encoded size info. */
> - if (ptr_and_UWord(ref->accs[i].rcec,3) != ptr_and_UWord(rcec,3))
> + if (ref->accs[i].szLg2B != szLg2B)
> continue;
> + if (ref->accs[i].isW != (UInt)(isW & 1))
> + continue;
> /* else we have a match, so stop looking. */
> break;
> }
>
> if (i < N_OLDREF_ACCS) {
> - /* thread 'thr' has an entry at index 'i'. Update it. */
> + /* thread 'thr' has an entry at index 'i'. Update its RCEC. */
> if (i > 0) {
> Thr_n_RCEC tmp = ref->accs[i-1];
> ref->accs[i-1] = ref->accs[i];
> @@ -4305,31 +4306,36 @@
> }
> if (rcec == ref->accs[i].rcec) stats__ctxt_rcdec1_eq++;
> stats__ctxt_rcdec1++;
> - ctxt__rcdec( ptr_and_UWord(ref->accs[i].rcec, ~3) );
> - ref->accs[i].rcec = rcec;
> - tl_assert(ref->accs[i].thr == thr);
> + ctxt__rcdec( ref->accs[i].rcec );
> + tl_assert(ref->accs[i].thrid == thrid);
> + /* Update the RCEC and the W-held lockset. */
> + ref->accs[i].rcec = rcec;
> + ref->accs[i].locksHeldW = locksHeldW;
> } else {
> - /* No entry for this (thread, R/W, size) triple. Shuffle all
> - of them down one slot, and put the new entry at the start
> - of the array. */
> - if (ref->accs[N_OLDREF_ACCS-1].thr) {
> + /* No entry for this (thread, R/W, size, nWHeld) quad.
> + Shuffle all of them down one slot, and put the new entry
> + at the start of the array. */
> + if (ref->accs[N_OLDREF_ACCS-1].thrid != 0) {
> /* the last slot is in use. We must dec the rc on the
> associated rcec. */
> tl_assert(ref->accs[N_OLDREF_ACCS-1].rcec);
> stats__ctxt_rcdec2++;
> if (0 && 0 == (stats__ctxt_rcdec2 & 0xFFF))
> VG_(printf)("QQQQ %lu overflows\n",stats__ctxt_rcdec2);
> - ctxt__rcdec( ptr_and_UWord(ref->accs[N_OLDREF_ACCS-1].rcec, ~3) );
> + ctxt__rcdec( ref->accs[N_OLDREF_ACCS-1].rcec );
> } else {
> tl_assert(!ref->accs[N_OLDREF_ACCS-1].rcec);
> }
> for (j = N_OLDREF_ACCS-1; j >= 1; j--)
> ref->accs[j] = ref->accs[j-1];
> - ref->accs[0].thr = thr;
> - ref->accs[0].rcec = rcec;
> - /* thr==NULL is used to signify an empty slot, so we can't
> - add a NULL thr. */
> - tl_assert(ptr_and_UWord(thr, ~3) != 0);
> + ref->accs[0].thrid = thrid;
> + ref->accs[0].szLg2B = szLg2B;
> + ref->accs[0].isW = (UInt)(isW & 1);
> + ref->accs[0].locksHeldW = locksHeldW;
> + ref->accs[0].rcec = rcec;
> + /* thrid==0 is used to signify an empty slot, so we can't
> + add zero thrid (such a ThrID is invalid anyway). */
> + /* tl_assert(thrid != 0); */ /* There's a dominating assert above. */
> }
>
> ref->gen = oldrefGen;
> @@ -4346,15 +4352,24 @@
>
> ref = alloc_OldRef();
> ref->magic = OldRef_MAGIC;
> - ref->gen = oldrefGen;
> - ref->accs[0].rcec = rcec;
> - ref->accs[0].thr = thr;
> - /* thr==NULL is used to signify an empty slot, so we can't add a
> - NULL thr. */
> - tl_assert(ptr_and_UWord(thr, ~3) != 0);
> + ref->gen = oldrefGen;
> + ref->accs[0].thrid = thrid;
> + ref->accs[0].szLg2B = szLg2B;
> + ref->accs[0].isW = (UInt)(isW & 1);
> + ref->accs[0].locksHeldW = locksHeldW;
> + ref->accs[0].rcec = rcec;
> +
> + /* thrid==0 is used to signify an empty slot, so we can't
> + add zero thrid (such a ThrID is invalid anyway). */
> + /* tl_assert(thrid != 0); */ /* There's a dominating assert above. */
> +
> + /* Clear out the rest of the entries */
> for (j = 1; j < N_OLDREF_ACCS; j++) {
> - ref->accs[j].thr = NULL;
> - ref->accs[j].rcec = NULL;
> + ref->accs[j].rcec = NULL;
> + ref->accs[j].thrid = 0;
> + ref->accs[j].szLg2B = 0;
> + ref->accs[j].isW = 0;
> + ref->accs[j].locksHeldW = 0;
> }
> VG_(addToSWA)( oldrefTree, a, (UWord)ref );
> oldrefTreeN++;
> @@ -4363,10 +4378,12 @@
> }
>
>
> +/* Extract info from the conflicting-access machinery. */
> Bool libhb_event_map_lookup ( /*OUT*/ExeContext** resEC,
> - /*OUT*/Thr** resThr,
> - /*OUT*/SizeT* resSzB,
> - /*OUT*/Bool* resIsW,
> + /*OUT*/Thr** resThr,
> + /*OUT*/SizeT* resSzB,
> + /*OUT*/Bool* resIsW,
> + /*OUT*/WordSetID* locksHeldW,
> Thr* thr, Addr a, SizeT szB, Bool isW )
> {
> Word i, j;
> @@ -4374,11 +4391,12 @@
> UWord keyW, valW;
> Bool b;
>
> - Thr* cand_thr;
> - RCEC* cand_rcec;
> - Bool cand_isW;
> - SizeT cand_szB;
> - Addr cand_a;
> + ThrID cand_thrid;
> + RCEC* cand_rcec;
> + Bool cand_isW;
> + SizeT cand_szB;
> + WordSetID cand_locksHeldW;
> + Addr cand_a;
>
> Addr toCheck[15];
> Int nToCheck = 0;
> @@ -4386,6 +4404,8 @@
> tl_assert(thr);
> tl_assert(szB == 8 || szB == 4 || szB == 2 || szB == 1);
>
> + ThrID thrid = thr->thrid;
> +
> toCheck[nToCheck++] = a;
> for (i = -7; i < (Word)szB; i++) {
> if (i != 0)
> @@ -4407,33 +4427,27 @@
> ref = (OldRef*)valW;
> tl_assert(keyW == cand_a);
> tl_assert(ref->magic == OldRef_MAGIC);
> - tl_assert(ref->accs[0].thr); /* first slot must always be used */
> + tl_assert(ref->accs[0].thrid != 0); /* first slot must always be used */
>
> - cand_thr = NULL;
> - cand_rcec = NULL;
> - cand_isW = False;
> - cand_szB = 0;
> + cand_thrid = 0; /* invalid; see comments in event_map_bind */
> + cand_rcec = NULL;
> + cand_isW = False;
> + cand_szB = 0;
> + cand_locksHeldW = 0; /* always valid; see initialise_data_structures() */
>
> for (i = 0; i < N_OLDREF_ACCS; i++) {
> Thr_n_RCEC* cand = &ref->accs[i];
> - cand_thr = ptr_and_UWord(cand->thr, ~3);
> - cand_rcec = ptr_and_UWord(cand->rcec, ~3);
> - /* Decode the writeness from the bottom bit of .thr. */
> - cand_isW = 1 == (UWord)ptr_and_UWord(cand->thr, 1);
> - /* Decode the size from the bottom two bits of .rcec. */
> - switch ((UWord)ptr_and_UWord(cand->rcec, 3)) {
> - case 0: cand_szB = 1; break;
> - case 1: cand_szB = 2; break;
> - case 2: cand_szB = 4; break;
> - case 3: cand_szB = 8; break;
> - default: tl_assert(0);
> - }
> + cand_rcec = cand->rcec;
> + cand_thrid = cand->thrid;
> + cand_isW = (Bool)cand->isW;
> + cand_szB = 1 << cand->szLg2B;
> + cand_locksHeldW = cand->locksHeldW;
>
> - if (cand_thr == NULL)
> + if (cand_thrid == 0)
> /* This slot isn't in use. Ignore it. */
> continue;
>
> - if (cand_thr == thr)
> + if (cand_thrid == thrid)
> /* This is an access by the same thread, but we're only
> interested in accesses from other threads. Ignore. */
> continue;
> @@ -4456,7 +4470,7 @@
> if (i < N_OLDREF_ACCS) {
> Int n, maxNFrames;
> /* return with success */
> - tl_assert(cand_thr);
> + tl_assert(cand_thrid);
> tl_assert(cand_rcec);
> tl_assert(cand_rcec->magic == RCEC_MAGIC);
> tl_assert(cand_szB >= 1);
> @@ -4465,10 +4479,12 @@
> for (n = 0; n < maxNFrames; n++) {
> if (0 == cand_rcec->frames[n]) break;
> }
> - *resEC = VG_(make_ExeContext_from_StackTrace)(cand_rcec->frames, n);
> - *resThr = cand_thr;
> - *resSzB = cand_szB;
> - *resIsW = cand_isW;
> + *resEC = VG_(make_ExeContext_from_StackTrace)
> + (cand_rcec->frames, n);
> + *resThr = Thr__from_ThrID(cand_thrid);
> + *resSzB = cand_szB;
> + *resIsW = cand_isW;
> + *locksHeldW = cand_locksHeldW;
> return True;
> }
>
> @@ -4555,9 +4571,9 @@
> oldref = (OldRef*)valW;
> tl_assert(oldref->magic == OldRef_MAGIC);
> for (i = 0; i < N_OLDREF_ACCS; i++) {
> - Thr* aThr = ptr_and_UWord(oldref->accs[i].thr, ~3);
> - RCEC* aRef = ptr_and_UWord(oldref->accs[i].rcec, ~3);
> - if (aThr) {
> + ThrID aThrID = oldref->accs[i].thrid;
> + RCEC* aRef = oldref->accs[i].rcec;
> + if (aThrID != 0) {
> tl_assert(aRef);
> tl_assert(aRef->magic == RCEC_MAGIC);
> aRef->rcX++;
> @@ -4798,14 +4814,14 @@
> tl_assert(keyW == ga2del);
> oldref = (OldRef*)valW;
> for (j = 0; j < N_OLDREF_ACCS; j++) {
> - Thr* aThr = ptr_and_UWord(oldref->accs[j].thr, ~3);
> - RCEC* aRef = ptr_and_UWord(oldref->accs[j].rcec, ~3);
> + ThrID aThrID = oldref->accs[j].thrid;
> + RCEC* aRef = oldref->accs[j].rcec;
> if (aRef) {
> - tl_assert(aThr);
> + tl_assert(aThrID != 0);
> stats__ctxt_rcdec3++;
> ctxt__rcdec( aRef );
> } else {
> - tl_assert(!aThr);
> + tl_assert(aThrID == 0);
> }
> }
>
> @@ -6158,9 +6174,27 @@
> // We will have to have to store a large number of these,
> // so make sure they're the size we expect them to be.
> tl_assert(sizeof(ScalarTS) == 8);
> - tl_assert(SCALARTS_N_THRBITS >= 11); /* because first 1024 unusable */
> - tl_assert(SCALARTS_N_THRBITS <= 32); /* so as to fit in a UInt */
>
> + /* because first 1024 unusable */
> + tl_assert(SCALARTS_N_THRBITS >= 11);
> + /* so as to fit in a UInt w/ 3 bits to spare (see defn of
> + Thr_n_RCEC). */
> + tl_assert(SCALARTS_N_THRBITS <= 29);
> +
> + /* Need to be sure that Thr_n_RCEC is 2 words (64-bit) or 3 words
> + (32-bit). It's not correctness-critical, but there are a lot of
> + them, so it's important from a space viewpoint. Unfortunately
> + we simply can't pack it into 2 words on a 32-bit target. */
> + if (sizeof(UWord) == 8) {
> + tl_assert(sizeof(Thr_n_RCEC) == 16);
> + } else {
> + tl_assert(sizeof(Thr_n_RCEC) == 12);
> + }
> +
> + /* Word sets really are 32 bits. Even on a 64 bit target. */
> + tl_assert(sizeof(WordSetID) == 4);
> + tl_assert(sizeof(WordSet) == sizeof(WordSetID));
> +
> tl_assert(get_stacktrace);
> tl_assert(get_EC);
> main_get_stacktrace = get_stacktrace;
>
>
> ------------------------------------------------------------------------------
> Colocation vs. Managed Hosting
> A question and answer guide to determining the best fit
> for your organization - today and in the future.
> http://p.sf.net/sfu/internap-sfd2d
> _______________________________________________
> Valgrind-developers mailing list
> Val...@li...
> https://lists.sourceforge.net/lists/listinfo/valgrind-developers
>
|
|
From: Julian S. <js...@ac...> - 2011-03-22 16:39:30
|
On 64 bit platforms it's free in both space and time. On 32 bit platforms it's free in time but adds some space overhead (20% ish) to the history mechanism that records older (conflicting) accesses. The amount of space used by the conflicting access info is limited by default to around 100MB by throwing away older info, so it's not a big deal. J On Monday, March 21, 2011, Konstantin Serebryany wrote: > Cool! > Is this for free, or it adds a non-zero overhead? > > --kcc > > On Sun, Mar 20, 2011 at 11:50 PM, <sv...@va...> wrote: > > Author: sewardj > > Date: 2011-03-20 20:50:54 +0000 (Sun, 20 Mar 2011) > > New Revision: 11659 > > > > Log: > > Add the ability to show the set of locks shown by each thread in a > > race, which makes diagnosing races a lot easier. As a side effect, > > change the way errors are printed to more clearly delimit the > > boundaries of errors. |
|
From: Konstantin S. <kon...@gm...> - 2011-03-22 19:18:48
|
nice! On Tue, Mar 22, 2011 at 7:34 PM, Julian Seward <js...@ac...> wrote: > > On 64 bit platforms it's free in both space and time. > > On 32 bit platforms it's free in time but adds some > space overhead (20% ish) to the history mechanism that > records older (conflicting) accesses. The amount of > space used by the conflicting access info is limited > by default to around 100MB by throwing away older info, > so it's not a big deal. > > J > > > On Monday, March 21, 2011, Konstantin Serebryany wrote: >> Cool! >> Is this for free, or it adds a non-zero overhead? >> >> --kcc >> >> On Sun, Mar 20, 2011 at 11:50 PM, <sv...@va...> wrote: >> > Author: sewardj >> > Date: 2011-03-20 20:50:54 +0000 (Sun, 20 Mar 2011) >> > New Revision: 11659 >> > >> > Log: >> > Add the ability to show the set of locks shown by each thread in a >> > race, which makes diagnosing races a lot easier. As a side effect, >> > change the way errors are printed to more clearly delimit the >> > boundaries of errors. > |