[llvm-commits] [llvm] r158342 - in /llvm/trunk: include/llvm/Support/ThreadLocal.h lib/Support/ThreadLocal.cpp lib/Support/Windows/ThreadLocal.inc

[David Blaikie]

On Mon, Jun 11, 2012 at 6:08 PM, Argyrios Kyrtzidis <akyrtzi at gmail.com> wrote:
> On Jun 11, 2012, at 5:36 PM, Chandler Carruth wrote:
>
> On Mon, Jun 11, 2012 at 5:21 PM, Argyrios Kyrtzidis <akyrtzi at gmail.com>
> wrote:
>>
>> Author: akirtzidis
>> Date: Mon Jun 11 19:21:31 2012
>> New Revision: 158342
>>
>> URL: http://llvm.org/viewvc/llvm-project?rev=158342&view=rev
>> Log:
>> For llvm::sys::ThreadLocalImpl instead of malloc'ing the platform-specific
>> thread local data, embed them in the class using a uint64_t and make sure
>> we get compiler errors if there's a platform where this is not big enough.
>>
>> This makes ThreadLocal more safe for using it in conjunction with
>> CrashRecoveryContext.
>>
>> Related to crash in rdar://11434201.
>>
>> Modified:
>>    llvm/trunk/include/llvm/Support/ThreadLocal.h
>>    llvm/trunk/lib/Support/ThreadLocal.cpp
>>    llvm/trunk/lib/Support/Windows/ThreadLocal.inc
>>
>> Modified: llvm/trunk/include/llvm/Support/ThreadLocal.h
>> URL:
>> http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/Support/ThreadLocal.h?rev=158342&r1=158341&r2=158342&view=diff
>>
>> ==============================================================================
>> --- llvm/trunk/include/llvm/Support/ThreadLocal.h (original)
>> +++ llvm/trunk/include/llvm/Support/ThreadLocal.h Mon Jun 11 19:21:31 2012
>> @@ -15,6 +15,7 @@
>>  #define LLVM_SYSTEM_THREAD_LOCAL_H
>>
>>  #include "llvm/Support/Threading.h"
>> +#include "llvm/Support/DataTypes.h"
>>  #include <cassert>
>>
>>  namespace llvm {
>> @@ -22,7 +23,12 @@
>>     // ThreadLocalImpl - Common base class of all ThreadLocal
>> instantiations.
>>     // YOU SHOULD NEVER USE THIS DIRECTLY.
>>     class ThreadLocalImpl {
>> -      void* data;
>> +      typedef uint64_t ThreadLocalDataTy;
>> +      /// \brief Platform-specific thread local data.
>> +      ///
>> +      /// This is embedded in the class and we avoid malloc'ing/free'ing
>> it,
>> +      /// to make this class more safe for use along with
>> CrashRecoveryContext.
>> +      ThreadLocalDataTy data;
>
>
> If you're embedding arbitrary data into this, please use a character array
> to satisfy C++ aliasing rules. Also I would add something to force
> alignment:
>
> union {
>   char data[sizeof(uint64_t)];
>   struct {
>     uint64_t dummy_alignment_member;
>   }

This uses a GNU extension (anonymous structs), causing Clang to warn.
It didn't seem like that changed the semantics at all (I don't believe
it hides the name from the rest of the class, for instance) so I just
removed the wrapping "struct { }" in r158364

- David

> }
>
>
> In r158346, thanks!
>
>
>
>>
>>     public:
>>       ThreadLocalImpl();
>>       virtual ~ThreadLocalImpl();
>>
>> Modified: llvm/trunk/lib/Support/ThreadLocal.cpp
>> URL:
>> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Support/ThreadLocal.cpp?rev=158342&r1=158341&r2=158342&view=diff
>>
>> ==============================================================================
>> --- llvm/trunk/lib/Support/ThreadLocal.cpp (original)
>> +++ llvm/trunk/lib/Support/ThreadLocal.cpp Mon Jun 11 19:21:31 2012
>> @@ -41,30 +41,29 @@
>>  using namespace sys;
>>
>>  ThreadLocalImpl::ThreadLocalImpl() : data(0) {
>> -  pthread_key_t* key = new pthread_key_t;
>> +  typedef int SIZE_TOO_BIG[sizeof(pthread_key_t) <= sizeof(data) ? 1 :
>> -1];
>> +  pthread_key_t* key = reinterpret_cast<pthread_key_t*>(&data);
>>   int errorcode = pthread_key_create(key, NULL);
>>   assert(errorcode == 0);
>>   (void) errorcode;
>> -  data = (void*)key;
>>  }
>>
>>  ThreadLocalImpl::~ThreadLocalImpl() {
>> -  pthread_key_t* key = static_cast<pthread_key_t*>(data);
>> +  pthread_key_t* key = reinterpret_cast<pthread_key_t*>(&data);
>>   int errorcode = pthread_key_delete(*key);
>>   assert(errorcode == 0);
>>   (void) errorcode;
>> -  delete key;
>>  }
>>
>>  void ThreadLocalImpl::setInstance(const void* d) {
>> -  pthread_key_t* key = static_cast<pthread_key_t*>(data);
>> +  pthread_key_t* key = reinterpret_cast<pthread_key_t*>(&data);
>>   int errorcode = pthread_setspecific(*key, d);
>>   assert(errorcode == 0);
>>   (void) errorcode;
>>  }
>>
>>  const void* ThreadLocalImpl::getInstance() {
>> -  pthread_key_t* key = static_cast<pthread_key_t*>(data);
>> +  pthread_key_t* key = reinterpret_cast<pthread_key_t*>(&data);
>>   return pthread_getspecific(*key);
>>  }
>>
>>
>> Modified: llvm/trunk/lib/Support/Windows/ThreadLocal.inc
>> URL:
>> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Support/Windows/ThreadLocal.inc?rev=158342&r1=158341&r2=158342&view=diff
>>
>> ==============================================================================
>> --- llvm/trunk/lib/Support/Windows/ThreadLocal.inc (original)
>> +++ llvm/trunk/lib/Support/Windows/ThreadLocal.inc Mon Jun 11 19:21:31
>> 2012
>> @@ -22,26 +22,25 @@
>>  namespace llvm {
>>  using namespace sys;
>>
>> -ThreadLocalImpl::ThreadLocalImpl() {
>> -  DWORD* tls = new DWORD;
>> +ThreadLocalImpl::ThreadLocalImpl() : data(0) {
>> +  typedef int SIZE_TOO_BIG[sizeof(DWORD) <= sizeof(data) ? 1 : -1];
>> +  DWORD* tls = reinterpret_cast<DWORD*>(&data);
>>   *tls = TlsAlloc();
>>   assert(*tls != TLS_OUT_OF_INDEXES);
>> -  data = tls;
>>  }
>>
>>  ThreadLocalImpl::~ThreadLocalImpl() {
>> -  DWORD* tls = static_cast<DWORD*>(data);
>> +  DWORD* tls = reinterpret_cast<DWORD*>(&data);
>>   TlsFree(*tls);
>> -  delete tls;
>>  }
>>
>>  const void* ThreadLocalImpl::getInstance() {
>> -  DWORD* tls = static_cast<DWORD*>(data);
>> +  DWORD* tls = reinterpret_cast<DWORD*>(&data);
>>   return TlsGetValue(*tls);
>>  }
>>
>>  void ThreadLocalImpl::setInstance(const void* d){
>> -  DWORD* tls = static_cast<DWORD*>(data);
>> +  DWORD* tls = reinterpret_cast<DWORD*>(&data);
>>   int errorcode = TlsSetValue(*tls, const_cast<void*>(d));
>>   assert(errorcode != 0);
>>   (void)errorcode;
>>
>>
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>
>
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>