|
From: Konstantin S. <kon...@gm...> - 2010-06-29 14:34:13
|
Hi Julian, Bart, Helgrind and DRD does not seem to detect races between a memory access and free(). (memcheck will find such bug only if free() happens before the access during a particular run). Do you plan to implement such functionality? (basically, free() should be treated as a write) I implemented such feature in ThreadSanitizer but was hit by libstdc++: the string implementation is unfriendly to race detectors because it uses atomic reference counting in the destructor. http://gcc.gnu.org/onlinedocs/libstdc++/latest-doxygen/a00762_source.html#l00228 Have you observed any issues around reference counting in string<>? Any thoughts? Thanks, --kcc P.S. Unittest where ThreadSanitizer correctly finds a race between a read and free(): http://code.google.com/p/data-race-test/source/browse/trunk/unittest/racecheck_unittest.cc?spec=svn2278&r=2278#6473 Unittest where ThreadSanitizer reports a false warning in ~basic_string: http://code.google.com/p/data-race-test/source/browse/trunk/unittest/racecheck_unittest.cc?spec=svn2278&r=2278#3186 |
|
From: Bart V. A. <bva...@ac...> - 2010-06-30 19:16:18
|
On Tue, Jun 29, 2010 at 4:33 PM, Konstantin Serebryany < kon...@gm...> wrote: > Helgrind and DRD does not seem to detect races between a memory access > and free(). > (memcheck will find such bug only if free() happens before the access > during a particular run). > Do you plan to implement such functionality? (basically, free() should > be treated as a write) > > I implemented such feature in ThreadSanitizer but was hit by > libstdc++: the string implementation is unfriendly to race detectors > because it uses atomic reference counting in the destructor. > > http://gcc.gnu.org/onlinedocs/libstdc++/latest-doxygen/a00762_source.html#l00228 > Have you observed any issues around reference counting in string<>? > Any thoughts? > Detecting races between memory accesses and freeing memory is certainly useful. But since the functions that manipulate reference counts are typically implemented as inline functions, it looks hard to me to let a data race detection tool recognize automatically the dependencies that are the result of a reference counting scheme. Not only std::string<> in libstdc++ is using this technique, but also std::tr1::shared_ptr<> in libstdc++, boost::shared_ptr<> in the Boost libraries, QSharedPointer in Qt and probably several other class implementations. I'm not sure it will be possible to convince the maintainers of all these libraries to add ANNOTATE_HAPPENS_BEFORE() / AFTER() annotations such that Valgrind-based race detectors can recognize the dependencies resulting from reference counting schemes. Bart. |
|
From: Konstantin S. <kon...@gm...> - 2010-07-01 09:20:40
|
On Wed, Jun 30, 2010 at 11:16 PM, Bart Van Assche <bva...@ac...> wrote: > On Tue, Jun 29, 2010 at 4:33 PM, Konstantin Serebryany > <kon...@gm...> wrote: >> >> Helgrind and DRD does not seem to detect races between a memory access >> and free(). >> (memcheck will find such bug only if free() happens before the access >> during a particular run). >> Do you plan to implement such functionality? (basically, free() should >> be treated as a write) >> >> I implemented such feature in ThreadSanitizer but was hit by >> libstdc++: the string implementation is unfriendly to race detectors >> because it uses atomic reference counting in the destructor. >> >> http://gcc.gnu.org/onlinedocs/libstdc++/latest-doxygen/a00762_source.html#l00228 >> Have you observed any issues around reference counting in string<>? >> Any thoughts? > > Detecting races between memory accesses and freeing memory is certainly > useful. But since the functions that manipulate reference counts are > typically implemented as inline functions, it looks hard to me to let a data > race detection tool recognize automatically the dependencies that are the > result of a reference counting scheme. Not only std::string<> in libstdc++ > is using this technique, but also std::tr1::shared_ptr<> in libstdc++, > boost::shared_ptr<> in the Boost libraries, QSharedPointer in Qt and > probably several other class implementations. Oh, yes! > I'm not sure it will be > possible to convince the maintainers of all these libraries to add > ANNOTATE_HAPPENS_BEFORE() / AFTER() annotations such that Valgrind-based > race detectors can recognize the dependencies resulting from reference > counting schemes. I don't think we need to use ANONTATE_* in standard libraries. Instead, I want to ask the library writers to arrange their code to make it easier to intercept by valgrind tools. In particular, for atomic ref counting I want to ask them to have a new function which simply means "atomically decrement reference count and return whether count is zero". That function could itself simply call __exchange_and_add_dispatch or whatever. The function should not be inlined when compiling in debug mode (or we need some way to intercept it). What do you think, Bart? Julian? Before reaching to the library folks I want to make sure that we agree with each other... --kcc > > Bart. > |
|
From: Bart V. A. <bva...@ac...> - 2010-07-06 16:53:26
|
On Thu, Jul 1, 2010 at 11:20 AM, Konstantin Serebryany <kon...@gm...> wrote: > > [ ... ] > > I don't think we need to use ANNOTATE_* in standard libraries. > Instead, I want to ask the library writers to arrange their code to > make it easier to intercept by valgrind tools. > In particular, for atomic ref counting I want to ask them to have a new > function which simply means "atomically decrement reference count and return > whether count is zero". That function could itself simply call > __exchange_and_add_dispatch or whatever. > The function should not be inlined when compiling in debug mode (or we > need some way to intercept it). Please keep in mind that just decrementing an atomic counter is not sufficient for proper reference counting semantics -- memory barriers have to be inserted before the decrement and after the counter has been decremented from one to zero too. Bart. |
|
From: Julian S. <js...@ac...> - 2010-07-02 07:02:54
|
> I don't think we need to use ANONTATE_* in standard libraries. > Instead, I want to ask the library writers to arrange their code to > make it easier to intercept by valgrind tools. > In particular, for atomic ref counting I want to ask them to have a new > function which simply means "atomically decrement reference count and > return whether count is zero". That function could itself simply call > __exchange_and_add_dispatch or whatever. Yes. Perhaps "T atomic_decrement(T*)", for some T (unsigned int etc) and returns the new T. Synthesising "is new count == 0" into a 32/64-bit value is expensive -- requires "testl ; set ; movzbl" -- better simply to return the new value, which we have for free on LL/SC based platforms (ARM, PPC) anyway, and also on x86/x86_64 if the atomic dec is encoded using "load, sub, cmpxchg, check-the-cmpxchg succeeded". Why not just use the gcc builtin naming scheme for atomic operations? It's uniform across multiple archs, and makes it clear whether the returned value is the value before or after the decrement. No need to reinvent the wheel. --------------- Kind-of related comment: One observation w.r.t. refcounting and C++ is, when the refcount makes a 1 -> 0 transition then the object becomes exclusively owned by the decrementing thread, and so it can run its destructor without any locking. However, at least Helgrind does not understand that, and so needs to be told the object is now exclusively owned by the calling thread before running the destructor. In this case, the obvious thing to do seems like VALGRIND_HG_CLEAN_MEMORY(this, sizeof(*this)) but this is wrong in the presence of inheritance, when a class defines a ::Release method, and is then subclassed, but the subclass doesn't provide its own ::Release. Then "sizeof(*this)" will be too small, because it is the size of the base class, not the runtime size. Also, in the presence of multiple inheritance, there is no guarantee that "this" points to the start of the object; we are only assured that it points either to the start or somewhere within it. For this reason I recently added to Helgrind a new clreq, VALGRIND_HG_CLEAN_MEMORY_HEAPBLOCK(ptr), which is the same as VALGRIND_HG_CLEAN_MEMORY applied to whatever heap block "ptr" points into, if any, and it's OK if it's an interior pointer. Do TSan and DRD have anything similar? This isn't theoretical. I found it out the hard way when analysing an ocean of false race reports when Helgrinding C++ code using atomic refcounting and inheritance, earlier this year. J |
|
From: Konstantin S. <kon...@gm...> - 2010-07-02 17:04:12
|
On Fri, Jul 2, 2010 at 11:03 AM, Julian Seward <js...@ac...> wrote:
>
>> I don't think we need to use ANONTATE_* in standard libraries.
>> Instead, I want to ask the library writers to arrange their code to
>> make it easier to intercept by valgrind tools.
>> In particular, for atomic ref counting I want to ask them to have a new
>> function which simply means "atomically decrement reference count and
>> return whether count is zero". That function could itself simply call
>> __exchange_and_add_dispatch or whatever.
>
> Yes. Perhaps "T atomic_decrement(T*)", for some T (unsigned int etc)
> and returns the new T.
Ok.
> Synthesising "is new count == 0" into a 32/64-bit
> value is expensive -- requires "testl ; set ; movzbl" -- better simply
> to return the new value, which we have for free on LL/SC based platforms
> (ARM, PPC) anyway, and also on x86/x86_64 if the atomic dec is encoded
> using "load, sub, cmpxchg, check-the-cmpxchg succeeded".
>
> Why not just use the gcc builtin naming scheme for atomic operations?
>From their naming it is not obvious that the functions will not be
used for something other that ref counting (they probably will).
> It's uniform across multiple archs, and makes it clear whether the
> returned value is the value before or after the decrement. No need to
> reinvent the wheel.
>
> ---------------
>
> Kind-of related comment:
>
> One observation w.r.t. refcounting and C++ is, when the refcount makes
> a 1 -> 0 transition then the object becomes exclusively owned by the
> decrementing thread, and so it can run its destructor without any
> locking. However, at least Helgrind does not understand that, and so
> needs to be told the object is now exclusively owned by the calling
> thread before running the destructor.
The only reliable way of annotating ref counting known to me is
ANONTATE_HAPPENS_BEFORE(&ref_count_);
if (AtomicRefcountDecrement(&ref_count_) == 0) {
ANONTATE_HAPPENS_AFTER(&ref_count_);
delete this;
}
Not that 'delete this' is not just a memory deallocation (free), but
also any amount of other stuff.
>
> In this case, the obvious thing to do seems like
>
> VALGRIND_HG_CLEAN_MEMORY(this, sizeof(*this))
>
> but this is wrong in the presence of inheritance, when a class defines a
> ::Release method, and is then subclassed, but the subclass doesn't provide
> its own ::Release. Then "sizeof(*this)" will be too small, because it is
> the size of the base class, not the runtime size. Also, in the presence of
> multiple inheritance, there is no guarantee that "this" points to the
> start of the object; we are only assured that it points either to the
> start or somewhere within it.
>
> For this reason I recently added to Helgrind a new clreq,
> VALGRIND_HG_CLEAN_MEMORY_HEAPBLOCK(ptr), which is the same as
> VALGRIND_HG_CLEAN_MEMORY applied to whatever heap block "ptr" points
> into, if any, and it's OK if it's an interior pointer.
This is not enough, imo.
When you make 'delete this', 'this' is not necessary a simple object
like string.
It could be a complicate class like e.g. a tree, which means you will
end up with lots of calls to free() and other computations.
>
> Do TSan and DRD have anything similar?
As I said, we need HAPPENS_BEFORE/AFTER.
>
> This isn't theoretical. I found it out the hard way when analysing
> an ocean of false race reports when Helgrinding C++ code using atomic
> refcounting and inheritance, earlier this year.
Aha! So, you share my pain! :)
--kcc
>
> J
>
|
|
From: Bart V. A. <bva...@ac...> - 2010-07-02 10:19:27
|
On Fri, Jul 2, 2010 at 9:03 AM, Julian Seward <js...@ac...> wrote: > > > I don't think we need to use ANNOTATE_* in standard libraries. > > Instead, I want to ask the library writers to arrange their code to > > make it easier to intercept by valgrind tools. > > In particular, for atomic ref counting I want to ask them to have a new > > function which simply means "atomically decrement reference count and > > return whether count is zero". That function could itself simply call > > __exchange_and_add_dispatch or whatever. > > Yes. Perhaps "T atomic_decrement(T*)", for some T (unsigned int etc) > and returns the new T. Synthesising "is new count == 0" into a 32/64-bit > value is expensive -- requires "testl ; set ; movzbl" -- better simply > to return the new value, which we have for free on LL/SC based platforms > (ARM, PPC) anyway, and also on x86/x86_64 if the atomic dec is encoded > using "load, sub, cmpxchg, check-the-cmpxchg succeeded". > > Why not just use the gcc builtin naming scheme for atomic operations? > It's uniform across multiple archs, and makes it clear whether the > returned value is the value before or after the decrement. No need to > reinvent the wheel. Only instrumenting the atomic decrement is not sufficient IMHO. The atomic increment has to be instrumented too, such that it is possible for a data race detection tool to find out which happens-before arcs to insert. > --------------- > > Kind-of related comment: > > One observation w.r.t. refcounting and C++ is, when the refcount makes > a 1 -> 0 transition then the object becomes exclusively owned by the > decrementing thread, and so it can run its destructor without any > locking. However, at least Helgrind does not understand that, and so > needs to be told the object is now exclusively owned by the calling > thread before running the destructor. > > In this case, the obvious thing to do seems like > > VALGRIND_HG_CLEAN_MEMORY(this, sizeof(*this)) > > but this is wrong in the presence of inheritance, when a class defines a > ::Release method, and is then subclassed, but the subclass doesn't provide > its own ::Release. Then "sizeof(*this)" will be too small, because it is > the size of the base class, not the runtime size. Also, in the presence of > multiple inheritance, there is no guarantee that "this" points to the > start of the object; we are only assured that it points either to the > start or somewhere within it. > > For this reason I recently added to Helgrind a new clreq, > VALGRIND_HG_CLEAN_MEMORY_HEAPBLOCK(ptr), which is the same as > VALGRIND_HG_CLEAN_MEMORY applied to whatever heap block "ptr" points > into, if any, and it's OK if it's an interior pointer. > > Do TSan and DRD have anything similar? > > This isn't theoretical. I found it out the hard way when analysing > an ocean of false race reports when Helgrinding C++ code using atomic > refcounting and inheritance, earlier this year. This kind of issue can indeed arise when letting the client program communicate to the tool how many bytes will be freed. But why to invoke such a client request from the client code while the tool already has exact information about the number of bytes that will be freed ? I'm assuming here that each reference counted object is allocated via an individual memory request, and that no memory region contains two or more reference-counted objects that are freed individually. Bart. |
|
From: Konstantin S. <kon...@gm...> - 2010-07-02 17:11:20
|
On Fri, Jul 2, 2010 at 2:19 PM, Bart Van Assche <bva...@ac...> wrote:
> On Fri, Jul 2, 2010 at 9:03 AM, Julian Seward <js...@ac...> wrote:
>>
>> > I don't think we need to use ANNOTATE_* in standard libraries.
>> > Instead, I want to ask the library writers to arrange their code to
>> > make it easier to intercept by valgrind tools.
>> > In particular, for atomic ref counting I want to ask them to have a new
>> > function which simply means "atomically decrement reference count and
>> > return whether count is zero". That function could itself simply call
>> > __exchange_and_add_dispatch or whatever.
>>
>> Yes. Perhaps "T atomic_decrement(T*)", for some T (unsigned int etc)
>> and returns the new T. Synthesising "is new count == 0" into a 32/64-bit
>> value is expensive -- requires "testl ; set ; movzbl" -- better simply
>> to return the new value, which we have for free on LL/SC based platforms
>> (ARM, PPC) anyway, and also on x86/x86_64 if the atomic dec is encoded
>> using "load, sub, cmpxchg, check-the-cmpxchg succeeded".
>>
>> Why not just use the gcc builtin naming scheme for atomic operations?
>> It's uniform across multiple archs, and makes it clear whether the
>> returned value is the value before or after the decrement. No need to
>> reinvent the wheel.
>
> Only instrumenting the atomic decrement is not sufficient IMHO. The
> atomic increment has to be instrumented too, such that it is possible
> for a data race detection tool to find out which happens-before arcs
> to insert.
Instrumenting ref count decrement was never needed in my practice.
In my practice we needed to instrument a function RefCountIsOne() in
addition to RefCountIncrement();
bool RefCountIsOne() {
bool res = AtomicRead(&ref_count_) == 1;
if (res) {
ANNOTATE_HAPPENS_AFTER(&ref_count_);
}
return res;
}
...
if (x.RefCountIsOne()) {
x.DoSomethingWithXWithoutLocksBecauseWeOwnXAndAreGoingToDeleteIt.
}
>
>> ---------------
>>
>> Kind-of related comment:
>>
>> One observation w.r.t. refcounting and C++ is, when the refcount makes
>> a 1 -> 0 transition then the object becomes exclusively owned by the
>> decrementing thread, and so it can run its destructor without any
>> locking. However, at least Helgrind does not understand that, and so
>> needs to be told the object is now exclusively owned by the calling
>> thread before running the destructor.
>>
>> In this case, the obvious thing to do seems like
>>
>> VALGRIND_HG_CLEAN_MEMORY(this, sizeof(*this))
>>
>> but this is wrong in the presence of inheritance, when a class defines a
>> ::Release method, and is then subclassed, but the subclass doesn't provide
>> its own ::Release. Then "sizeof(*this)" will be too small, because it is
>> the size of the base class, not the runtime size. Also, in the presence of
>> multiple inheritance, there is no guarantee that "this" points to the
>> start of the object; we are only assured that it points either to the
>> start or somewhere within it.
>>
>> For this reason I recently added to Helgrind a new clreq,
>> VALGRIND_HG_CLEAN_MEMORY_HEAPBLOCK(ptr), which is the same as
>> VALGRIND_HG_CLEAN_MEMORY applied to whatever heap block "ptr" points
>> into, if any, and it's OK if it's an interior pointer.
>>
>> Do TSan and DRD have anything similar?
>>
>> This isn't theoretical. I found it out the hard way when analysing
>> an ocean of false race reports when Helgrinding C++ code using atomic
>> refcounting and inheritance, earlier this year.
>
> This kind of issue can indeed arise when letting the client program
> communicate to the tool how many bytes will be freed. But why to
> invoke such a client request from the client code while the tool
> already has exact information about the number of bytes that will be
> freed ? I'm assuming here that each reference counted object is
> allocated via an individual memory request, and that no memory region
> contains two or more reference-counted objects that are freed
> individually.
See my previous message. The deleted object could be a compound
structure, not just a sequence of bytes.
The is not the case for std::string, but could be easily the case for
some other C++ stuff from e.g. boost.
We have tons of such classes, but they all use a single ref-counting
utility annotated with HAPPENS_BEFORE/AFTER and so they are
race-detector friendly.
--kcc
>
> Bart.
>
|
|
From: Konstantin S. <kon...@gm...> - 2010-07-05 09:32:55
|
+da...@go... Julian, Bart, I've prepared a (draft) message to libstdc++. Please review it. You are welcome to edit it directly too. http://code.google.com/p/data-race-test/wiki/AtomicReferenceCounting --kcc On Fri, Jul 2, 2010 at 9:10 PM, Konstantin Serebryany <kon...@gm...> wrote: > On Fri, Jul 2, 2010 at 2:19 PM, Bart Van Assche <bva...@ac...> wrote: >> On Fri, Jul 2, 2010 at 9:03 AM, Julian Seward <js...@ac...> wrote: >>> >>> > I don't think we need to use ANNOTATE_* in standard libraries. >>> > Instead, I want to ask the library writers to arrange their code to >>> > make it easier to intercept by valgrind tools. >>> > In particular, for atomic ref counting I want to ask them to have a new >>> > function which simply means "atomically decrement reference count and >>> > return whether count is zero". That function could itself simply call >>> > __exchange_and_add_dispatch or whatever. >>> >>> Yes. Perhaps "T atomic_decrement(T*)", for some T (unsigned int etc) >>> and returns the new T. Synthesising "is new count == 0" into a 32/64-bit >>> value is expensive -- requires "testl ; set ; movzbl" -- better simply >>> to return the new value, which we have for free on LL/SC based platforms >>> (ARM, PPC) anyway, and also on x86/x86_64 if the atomic dec is encoded >>> using "load, sub, cmpxchg, check-the-cmpxchg succeeded". >>> >>> Why not just use the gcc builtin naming scheme for atomic operations? >>> It's uniform across multiple archs, and makes it clear whether the >>> returned value is the value before or after the decrement. No need to >>> reinvent the wheel. >> >> Only instrumenting the atomic decrement is not sufficient IMHO. The >> atomic increment has to be instrumented too, such that it is possible >> for a data race detection tool to find out which happens-before arcs >> to insert. > > Instrumenting ref count decrement was never needed in my practice. > In my practice we needed to instrument a function RefCountIsOne() in > addition to RefCountIncrement(); > > bool RefCountIsOne() { > bool res = AtomicRead(&ref_count_) == 1; > if (res) { > ANNOTATE_HAPPENS_AFTER(&ref_count_); > } > return res; > } > ... > if (x.RefCountIsOne()) { > x.DoSomethingWithXWithoutLocksBecauseWeOwnXAndAreGoingToDeleteIt. > } > > >> >>> --------------- >>> >>> Kind-of related comment: >>> >>> One observation w.r.t. refcounting and C++ is, when the refcount makes >>> a 1 -> 0 transition then the object becomes exclusively owned by the >>> decrementing thread, and so it can run its destructor without any >>> locking. However, at least Helgrind does not understand that, and so >>> needs to be told the object is now exclusively owned by the calling >>> thread before running the destructor. >>> >>> In this case, the obvious thing to do seems like >>> >>> VALGRIND_HG_CLEAN_MEMORY(this, sizeof(*this)) >>> >>> but this is wrong in the presence of inheritance, when a class defines a >>> ::Release method, and is then subclassed, but the subclass doesn't provide >>> its own ::Release. Then "sizeof(*this)" will be too small, because it is >>> the size of the base class, not the runtime size. Also, in the presence of >>> multiple inheritance, there is no guarantee that "this" points to the >>> start of the object; we are only assured that it points either to the >>> start or somewhere within it. >>> >>> For this reason I recently added to Helgrind a new clreq, >>> VALGRIND_HG_CLEAN_MEMORY_HEAPBLOCK(ptr), which is the same as >>> VALGRIND_HG_CLEAN_MEMORY applied to whatever heap block "ptr" points >>> into, if any, and it's OK if it's an interior pointer. >>> >>> Do TSan and DRD have anything similar? >>> >>> This isn't theoretical. I found it out the hard way when analysing >>> an ocean of false race reports when Helgrinding C++ code using atomic >>> refcounting and inheritance, earlier this year. >> >> This kind of issue can indeed arise when letting the client program >> communicate to the tool how many bytes will be freed. But why to >> invoke such a client request from the client code while the tool >> already has exact information about the number of bytes that will be >> freed ? I'm assuming here that each reference counted object is >> allocated via an individual memory request, and that no memory region >> contains two or more reference-counted objects that are freed >> individually. > > See my previous message. The deleted object could be a compound > structure, not just a sequence of bytes. > The is not the case for std::string, but could be easily the case for > some other C++ stuff from e.g. boost. > We have tons of such classes, but they all use a single ref-counting > utility annotated with HAPPENS_BEFORE/AFTER and so they are > race-detector friendly. > > > --kcc > >> >> Bart. >> > |
|
From: Bart V. A. <bva...@ac...> - 2010-07-05 10:13:51
|
On Mon, Jul 5, 2010 at 11:32 AM, Konstantin Serebryany <kon...@gm...> wrote: > > +da...@go... > Julian, Bart, > > I've prepared a (draft) message to libstdc++. > Please review it. You are welcome to edit it directly too. > http://code.google.com/p/data-race-test/wiki/AtomicReferenceCounting While I welcome such an initiative, I have a few comments: - Asking the libstdc++ maintainers to use ANNOTATE_HAPPENS_BEFORE() and ANNOTATE_HAPPENS_AFTER() is not an option as long as there exist three different implementations of these macros (one in ThreadSanitizer, one in Helgrind and one in DRD). - Asking to implement a function __decrement_refcount() will only help if this function is never inlined. That will cause a slight slowdown of every reference counting implementation. I'm afraid that such a request will not be accepted by the libstdc++ maintainers. - A nitpick: are you sure that the following statement is correct: "However, it is hard or impossible to interpret raw atomic instructions as synchronization" ? While - at least in DRD - it is easy to associate ordering semantics with all atomic instructions, doing so would cause many real races not to be reported. Bart. |
|
From: Konstantin S. <kon...@gm...> - 2010-07-05 10:21:21
|
On Mon, Jul 5, 2010 at 2:13 PM, Bart Van Assche <bva...@ac...> wrote: > On Mon, Jul 5, 2010 at 11:32 AM, Konstantin Serebryany > <kon...@gm...> wrote: >> >> +da...@go... >> Julian, Bart, >> >> I've prepared a (draft) message to libstdc++. >> Please review it. You are welcome to edit it directly too. >> http://code.google.com/p/data-race-test/wiki/AtomicReferenceCounting > > While I welcome such an initiative, I have a few comments: > - Asking the libstdc++ maintainers to use ANNOTATE_HAPPENS_BEFORE() > and ANNOTATE_HAPPENS_AFTER() is not an option as long as there exist > three different implementations of these macros (one in > ThreadSanitizer, one in Helgrind and one in DRD). Agree. Shall we do something with this? > - Asking to implement a function __decrement_refcount() will only help > if this function is never inlined. Yes (and this is mentioned in the wiki). We are thinking about ways to avoid this restriction, but currently, yes, you are right. > That will cause a slight slowdown > of every reference counting implementation. I'm afraid that such a > request will not be accepted by the libstdc++ maintainers. > - A nitpick: are you sure that the following statement is correct: > "However, it is hard or impossible to interpret raw atomic > instructions as synchronization" ? There is explanation in the same phrase: (e.g. treating every compare-and-exchange instruction or every instruction with lock prefix as a synchronization event will make the analysis very slow and conservative). Perhaps you could write a more understandable sentence. :) --kcc > While - at least in DRD - it is > easy to associate ordering semantics with all atomic instructions, > doing so would cause many real races not to be reported. > > Bart. > |
|
From: Bart V. A. <bva...@ac...> - 2010-07-05 17:00:17
|
On Mon, Jul 5, 2010 at 12:20 PM, Konstantin Serebryany <kon...@gm...> wrote: > > On Mon, Jul 5, 2010 at 2:13 PM, Bart Van Assche <bva...@ac...> wrote: > > On Mon, Jul 5, 2010 at 11:32 AM, Konstantin Serebryany > > <kon...@gm...> wrote: > >> > >> +da...@go... > >> Julian, Bart, > >> > >> I've prepared a (draft) message to libstdc++. > >> Please review it. You are welcome to edit it directly too. > >> http://code.google.com/p/data-race-test/wiki/AtomicReferenceCounting > > > > While I welcome such an initiative, I have a few comments: > > - Asking the libstdc++ maintainers to use ANNOTATE_HAPPENS_BEFORE() > > and ANNOTATE_HAPPENS_AFTER() is not an option as long as there exist > > three different implementations of these macros (one in > > ThreadSanitizer, one in Helgrind and one in DRD). > > Agree. Shall we do something with this? I see three possible approaches: (a) Create a new shared library that contains at least a function for atomically decrementing a reference counter and kindly ask all authors of libraries that use reference-counted objects to use that new library for manipulating reference counts. That way data-race detection tools can recognize reference counters and the ordering imposed by decrementing reference counters by intercepting a single function. (b) Ask all authors of libraries that use reference-counted objects to add and export a function that does nothing else than decrementing a reference count. Also agree with these authors about a naming scheme for such a function such that all such functions can be intercepted via a single wildcard pattern in Valgrind. (c) Ask library authors to insert a Valgrind client request before each reference counter decrement and after each reference counter decrement that set the reference count equal to zero. We should also consider wherever reference counts are decremented in inline functions in public header files, to make the above behavior configurable via preprocessor symbol. Certain behavior of libstdc++ is already configurable via preprocessor symbols, e.g. via the preprocessor symbol GLIBCXX_FORCE_NEW. We can either try to agree upon one of the above three approaches or ask the libstdc++ authors for their opinion. Note: as is documented in the header file <valgrind/valgrind.h>, the preprocessor symbol NVALGRIND controls whether or not Valgrind client requests are inserted in the output code. Bart. |
|
From: Konstantin S. <kon...@gm...> - 2010-07-06 06:58:06
|
On Mon, Jul 5, 2010 at 9:00 PM, Bart Van Assche <bva...@ac...> wrote: > On Mon, Jul 5, 2010 at 12:20 PM, Konstantin Serebryany > <kon...@gm...> wrote: >> >> On Mon, Jul 5, 2010 at 2:13 PM, Bart Van Assche <bva...@ac...> wrote: >> > On Mon, Jul 5, 2010 at 11:32 AM, Konstantin Serebryany >> > <kon...@gm...> wrote: >> >> >> >> +da...@go... >> >> Julian, Bart, >> >> >> >> I've prepared a (draft) message to libstdc++. >> >> Please review it. You are welcome to edit it directly too. >> >> http://code.google.com/p/data-race-test/wiki/AtomicReferenceCounting >> > >> > While I welcome such an initiative, I have a few comments: >> > - Asking the libstdc++ maintainers to use ANNOTATE_HAPPENS_BEFORE() >> > and ANNOTATE_HAPPENS_AFTER() is not an option as long as there exist >> > three different implementations of these macros (one in >> > ThreadSanitizer, one in Helgrind and one in DRD). >> >> Agree. Shall we do something with this? > > I see three possible approaches: > (a) Create a new shared library that contains at least a function for > atomically decrementing a reference counter and kindly ask all authors > of libraries that use reference-counted objects to use that new > library for manipulating reference counts. That way data-race > detection tools can recognize reference counters and the ordering > imposed by decrementing reference counters by intercepting a single > function. > (b) Ask all authors of libraries that use reference-counted objects to > add and export a function that does nothing else than decrementing a > reference count. Also agree with these authors about a naming scheme > for such a function such that all such functions can be intercepted > via a single wildcard pattern in Valgrind. I am in favor of (b). But, as we both mentioned, this will *currently* require the function not to be inlined. > (c) Ask library authors to insert a Valgrind client request before > each reference counter decrement and after each reference counter > decrement that set the reference count equal to zero. I am slightly against (c). The problem with valgrind client requests is that they (currently) support only some architectures and only some compilers. > > We should also consider wherever reference counts are decremented in > inline functions in public header files, to make the above behavior > configurable via preprocessor symbol. Certain behavior of libstdc++ is > already configurable via preprocessor symbols, e.g. via the > preprocessor symbol GLIBCXX_FORCE_NEW. > > We can either try to agree upon one of the above three approaches or > ask the libstdc++ authors for their opinion. Yep, they may have a better idea. I'll send the bug report (with the link to http://code.google.com/p/data-race-test/wiki/AtomicReferenceCounting) tomorrow if nobody objects. --kcc > > Note: as is documented in the header file <valgrind/valgrind.h>, the > preprocessor symbol NVALGRIND controls whether or not Valgrind client > requests are inserted in the output code. > > Bart. > |
|
From: Bart V. A. <bva...@ac...> - 2010-07-06 10:45:27
|
On Tue, Jul 6, 2010 at 8:57 AM, Konstantin Serebryany <kon...@gm...> wrote: > > On Mon, Jul 5, 2010 at 9:00 PM, Bart Van Assche <bva...@ac...> wrote: > > [ ... ] > > > > We can either try to agree upon one of the above three approaches or > > ask the libstdc++ authors for their opinion. > > Yep, they may have a better idea. > I'll send the bug report (with the link to > http://code.google.com/p/data-race-test/wiki/AtomicReferenceCounting) > tomorrow if nobody objects. Please copy the whole text from the wiki in the bug report. Only mentioning the link has the risk that the link gets broken in the future or that people get confused when the text on the wiki is updated after a discussion on the bug report has been started. Bart. |
|
From: Konstantin S. <kon...@gm...> - 2010-07-08 10:49:40
|
I changed the code which I want to suggest to libstdc++: http://code.google.com/p/data-race-test/wiki/AtomicReferenceCounting#Proposal_to_libstdc++_developers This will allow to use any kind of annotations (including Intel Parallel Inspector's itt_notify_sync_releasing / itt_notify_sync_acquired). Please have another look. --kcc On Tue, Jul 6, 2010 at 2:38 PM, Bart Van Assche <bva...@ac...> wrote: > On Tue, Jul 6, 2010 at 8:57 AM, Konstantin Serebryany > <kon...@gm...> wrote: >> >> On Mon, Jul 5, 2010 at 9:00 PM, Bart Van Assche <bva...@ac...> wrote: >> > [ ... ] >> > >> > We can either try to agree upon one of the above three approaches or >> > ask the libstdc++ authors for their opinion. >> >> Yep, they may have a better idea. >> I'll send the bug report (with the link to >> http://code.google.com/p/data-race-test/wiki/AtomicReferenceCounting) >> tomorrow if nobody objects. > > Please copy the whole text from the wiki in the bug report. Only > mentioning the link has the risk that the link gets broken in the > future or that people get confused when the text on the wiki is > updated after a discussion on the bug report has been started. > > Bart. > > -- > You received this message because you are subscribed to the Google Groups "data-race-test" group. > To post to this group, send email to dat...@go.... > To unsubscribe from this group, send email to dat...@go.... > For more options, visit this group at http://groups.google.com/group/data-race-test?hl=en. > > |
|
From: Bart V. A. <bva...@ac...> - 2010-07-08 15:08:53
|
On Thu, Jul 8, 2010 at 12:49 PM, Konstantin Serebryany <kon...@gm...> wrote: > I changed the code which I want to suggest to libstdc++: > http://code.google.com/p/data-race-test/wiki/AtomicReferenceCounting#Proposal_to_libstdc++_developers > This will allow to use any kind of annotations (including Intel > Parallel Inspector's itt_notify_sync_releasing / > itt_notify_sync_acquired). > Please have another look. A few minor comments: in the text cited below, please add the word "otherwise" at the beginning of the second line, replace "other" by "else" and "reference counter" by "reference counters". // Do not use this function for anything other than decrementing reference counter -- // this will confuse race detectors and will make them more conservative and slow. Bart. |
|
From: Konstantin S. <kon...@gm...> - 2010-07-09 11:48:29
|
Sent a message to libstdc++ folks: http://gcc.gnu.org/ml/libstdc++/2010-07/msg00029.html --kcc On Thu, Jul 8, 2010 at 7:08 PM, Bart Van Assche <bva...@ac...> wrote: > On Thu, Jul 8, 2010 at 12:49 PM, Konstantin Serebryany > <kon...@gm...> wrote: >> I changed the code which I want to suggest to libstdc++: >> http://code.google.com/p/data-race-test/wiki/AtomicReferenceCounting#Proposal_to_libstdc++_developers >> This will allow to use any kind of annotations (including Intel >> Parallel Inspector's itt_notify_sync_releasing / >> itt_notify_sync_acquired). >> Please have another look. > > A few minor comments: in the text cited below, please add the word > "otherwise" at the beginning of the second line, replace "other" by > "else" and "reference counter" by "reference counters". > > // Do not use this function for anything other than decrementing > reference counter -- > // this will confuse race detectors and will make them more > conservative and slow. > > Bart. > |
|
From: Konstantin S. <kon...@gm...> - 2010-07-14 12:29:28
|
FYI:
I've constructed a test case which produces a false report with all
three tools.
The test case uses shared_ptr from the soon-to-become C++ standard
(I've built it with the fresh gcc trunk).
With the annotations which we are discussing with libstdc++ folks
these reports should be gone.
--kcc
% cat shared_ptr_test.cc
//#define _GLIBCXX_SYNCHRONIZATION_HAPPENS_BEFORE(a) ANNOTATE_HAPPENS_BEFORE(a)
//#define _GLIBCXX_SYNCHRONIZATION_HAPPENS_AFTER(a) ANNOTATE_HAPPENS_AFTER(a)
//#include "dynamic_annotations.h"
#include <stdio.h>
#include <pthread.h>
#include <unistd.h>
#include <assert.h>
#include <memory>
using namespace std;
class Foo {
public:
Foo() : a_(777) { }
~Foo() {
a_ = 0xDEAD;
}
void Check() {
assert(a_ == 777);
}
private:
int a_;
};
shared_ptr<Foo> *s;
pthread_mutex_t mu;
pthread_cond_t cv;
int done = 0;
void *Thread(void*) {
shared_ptr<Foo> x(*s);
pthread_mutex_lock(&mu);
done++;
pthread_cond_signal(&cv);
pthread_mutex_unlock(&mu);
x->Check();
// x is destructed
}
const int kNThreads = 3;
int main() {
s = new shared_ptr<Foo>(new Foo);
pthread_t t[kNThreads];
pthread_mutex_init(&mu, 0);
pthread_cond_init(&cv, 0);
// start threads.
for (int i = 0; i < kNThreads; i++) {
pthread_create(&t[i], 0, Thread, 0);
}
// wait for threads to copy 's', but don't wait for threads to exit.
pthread_mutex_lock(&mu);
while (done != kNThreads)
pthread_cond_wait(&cv, &mu);
pthread_mutex_unlock(&mu);
delete s;
}
% g++ -g -std=c++0x -pthread shared_ptr_test.cc && echo
-----Helgrind---------; ~/valgrind/trunk/inst/bin/valgrind
--tool=helgrind ./a.out && echo --------------DRD----------- &&
~/valgrind/trunk/inst/bin/valgrind --tool=drd ./a.out
-----Helgrind---------
==7788== Helgrind, a thread error detector
==7788== Copyright (C) 2007-2009, and GNU GPL'd, by OpenWorks LLP et al.
==7788== Using Valgrind-3.6.0.SVN and LibVEX; rerun with -h for copyright info
==7788== Command: ./a.out
==7788==
==7788== Thread #1 is the program's root thread
==7788==
==7788== Thread #4 was created
==7788== at 0x58E06BE: clone (clone.S:77)
==7788== by 0x55E4172: pthread_create@@GLIBC_2.2.5 (createthread.c:75)
==7788== by 0x4C2C210: pthread_create_WRK (hg_intercepts.c:241)
==7788== by 0x4C2C2B9: pthread_create@* (hg_intercepts.c:268)
==7788== by 0x400E28: main (shared_ptr_test.cc:52)
==7788==
==7788== Possible data race during write of size 4 at 0x5b7d040 by thread #1
==7788== at 0x400ECE: Foo::~Foo() (shared_ptr_test.cc:16)
==7788== by 0x4013B7: std::_Sp_counted_ptr<Foo*,
(__gnu_cxx::_Lock_policy)2>::_M_dispose() (in /tmp/z/a.out)
==7788== by 0x401047:
std::_Sp_counted_base<(__gnu_cxx::_Lock_policy)2>::_M_release()
(boost_sp_counted_base.h:146)
==7788== by 0x400F8E:
std::__shared_count<(__gnu_cxx::_Lock_policy)2>::~__shared_count()
(shared_ptr_base.h:353)
==7788== by 0x400F25: std::__shared_ptr<Foo,
(__gnu_cxx::_Lock_policy)2>::~__shared_ptr() (shared_ptr_base.h:553)
==7788== by 0x400F3F: std::shared_ptr<Foo>::~shared_ptr() (shared_ptr.h:91)
==7788== by 0x400E80: main (shared_ptr_test.cc:59)
==7788== This conflicts with a previous read of size 4 by thread #4
==7788== at 0x400EE6: Foo::Check() (shared_ptr_test.cc:19)
==7788== by 0x400D89: Thread(void*) (shared_ptr_test.cc:39)
==7788== by 0x4C2C342: mythread_wrapper (hg_intercepts.c:213)
==7788== by 0x55E39C9: start_thread (pthread_create.c:300)
==7788== by 0x58E06FC: clone (clone.S:112)
==7788== Address 0x5b7d040 is 0 bytes inside a block of size 4 alloc'd
==7788== at 0x4C27ED5: operator new(unsigned long) (vg_replace_malloc.c:261)
==7788== by 0x400DAC: main (shared_ptr_test.cc:46)
==7788==
==7788==
==7788== For counts of detected and suppressed errors, rerun with: -v
==7788== Use --history-level=approx or =none to gain increased speed, at
==7788== the cost of reduced accuracy of conflicting-access information
==7788== ERROR SUMMARY: 1 errors from 1 contexts (suppressed: 24 from 15)
--------------DRD-----------
==7792== drd, a thread error detector
==7792== Copyright (C) 2006-2009, and GNU GPL'd, by Bart Van Assche.
==7792== Using Valgrind-3.6.0.SVN and LibVEX; rerun with -h for copyright info
==7792== Command: ./a.out
==7792==
==7792== Conflicting store by thread 1 at 0x05b86030 size 4
==7792== at 0x400ECE: Foo::~Foo() (shared_ptr_test.cc:16)
==7792== by 0x4013B7: std::_Sp_counted_ptr<Foo*,
(__gnu_cxx::_Lock_policy)2>::_M_dispose() (in /tmp/z/a.out)
==7792== by 0x401047:
std::_Sp_counted_base<(__gnu_cxx::_Lock_policy)2>::_M_release()
(boost_sp_counted_base.h:146)
==7792== by 0x400F8E:
std::__shared_count<(__gnu_cxx::_Lock_policy)2>::~__shared_count()
(shared_ptr_base.h:353)
==7792== by 0x400F25: std::__shared_ptr<Foo,
(__gnu_cxx::_Lock_policy)2>::~__shared_ptr() (shared_ptr_base.h:553)
==7792== by 0x400F3F: std::shared_ptr<Foo>::~shared_ptr() (shared_ptr.h:91)
==7792== by 0x400E80: main (shared_ptr_test.cc:59)
==7792== Address 0x5b86030 is at offset 0 from 0x5b86030. Allocation context:
==7792== at 0x4C29A05: operator new(unsigned long) (vg_replace_malloc.c:261)
==7792== by 0x400DAC: main (shared_ptr_test.cc:46)
==7792== Other segment start (thread 2)
==7792== (thread finished, call stack no longer available)
==7792== Other segment end (thread 2)
==7792== (thread finished, call stack no longer available)
==7792== Other segment start (thread 3)
==7792== (thread finished, call stack no longer available)
==7792== Other segment end (thread 3)
==7792== (thread finished, call stack no longer available)
==7792== Other segment start (thread 4)
==7792== at 0x4C2BD19: pthread_mutex_unlock (drd_pthread_intercepts.c:633)
==7792== by 0x400D75: Thread(void*) (shared_ptr_test.cc:37)
==7792== by 0x4C344C4: vgDrd_thread_wrapper (drd_pthread_intercepts.c:272)
==7792== by 0x55EC9C9: start_thread (pthread_create.c:300)
==7792== by 0x58E96FC: clone (clone.S:112)
==7792== Other segment end (thread 4)
==7792== at 0x58E5AE7: madvise (syscall-template.S:82)
==7792==
==7792==
==7792== For counts of detected and suppressed errors, rerun with: -v
==7792== ERROR SUMMARY: 1 errors from 1 contexts (suppressed: 16 from 15)
On Fri, Jul 9, 2010 at 3:48 PM, Konstantin Serebryany
<kon...@gm...> wrote:
> Sent a message to libstdc++ folks:
> http://gcc.gnu.org/ml/libstdc++/2010-07/msg00029.html
>
> --kcc
>
> On Thu, Jul 8, 2010 at 7:08 PM, Bart Van Assche <bva...@ac...> wrote:
>> On Thu, Jul 8, 2010 at 12:49 PM, Konstantin Serebryany
>> <kon...@gm...> wrote:
>>> I changed the code which I want to suggest to libstdc++:
>>> http://code.google.com/p/data-race-test/wiki/AtomicReferenceCounting#Proposal_to_libstdc++_developers
>>> This will allow to use any kind of annotations (including Intel
>>> Parallel Inspector's itt_notify_sync_releasing /
>>> itt_notify_sync_acquired).
>>> Please have another look.
>>
>> A few minor comments: in the text cited below, please add the word
>> "otherwise" at the beginning of the second line, replace "other" by
>> "else" and "reference counter" by "reference counters".
>>
>> // Do not use this function for anything other than decrementing
>> reference counter --
>> // this will confuse race detectors and will make them more
>> conservative and slow.
>>
>> Bart.
>>
>
|
|
From: Konstantin S. <kon...@gm...> - 2010-08-13 12:27:52
|
The annotations have been submitted to libstdc++ trunk and will appear in gcc 4.6. See also: http://gcc.gnu.org/bugzilla/show_bug.cgi?id=45276 --kcc On Wed, Jul 14, 2010 at 4:21 PM, Konstantin Serebryany <kon...@gm...> wrote: > FYI: > I've constructed a test case which produces a false report with all > three tools. > The test case uses shared_ptr from the soon-to-become C++ standard > (I've built it with the fresh gcc trunk). > With the annotations which we are discussing with libstdc++ folks > these reports should be gone. > > --kcc > > % cat shared_ptr_test.cc > //#define _GLIBCXX_SYNCHRONIZATION_HAPPENS_BEFORE(a) ANNOTATE_HAPPENS_BEFORE(a) > //#define _GLIBCXX_SYNCHRONIZATION_HAPPENS_AFTER(a) ANNOTATE_HAPPENS_AFTER(a) > //#include "dynamic_annotations.h" > > #include <stdio.h> > #include <pthread.h> > #include <unistd.h> > #include <assert.h> > #include <memory> > using namespace std; > > class Foo { > public: > Foo() : a_(777) { } > ~Foo() { > a_ = 0xDEAD; > } > void Check() { > assert(a_ == 777); > } > private: > int a_; > }; > > shared_ptr<Foo> *s; > > pthread_mutex_t mu; > pthread_cond_t cv; > int done = 0; > > void *Thread(void*) { > shared_ptr<Foo> x(*s); > > pthread_mutex_lock(&mu); > done++; > pthread_cond_signal(&cv); > pthread_mutex_unlock(&mu); > > x->Check(); > // x is destructed > } > > const int kNThreads = 3; > > int main() { > s = new shared_ptr<Foo>(new Foo); > pthread_t t[kNThreads]; > pthread_mutex_init(&mu, 0); > pthread_cond_init(&cv, 0); > // start threads. > for (int i = 0; i < kNThreads; i++) { > pthread_create(&t[i], 0, Thread, 0); > } > // wait for threads to copy 's', but don't wait for threads to exit. > pthread_mutex_lock(&mu); > while (done != kNThreads) > pthread_cond_wait(&cv, &mu); > pthread_mutex_unlock(&mu); > delete s; > } > > > % g++ -g -std=c++0x -pthread shared_ptr_test.cc && echo > -----Helgrind---------; ~/valgrind/trunk/inst/bin/valgrind > --tool=helgrind ./a.out && echo --------------DRD----------- && > ~/valgrind/trunk/inst/bin/valgrind --tool=drd ./a.out > -----Helgrind--------- > ==7788== Helgrind, a thread error detector > ==7788== Copyright (C) 2007-2009, and GNU GPL'd, by OpenWorks LLP et al. > ==7788== Using Valgrind-3.6.0.SVN and LibVEX; rerun with -h for copyright info > ==7788== Command: ./a.out > ==7788== > ==7788== Thread #1 is the program's root thread > ==7788== > ==7788== Thread #4 was created > ==7788== at 0x58E06BE: clone (clone.S:77) > ==7788== by 0x55E4172: pthread_create@@GLIBC_2.2.5 (createthread.c:75) > ==7788== by 0x4C2C210: pthread_create_WRK (hg_intercepts.c:241) > ==7788== by 0x4C2C2B9: pthread_create@* (hg_intercepts.c:268) > ==7788== by 0x400E28: main (shared_ptr_test.cc:52) > ==7788== > ==7788== Possible data race during write of size 4 at 0x5b7d040 by thread #1 > ==7788== at 0x400ECE: Foo::~Foo() (shared_ptr_test.cc:16) > ==7788== by 0x4013B7: std::_Sp_counted_ptr<Foo*, > (__gnu_cxx::_Lock_policy)2>::_M_dispose() (in /tmp/z/a.out) > ==7788== by 0x401047: > std::_Sp_counted_base<(__gnu_cxx::_Lock_policy)2>::_M_release() > (boost_sp_counted_base.h:146) > ==7788== by 0x400F8E: > std::__shared_count<(__gnu_cxx::_Lock_policy)2>::~__shared_count() > (shared_ptr_base.h:353) > ==7788== by 0x400F25: std::__shared_ptr<Foo, > (__gnu_cxx::_Lock_policy)2>::~__shared_ptr() (shared_ptr_base.h:553) > ==7788== by 0x400F3F: std::shared_ptr<Foo>::~shared_ptr() (shared_ptr.h:91) > ==7788== by 0x400E80: main (shared_ptr_test.cc:59) > ==7788== This conflicts with a previous read of size 4 by thread #4 > ==7788== at 0x400EE6: Foo::Check() (shared_ptr_test.cc:19) > ==7788== by 0x400D89: Thread(void*) (shared_ptr_test.cc:39) > ==7788== by 0x4C2C342: mythread_wrapper (hg_intercepts.c:213) > ==7788== by 0x55E39C9: start_thread (pthread_create.c:300) > ==7788== by 0x58E06FC: clone (clone.S:112) > ==7788== Address 0x5b7d040 is 0 bytes inside a block of size 4 alloc'd > ==7788== at 0x4C27ED5: operator new(unsigned long) (vg_replace_malloc.c:261) > ==7788== by 0x400DAC: main (shared_ptr_test.cc:46) > ==7788== > ==7788== > ==7788== For counts of detected and suppressed errors, rerun with: -v > ==7788== Use --history-level=approx or =none to gain increased speed, at > ==7788== the cost of reduced accuracy of conflicting-access information > ==7788== ERROR SUMMARY: 1 errors from 1 contexts (suppressed: 24 from 15) > --------------DRD----------- > ==7792== drd, a thread error detector > ==7792== Copyright (C) 2006-2009, and GNU GPL'd, by Bart Van Assche. > ==7792== Using Valgrind-3.6.0.SVN and LibVEX; rerun with -h for copyright info > ==7792== Command: ./a.out > ==7792== > ==7792== Conflicting store by thread 1 at 0x05b86030 size 4 > ==7792== at 0x400ECE: Foo::~Foo() (shared_ptr_test.cc:16) > ==7792== by 0x4013B7: std::_Sp_counted_ptr<Foo*, > (__gnu_cxx::_Lock_policy)2>::_M_dispose() (in /tmp/z/a.out) > ==7792== by 0x401047: > std::_Sp_counted_base<(__gnu_cxx::_Lock_policy)2>::_M_release() > (boost_sp_counted_base.h:146) > ==7792== by 0x400F8E: > std::__shared_count<(__gnu_cxx::_Lock_policy)2>::~__shared_count() > (shared_ptr_base.h:353) > ==7792== by 0x400F25: std::__shared_ptr<Foo, > (__gnu_cxx::_Lock_policy)2>::~__shared_ptr() (shared_ptr_base.h:553) > ==7792== by 0x400F3F: std::shared_ptr<Foo>::~shared_ptr() (shared_ptr.h:91) > ==7792== by 0x400E80: main (shared_ptr_test.cc:59) > ==7792== Address 0x5b86030 is at offset 0 from 0x5b86030. Allocation context: > ==7792== at 0x4C29A05: operator new(unsigned long) (vg_replace_malloc.c:261) > ==7792== by 0x400DAC: main (shared_ptr_test.cc:46) > ==7792== Other segment start (thread 2) > ==7792== (thread finished, call stack no longer available) > ==7792== Other segment end (thread 2) > ==7792== (thread finished, call stack no longer available) > ==7792== Other segment start (thread 3) > ==7792== (thread finished, call stack no longer available) > ==7792== Other segment end (thread 3) > ==7792== (thread finished, call stack no longer available) > ==7792== Other segment start (thread 4) > ==7792== at 0x4C2BD19: pthread_mutex_unlock (drd_pthread_intercepts.c:633) > ==7792== by 0x400D75: Thread(void*) (shared_ptr_test.cc:37) > ==7792== by 0x4C344C4: vgDrd_thread_wrapper (drd_pthread_intercepts.c:272) > ==7792== by 0x55EC9C9: start_thread (pthread_create.c:300) > ==7792== by 0x58E96FC: clone (clone.S:112) > ==7792== Other segment end (thread 4) > ==7792== at 0x58E5AE7: madvise (syscall-template.S:82) > ==7792== > ==7792== > ==7792== For counts of detected and suppressed errors, rerun with: -v > ==7792== ERROR SUMMARY: 1 errors from 1 contexts (suppressed: 16 from 15) > > > > > > > On Fri, Jul 9, 2010 at 3:48 PM, Konstantin Serebryany > <kon...@gm...> wrote: >> Sent a message to libstdc++ folks: >> http://gcc.gnu.org/ml/libstdc++/2010-07/msg00029.html >> >> --kcc >> >> On Thu, Jul 8, 2010 at 7:08 PM, Bart Van Assche <bva...@ac...> wrote: >>> On Thu, Jul 8, 2010 at 12:49 PM, Konstantin Serebryany >>> <kon...@gm...> wrote: >>>> I changed the code which I want to suggest to libstdc++: >>>> http://code.google.com/p/data-race-test/wiki/AtomicReferenceCounting#Proposal_to_libstdc++_developers >>>> This will allow to use any kind of annotations (including Intel >>>> Parallel Inspector's itt_notify_sync_releasing / >>>> itt_notify_sync_acquired). >>>> Please have another look. >>> >>> A few minor comments: in the text cited below, please add the word >>> "otherwise" at the beginning of the second line, replace "other" by >>> "else" and "reference counter" by "reference counters". >>> >>> // Do not use this function for anything other than decrementing >>> reference counter -- >>> // this will confuse race detectors and will make them more >>> conservative and slow. >>> >>> Bart. >>> >> > |