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

[Chandler Carruth]

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;
  }
}



>     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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20120611/4faf2db3/attachment.html>