Patchwork libstdc++ / mt_allocator.cc when using gthreads

login
register
mail settings
Submitter Laurent Alfonsi
Date April 19, 2012, 1:17 p.m.
Message ID <4F90107D.6010200@st.com>
Download mbox | patch
Permalink /patch/153751/
State New
Headers show

Comments

Laurent Alfonsi - April 19, 2012, 1:17 p.m.
All,

The enclosed testcases (very close to 
ext/mt_allocator/deallocate_global_thread-1.cc) exposes a pattern where 
the following sequence is called (when __gthread_active_p is enabled) :

     __gthread_key_create(&key,::_M_destroy_thread_key);
     __gthread_setspecific(key, (void*)_M_id);
     __gthread_key_delete(key);
     __gthread_setspecific(key, (void*)_M_id);


Even, if this works perfectly using pthreads, it seems to me that this 
is an illegal use of threads. When using another thread implementation, 
(i.e. when the key is implemented as a pointer). In my case:
  - the key_create allocates the memory, and returns the pointer to the 
allocated memory
  - the key_delete frees this memory,
  - the setspecific uses this freed memory.

If I m right saying this is an illegal use of threads, a possible quick 
patch for mt_allocator.cc could be the one enclosed.

Let me know what you think of this approach.

Thanks.
Laurent
// 20.4.1.1 allocator members

#include <list>
#include <string>
#include <stdexcept>
#include <cstdio>
#include <ext/mt_allocator.h>

#include <pthread.h>

typedef std::string value_type;
using __gnu_cxx::__pool;
using __gnu_cxx::__common_pool_policy;
typedef __common_pool_policy<__pool, true> policy_type;
typedef __gnu_cxx::__mt_alloc<value_type, policy_type> allocator_type;
typedef std::char_traits<value_type> traits_type;
typedef std::list<value_type, allocator_type> list_type;

list_type l;


void *my_thread_process (void * arg)
{
  printf ("Thread %s\n", (char*)arg);
  pthread_exit (0);
}


int main()
{
  pthread_t th1;
  if (pthread_create (&th1, NULL, my_thread_process, (void*)"1") < 0) {
    fprintf (stderr, "pthread_create error for thread 1\n");
    exit (1);
  }

  l.push_back("bayou bend");

  return 0;
}
2012-04-19  Laurent Alfonsi  <laurent.alfonsi@st.com>

    * src/c++98/mt_allocator.cc: Define/use an illegal thread key
    (struct __freelist, __freelist::~__freelist()): set to illegal key
    (pool<true>::_M_get_thread_id()): Check _M_key before using it
Paolo Carlini - April 19, 2012, 2:26 p.m.
Hi,
>     All,
>
> The enclosed testcases (very close to 
> ext/mt_allocator/deallocate_global_thread-1.cc) exposes a pattern 
> where the following sequence is called (when __gthread_active_p is 
> enabled) :
>
>     __gthread_key_create(&key,::_M_destroy_thread_key);
>     __gthread_setspecific(key, (void*)_M_id);
>     __gthread_key_delete(key);
>     __gthread_setspecific(key, (void*)_M_id);
I didn't look into the issue in any detail - and I'm really glad that 
you are interested in improving mt_allocator - but I think that before 
going for quick patches, we should strive for fixing the underlying 
issue, eg prevent the above from occurring. After all, if you are right, 
the issue is very, very old, and nobody complained yet, thus better try 
very hard to fix it for real, finally. Do you have any plans for that?

Thanks,
Paolo.
Laurent Alfonsi - April 19, 2012, 3:02 p.m.
Well, I don't know mt_allocator enough to know if this is a fix for real 
or a quick fix.
Regards,
Laurent

On 04/19/12 16:26, Paolo Carlini wrote:
> Hi,
>>      All,
>>
>> The enclosed testcases (very close to
>> ext/mt_allocator/deallocate_global_thread-1.cc) exposes a pattern
>> where the following sequence is called (when __gthread_active_p is
>> enabled) :
>>
>>      __gthread_key_create(&key,::_M_destroy_thread_key);
>>      __gthread_setspecific(key, (void*)_M_id);
>>      __gthread_key_delete(key);
>>      __gthread_setspecific(key, (void*)_M_id);
> I didn't look into the issue in any detail - and I'm really glad that
> you are interested in improving mt_allocator - but I think that before
> going for quick patches, we should strive for fixing the underlying
> issue, eg prevent the above from occurring. After all, if you are right,
> the issue is very, very old, and nobody complained yet, thus better try
> very hard to fix it for real, finally. Do you have any plans for that?
>
> Thanks,
> Paolo.
>
Paolo Carlini - April 19, 2012, 3:52 p.m.
On 04/19/2012 05:02 PM, Laurent Alfonsi wrote:
> Well, I don't know mt_allocator enough to know if this is a fix for 
> real or a quick fix.
I'm pretty sure we could do better. Whether in practice we are going to 
do better, any time soon, it's another matter ;)

Paolo.
Laurent Alfonsi - April 20, 2012, 3:21 p.m.
Thanks very much Paolo.
I'll apply this patch on my side for a while.
I ll tell you if i see anything strange.
Regards,
Laurent

On 04/19/12 17:52, Paolo Carlini wrote:
> On 04/19/2012 05:02 PM, Laurent Alfonsi wrote:
>> Well, I don't know mt_allocator enough to know if this is a fix for
>> real or a quick fix.
> I'm pretty sure we could do better. Whether in practice we are going to
> do better, any time soon, it's another matter ;)
>
> Paolo.
>

Patch

Index: src/c++98/mt_allocator.cc
===================================================================
--- src/c++98/mt_allocator.cc.orig
+++ src/c++98/mt_allocator.cc
@@ -34,19 +34,22 @@ 
 namespace
 {
 #ifdef __GTHREADS
+  const __gthread_key_t ILLEGAL_THREAD_KEY = -1;
+
   struct __freelist
   {
     typedef __gnu_cxx::__pool<true>::_Thread_record _Thread_record;
     _Thread_record* 	_M_thread_freelist;
     _Thread_record* 	_M_thread_freelist_array;
     size_t 		_M_max_threads;
-    __gthread_key_t 	_M_key;
+    __gthread_key_t 	_M_key = ILLEGAL_THREAD_KEY;
 
     ~__freelist()
     {
       if (_M_thread_freelist_array)
 	{
 	  __gthread_key_delete(_M_key);
+	  _M_key = ILLEGAL_THREAD_KEY;
 	  ::operator delete(static_cast<void*>(_M_thread_freelist_array));
 	  _M_thread_freelist = 0;
 	}
@@ -640,7 +643,8 @@ 
 		}
 	    }
 
-	     __gthread_setspecific(freelist._M_key, (void*)_M_id);
+	    if (freelist._M_key!=ILLEGAL_THREAD_KEY)
+	      __gthread_setspecific(freelist._M_key, (void*)_M_id);
 	  }
 	return _M_id >= _M_options._M_max_threads ? 0 : _M_id;
       }