[PATCH] libstdc++: Make it possible to annotate the shared pointer operations in the std::thread implementation

[Bart Van Assche]

As documented in the libstdc++ manual, the shared pointer operations in
libstdc++ headers can be instrumented by defining the macros
_GLIBCXX_SYNCHRONIZATION_HAPPENS_BEFORE()/AFTER() and libstdc++ has to be
rebuilt in order to instrument the remaining shared pointer operations.
However, rebuilding libstdc++ is inconvenient. So let's move the thread
wrapper code from thread.cc into .

See also:
* http://gcc.gnu.org/onlinedocs/libstdc++/manual/debug.html.
* http://gcc.gnu.org/bugzilla/show_bug.cgi?id=51504.

Signed-off-by: Bart Van Assche 

Index: libstdc++-v3/src/thread.cc
===
--- libstdc++-v3/src/thread.cc  (revision 182271)
+++ libstdc++-v3/src/thread.cc  (working copy)
@@ -59,28 +59,6 @@ static inline int get_nprocs()
 
 namespace std _GLIBCXX_VISIBILITY(default)
 {
-  namespace
-  {
-extern "C" void*
-execute_native_thread_routine(void* __p)
-{
-  thread::_Impl_base* __t = static_cast(__p);
-  thread::__shared_base_type __local;
-  __local.swap(__t->_M_this_ptr);
-
-  __try
-   {
- __t->_M_run();
-   }
-  __catch(...)
-   {
- std::terminate();
-   }
-
-  return 0;
-}
-  }
-
 _GLIBCXX_BEGIN_NAMESPACE_VERSION
 
   void
@@ -114,12 +92,17 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
   void
   thread::_M_start_thread(__shared_base_type __b)
   {
+  _M_start_thread(__b, &_M_entry);
+  }
+
+  void
+  thread::_M_start_thread(__shared_base_type __b, void* (*__pf)(void*))
+  {
 if (!__gthread_active_p())
   __throw_system_error(int(errc::operation_not_permitted));
 
 __b->_M_this_ptr = __b;
-int __e = __gthread_create(&_M_id._M_thread,
-  &execute_native_thread_routine, __b.get());
+int __e = __gthread_create(&_M_id._M_thread, __pf, __b.get());
 if (__e)
 {
   __b->_M_this_ptr.reset();
Index: libstdc++-v3/include/std/thread
===
--- libstdc++-v3/include/std/thread (revision 182271)
+++ libstdc++-v3/include/std/thread (working copy)
@@ -132,7 +132,8 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
   {
 _M_start_thread(_M_make_routine(std::__bind_simple(
 std::forward<_Callable>(__f),
-std::forward<_Args>(__args)...)));
+std::forward<_Args>(__args)...)),
+&thread::_M_entry);
   }
 
 ~thread()
@@ -180,9 +181,30 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
 hardware_concurrency() noexcept;
 
   private:
+static void* _M_entry(void* __p)
+{
+  thread::_Impl_base* __t = static_cast(__p);
+  thread::__shared_base_type __local;
+  __local.swap(__t->_M_this_ptr);
+  
+  __try
+{
+  __t->_M_run();
+}
+  __catch(...)
+{
+  std::terminate();
+}
+  
+  return 0;
+}
+
 void
 _M_start_thread(__shared_base_type);
 
+void
+_M_start_thread(__shared_base_type, void* (*)(void*));
+
 template
   shared_ptr<_Impl<_Callable>>
   _M_make_routine(_Callable&& __f)
Index: libstdc++-v3/config/abi/post/s390-linux-gnu/baseline_symbols.txt
===
--- libstdc++-v3/config/abi/post/s390-linux-gnu/baseline_symbols.txt
(revision 182271)
+++ libstdc++-v3/config/abi/post/s390-linux-gnu/baseline_symbols.txt
(working copy)
@@ -2145,6 +2145,7 @@ FUNC:_ZNSt6localeD1Ev@@GLIBCXX_3.4
 FUNC:_ZNSt6localeD2Ev@@GLIBCXX_3.4
 FUNC:_ZNSt6localeaSERKS_@@GLIBCXX_3.4
 
FUNC:_ZNSt6thread15_M_start_threadESt10shared_ptrINS_10_Impl_baseEE@@GLIBCXX_3.4.11
+FUNC:_ZNSt6thread15_M_start_threadESt10shared_ptrINS_10_Impl_baseEEPFPvS3_E@@GLIBCXX_3.4.17
 FUNC:_ZNSt6thread4joinEv@@GLIBCXX_3.4.11
 FUNC:_ZNSt6thread6detachEv@@GLIBCXX_3.4.11
 FUNC:_ZNSt7codecvtIcc11__mbstate_tEC1EP15__locale_structm@@GLIBCXX_3.4
Index: libstdc++-v3/config/abi/post/x86_64-linux-gnu/baseline_symbols.txt
===
--- libstdc++-v3/config/abi/post/x86_64-linux-gnu/baseline_symbols.txt  
(revision 182271)
+++ libstdc++-v3/config/abi/post/x86_64-linux-gnu/baseline_symbols.txt  
(working copy)
@@ -1955,6 +1955,7 @@ FUNC:_ZNSt6localeD1Ev@@GLIBCXX_3.4
 FUNC:_ZNSt6localeD2Ev@@GLIBCXX_3.4
 FUNC:_ZNSt6localeaSERKS_@@GLIBCXX_3.4
 
FUNC:_ZNSt6thread15_M_start_threadESt10shared_ptrINS_10_Impl_baseEE@@GLIBCXX_3.4.11
+FUNC:_ZNSt6thread15_M_start_threadESt10shared_ptrINS_10_Impl_baseEEPFPvS3_E@@GLIBCXX_3.4.17
 FUNC:_ZNSt6thread4joinEv@@GLIBCXX_3.4.11
 FUNC:_ZNSt6thread6detachEv@@GLIBCXX_3.4.11
 FUNC:_ZNSt7codecvtIcc11__mbstate_tEC1EP15__locale_structm@@GLIBCXX_3.4
Index: libstdc++-v3/config/abi/post/ia64-linux-gnu/baseline_symbols.txt
===
--- libstdc++-v3/config/abi/post/ia64-linux-gnu

Re: [PATCH] libstdc++: Make it possible to annotate the shared pointer operations in the std::thread implementation

[Bart Van Assche]

On Fri, Dec 23, 2011 at 5:04 PM, Paolo Carlini  wrote:
> First, do you have already a Copyright assignment on file? It's a 
> precondition for any non trivial contribution.

I've just started the procedure to get a copyright assignment form filed.

> That said, please leave alone the baselines. Otherwise, Jon can comment on 
> whether the reshuffling makes
> sense and would be safe from the Abi point of view.

I'll repost the patch on the libstdc++ mailing list as Jon asked.

Bart.

Re: [PATCH] Make it possible to annotate the shared pointer operations in the std::thread implementation

[Bart Van Assche]

On Sat, Dec 24, 2011 at 11:15 AM, Jonathan Wakely  wrote:
> On 24 December 2011 11:11, Jonathan Wakely wrote:
>> Apart from that, I prefer to keep the thread-launching logic in the
>> library rather than headers, as it allows us to change it more easily
>> and for users to benefit just by linking to a newer library rather
>> than recompiling. I don't think the data race detection macros are a
>> compelling reason to change that.
>
> An example improvement (which I'm working on) is to give better stack
> traces when a process is terminated by an uncaught exception in a
> function run by std::thread. If that's improved in the library, all
> existing code using std::thread would get better stack traces just by
> linking to a newer libstdc++.so, rather than having to recompile with
> a newer GCC.

Would it be acceptable to replace the
_GLIBCXX_SYNCHRONIZATION_HAPPENS_BEFORE() and ..._AFTER() macros by
something like this:

  if (pf) (*pf)(addr, before_or_after_flag)

where pf is a function pointer that is NULL by default and can be set
by libstdc++ users. That approach would also avoid that libstdc++ has
to be recompiled in order to instrument reference count decrementing
for data race detectors.

Bart.

Re: [PATCH] Make it possible to annotate the shared pointer operations in the std::thread implementation

[Bart Van Assche]

On Sat, Dec 24, 2011 at 12:05 PM, Jonathan Wakely  wrote:
> On 24 December 2011 11:46, Bart Van Assche wrote:
>> Would it be acceptable to replace the
>> _GLIBCXX_SYNCHRONIZATION_HAPPENS_BEFORE() and ..._AFTER() macros by
>> something like this:
>>
>>  if (pf) (*pf)(addr, before_or_after_flag)
>>
>> where pf is a function pointer that is NULL by default and can be set
>> by libstdc++ users. That approach would also avoid that libstdc++ has
>> to be recompiled in order to instrument reference count decrementing
>> for data race detectors.
>
> I think the cost of checking that on every ref-count update would be
> too high for the vast majority of users who don't use race detectors.
> A macro has zero cost if it's not used, and even people using race
> detectors don't want them enabled all the time. As I said, you don't
> need to recompile the whole library to instrument the parts that are
> affected, only specific objects.  I think the right thing to do is
> identify the parts that need to be recompiled and document how to do
> just that.

Would it be technically possible to implement a binary patching
approach in a user space shared library similar to the binary patching
technique used in the kprobe implementation in the Linux kernel ?

For more information, see also
http://git.kernel.org/?p=linux/kernel/git/next/linux-next.git;a=blob;f=Documentation/kprobes.txt.

Bart.

Bart.