Add ALIGNAS and ALIGNOF macros to ensure proper alignment of StaticMemorySingletonTraits

base/memory/singleton.h

Index: base/memory/singleton.h
diff --git a/base/memory/singleton.h b/base/memory/singleton.h
index d76070019435323869c9b2d13e6baa2f982e271e..ba05e088baa6e021785a95e9df008ef2043269a6 100644
--- a/base/memory/singleton.h
+++ b/base/memory/singleton.h
@@ -108,11 +108,15 @@ struct StaticMemorySingletonTraits {
   static Type* New() {
     if (base::subtle::NoBarrier_AtomicExchange(&dead_, 1))
       return NULL;
-    Type* ptr = reinterpret_cast<Type*>(buffer_);
+
+    // Calculate aligned address for object pointer. This rounds up to the
+    // nearest address whose alignment is ALIGNOF(Type).
+    intptr_t aligned_addr =
+        (reinterpret_cast<intptr_t>(buffer_) + ALIGNOF(Type) - 1) &
+        ~(ALIGNOF(Type) - 1);
 
     // We are protected by a memory barrier.

[Jeffrey Yasskin (google), 2012/02/15 22:27:19]

Unrelated to the current CL, but, really? I see a pretty explicit "NoBarrier" up
there, which can bite you on non-x86 platforms.

[jbates, 2012/02/15 22:51:33]

Hmm. I just assumed this code was already thread safe, but looking at it again,
it appears that the atomic code here doesn't protect from a race between New()
and Delete(). Perhaps New() and Delete() are always called sequentially on the
same thread. Is that the case?

-    new(ptr) Type();
-    return ptr;
+    return new(reinterpret_cast<void*>(aligned_addr)) Type();
   }
 
   static void Delete(Type* p) {
@@ -131,15 +135,14 @@ struct StaticMemorySingletonTraits {
   }
 
  private:
-  static const size_t kBufferSize = (sizeof(Type) +
-                                     sizeof(intptr_t) - 1) / sizeof(intptr_t);
-  static intptr_t buffer_[kBufferSize];
+  static const size_t kBufferSize = sizeof(Type) + ALIGNOF(Type) - 1;

[Sigurður Ásgeirsson, 2012/01/13 14:32:18]

This is pretty wasteful, as many existing instances will effectively double in
size. This will increase the binary size, increase cache misses etc.
If there isn't a good way to make the the alignment declaration stick to the
buffer declaration, I'd suggest using a new traits class, and using that for
instances that need non-default alignment. Instead of penalizing everyone, this
will only affect those who need non-default alignment.

[jbates, 2012/01/13 17:29:18]

How so? ALIGNOF will typically be 4 or 8 for most singletons.
Sadly ALIGNED(ALIGNOF(x)) only works in GCC. Really lame that MSVC complains
about that.
If we end up going that route, I think we should bump the alignment of this
class to 8 or 16. As is, if someone has uint64 types in their static singleton,
it may not be guaranteed to be aligned in the 32-bit build (it would depend on
the linker's default alignment for global data types, which is where we may be
getting lucky). A worse case would be vector types, which require 16 byte
alignment for the fast instructions.

[Jeffrey Yasskin (google), 2012/02/15 22:27:19]

Does MSVC accept ALIGNED(size_t-template-argument)? Does it allow passing
ALIGNOF(x) as a template argument? If so, you may be able to sneak it in that
way.

[jbates, 2012/02/15 22:51:33]

Yeah, hopefully that will work. I'll give it a try soon.

+  static uint8 buffer_[kBufferSize];
 
   // Signal the object was already deleted, so it is not revived.
   static base::subtle::Atomic32 dead_;
 };
 
-template <typename Type> intptr_t
+template <typename Type> uint8
     StaticMemorySingletonTraits<Type>::buffer_[kBufferSize];
 template <typename Type> base::subtle::Atomic32
     StaticMemorySingletonTraits<Type>::dead_ = 0;