From patchwork Tue Sep 1 12:51:35 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Dmitry Vyukov X-Patchwork-Id: 512809 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from sourceware.org (server1.sourceware.org [209.132.180.131]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id D5F03140761 for ; Tue, 1 Sep 2015 22:52:08 +1000 (AEST) Authentication-Results: ozlabs.org; dkim=pass (1024-bit key; unprotected) header.d=gcc.gnu.org header.i=@gcc.gnu.org header.b=SDoLLMhB; dkim-atps=neutral DomainKey-Signature: a=rsa-sha1; c=nofws; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender :mime-version:from:date:message-id:subject:to:cc:content-type; q=dns; s=default; b=hbAjPxJzVfwWUgzLPKEJ5SbRLfdAvzqQ5D/wLVz+Rjv Dalq4hc8jXKaXmyV7LMrBwgcNqcVwkHWhzYfmNQxi9kuJxhTUJUZSd8zwPw8JkWf 3klGSzUfAFONcxlFPo5YKPRChyYoIrciXnQGyr/KnnbcKAthAeK7gqIUqy7vQcW8 = DKIM-Signature: v=1; a=rsa-sha1; c=relaxed; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender :mime-version:from:date:message-id:subject:to:cc:content-type; s=default; bh=pN784Rpomh3Px1jLStinSQmZ6D8=; b=SDoLLMhB9qMuG1GUV Qbx3cp8IusHHO4vjOYIfeYoi5Q4IwHVhs0F8Q2zj1GsiAXHRTiQuBInw8Ngk7cya D04T5NRGsKWWoFEUATCjszSiHuo984rOPrFy1Ue4A7IOnxZd0AqRHgIoG1+t3OqW hf9gMJ1N5/nPQO+zXP6yieP4P0= Received: (qmail 23384 invoked by alias); 1 Sep 2015 12:52:01 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Delivered-To: mailing list gcc-patches@gcc.gnu.org Received: (qmail 23354 invoked by uid 89); 1 Sep 2015 12:51:59 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-0.8 required=5.0 tests=AWL, BAYES_20, KAM_ASCII_DIVIDERS, RCVD_IN_DNSWL_LOW, SPF_PASS, T_RP_MATCHES_RCVD autolearn=ham version=3.3.2 X-HELO: mail-wi0-f179.google.com Received: from mail-wi0-f179.google.com (HELO mail-wi0-f179.google.com) (209.85.212.179) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES128-GCM-SHA256 encrypted) ESMTPS; Tue, 01 Sep 2015 12:51:58 +0000 Received: by wicmc4 with SMTP id mc4so31978549wic.0 for ; Tue, 01 Sep 2015 05:51:55 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:mime-version:from:date:message-id:subject:to:cc :content-type; bh=6OPMq4EAOVFsODLQ5ycpWFCEBxrvkPSQ5n/ydeTzxrA=; b=SgSOZGBgvkygB3Z06oZNVPpUBzbu5d6u1X/PH3v+GYWsXKSDst2oudtokkzRXqcSw6 jpya+V3F/+AzNujkQKgseGoDHPszCZBn8jzFG4rd3zco83HAJ1gRye48/WZoCM3GrDp9 GMVn5KNM+8PedcCIpkkfIVyvR1HQkTXdmW73+5o36MjrcmaSfByAp6qHwvj6c+nTAFJO GlX1BJgtaVCh9LFyc7a/yejYdJwF9GKl3Vg3K/pAMNrBca4h3rFsGyAga7qV0VuSW2DM XcxNW6dmnrqKs7/79FFJH2BmSSuPBKoHB40JOyFoqyR8HHztI+TRZ7YJojzRbWKMNa0c i+Lw== X-Gm-Message-State: ALoCoQnrFfmHJxQMDjZTraUkzsQqH2/U+EZlW6LW7wtmG/uWLlvwRAyQ+exhMT3OaJdRK5BWWfwI X-Received: by 10.180.206.176 with SMTP id lp16mr3154163wic.85.1441111914979; Tue, 01 Sep 2015 05:51:54 -0700 (PDT) MIME-Version: 1.0 Received: by 10.28.64.193 with HTTP; Tue, 1 Sep 2015 05:51:35 -0700 (PDT) From: Dmitry Vyukov Date: Tue, 1 Sep 2015 14:51:35 +0200 Message-ID: Subject: [Patch, libstdc++] Fix data races in basic_string implementation To: GCC Patches , libstdc++@gcc.gnu.org Cc: Alexander Potapenko , Kostya Serebryany X-IsSubscribed: yes Hello, The refcounted basic_string implementation contains several data races on _M_refcount: 1. _M_is_leaked loads _M_refcount concurrently with mutations of _M_refcount. This loads needs to be memory_order_relaxed load, as _M_is_leaked predicate does not change under the feet. 2. _M_is_shared loads _M_refcount concurrently with mutations of _M_refcount. This loads needs to be memory_order_acquire, as another thread can drop _M_refcount to zero concurrently which makes the string non-shared and so the current thread can mutate the string. We need reads of the string in another thread (while it was shared) to happen-before the writes to the string in this thread (now that it is non-shared). This patch adds __gnu_cxx::__atomic_load_dispatch function to do the loads of _M_refcount. The function does an acquire load. Acquire is non needed for _M_is_leaked, but for simplicity as still do acquire (hopefully the refcounted impl will go away in future). This patch also uses the new function to do loads of _M_refcount in string implementation. I did non update doc/xml/manual/concurrency_extensions.xml to document __gnu_cxx::__atomic_load_dispatch, because I am not sure we want to extend that api now that we have language-level atomics. If you still want me to update it, please say how to regenerate doc/html/manual/ext_concurrency.html. The race was detected with ThreadSanitizer on the following program: #define _GLIBCXX_USE_CXX11_ABI 0 #include #include #include #include int main() { std::string s = "foo"; std::thread t([=](){ std::string x = s; std::cout << &x << std::endl; }); std::this_thread::sleep_for(std::chrono::seconds(1)); std::cout << &s[0] << std::endl; t.join(); } $ g++ string.cc -std=c++11 -pthread -O1 -g -fsanitize=thread $ ./a.out WARNING: ThreadSanitizer: data race (pid=98135) Read of size 4 at 0x7d080000efd0 by main thread: #0 std::string::_Rep::_M_is_leaked() const include/c++/6.0.0/bits/basic_string.h:2605 (a.out+0x000000401d7c) #1 std::string::_M_leak() include/c++/6.0.0/bits/basic_string.h:2730 (a.out+0x000000401d7c) #2 std::string::operator[](unsigned long) include/c++/6.0.0/bits/basic_string.h:3274 (a.out+0x000000401d7c) #3 main /tmp/string.cc:14 (a.out+0x000000401d7c) Previous atomic write of size 4 at 0x7d080000efd0 by thread T1: #0 __tsan_atomic32_fetch_add ../../../../libsanitizer/tsan/tsan_interface_atomic.cc:611 (libtsan.so.0+0x00000005811f) #1 __exchange_and_add include/c++/6.0.0/ext/atomicity.h:59 (a.out+0x000000401a19) #2 __exchange_and_add_dispatch include/c++/6.0.0/ext/atomicity.h:92 (a.out+0x000000401a19) #3 std::string::_Rep::_M_dispose(std::allocator const&) include/c++/6.0.0/bits/basic_string.h:2659 (a.out+0x000000401a19) #4 std::basic_string, std::allocator >::~basic_string() include/c++/6.0.0/bits/basic_string.h:2961 (a.out+0x000000401a19) #5 ~ /tmp/string.cc:9 (a.out+0x000000401a19) #6 ~_Head_base include/c++/6.0.0/tuple:102 (a.out+0x000000401a19) #7 ~_Tuple_impl include/c++/6.0.0/tuple:338 (a.out+0x000000401a19) #8 ~tuple include/c++/6.0.0/tuple:521 (a.out+0x000000401a19) #9 ~_Bind_simple include/c++/6.0.0/functional:1503 (a.out+0x000000401a19) #10 ~_Impl include/c++/6.0.0/thread:107 (a.out+0x000000401a19) #11 destroy()> > > include/c++/6.0.0/ext/new_allocator.h:124 (a.out+0x0000004015c7) #12 _S_destroy()> > >, std::thread::_Impl()> > > include/c++/6.0.0/bits/alloc_traits.h:236 (a.out+0x0000004015c7) #13 destroy()> > > include/c++/6.0.0/bits/alloc_traits.h:336 (a.out+0x0000004015c7) #14 _M_dispose include/c++/6.0.0/bits/shared_ptr_base.h:529 (a.out+0x0000004015c7) #15 std::_Sp_counted_base<(__gnu_cxx::_Lock_policy)2>::_M_release() /usr/local/google/home/dvyukov/src/gcc/build/x86_64-pc-linux-gnu/libstdc++-v3/include/bits/shared_ptr_base.h:150 (libstdc++.so.6+0x0000000b6da4) #16 std::__shared_count<(__gnu_cxx::_Lock_policy)2>::~__shared_count() /usr/local/google/home/dvyukov/src/gcc/build/x86_64-pc-linux-gnu/libstdc++-v3/include/bits/shared_ptr_base.h:662 (libstdc++.so.6+0x0000000b6da4) #17 std::__shared_ptr::~__shared_ptr() /usr/local/google/home/dvyukov/src/gcc/build/x86_64-pc-linux-gnu/libstdc++-v3/include/bits/shared_ptr_base.h:928 (libstdc++.so.6+0x0000000b6da4) #18 std::shared_ptr::~shared_ptr() /usr/local/google/home/dvyukov/src/gcc/build/x86_64-pc-linux-gnu/libstdc++-v3/include/bits/shared_ptr.h:93 (libstdc++.so.6+0x0000000b6da4) #19 execute_native_thread_routine libstdc++-v3/src/c++11/thread.cc:79 (libstdc++.so.6+0x0000000b6da4) Location is heap block of size 28 at 0x7d080000efc0 allocated by main thread: #0 operator new(unsigned long) ../../../../libsanitizer/tsan/tsan_interceptors.cc:571 (libtsan.so.0+0x0000000255a3) #1 __gnu_cxx::new_allocator::allocate(unsigned long, void const*) /usr/local/google/home/dvyukov/src/gcc/build/x86_64-pc-linux-gnu/libstdc++-v3/include/ext/new_allocator.h:104 (libstdc++.so.6+0x0000000cbaa8) #2 std::string::_Rep::_S_create(unsigned long, unsigned long, std::allocator const&) /usr/local/google/home/dvyukov/src/gcc/build/x86_64-pc-linux-gnu/libstdc++-v3/include/bits/basic_string.tcc:1051 (libstdc++.so.6+0x0000000cbaa8) #3 __libc_start_main (libc.so.6+0x000000021ec4) Thread T1 (tid=98137, finished) created by main thread at: #0 pthread_create ../../../../libsanitizer/tsan/tsan_interceptors.cc:895 (libtsan.so.0+0x000000026d04) #1 __gthread_create /usr/local/google/home/dvyukov/src/gcc/build/x86_64-pc-linux-gnu/libstdc++-v3/include/x86_64-pc-linux-gnu/bits/gthr-default.h:662 (libstdc++.so.6+0x0000000b6e52) #2 std::thread::_M_start_thread(std::shared_ptr, void (*)()) libstdc++-v3/src/c++11/thread.cc:149 (libstdc++.so.6+0x0000000b6e52) #3 __libc_start_main (libc.so.6+0x000000021ec4) OK for trunk? Index: include/bits/basic_string.h =================================================================== --- include/bits/basic_string.h (revision 227363) +++ include/bits/basic_string.h (working copy) @@ -2601,11 +2601,11 @@ bool _M_is_leaked() const _GLIBCXX_NOEXCEPT - { return this->_M_refcount < 0; } + { return __gnu_cxx::__atomic_load_dispatch(&this->_M_refcount) < 0; } bool _M_is_shared() const _GLIBCXX_NOEXCEPT - { return this->_M_refcount > 0; } + { return __gnu_cxx::__atomic_load_dispatch(&this->_M_refcount) > 0; } void _M_set_leaked() _GLIBCXX_NOEXCEPT Index: include/ext/atomicity.h =================================================================== --- include/ext/atomicity.h (revision 227363) +++ include/ext/atomicity.h (working copy) @@ -35,6 +35,16 @@ #include #include +// Even if the CPU doesn't need a memory barrier, we need to ensure +// that the compiler doesn't reorder memory accesses across the +// barriers. +#ifndef _GLIBCXX_READ_MEM_BARRIER +#define _GLIBCXX_READ_MEM_BARRIER __atomic_thread_fence (__ATOMIC_ACQUIRE) +#endif +#ifndef _GLIBCXX_WRITE_MEM_BARRIER +#define _GLIBCXX_WRITE_MEM_BARRIER __atomic_thread_fence (__ATOMIC_RELEASE) +#endif + namespace __gnu_cxx _GLIBCXX_VISIBILITY(default) { _GLIBCXX_BEGIN_NAMESPACE_VERSION @@ -50,7 +60,7 @@ static inline void __atomic_add(volatile _Atomic_word* __mem, int __val) - { __atomic_fetch_add(__mem, __val, __ATOMIC_ACQ_REL); } + { __atomic_fetch_add(__mem, __val, __ATOMIC_RELEASE); } #else _Atomic_word __attribute__ ((__unused__)) @@ -101,17 +111,27 @@ #endif } + static inline _Atomic_word + __attribute__ ((__unused__)) + __atomic_load_dispatch(const _Atomic_word* __mem) + { +#ifdef __GTHREADS + if (__gthread_active_p()) + { +#ifdef _GLIBCXX_ATOMIC_BUILTINS + return __atomic_load_n(__mem, __ATOMIC_ACQUIRE); +#else + // The best we can get with an old compiler. + _Atomic_word v = *(volatile _Atomic_word*)__mem; + _GLIBCXX_READ_MEM_BARRIER; + return v; +#endif + } +#endif + return *__mem; + } + _GLIBCXX_END_NAMESPACE_VERSION } // namespace -// Even if the CPU doesn't need a memory barrier, we need to ensure -// that the compiler doesn't reorder memory accesses across the -// barriers. -#ifndef _GLIBCXX_READ_MEM_BARRIER -#define _GLIBCXX_READ_MEM_BARRIER __atomic_thread_fence (__ATOMIC_ACQUIRE) -#endif -#ifndef _GLIBCXX_WRITE_MEM_BARRIER -#define _GLIBCXX_WRITE_MEM_BARRIER __atomic_thread_fence (__ATOMIC_RELEASE) -#endif - #endif