|
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.
|