|
From: Philippe W. <phi...@sk...> - 2011-03-03 22:43:57
|
In evhH__pre_thread_releases_lock, I do not understand why
the a Read lock is removed from the locksetW.
>From what I can see, it does not harm to remove it (as a read
lock can't be present in the locksetW) but it is probably a no-op
which can be avoided.
Is there a reason why the below:
thr->locksetA
= HG_(delFromWS)( univ_lsets, thr->locksetA, (Word)lock );
thr->locksetW
= HG_(delFromWS)( univ_lsets, thr->locksetW, (Word)lock );
/* push our VC into the lock */
tl_assert(thr->hbthr);
tl_assert(lock->hbso);
/* If the lock was previously W-held, then we want to do a
strong send, and if previously R-held, then a weak send. */
libhb_so_send( thr->hbthr, lock->hbso, was_heldW );
}
should not be replaced by:
thr->locksetA
= HG_(delFromWS)( univ_lsets, thr->locksetA, (Word)lock );
if (was_heldW) {
thr->locksetW
= HG_(delFromWS)( univ_lsets, thr->locksetW, (Word)lock );
}
/* push our VC into the lock */
tl_assert(thr->hbthr);
tl_assert(lock->hbso);
/* If the lock was previously W-held, then we want to do a
strong send, and if previously R-held, then a weak send. */
libhb_so_send( thr->hbthr, lock->hbso, was_heldW );
}
With this change, the helgrind regression tests are the same.
|
|
From: Julian S. <js...@ac...> - 2011-03-05 00:07:16
|
Are you sure your analysis is correct even in the case where the
program is buggy and is doing stupid stuff like wr-unlock of a
rd-lock, or vice versa? I think your analysis is correct in the
case of a correct program.
Probably I was being conservative. AFAIK this piece of code is
not performance critical, so I prefer to be conservative rather
than prove that the improved version is correct even when the
program being run is buggy.
J
On Thursday, March 03, 2011, Philippe Waroquiers wrote:
> In evhH__pre_thread_releases_lock, I do not understand why
> the a Read lock is removed from the locksetW.
>
> >From what I can see, it does not harm to remove it (as a read
>
> lock can't be present in the locksetW) but it is probably a no-op
> which can be avoided.
>
> Is there a reason why the below:
> thr->locksetA
> = HG_(delFromWS)( univ_lsets, thr->locksetA, (Word)lock );
> thr->locksetW
> = HG_(delFromWS)( univ_lsets, thr->locksetW, (Word)lock );
> /* push our VC into the lock */
> tl_assert(thr->hbthr);
> tl_assert(lock->hbso);
> /* If the lock was previously W-held, then we want to do a
> strong send, and if previously R-held, then a weak send. */
> libhb_so_send( thr->hbthr, lock->hbso, was_heldW );
> }
>
> should not be replaced by:
>
> thr->locksetA
> = HG_(delFromWS)( univ_lsets, thr->locksetA, (Word)lock );
> if (was_heldW) {
> thr->locksetW
> = HG_(delFromWS)( univ_lsets, thr->locksetW, (Word)lock );
> }
> /* push our VC into the lock */
> tl_assert(thr->hbthr);
> tl_assert(lock->hbso);
> /* If the lock was previously W-held, then we want to do a
> strong send, and if previously R-held, then a weak send. */
> libhb_so_send( thr->hbthr, lock->hbso, was_heldW );
> }
>
>
> With this change, the helgrind regression tests are the same.
>
>
> ---------------------------------------------------------------------------
> --- What You Don't Know About Data Connectivity CAN Hurt You
> This paper provides an overview of data connectivity, details
> its effect on application quality, and explores various alternative
> solutions. http://p.sf.net/sfu/progress-d2d
> _______________________________________________
> Valgrind-developers mailing list
> Val...@li...
> https://lists.sourceforge.net/lists/listinfo/valgrind-developers
|
|
From: Philippe W. <phi...@sk...> - 2011-03-06 23:50:26
|
> Are you sure your analysis is correct even in the case where the
> program is buggy and is doing stupid stuff like wr-unlock of a
> rd-lock, or vice versa? I think your analysis is correct in the
> case of a correct program.
Some lines above, there is a consistency check done between
the lock kind and the unlock operation:
if (isRDWR && lock->kind != LK_rdwr) {
HG_(record_error_Misc)( thr, "pthread_rwlock_unlock with a "
"pthread_mutex_t* argument " );
}
if ((!isRDWR) && lock->kind == LK_rdwr) {
HG_(record_error_Misc)( thr, "pthread_mutex_unlock with a "
"pthread_rwlock_t* argument " );
}
So, at least, I believe such buggy behaviour will be reported.
>
> Probably I was being conservative. AFAIK this piece of code is
> not performance critical, so I prefer to be conservative rather
> than prove that the improved version is correct even when the
> program being run is buggy.
Looks reasonable to me.
It would be worth adding a small comment to explain this in the
code so as to not surprise (future) readers.
Alternatively, an assert that the write lockset before and after
are the same in case of !isRDWR (this will just be a pointer comparison
if I have properly understood the laog data structures).
Another assert can be done just above : the lockset should not be
equal (again, it will be a pointer comparison I believe).
Philippe
|