On Thursday 13 December 2007 09:07, Konstantin Serebryany wrote:
> But isn't it expensive?
>
> On Dec 13, 2007 11:01 AM, Julian Seward <[EMAIL PROTECTED]> wrote:
> > On Thursday 13 December 2007 08:44, Konstantin Serebryany wrote:
> > > Hi,
> > >
> > > The same false positive appears when mixing mutexes and semaphores.
> > > Test attached.
> > > As with cond vars, we are able to transition Excl(T1)->Excl(T2), but
> > > can not do ShM(T1,T2)->Excl(T2).
> > >
> > > As I understand, fixing this properly will require storing SegmentID in
> >
> > all
> >
> > > shadow words, not only in those that are in Exclusive state.
> > > This will hardly squeeze into 32 bits though, need 64... :(
> >
> > No, I don't think so. We just need to do the same memory ownership
> > transition for semaphores that the "cvhack" patch does for CVs. I'll
> > extend the current patch.
Here is version 2 of the cvhack patch. This makes your semaphore test
run clean (with --happens-before=cvhack).
J
Index: helgrind/hg_main.c
===================================================================
--- helgrind/hg_main.c (revision 7299)
+++ helgrind/hg_main.c (working copy)
@@ -90,7 +90,7 @@
// shadow_mem_make_NoAccess: 29156 SMs, 1728 scanned
// happens_before_wrk: 1000
// ev__post_thread_join: 3360 SMs, 29 scanned, 252 re-Excls
-#define SHOW_EXPENSIVE_STUFF 0
+#define SHOW_EXPENSIVE_STUFF 1
// 0 for silent, 1 for some stuff, 2 for lots of stuff
#define SHOW_EVENTS 0
@@ -132,6 +132,7 @@
// 1 = segments at thread create/join
// 2 = as 1 + segments at condition variable signal/broadcast/wait
// + segments at sem_wait/sem_post
+// 3 = as 2 + mess with mem ownership at cv wait completion
static Int clo_happens_before = 2; /* default setting */
/* Generate .vcg output of the happens-before graph?
@@ -2974,6 +2975,7 @@
case 'r': how = "rd"; break;
case 'w': how = "wr"; break;
case 'p': how = "pa"; break;
+ case 'm': how = "MA"; break;
default: tl_assert(0);
}
show_shadow_w32_for_user(txt_old, sizeof(txt_old), sv_old);
@@ -5589,70 +5591,28 @@
all__sanity_check("evh__pre_thread_ll_exit-post");
}
+/* Examine all ShM/ShR states in the system. For each one, look at
+ its thread set. If the thread set contains "change_this", remove
+ it and insert "to_this" instead. If the resulting thread set is a
+ singleton, change the shadow word to be an Excl state belonging to
+ the current segment for "to_this". */
static
-void evh__HG_PTHREAD_JOIN_POST ( ThreadId stay_tid, Thread* quit_thr )
+void evh__substitute_in_threadsets ( HChar* who,
+ Thread* change_this,
+ Thread* to_this )
{
Int stats_SMs, stats_SMs_scanned, stats_reExcls;
Addr ga;
SecMap* sm;
- Thread* thr_s;
- Thread* thr_q;
- if (SHOW_EVENTS >= 1)
- VG_(printf)("evh__post_thread_join(stayer=%d, quitter=%p)\n",
- (Int)stay_tid, quit_thr );
+ Thread* thr_q = change_this;
+ Thread* thr_s = to_this;
- tl_assert(is_sane_ThreadId(stay_tid));
+ tl_assert( is_sane_Thread(thr_q) );
+ tl_assert( is_sane_Thread(thr_s) );
+ tl_assert(thr_q != thr_s);
- thr_s = map_threads_maybe_lookup( stay_tid );
- thr_q = quit_thr;
- tl_assert(thr_s != NULL);
- tl_assert(thr_q != NULL);
- tl_assert(thr_s != thr_q);
-
- if (clo_happens_before >= 1) {
- /* Start a new segment for the stayer */
- SegmentID new_segid = 0; /* bogus */
- Segment* new_seg = NULL;
- evhH__start_new_segment_for_thread( &new_segid, &new_seg, thr_s );
- tl_assert(is_sane_SegmentID(new_segid));
- tl_assert(is_sane_Segment(new_seg));
- /* and make it depend on the quitter's last segment */
- tl_assert(new_seg->other == NULL);
- new_seg->other = map_segments_lookup( thr_q->csegid );
- new_seg->other_hint = 'j';
- tl_assert(new_seg->thr == thr_s);
- new_seg->vts = tickL_and_joinR_VTS( thr_s, new_seg->prev->vts,
- new_seg->other->vts );
- }
-
- // FIXME: error-if: exiting thread holds any locks
- // or should evh__pre_thread_ll_exit do that?
-
- /* Delete thread from ShM/ShR thread sets and restore Excl states
- where appropriate */
-
- /* When Thread(t) joins to Thread(u):
-
- scan all shadow memory. For each ShM/ShR thread set, replace
- 't' in each set with 'u'. If this results in a singleton 'u',
- change the state to Excl(u->csegid).
-
- Optimisation: tag each SecMap with a superset of the union of
- the thread sets in the SecMap. Then if the tag set does not
- include 't' then the SecMap can be skipped, because there is no
- 't' to change to anything else.
-
- Problem is that the tag set needs to be updated often, after
- every ShR/ShM store. (that increases the thread set of the
- shadow value.)
-
- --> Compromise. Tag each SecMap with a .mbHasShared bit which
- must be set true if any ShR/ShM on the page. Set this for
- any transitions into ShR/ShM on the page. Then skip page if
- not set.
-
- .mbHasShared bits are (effectively) cached in cache_shmem.
+ /* .mbHasShared bits are (effectively) cached in cache_shmem.
Hence that must be flushed before we can safely consult them.
Since we're modifying the backing store, we also need to
@@ -5717,16 +5677,92 @@
wnew = isM ? mk_SHVAL_ShM(tset_new, lset_old)
: mk_SHVAL_ShR(tset_new, lset_old);
}
+ if (clo_trace_level > 0
+ && *w32p != wnew
+ && ga <= clo_trace_addr
+ && clo_trace_addr < ga+N_SECMAP_ARANGE) {
+ msm__show_state_change( thr_s, ga, 0, 'm', *w32p, wnew );
+ }
*w32p = wnew;
}
}
HG_(doneIterFM)( map_shmem );
if (SHOW_EXPENSIVE_STUFF)
- VG_(printf)("evh__post_thread_join: %d SMs, "
- "%d scanned, %d re-Excls\n",
- stats_SMs, stats_SMs_scanned, stats_reExcls);
+ VG_(printf)("%s: %d SMs, %d scanned, %d re-Excls\n",
+ who, stats_SMs, stats_SMs_scanned, stats_reExcls);
+}
+static
+void evh__HG_PTHREAD_JOIN_POST ( ThreadId stay_tid, Thread* quit_thr )
+{
+ Thread* thr_s;
+ Thread* thr_q;
+
+ if (SHOW_EVENTS >= 1)
+ VG_(printf)("evh__post_thread_join(stayer=%d, quitter=%p)\n",
+ (Int)stay_tid, quit_thr );
+
+ tl_assert(is_sane_ThreadId(stay_tid));
+
+ thr_s = map_threads_maybe_lookup( stay_tid );
+ thr_q = quit_thr;
+ tl_assert(thr_s != NULL);
+ tl_assert(thr_q != NULL);
+ tl_assert(thr_s != thr_q);
+
+ if (clo_happens_before >= 1) {
+ /* Start a new segment for the stayer */
+ SegmentID new_segid = 0; /* bogus */
+ Segment* new_seg = NULL;
+ evhH__start_new_segment_for_thread( &new_segid, &new_seg, thr_s );
+ tl_assert(is_sane_SegmentID(new_segid));
+ tl_assert(is_sane_Segment(new_seg));
+ /* and make it depend on the quitter's last segment */
+ tl_assert(new_seg->other == NULL);
+ new_seg->other = map_segments_lookup( thr_q->csegid );
+ new_seg->other_hint = 'j';
+ tl_assert(new_seg->thr == thr_s);
+ new_seg->vts = tickL_and_joinR_VTS( thr_s, new_seg->prev->vts,
+ new_seg->other->vts );
+ }
+
+ // FIXME: error-if: exiting thread holds any locks
+ // or should evh__pre_thread_ll_exit do that?
+
+ /* Delete thread from ShM/ShR thread sets and restore Excl states
+ where appropriate */
+
+ /* When Thread(t) joins to Thread(u):
+
+ scan all shadow memory. For each ShM/ShR thread set, replace
+ 't' in each set with 'u'. If this results in a singleton 'u',
+ change the state to Excl(u->csegid).
+
+ Optimisation: tag each SecMap with a superset of the union of
+ the thread sets in the SecMap. Then if the tag set does not
+ include 't' then the SecMap can be skipped, because there is no
+ 't' to change to anything else.
+
+ Problem is that the tag set needs to be updated often, after
+ every ShR/ShM store. (that increases the thread set of the
+ shadow value.)
+
+ --> Compromise. Tag each SecMap with a .mbHasShared bit which
+ must be set true if any ShR/ShM on the page. Set this for
+ any transitions into ShR/ShM on the page. Then skip page if
+ not set.
+
+ .mbHasShared bits are (effectively) cached in cache_shmem.
+ Hence that must be flushed before we can safely consult them.
+
+ Since we're modifying the backing store, we also need to
+ invalidate cache_shmem, so that subsequent memory references get
+ up to date shadow values.
+ */
+ evh__substitute_in_threadsets( "evh__HG_PTHREAD_JOIN_POST",
+ thr_q, thr_s );
+
/* This holds because, at least when using NPTL as the thread
library, we should be notified the low level thread exit before
we hear of any join event on it. The low level exit
@@ -6173,6 +6209,16 @@
new_seg->thr,
new_seg->prev->vts,
new_seg->other->vts );
+ /* Do nasty hack: signalling thread gives up ownership of
+ shared locations, giving them to the waiting thread
+ instead. If a location was previously accessed only by
+ the waiting thread and the signalling thread, then the
+ waiting thread acquires exclusive ownership. See
+ analogous hack in evh__HG_POSIX_SEM_WAIT_POST. */
+ if (clo_happens_before >= 3)
+ evh__substitute_in_threadsets(
+ "evh__HG_PTHREAD_COND_WAIT_POST",
+ signalling_seg->thr, new_seg->thr );
} else {
/* Hmm. How can a wait on 'cond' succeed if nobody signalled
it? If this happened it would surely be a bug in the
@@ -6548,6 +6594,16 @@
new_seg->thr,
new_seg->prev->vts,
new_seg->other->vts );
+ /* Do nasty hack: posting thread gives up ownership of shared
+ locations, giving them to the waiting thread instead. If
+ a location was previously accessed only by the waiting
+ thread and the posting thread, then the waiting thread
+ acquires exclusive ownership. See analogous hack in
+ evh__HG_PTHREAD_COND_WAIT_POST.*/
+ if (clo_happens_before >= 3)
+ evh__substitute_in_threadsets(
+ "evh__HG_POSIX_SEM_WAIT_POST",
+ posting_seg->thr, new_seg->thr );
} else {
/* Hmm. How can a wait on 'sem' succeed if nobody posted to
it? If this happened it would surely be a bug in the
@@ -8531,6 +8587,8 @@
clo_happens_before = 1;
else if (VG_CLO_STREQ(arg, "--happens-before=all"))
clo_happens_before = 2;
+ else if (VG_CLO_STREQ(arg, "--happens-before=cvhack"))
+ clo_happens_before = 3;
else if (VG_CLO_STREQ(arg, "--gen-vcg=no"))
clo_gen_vcg = 0;
-------------------------------------------------------------------------
SF.Net email is sponsored by:
Check out the new SourceForge.net Marketplace.
It's the best place to buy or sell services
for just about anything Open Source.
http://ad.doubleclick.net/clk;164216239;13503038;w?http://sf.net/marketplace
_______________________________________________
Valgrind-developers mailing list
Valgrind-developers@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/valgrind-developers
