|
From: <sv...@va...> - 2011-03-11 21:07:10
|
Author: sewardj
Date: 2011-03-11 21:06:59 +0000 (Fri, 11 Mar 2011)
New Revision: 11627
Log:
Add free-is-write functionality (experimental, not enabled by default).
Added:
trunk/helgrind/tests/free_is_write.c
trunk/helgrind/tests/free_is_write.stderr.exp
trunk/helgrind/tests/free_is_write.stdout.exp
trunk/helgrind/tests/free_is_write.vgtest
Modified:
trunk/helgrind/docs/hg-manual.xml
trunk/helgrind/hg_basics.c
trunk/helgrind/hg_basics.h
trunk/helgrind/hg_main.c
trunk/helgrind/tests/Makefile.am
Modified: trunk/helgrind/docs/hg-manual.xml
===================================================================
--- trunk/helgrind/docs/hg-manual.xml 2011-03-11 20:02:47 UTC (rev 11626)
+++ trunk/helgrind/docs/hg-manual.xml 2011-03-11 21:06:59 UTC (rev 11627)
@@ -955,6 +955,28 @@
<!-- start of xi:include in the manpage -->
<variablelist id="hg.opts.list">
+ <varlistentry id="opt.free-is-write"
+ xreflabel="--free-is-write">
+ <term>
+ <option><![CDATA[--free-is-write=no|yes
+ [default: no] ]]></option>
+ </term>
+ <listitem>
+ <para>When enabled (not the default), Helgrind treats freeing of
+ heap memory as if the memory was written immediately before
+ the free. This exposes races where memory is referenced by
+ one thread, and freed by another, but there is no observable
+ synchronisation event to ensure that the reference happens
+ before the free.
+ </para>
+ <para>This functionality is new in Valgrind 3.7.0, and is
+ regarded as experimental. It is not enabled by default
+ because its interaction with custom memory allocators is not
+ well understood at present. User feedback is welcomed.
+ </para>
+ </listitem>
+ </varlistentry>
+
<varlistentry id="opt.track-lockorders"
xreflabel="--track-lockorders">
<term>
Modified: trunk/helgrind/hg_basics.c
===================================================================
--- trunk/helgrind/hg_basics.c 2011-03-11 20:02:47 UTC (rev 11626)
+++ trunk/helgrind/hg_basics.c 2011-03-11 21:06:59 UTC (rev 11627)
@@ -80,7 +80,9 @@
Word HG_(clo_sanity_flags) = 0;
+Bool HG_(clo_free_is_write) = False;
+
/*--------------------------------------------------------------------*/
/*--- end hg_basics.c ---*/
/*--------------------------------------------------------------------*/
Modified: trunk/helgrind/hg_basics.h
===================================================================
--- trunk/helgrind/hg_basics.h 2011-03-11 20:02:47 UTC (rev 11626)
+++ trunk/helgrind/hg_basics.h 2011-03-11 21:06:59 UTC (rev 11627)
@@ -103,6 +103,12 @@
SCE_{THREADS,LOCKS,BIGRANGE,ACCESS,LAOG}. */
extern Word HG_(clo_sanity_flags);
+/* Treat heap frees as if the memory was written immediately prior to
+ the free. This shakes out races in which memory is referenced by
+ one thread, and freed by another, and there's no observable
+ synchronisation event to guarantee that the reference happens
+ before the free. */
+extern Bool HG_(clo_free_is_write);
#endif /* ! __HG_BASICS_H */
Modified: trunk/helgrind/hg_main.c
===================================================================
--- trunk/helgrind/hg_main.c 2011-03-11 20:02:47 UTC (rev 11626)
+++ trunk/helgrind/hg_main.c 2011-03-11 21:06:59 UTC (rev 11627)
@@ -1705,9 +1705,20 @@
static
void evh__die_mem_heap ( Addr a, SizeT len ) {
+ Thread* thr;
if (SHOW_EVENTS >= 1)
VG_(printf)("evh__die_mem_heap(%p, %lu)\n", (void*)a, len );
- shadow_mem_make_NoAccess( get_current_Thread(), a, len );
+ thr = get_current_Thread();
+ tl_assert(thr);
+ if (HG_(clo_free_is_write)) {
+ /* Treat frees as if the memory was written immediately prior to
+ the free. This shakes out more races, specifically, cases
+ where memory is referenced by one thread, and freed by
+ another, and there's no observable synchronisation event to
+ guarantee that the reference happens before the free. */
+ shadow_mem_cwrite_range(thr, a, len);
+ }
+ shadow_mem_make_NoAccess( thr, a, len );
if (len >= SCE_BIGRANGE_T && (HG_(clo_sanity_flags) & SCE_BIGRANGE))
all__sanity_check("evh__pre_mem_read-post");
}
@@ -4580,6 +4591,8 @@
if (0) VG_(printf)("XXX sanity flags: 0x%lx\n", HG_(clo_sanity_flags));
}
+ else if VG_BOOL_CLO(arg, "--free-is-write",
+ HG_(clo_free_is_write)) {}
else
return VG_(replacement_malloc_process_cmd_line_option)(arg);
@@ -4589,6 +4602,7 @@
static void hg_print_usage ( void )
{
VG_(printf)(
+" --free-is-write=no|yes treat heap frees as writes [no]\n"
" --track-lockorders=no|yes show lock ordering errors? [yes]\n"
" --history-level=none|approx|full [full]\n"
" full: show both stack traces for a data race (can be very slow)\n"
Modified: trunk/helgrind/tests/Makefile.am
===================================================================
--- trunk/helgrind/tests/Makefile.am 2011-03-11 20:02:47 UTC (rev 11626)
+++ trunk/helgrind/tests/Makefile.am 2011-03-11 21:06:59 UTC (rev 11627)
@@ -12,6 +12,8 @@
annotate_smart_pointer.stderr.exp \
bar_bad.vgtest bar_bad.stdout.exp bar_bad.stderr.exp \
bar_trivial.vgtest bar_trivial.stdout.exp bar_trivial.stderr.exp \
+ free_is_write.vgtest free_is_write.stdout.exp \
+ free_is_write.stderr.exp \
hg01_all_ok.vgtest hg01_all_ok.stdout.exp hg01_all_ok.stderr.exp \
hg02_deadlock.vgtest hg02_deadlock.stdout.exp hg02_deadlock.stderr.exp \
hg03_inherit.vgtest hg03_inherit.stdout.exp hg03_inherit.stderr.exp \
@@ -79,6 +81,7 @@
# should be conditionally compiled like tc20_verifywrap is.
check_PROGRAMS = \
annotate_hbefore \
+ free_is_write \
hg01_all_ok \
hg02_deadlock \
hg03_inherit \
Added: trunk/helgrind/tests/free_is_write.c
===================================================================
--- trunk/helgrind/tests/free_is_write.c (rev 0)
+++ trunk/helgrind/tests/free_is_write.c 2011-03-11 21:06:59 UTC (rev 11627)
@@ -0,0 +1,42 @@
+
+#include <assert.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <pthread.h>
+#include <unistd.h>
+
+static char* s_mem;
+
+/* wait a second, so as to guarantee that the parent access
+ the malloc'd area, then free it. */
+static void* thread_func(void* arg)
+{
+ sleep(1);
+ free(s_mem);
+ return NULL;
+}
+
+int main(int argc, char** argv)
+{
+ pthread_t tid;
+ int quiet;
+
+ fprintf(stderr, "Start.\n");
+
+ quiet = argc > 1;
+
+ s_mem = malloc(10);
+ if (0 && !quiet)
+ fprintf(stderr, "Pointer to allocated memory: %p\n", s_mem);
+ assert(s_mem);
+ pthread_create(&tid, NULL, thread_func, NULL);
+
+ /* Write, which isn't coordinated with the free ==> a race
+ should be reported. */
+ char c = s_mem[5];
+ __asm__ __volatile__("" : : "r"((long)c) );
+
+ pthread_join(tid, NULL);
+ fprintf(stderr, "Done.\n");
+ return 0;
+}
Added: trunk/helgrind/tests/free_is_write.stderr.exp
===================================================================
--- trunk/helgrind/tests/free_is_write.stderr.exp (rev 0)
+++ trunk/helgrind/tests/free_is_write.stderr.exp 2011-03-11 21:06:59 UTC (rev 11627)
@@ -0,0 +1,20 @@
+
+Start.
+Thread #x was created
+ ...
+ by 0x........: pthread_create@* (hg_intercepts.c:...)
+ by 0x........: main (free_is_write.c:32)
+
+Thread #x is the program's root thread
+
+Possible data race during write of size 1 at 0x........ by thread #x
+ at 0x........: free (vg_replace_malloc.c:...)
+ by 0x........: thread_func (free_is_write.c:15)
+ by 0x........: mythread_wrapper (hg_intercepts.c:...)
+ ...
+ This conflicts with a previous read of size 1 by thread #x
+ at 0x........: main (free_is_write.c:36)
+
+Done.
+
+ERROR SUMMARY: 1 errors from 1 contexts (suppressed: 0 from 0)
Added: trunk/helgrind/tests/free_is_write.stdout.exp
===================================================================
Added: trunk/helgrind/tests/free_is_write.vgtest
===================================================================
--- trunk/helgrind/tests/free_is_write.vgtest (rev 0)
+++ trunk/helgrind/tests/free_is_write.vgtest 2011-03-11 21:06:59 UTC (rev 11627)
@@ -0,0 +1,2 @@
+prog: free_is_write
+vgopts: --free-is-write=yes
|
|
From: Bart V. A. <bva...@ac...> - 2011-03-12 14:33:07
|
On Fri, Mar 11, 2011 at 10:07 PM, <sv...@va...> wrote: > Author: sewardj > Date: 2011-03-11 21:06:59 +0000 (Fri, 11 Mar 2011) > New Revision: 11627 > > Log: > Add free-is-write functionality (experimental, not enabled by default). I have a quiz question for the fans of the --free-is-write=yes mode: why do Helgrind and DRD (and TSan probably too) report a race on drd/tests/annotate_smart_pointer ? And once you have found the answer to that question: any suggestions for how to avoid such complaints ? $ ./vg-in-place -q --tool=helgrind drd/tests/annotate_smart_pointer Done. $ ./vg-in-place -q --tool=helgrind --free-is-write=yes drd/tests/annotate_smart_pointer ==811== Thread #1 is the program's root thread ==811== ==811== Thread #2 was created ==811== at 0x589463E: clone (clone.S:77) ==811== by 0x4E33D23: do_clone.clone.0 (createthread.c:75) ==811== by 0x4E350CC: pthread_create@@GLIBC_2.2.5 (createthread.c:249) ==811== by 0x4C27407: pthread_create_WRK (hg_intercepts.c:257) ==811== by 0x4C275AA: pthread_create@* (hg_intercepts.c:288) ==811== by 0x4011D0: main (annotate_smart_pointer.cpp:145) ==811== ==811== Possible data race during write of size 4 at 0x5b2e0b0 by thread #1 ==811== at 0x4C256D4: operator delete(void*) (vg_replace_malloc.c:387) ==811== by 0x40124B: main (annotate_smart_pointer.cpp:246) ==811== This conflicts with a previous read of size 4 by thread #2 ==811== at 0x400DE6: thread_func(void*) (annotate_smart_pointer.cpp:114) ==811== by 0x4C27593: mythread_wrapper (hg_intercepts.c:221) ==811== by 0x4E34A3E: start_thread (pthread_create.c:297) ==811== by 0x589467C: clone (clone.S:112) ==811== Done. Bart. |
|
From: Julian S. <js...@ac...> - 2011-03-13 11:00:34
|
On Saturday, March 12, 2011, Bart Van Assche wrote:
> I have a quiz question for the fans of the --free-is-write=yes mode:
> why do Helgrind and DRD (and TSan probably too) report a race on
> drd/tests/annotate_smart_pointer ?
I think it's because the annotation of your release method -- "set" --
is not quite right. FWIW, I believe Konstantin's description of
marking up threadsafe refcounting has the same problem, but I can't
find the URL now. Anyway. The relevant bit of code is (after folding
in "s_enable_annotations == true"):
U_ANNOTATE_HAPPENS_BEFORE(m_count_ptr);
if (--(*m_count_ptr) == 0)
{
U_ANNOTATE_HAPPENS_AFTER(m_count_ptr);
delete m_ptr;
m_ptr = NULL;
delete m_count_ptr;
m_count_ptr = NULL;
}
}
and the --(*m_count_ptr) is just a call to __sync_sub_and_fetch.
Problem is that the h-b edge is created too early, and results in
the load and store from --(*m_count_ptr) being concurrent with
the delete. A version which I believe is correct is:
int new_count = --(*m_count_ptr);
U_ANNOTATE_HAPPENS_BEFORE(m_count_ptr);
if (new_count == 0)
{
U_ANNOTATE_HAPPENS_AFTER(m_count_ptr);
delete m_ptr;
m_ptr = NULL;
delete m_count_ptr;
m_count_ptr = NULL;
}
}
and if you change the code like that, the complaint goes away.
J
|
|
From: Bart V. A. <bva...@ac...> - 2011-03-13 11:16:27
|
On Sun, Mar 13, 2011 at 11:56 AM, Julian Seward <js...@ac...> wrote:
> On Saturday, March 12, 2011, Bart Van Assche wrote:
>
>> I have a quiz question for the fans of the --free-is-write=yes mode:
>> why do Helgrind and DRD (and TSan probably too) report a race on
>> drd/tests/annotate_smart_pointer ?
>
> I think it's because the annotation of your release method -- "set" --
> is not quite right. FWIW, I believe Konstantin's description of
> marking up threadsafe refcounting has the same problem, but I can't
> find the URL now. Anyway. The relevant bit of code is (after folding
> in "s_enable_annotations == true"):
>
> U_ANNOTATE_HAPPENS_BEFORE(m_count_ptr);
> if (--(*m_count_ptr) == 0)
> {
> U_ANNOTATE_HAPPENS_AFTER(m_count_ptr);
> delete m_ptr;
> m_ptr = NULL;
> delete m_count_ptr;
> m_count_ptr = NULL;
> }
> }
>
> and the --(*m_count_ptr) is just a call to __sync_sub_and_fetch.
> Problem is that the h-b edge is created too early, and results in
> the load and store from --(*m_count_ptr) being concurrent with
> the delete. A version which I believe is correct is:
>
> int new_count = --(*m_count_ptr);
> U_ANNOTATE_HAPPENS_BEFORE(m_count_ptr);
> if (new_count == 0)
> {
> U_ANNOTATE_HAPPENS_AFTER(m_count_ptr);
> delete m_ptr;
> m_ptr = NULL;
> delete m_count_ptr;
> m_count_ptr = NULL;
> }
> }
>
> and if you change the code like that, the complaint goes away.
Sorry, but I disagree - the above code introduces a race condition and
hence it is incorrect. It is possible that for whatever reason a delay
occurs after the atomic decrement and before
U_ANNOTATE_HAPPENS_BEFORE(). So although the
U_ANNOTATE_HAPPENS_BEFORE() must occur before any (other) thread
invokes the destructor, with the above code it becomes possible that
U_ANNOTATE_HAPPENS_BEFORE() is invoked after another thread has
invoked the destructor. Inserting the code "if (new_count != 0)
sleep(1);" after the atomic decrement and before
U_ANNOTATE_HAPPENS_BEFORE() is sufficient to illustrate that the above
code is incorrect - any data race detector will report one or more
races on that code because of it hasn't been annotated incorrectly.
Bart.
|
|
From: Julian S. <js...@ac...> - 2011-03-13 12:14:52
|
> Sorry, but I disagree - the above code introduces a race condition and
> hence it is incorrect.
Ok -- I'm not sure I'm correct on this either :-) If anything it goes
to show how hard it is to reason about the behaviour of concurrent
programs.
I think it would be useful to separate the question of why a race
is reported in the original version from the question of how to fix
it. So, first of all, do you agree with my analysis of why the
race is reported -- because the atomic load/store is concurrent with
the delete ?
> > U_ANNOTATE_HAPPENS_BEFORE(m_count_ptr);
> > if (--(*m_count_ptr) == 0)
> > {
> > U_ANNOTATE_HAPPENS_AFTER(m_count_ptr);
> > delete m_ptr;
> > m_ptr = NULL;
> > delete m_count_ptr;
> > m_count_ptr = NULL;
> > }
> > }
J
|
|
From: Bart V. A. <bva...@ac...> - 2011-03-13 12:46:39
|
On Sun, Mar 13, 2011 at 1:10 PM, Julian Seward <js...@ac...> wrote: > > I think it would be useful to separate the question of why a race > is reported in the original version from the question of how to fix > it. So, first of all, do you agree with my analysis of why the > race is reported -- because the atomic load/store is concurrent with > the delete ? Not completely. What the tools are reporting IMHO is that there is no tool-visible h-b arc between the atomic decrement in one thread and freeing memory in another thread. So the test program is fine but the race report is a false positive. The reason I posted this on the list is that I haven't found a solution yet for letting a data race detection tool recognize that pattern without introducing new annotations. Bart. |
|
From: Julian S. <js...@ac...> - 2011-03-13 12:59:02
|
On Sunday, March 13, 2011, Bart Van Assche wrote:
> On Sun, Mar 13, 2011 at 1:10 PM, Julian Seward <js...@ac...> wrote:
> > I think it would be useful to separate the question of why a race
> > is reported in the original version from the question of how to fix
> > it. So, first of all, do you agree with my analysis of why the
> > race is reported -- because the atomic load/store is concurrent with
> > the delete ?
>
> Not completely. What the tools are reporting IMHO is that there is no
> tool-visible h-b arc between the atomic decrement in one thread and
> freeing memory in another thread. So the test program is fine but the
> race report is a false positive. The reason I posted this on the list
> is that I haven't found a solution yet for letting a data race
> detection tool recognize that pattern without introducing new
> annotations.
Err, I think we're agreeing. Problem was imprecise wording on
my part, sorry. To clarify:
I agree that the test program (original version) doesn't have
a race.
I agree also that the race reported in the original program is
a false positive, due to the reason you mention.
I am wondering if this is an atomicity problem. Perhaps what we
need is the decrement and creation of the h-b edge to be atomic.
I say this because (as we seem to have established) neither the
original ordering
U_ANNOTATE_HAPPENS_BEFORE(m_count_ptr);
int new_count = --(*m_count_ptr);
if (new_count == 0)
nor the version I proposed
int new_count = --(*m_count_ptr);
U_ANNOTATE_HAPPENS_BEFORE(m_count_ptr);
if (new_count == 0)
are exactly correct.
J
|
|
From: Bart V. A. <bva...@ac...> - 2011-03-20 13:24:36
|
On Sun, Mar 13, 2011 at 1:54 PM, Julian Seward <js...@ac...> wrote: > On Sunday, March 13, 2011, Bart Van Assche wrote: > > On Sun, Mar 13, 2011 at 1:10 PM, Julian Seward <js...@ac...> wrote: > > > I think it would be useful to separate the question of why a race > > > is reported in the original version from the question of how to fix > > > it. So, first of all, do you agree with my analysis of why the > > > race is reported -- because the atomic load/store is concurrent with > > > the delete ? > > > > Not completely. What the tools are reporting IMHO is that there is no > > tool-visible h-b arc between the atomic decrement in one thread and > > freeing memory in another thread. So the test program is fine but the > > race report is a false positive. The reason I posted this on the list > > is that I haven't found a solution yet for letting a data race > > detection tool recognize that pattern without introducing new > > annotations. > > Err, I think we're agreeing. Problem was imprecise wording on > my part, sorry. To clarify: > > I agree that the test program (original version) doesn't have > a race. > > I agree also that the race reported in the original program is > a false positive, due to the reason you mention. > > I am wondering if this is an atomicity problem. Perhaps what we > need is the decrement and creation of the h-b edge to be atomic. How about adding a new macro ANNOTATED_ADD_AND_FETCH(addr, value) that does the following: * Find out via sizeof(*(addr)) whether addr points to an 8-bit, 16-bit, 32-bit or 64-bit variable. * Invoke the following sequence such that no intervening context switch between threads occurs: - ANNOTATE_HAPPENS_BEFORE(addr) - Atomically add value to *addr and remember the new value. - ANNOTATE_HAPPENS_AFTER(addr) - Return the new value. * It remains the responsibility of the programmer invoking ANNOTATED_ADD_AND_FETCH(addr, value) to insert a memory before and/or after this macro if necessary. Bart. |
|
From: Konstantin S. <kon...@gm...> - 2011-03-20 20:00:40
|
> How about adding a new macro ANNOTATED_ADD_AND_FETCH(addr, value) that
What is the benefit compared to just having the BEFORE/AFTER
annotation and doing this?
ANNOTATE_HAPPENS_BEFORE(&refcount_);
if (!AtomicDecrementByOne(&refcount_)) {
ANNOTATE_HAPPENS_AFTER(&refcount_);
}
Remember:
1. Every additional annotation multiplies user confusion.
2. Annotating the code should be done by experts (otherwise they'll be
done wrong).
3. Only small amount of code should need annotations (otherwise, your
code sucks).
--kcc
> does the following:
> * Find out via sizeof(*(addr)) whether addr points to an 8-bit,
> 16-bit, 32-bit or 64-bit variable.
> * Invoke the following sequence such that no intervening context
> switch between threads occurs:
> - ANNOTATE_HAPPENS_BEFORE(addr)
> - Atomically add value to *addr and remember the new value.
> - ANNOTATE_HAPPENS_AFTER(addr)
> - Return the new value.
> * It remains the responsibility of the programmer invoking
> ANNOTATED_ADD_AND_FETCH(addr, value) to insert a memory before and/or
> after this macro if necessary.
>
> Bart.
>
|
|
From: Bart V. A. <bva...@ac...> - 2011-03-20 20:06:50
|
On Sun, Mar 20, 2011 at 9:00 PM, Konstantin Serebryany
<kon...@gm...> wrote:
>> How about adding a new macro ANNOTATED_ADD_AND_FETCH(addr, value) that
>
> What is the benefit compared to just having the BEFORE/AFTER
> annotation and doing this?
> ANNOTATE_HAPPENS_BEFORE(&refcount_);
> if (!AtomicDecrementByOne(&refcount_)) {
> ANNOTATE_HAPPENS_AFTER(&refcount_);
> }
>
> Remember:
> 1. Every additional annotation multiplies user confusion.
> 2. Annotating the code should be done by experts (otherwise they'll be
> done wrong).
> 3. Only small amount of code should need annotations (otherwise, your
> code sucks).
Hello Konstantin,
The goal is to avoid the kind of false positive reports described in
the start of this thread -- see also
http://article.gmane.org/gmane.comp.debugging.valgrind.devel/12947.
Bart.
|
|
From: Konstantin S. <kon...@gm...> - 2011-03-21 09:40:21
|
On Sun, Mar 20, 2011 at 11:06 PM, Bart Van Assche <bva...@ac...> wrote:
> On Sun, Mar 20, 2011 at 9:00 PM, Konstantin Serebryany
> <kon...@gm...> wrote:
>>> How about adding a new macro ANNOTATED_ADD_AND_FETCH(addr, value) that
>>
>> What is the benefit compared to just having the BEFORE/AFTER
>> annotation and doing this?
>> ANNOTATE_HAPPENS_BEFORE(&refcount_);
>> if (!AtomicDecrementByOne(&refcount_)) {
>> ANNOTATE_HAPPENS_AFTER(&refcount_);
>> }
>>
>> Remember:
>> 1. Every additional annotation multiplies user confusion.
>> 2. Annotating the code should be done by experts (otherwise they'll be
>> done wrong).
>> 3. Only small amount of code should need annotations (otherwise, your
>> code sucks).
>
> Hello Konstantin,
>
> The goal is to avoid the kind of false positive reports described in
> the start of this thread -- see also
> http://article.gmane.org/gmane.comp.debugging.valgrind.devel/12947.
The scheme currently used by tsan works fine, I don't see why it needs
a change.
ANNOTATE_HAPPENS_BEFORE(&refcount_);
if (!AtomicDecrementByOne(&refcount_)) {
ANNOTATE_HAPPENS_AFTER(&refcount_);
}
Note that the accesses done inside AtomicDecrementByOne should be
ignored by the tool -- otherwise indeed you will have false reports.
See http://code.google.com/p/data-race-test/wiki/ThreadSanitizerIgnores
on how tsan ignores stuff.
--kcc
>
> Bart.
>
|
|
From: Bart V. A. <bva...@ac...> - 2011-03-21 11:05:52
|
On Mon, Mar 21, 2011 at 10:39 AM, Konstantin Serebryany
<kon...@gm...> wrote:
> On Sun, Mar 20, 2011 at 11:06 PM, Bart Van Assche <bva...@ac...> wrote:
>> On Sun, Mar 20, 2011 at 9:00 PM, Konstantin Serebryany
>> <kon...@gm...> wrote:
>>>> How about adding a new macro ANNOTATED_ADD_AND_FETCH(addr, value) that
>>>
>>> What is the benefit compared to just having the BEFORE/AFTER
>>> annotation and doing this?
>>> ANNOTATE_HAPPENS_BEFORE(&refcount_);
>>> if (!AtomicDecrementByOne(&refcount_)) {
>>> ANNOTATE_HAPPENS_AFTER(&refcount_);
>>> }
>>>
>>> Remember:
>>> 1. Every additional annotation multiplies user confusion.
>>> 2. Annotating the code should be done by experts (otherwise they'll be
>>> done wrong).
>>> 3. Only small amount of code should need annotations (otherwise, your
>>> code sucks).
>>
>> Hello Konstantin,
>>
>> The goal is to avoid the kind of false positive reports described in
>> the start of this thread -- see also
>> http://article.gmane.org/gmane.comp.debugging.valgrind.devel/12947.
>
> The scheme currently used by tsan works fine, I don't see why it needs
> a change.
> ANNOTATE_HAPPENS_BEFORE(&refcount_);
> if (!AtomicDecrementByOne(&refcount_)) {
> ANNOTATE_HAPPENS_AFTER(&refcount_);
> }
>
> Note that the accesses done inside AtomicDecrementByOne should be
> ignored by the tool -- otherwise indeed you will have false reports.
Hello Konstantin,
Sorry, but ignoring accesses inside AtomicDecrementByOne() entirely
seems like a bad idea to me. If a data race detection tool ignores
such accesses, the following class of errors will not be reported at
all:
* Atomic modification of a memory location in one thread.
* Concurrent non-atomic modification of a memory location in another thread.
DRD is able to report these errors because it models atomic
modifications of a memory location as a load.
Bart.
|
|
From: Konstantin S. <kon...@gm...> - 2011-03-21 11:55:12
|
On Mon, Mar 21, 2011 at 2:05 PM, Bart Van Assche <bva...@ac...> wrote:
> On Mon, Mar 21, 2011 at 10:39 AM, Konstantin Serebryany
> <kon...@gm...> wrote:
>> On Sun, Mar 20, 2011 at 11:06 PM, Bart Van Assche <bva...@ac...> wrote:
>>> On Sun, Mar 20, 2011 at 9:00 PM, Konstantin Serebryany
>>> <kon...@gm...> wrote:
>>>>> How about adding a new macro ANNOTATED_ADD_AND_FETCH(addr, value) that
>>>>
>>>> What is the benefit compared to just having the BEFORE/AFTER
>>>> annotation and doing this?
>>>> ANNOTATE_HAPPENS_BEFORE(&refcount_);
>>>> if (!AtomicDecrementByOne(&refcount_)) {
>>>> ANNOTATE_HAPPENS_AFTER(&refcount_);
>>>> }
>>>>
>>>> Remember:
>>>> 1. Every additional annotation multiplies user confusion.
>>>> 2. Annotating the code should be done by experts (otherwise they'll be
>>>> done wrong).
>>>> 3. Only small amount of code should need annotations (otherwise, your
>>>> code sucks).
>>>
>>> Hello Konstantin,
>>>
>>> The goal is to avoid the kind of false positive reports described in
>>> the start of this thread -- see also
>>> http://article.gmane.org/gmane.comp.debugging.valgrind.devel/12947.
>>
>> The scheme currently used by tsan works fine, I don't see why it needs
>> a change.
>> ANNOTATE_HAPPENS_BEFORE(&refcount_);
>> if (!AtomicDecrementByOne(&refcount_)) {
>> ANNOTATE_HAPPENS_AFTER(&refcount_);
>> }
>>
>> Note that the accesses done inside AtomicDecrementByOne should be
>> ignored by the tool -- otherwise indeed you will have false reports.
>
> Hello Konstantin,
>
> Sorry, but ignoring accesses inside AtomicDecrementByOne() entirely
> seems like a bad idea to me. If a data race detection tool ignores
> such accesses, the following class of errors will not be reported at
> all:
> * Atomic modification of a memory location in one thread.
> * Concurrent non-atomic modification of a memory location in another thread.
>
> DRD is able to report these errors because it models atomic
> modifications of a memory location as a load.
Then DRD does not have to ignore AtomicDecrementByOne(), at least if
it is implemented only using LOCK-prefixed instructions.
ThreadSanitizer simply ignores all LOCK-prefixed instructions.
Initially we inherited the idea of 'bus lock' from helgrind (it worked
as you describe), but we saw too many reports from CAS loops (a CAS is
usually accompanied with a regular load).
Then we disabled it and never enabled it back.
--kcc
>
> Bart.
>
|