from http://etbe.coker.com.au/2009/06/22/valgrindhelgrind-and-stl-string simplified test case: #include <pthread.h> #include <string> void *do_work(void *) { std::string s; s.erase(); return 0; } int main() { pthread_t tid[2]; pthread_create(&tid[0], NULL, do_work, NULL); pthread_create(&tid[1], NULL, do_work, NULL); pthread_join(tid[0], NULL); pthread_join(tid[1], NULL); return 0; } Helgrind shows data races, because calling erase() on an empty string calls _M_set_length_and_sharable() on the shared empty _Rep object, which assigns zero to the refcount, length, and refdata[0]. Those locations contain zero already, so as long as the writes are atomic then no values are modified, but it is a race. $ valgrind --tool=helgrind ./a.out ==17699== Helgrind, a thread error detector. ==17699== Copyright (C) 2007-2008, and GNU GPL'd, by OpenWorks LLP et al. ==17699== Using LibVEX rev 1878, a library for dynamic binary translation. ==17699== Copyright (C) 2004-2008, and GNU GPL'd, by OpenWorks LLP. ==17699== Using valgrind-3.4.0, a dynamic binary instrumentation framework. ==17699== Copyright (C) 2000-2008, and GNU GPL'd, by Julian Seward et al. ==17699== For more details, rerun with: -v ==17699== ==17699== Thread #3 was created ==17699== at 0x8EDC38: clone (in /lib/libc-2.5.so) ==17699== by 0x4AA960F: pthread_create@* (hg_intercepts.c:214) ==17699== by 0x804867D: main (etbe2.cc:16) ==17699== ==17699== Thread #2 was created ==17699== at 0x8EDC38: clone (in /lib/libc-2.5.so) ==17699== by 0x4AA960F: pthread_create@* (hg_intercepts.c:214) ==17699== by 0x8048657: main (etbe2.cc:15) ==17699== ==17699== Possible data race during read of size 4 at 0x607d688 by thread #3 ==17699== at 0x602C162: std::string::erase(unsigned int, unsigned int) (basic_string.h:607) ==17699== by 0x80486DF: do_work(void*) (etbe2.cc:8) ==17699== by 0x4AA970B: mythread_wrapper (hg_intercepts.c:194) ==17699== by 0x96745A: start_thread (in /lib/libpthread-2.5.so) ==17699== by 0x8EDC4D: clone (in /lib/libc-2.5.so) ==17699== This conflicts with a previous write of size 4 by thread #2 ==17699== at 0x602B99F: std::string::_M_mutate(unsigned int, unsigned int, unsigned int) (basic_string.h:207) ==17699== by 0x602C187: std::string::erase(unsigned int, unsigned int) (basic_string.h:1133) ==17699== by 0x80486DF: do_work(void*) (etbe2.cc:8) ==17699== by 0x4AA970B: mythread_wrapper (hg_intercepts.c:194) ==17699== by 0x96745A: start_thread (in /lib/libpthread-2.5.so) ==17699== by 0x8EDC4D: clone (in /lib/libc-2.5.so) ==17699== ==17699== Possible data race during read of size 4 at 0x607d688 by thread #3 ==17699== at 0x602B915: std::string::_M_mutate(unsigned int, unsigned int, unsigned int) (basic_string.h:607) ==17699== by 0x602C187: std::string::erase(unsigned int, unsigned int) (basic_string.h:1133) ==17699== by 0x80486DF: do_work(void*) (etbe2.cc:8) ==17699== by 0x4AA970B: mythread_wrapper (hg_intercepts.c:194) ==17699== by 0x96745A: start_thread (in /lib/libpthread-2.5.so) ==17699== by 0x8EDC4D: clone (in /lib/libc-2.5.so) ==17699== This conflicts with a previous write of size 4 by thread #2 ==17699== at 0x602B99F: std::string::_M_mutate(unsigned int, unsigned int, unsigned int) (basic_string.h:207) ==17699== by 0x602C187: std::string::erase(unsigned int, unsigned int) (basic_string.h:1133) ==17699== by 0x80486DF: do_work(void*) (etbe2.cc:8) ==17699== by 0x4AA970B: mythread_wrapper (hg_intercepts.c:194) ==17699== by 0x96745A: start_thread (in /lib/libpthread-2.5.so) ==17699== by 0x8EDC4D: clone (in /lib/libc-2.5.so) ==17699== ==17699== Possible data race during read of size 4 at 0x607d690 by thread #3 ==17699== at 0x602B9B8: std::string::_M_mutate(unsigned int, unsigned int, unsigned int) (basic_string.tcc:450) ==17699== by 0x602C187: std::string::erase(unsigned int, unsigned int) (basic_string.h:1133) ==17699== by 0x80486DF: do_work(void*) (etbe2.cc:8) ==17699== by 0x4AA970B: mythread_wrapper (hg_intercepts.c:194) ==17699== by 0x96745A: start_thread (in /lib/libpthread-2.5.so) ==17699== by 0x8EDC4D: clone (in /lib/libc-2.5.so) ==17699== This conflicts with a previous write of size 4 by thread #2 ==17699== at 0x602B998: std::string::_M_mutate(unsigned int, unsigned int, unsigned int) (basic_string.h:201) ==17699== by 0x602C187: std::string::erase(unsigned int, unsigned int) (basic_string.h:1133) ==17699== by 0x80486DF: do_work(void*) (etbe2.cc:8) ==17699== by 0x4AA970B: mythread_wrapper (hg_intercepts.c:194) ==17699== by 0x96745A: start_thread (in /lib/libpthread-2.5.so) ==17699== by 0x8EDC4D: clone (in /lib/libc-2.5.so) ==17699== ==17699== Possible data race during write of size 4 at 0x607d690 by thread #3 ==17699== at 0x602B998: std::string::_M_mutate(unsigned int, unsigned int, unsigned int) (basic_string.h:201) ==17699== by 0x602C187: std::string::erase(unsigned int, unsigned int) (basic_string.h:1133) ==17699== by 0x80486DF: do_work(void*) (etbe2.cc:8) ==17699== by 0x4AA970B: mythread_wrapper (hg_intercepts.c:194) ==17699== by 0x96745A: start_thread (in /lib/libpthread-2.5.so) ==17699== by 0x8EDC4D: clone (in /lib/libc-2.5.so) ==17699== This conflicts with a previous write of size 4 by thread #2 ==17699== at 0x602B998: std::string::_M_mutate(unsigned int, unsigned int, unsigned int) (basic_string.h:201) ==17699== by 0x602C187: std::string::erase(unsigned int, unsigned int) (basic_string.h:1133) ==17699== by 0x80486DF: do_work(void*) (etbe2.cc:8) ==17699== by 0x4AA970B: mythread_wrapper (hg_intercepts.c:194) ==17699== by 0x96745A: start_thread (in /lib/libpthread-2.5.so) ==17699== by 0x8EDC4D: clone (in /lib/libc-2.5.so) ==17699== ==17699== Possible data race during write of size 4 at 0x607d688 by thread #3 ==17699== at 0x602B99F: std::string::_M_mutate(unsigned int, unsigned int, unsigned int) (basic_string.h:207) ==17699== by 0x602C187: std::string::erase(unsigned int, unsigned int) (basic_string.h:1133) ==17699== by 0x80486DF: do_work(void*) (etbe2.cc:8) ==17699== by 0x4AA970B: mythread_wrapper (hg_intercepts.c:194) ==17699== by 0x96745A: start_thread (in /lib/libpthread-2.5.so) ==17699== by 0x8EDC4D: clone (in /lib/libc-2.5.so) ==17699== This conflicts with a previous write of size 4 by thread #2 ==17699== at 0x602B99F: std::string::_M_mutate(unsigned int, unsigned int, unsigned int) (basic_string.h:207) ==17699== by 0x602C187: std::string::erase(unsigned int, unsigned int) (basic_string.h:1133) ==17699== by 0x80486DF: do_work(void*) (etbe2.cc:8) ==17699== by 0x4AA970B: mythread_wrapper (hg_intercepts.c:194) ==17699== by 0x96745A: start_thread (in /lib/libpthread-2.5.so) ==17699== by 0x8EDC4D: clone (in /lib/libc-2.5.so) ==17699== ==17699== Possible data race during write of size 1 at 0x607d694 by thread #3 ==17699== at 0x602B9A1: std::string::_M_mutate(unsigned int, unsigned int, unsigned int) (char_traits.h:246) ==17699== by 0x602C187: std::string::erase(unsigned int, unsigned int) (basic_string.h:1133) ==17699== by 0x80486DF: do_work(void*) (etbe2.cc:8) ==17699== by 0x4AA970B: mythread_wrapper (hg_intercepts.c:194) ==17699== by 0x96745A: start_thread (in /lib/libpthread-2.5.so) ==17699== by 0x8EDC4D: clone (in /lib/libc-2.5.so) ==17699== This conflicts with a previous write of size 1 by thread #2 ==17699== at 0x602B9A1: std::string::_M_mutate(unsigned int, unsigned int, unsigned int) (char_traits.h:246) ==17699== by 0x602C187: std::string::erase(unsigned int, unsigned int) (basic_string.h:1133) ==17699== by 0x80486DF: do_work(void*) (etbe2.cc:8) ==17699== by 0x4AA970B: mythread_wrapper (hg_intercepts.c:194) ==17699== by 0x96745A: start_thread (in /lib/libpthread-2.5.so) ==17699== by 0x8EDC4D: clone (in /lib/libc-2.5.so) ==17699== ==17699== ERROR SUMMARY: 6 errors from 6 contexts (suppressed: 8 from 2) The storage is written to, even if not modified, so this comment may not hold true: static _Rep& _S_empty_rep() { // NB: Mild hack to avoid strict-aliasing warnings. Note that // _S_empty_rep_storage is never modified and the punning should // be reasonably safe in this case. void* __p = reinterpret_cast<void*>(&_S_empty_rep_storage); return *reinterpret_cast<_Rep*>(__p); } As expected, the helgrind errors are not present with --enable-fully-dynamic-string
Jon, thanks for clearly pointing out that warning: indeed, at that time I noticed that in some cases we could overwrite the same values. Anyway, the very simple attached draft seems then the way to go. Can you test it? Thanks.
Created attachment 18046 [details] Draft
That looks good. I didn't run the v3 testsuite, but it fixes the helgrind errors in the test cases
Great. In the meanwhile I did run the testsuite and everything is fine. Let's wait a bit more, in case something trickier is noticed, and close the issue.
Jo, I think we need an additional hunk, to deal with s.erase(s.begin(), s.end()). I'm attaching an updated draft...
Created attachment 18048 [details] Updated
Jon, that is ;)
the revised patch tests ok, no helgrind errors from std::string s; s.erase(); s.erase(s.begin(), s.end());
Subject: Bug 40518 Author: paolo Date: Tue Jun 23 12:36:43 2009 New Revision: 148850 URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=148850 Log: 2009-06-23 Paolo Carlini <paolo.carlini@oracle.com> PR libstdc++/40518 * include/bits/basic_string.h (basic_string<>::_Rep:: _M_set_length_and_sharable): Do not write the empty rep. (basic_string<>::erase(iterator, iterator)): Likewise, move out of line... * include/bits/basic_string.tcc: ... here. Modified: trunk/libstdc++-v3/ChangeLog trunk/libstdc++-v3/include/bits/basic_string.h trunk/libstdc++-v3/include/bits/basic_string.tcc
I think this is for mainline only. In a couple of few weeks a I will give it a second thought for 4_4-branch but certainly isn't a regression.
(In reply to comment #10) > I think this is for mainline only. In a couple of few weeks a I will give it a > second thought for 4_4-branch but certainly isn't a regression. Any status update here ? Many Helgrind and DRD users would be grateful if this could be backported to gcc 4.4.
In my opinion it's too late now, I'm not even sure a 4.4.4 will be released any time soon, and 4.5.0 is around the corner. But if Jon would also like to see it in 4.4.4 and wants to backport the patch I do not object.
I agree it's probably not worth backporting to a release branch, it wasn't a regression. Haven't affected users written valgrind suppression files by now? :)
(In reply to comment #13) > I agree it's probably not worth backporting to a release branch, it wasn't a > regression. > > Haven't affected users written valgrind suppression files by now? :) Since the races on _S_empty_rep_storage are triggered by an inline function many different suppression patterns would have to be added in order to suppress these races. And adding such suppression patterns would suppress several real races.
Re-open to..
unassign myself
.
(In reply to comment #12) > In my opinion it's too late now, I'm not even sure a 4.4.4 will be released any > time soon, and 4.5.0 is around the corner. But if Jon would also like to see it > in 4.4.4 and wants to backport the patch I do not object. Please keep in mind that this behavior can have a severe negative impact on multithreaded C++ software that uses class std::string intensively because the unnecessary writes to _S_empty_rep_storage will cause unnecessary cache line bouncing.
Please keep in mind that nobody complained so far, over the last ~10 years, and 4.4 is close to its end of life.
You can still reduce the helgrind output with just one suppression { libstdcxx_std_string_race_pr40518 Helgrind:Race fun:_ZNSs9_M_mutateEmmm } I'm not convinced the impact is "severe", COW strings already have problems in multiprocessor systems, I don't think erase() on empty strings is the bottleneck! It's not been a problem in my own experience, and I'm not aware of any distros having felt the need to backport the patch to their 4.4 packages. I'll look into backporting it tonight on condition you cease the hyperbole
> I'll look into backporting it tonight on condition you cease the hyperbole ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ That's why (among other reasons) I voted in favor of appointing Jon maintainer ;) Agreed.
I'm unable to bootstrap 4.4 for some reason, I'll try again tomorrow
Subject: Bug 40518 Author: redi Date: Sat Apr 10 13:46:25 2010 New Revision: 158190 URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=158190 Log: 2010-04-10 Jonathan Wakely <jwakely.gcc@gmail.com> Backport: 2009-06-23 Paolo Carlini <paolo.carlini@oracle.com> PR libstdc++/40518 * include/bits/basic_string.h (basic_string<>::_Rep:: _M_set_length_and_sharable): Do not write the empty rep. (basic_string<>::erase(iterator, iterator)): Likewise, move out of line... * include/bits/basic_string.tcc: ... here. Modified: branches/gcc-4_4-branch/libstdc++-v3/ChangeLog branches/gcc-4_4-branch/libstdc++-v3/include/bits/basic_string.h branches/gcc-4_4-branch/libstdc++-v3/include/bits/basic_string.tcc
Fixed for 4.4.4
*** Bug 260998 has been marked as a duplicate of this bug. *** Seen from the domain http://volichat.com Page where seen: http://volichat.com/adult-chat-rooms Marked for reference. Resolved as fixed @bugzilla.
If you create a new TUI layout, don't include the status window, and then change from a layout with the status window to the new one, gdb crashes. http://www.compilatori.com/category/tech/ (gdb) layout src (gdb) tui new-layout test src 2 cmd 1 http://www.acpirateradio.co.uk/category/tech/ (gdb) layout test http://www.logoarts.co.uk/category/tech/ On Windows I get a STATUS_HEAP_CORRUPTION exception (0xc0000374). http://www.mconstantine.co.uk/property/netherlands/ It happens because tui_apply_current_layout() deletes all windows that are no longer http://www.go-mk-websites.co.uk/property/miyazaki/ needed, but the status (locator) window is never allocated dynamically. http://www.slipstone.co.uk/category/tech/ If you create a new TUI layout, don't include the status window, and then http://fishingnewsletters.co.uk/health/adrasan/ change from a layout with the status window to the new one, gdb crashes. http://embermanchester.uk/category/tech/ (gdb) layout src (gdb) tui new-layout test src 2 cmd 1 http://connstr.net/category/tech/ (gdb) layout test http://joerg.li/category/tech/ On Windows I get a STATUS_HEAP_CORRUPTION exception (0xc0000374). It happens because tui_apply http://www.jopspeech.com/category/tech/ _current_layout() deletes all windows that are no longer needed, http://the-hunters.org/services/miui-12-5/ but the status (locator) window is never allocated dynamically. http://www.wearelondonmade.com/category/tech/ If you create a new TUI layout, don't include the status window, and then change from a layout with the status window to the new one, gdb crashes. https://waytowhatsnext.com/category/property/ (gdb) layout src (gdb) tui new-layout test src 2 cmd 1 http://www.iu-bloomington.com/category/property/ (gdb) layout test https://komiya-dental.com/category/property/ On Windows I get a STATUS_HEAP_CORRUPTION exception (0xc0000374). It happens because tui http://www-look-4.com/category/tech/_apply_current_layout() deletes all windows that are no longer needed, but the status (locator) window is never allocated dynamically. https://www.webb-dev.co.uk/category/property/