libstdc++ / mt_allocator.cc when using gthreads

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

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.

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.
>

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.

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.
>