Upgrade AlignedMemory to support dynamic allocations.

base/memory/aligned_memory.h

Index: base/memory/aligned_memory.h
diff --git a/base/memory/aligned_memory.h b/base/memory/aligned_memory.h
index 0a176347a6dd47768c20c9585870fcd9cef417f1..2cb2929c0eb807eb883c6145be600290bac800e2 100644
--- a/base/memory/aligned_memory.h
+++ b/base/memory/aligned_memory.h
@@ -8,7 +8,7 @@
 // is constructed and destructed (you don't want static initialization and
 // destruction), use AlignedMemory:
 //
-//   static AlignedMemory<sizeof(MyClass), ALIGNOF(MyClass)> my_class;
+//   static AlignedMemory<ALIGNOF(MyClass), sizeof(MyClass)> my_class;
 //
 //   // ... at runtime:
 //   new(my_class.void_data()) MyClass();
@@ -18,26 +18,67 @@
 //
 //   // ... later, to destruct my_class:
 //   my_class.data_as<MyClass>()->MyClass::~MyClass();
+//
+// Alternatively, a runtime sized AlignedMemory instance can be created:
+//
+//   AlignedMemory<|alignment|> my_memory(|size|);
+//
+//   // ... use it:
+//   float* float_array = my_memory.data_as<float>();
+//
+// Memory will be freed when AlignedMemory is destructed.
 
 #ifndef BASE_MEMORY_ALIGNED_MEMORY_H_
 #define BASE_MEMORY_ALIGNED_MEMORY_H_
 
+#if !defined(_XOPEN_SOURCE) || (_XOPEN_SOURCE < 600)
+#define _XOPEN_SOURCE 600  // for posix_memalign

[Jeffrey Yasskin, 2012/07/20 16:41:42]

If _XOPEN_SOURCE was already defined as <600, this will cause a duplicate
definition warning/error.

[DaleCurtis, 2012/07/20 18:25:28]

Well if we keep the extern below this isn't really necessary anyways, so I can
remove it. Alternatively, do you have a suggested improvement?

[Jeffrey Yasskin, 2012/07/20 23:36:56]

Make sure linux and mac set _XOPEN_SOURCE >=600 or _POSIX_C_SOURCE >= 200112L in
the build system, and then remove it from here, I think.

[DaleCurtis, 2012/07/21 03:04:40]

I'm wary of setting this all over the code base and things seem to compile fine
without it, so removed?

+#endif
+
+#if defined(COMPILER_MSVC) || defined(OS_ANDROID)
+#include <malloc.h>
+#endif
+#include <stdlib.h>
+
 #include "base/basictypes.h"
 #include "base/compiler_specific.h"
-#include "base/logging.h"
+#include "build/build_config.h"
+
+#if defined(COMPILER_MSVC)
+#define INTERNAL_ALIGNED_MALLOC(ptr, align, size) \
+    ptr = _aligned_malloc(size, align)
+#define INTERNAL_ALIGNED_FREE(ptr) \
+    _aligned_free(ptr)
+#else
+#if defined(OS_ANDROID)
+#define INTERNAL_ALIGNED_MALLOC(ptr, align, size) \
+    ptr = memalign(align, size)

[Jeffrey Yasskin, 2012/07/20 16:41:42]

http://linux.die.net/man/3/memalign says, "Some systems provide no way to
reclaim memory allocated with memalign() or valloc() (because one can only pass
to free(3) a pointer gotten from malloc(3), while, for example, memalign() would
call malloc(3) and then align the obtained value)." Could you link to the
android documentation saying this is safe?

[DaleCurtis, 2012/07/20 18:25:28]

Sadly, NDK documentation is incredibly sparse. I tried this on a hunch to get
the android trybot to pass, but have no documentation to back it up and that bot
doesn't run the unit tests (or valgrind for that matter). Do we have any Android
experts in the house that could answer? I downloaded the NDK and grep'd around
without finding anything useful. (no online source search makes me sad :( 

I'm happy turning this into a NOTREACHED() or something similar for now.

[DaleCurtis, 2012/07/20 20:33:37]

Actually, NOTREACHED is probably a bad idea, we could hack up a memalign for
Android if it's unavailable:

#define INTERNAL_ALIGNED_MALLOC(real_ptr, ptr, align, size) do { \
      ptr = real_ptr = malloc(size + (align - 1)); \
      ptr += align - (reinterpret_cast<uintptr_t>(real_ptr) & (align - 1)); \
    } while (0)
#define INTERNAL_ALIGNED_FREE(real_ptr, ptr) \
    free(real_ptr)

[Jeffrey Yasskin, 2012/07/20 23:36:56]

Yeah, NOTREACHED would be bad. Your option is an ok last resort, but you need to
check with an Android person first to see whether they include posix_memalign or
support free(memalign(...)). Will mentions joth@. Bionic claims to use dlmalloc,
which does include posix_memalign: ftp://g.oswego.edu/pub/misc/malloc.c.

[DaleCurtis, 2012/07/21 03:04:40]

Per
https://github.com/android/platform_bionic/commit/85aad909560508410101c18c6ec...
aligned_malloc() supports free on Android. While it claims posix_memalign is
available, the bots can't seem to find it.

+#else
+// TODO(dalecurtis): Not sure why this is necessary, but without it none of the
+// Buildbots can find the posix_memalign definition.  Everything builds fine

[Jeffrey Yasskin, 2012/07/20 16:41:42]

What error do they give? It's very weird that <stdlib.h> wouldn't be enough and
may indicate something else going on.

[DaleCurtis, 2012/07/20 18:25:28]

"error: ‘posix_memalign’ was not declared in this scope" -- same message on all
posix platforms, Windows and Android happily find their respective versions.

http://build.chromium.org/p/tryserver.chromium/builders/linux_clang/builds/35...
http://build.chromium.org/p/tryserver.chromium/builders/mac_rel/builds/40630/...

[Jeffrey Yasskin, 2012/07/20 23:36:56]

Definitely figure out what's going wrong with the trybots rather than declaring
a standard library function yourself.

[DaleCurtis, 2012/07/21 03:04:40]

Turns out this is a NaCl issue,
http://code.google.com/p/nativeclient/issues/detail?id=2505

+jvoung who filed the issue there.

[jvoung (off chromium), 2012/07/21 04:45:26]

Hmm, so this is while building the nacl-ized version of base, using the newlib
toolchain.

newlib doesn't have that function, but we've ditched the newlib malloc and used
dlmalloc, so the nacl libary does technically have posix_memalign().

You can #ifdef __native_client__ to make this more specific / descriptive (or
maybe there is an OS_NACL).


Before you do this though, unfortunately, there is also a bug in dlmalloc's
posix_memalign() (see comment 3 of the bug).  I've reported that to doug lea,
but I'm not sure when he will roll in a fix, and I haven't patched our version
of newlib yet (and haven't patched any headers).

What DOES work though, is memalign().  It might even be in the newlib header. 
Since you already use memalign() for android, can you do the same for NaCl?

[DaleCurtis, 2012/07/21 18:30:32]

Thanks for the quick response! SGTM. I'll add OS_NACL to the memalign() #if
check in the next patch set after jyasskin makes a pass.

[Jeffrey Yasskin, 2012/07/21 20:38:48]

Thanks for the reply. Note that AFAIK Dale wasn't trying to build this code for
NaCl; he was just trying to build it on the linux and mac trybots. Are the
trybots actually building against NaCl's libc instead of their target's libc?
Is there documentation for NaCl saying that its memalign() returns pointers that
can be passed to free()? I'd like a link to that documentation here.

[DaleCurtis, 2012/07/22 00:45:40]

Looks like the extern definition is still necessary for memalign as well. :(
http://build.chromium.org/p/tryserver.chromium/builders/mac_rel/builds/41602/...

[DaleCurtis, 2012/07/22 01:16:13]

Actually scratch that, I just forgot the header.

[jvoung (off chromium), 2012/07/23 17:06:36]

Hrmmm... just checked the newlib sources.  I don't think it exports memalign
either.

[jvoung (off chromium), 2012/07/23 17:06:36]

My guess is that the trybots are also building against NaCl's libc because of
some of the NaCl pepper proxy.  That might be a prerequisite for running NaCl
integration tests, but I'm not sure (could ask bbudge or dmichael).
I don't think there is actual documentation.  NaCl has two libc choices right
now: (1) dlmalloc + newlib, or (2) glibc.

Under the page you listed glibc (2) is in the clear.

For (1), the dlmalloc source (around line 5248) shows that both memalign() and 
posix_memalign() return memory from an internal_memalign() function.  Since you
can free memory from posix_memalign(), I think you can free memory from
internal_memalign() and thus from memalign() too.

[dmichael (off chromium), 2012/07/23 17:19:28]

We're building some bits of Chrome in base, ipc, and gpu [possibly others] with
the newlib NaCl toolchain now. This is part of the work for the NaCl ppapi
proxy, which is going to transition to using Chrome IPC "soon".

We need to use newlib, because this code goes in to the NaCl IRT, which must use
newlib.

HTH.

[jvoung - send to chromium..., 2012/07/23 17:25:23]

n/m, it's in malloc.h there

+// locally with or without it.
+extern int posix_memalign(void **ptr, size_t align, size_t size);
+#define INTERNAL_ALIGNED_MALLOC(ptr, align, size) do { \
+      if (posix_memalign(&ptr, align, size)) \
+        ptr = NULL; \
+    } while (0)
+#endif
+#define INTERNAL_ALIGNED_FREE(ptr) \
+    free(ptr)
+#endif
 
 namespace base {
 
 // AlignedMemory is specialized for all supported alignments.
 // Make sure we get a compiler error if someone uses an unsupported alignment.
-template <size_t Size, size_t ByteAlignment>
+template <size_t ByteAlignment, size_t Size = 0>
 struct AlignedMemory {};
 
-#define BASE_DECL_ALIGNED_MEMORY(byte_alignment) \
+// Compile time based aligned memory allocator.
+#define BASE_DECL_ALIGNED_MEMORY_STATIC(byte_alignment) \
     template <size_t Size> \
-    class AlignedMemory<Size, byte_alignment> { \
+    class AlignedMemory<byte_alignment, Size> { \
      public: \
-      ALIGNAS(byte_alignment) uint8 data_[Size]; \
       void* void_data() { return reinterpret_cast<void*>(data_); } \
       const void* void_data() const { \
         return reinterpret_cast<const void*>(data_); \
@@ -49,17 +90,61 @@ struct AlignedMemory {};
         return reinterpret_cast<const Type*>(void_data()); \
       } \
      private: \
+      ALIGNAS(byte_alignment) uint8 data_[Size]; \
+      void* operator new(size_t); \
+      void operator delete(void*); \
+    }
+
+// Run time based aligned memory allocator.  Since the alignment allocators may

[Jeffrey Yasskin, 2012/07/20 16:41:42]

Why do you want the posix_memalign wrapper to be named the same as the
non-allocating class? It seems better to use different names. I suspect the
memalign wrapper should be a function returning a scoped_ptr_malloc<T,
AlignedFree> instead of a class.

[DaleCurtis, 2012/07/20 18:25:28]

1. Type safety. scoped_ptr_malloc prevents you from having type safety based on
alignment for methods requiring aligned data. The current method allows you to
do stuff like:

void Vector_Multiply_SSE2(const AlignedMemory<16>& src, ...);

2. Encapsulation. scoped_ptr_malloc<T, AlignedFree> means every aligned memory
instance now knows about AlignedFree, vs simply AlignedMemory<16> input_array_;

3. Simplicity. Having a single AlignedMemory class feels simpler, cleaner than
having AlignedMemory and say DynamicAlignedMemory.

It occurs to me now, that requiring a type parameter might make this even safer
and more readable: e.g. AlignedMemory<float, 16> or AlignedMemory<void, 32>,
etc. This would allow us simplify the templates by removing the void_data() and
data_as() in favor of just T* data(), const T* data(). Existing static instances
would also have a type. WDYT?

[Jeffrey Yasskin, 2012/07/20 23:36:56]

What you have doesn't actually work for this because it needs to accept any
larger alignment, and it should accept inline storage as an argument.

That brings up another question: where's the change that requires this new
feature?
I don't think this is a big deal since the details are still wrapped up in the
AlignedFree type.
Generally putting two different behaviors under the same name doesn't make the
code simpler to use. Here you've hidden the cost of allocating memory, and by
increasing the amount of template specialization you made the code harder to
read for non-experts.
A type parameter makes this slightly less general, which is likely to be a bad
idea, and likely indicates that the dynamic version shouldn't actually return
scoped_ptr_malloc<>. Any typed thing should wrap the raw aligning function.

Another reason to keep AlignedMemory as-is is that C++11 defines
std::aligned_storage with the same meaning as the current AlignedMemory. C11
defines "void* aligned_alloc(alignment, size)" as the dynamically-allocated
version.

[DaleCurtis, 2012/07/21 03:04:40]

After discussing over chat with Jeffrey I came to the conclusion that simply
having AllignedAlloc and AlignedFree functions will be easier and cleaner for
all involved.

+// fail for non-memory related reasons, the code will force a crash immediately
+// via NULL dereference if the calls fail for any reason; maintaining consistent
+// behavior with a normal failed allocation in Chrome.
+#define BASE_DECL_ALIGNED_MEMORY_DYNAMIC(byte_alignment) \
+    template <> \
+    class AlignedMemory<byte_alignment, 0> { \
+     public: \
+      explicit AlignedMemory(size_t size) { \
+        INTERNAL_ALIGNED_MALLOC(data_, byte_alignment, size); \
+        /* If you crashed here, your aligned allocation is incorrect. */ \
+        if (!data_) *reinterpret_cast<uint8*>(data_) = 0; \

[Jeffrey Yasskin, 2012/07/20 16:41:42]

Assigning to a null pointer is undefined behavior and may be optimized away
instead of causing a crash. Use CHECK instead.

[DaleCurtis, 2012/07/20 18:25:28]

jbates was worried about the weight of CHECK/DCHECK here, so I switched to this
method. Is that a valid concern? assert() (like used in scoped_ptr) is another
option.

[Jeffrey Yasskin, 2012/07/20 23:36:56]

Anything other than dereferencing null would be fine with me.

[DaleCurtis, 2012/07/21 03:04:40]

Since this is now in a .cc file, switched to CHECK().

+      } \
+      ~AlignedMemory() { if (data_) INTERNAL_ALIGNED_FREE(data_); } \
+      void* void_data() { return data_; } \
+      const void* void_data() const { \
+        return reinterpret_cast<const void*>(data_); \
+      } \
+      template<typename Type> \
+      Type* data_as() { return reinterpret_cast<Type*>(void_data()); } \
+      template<typename Type> \
+      const Type* data_as() const { \
+        return reinterpret_cast<const Type*>(void_data()); \
+      } \
+     private: \
+      void* data_; \
       void* operator new(size_t); \
       void operator delete(void*); \
+      DISALLOW_COPY_AND_ASSIGN(AlignedMemory); \
     }
 
+#define BASE_DECL_ALIGNED_MEMORY(byte_alignment) \
+    BASE_DECL_ALIGNED_MEMORY_STATIC(byte_alignment); \
+    BASE_DECL_ALIGNED_MEMORY_DYNAMIC(byte_alignment)
+
 // Specialization for all alignments is required because MSVC (as of VS 2008)
 // does not understand ALIGNAS(ALIGNOF(Type)) or ALIGNAS(template_param).
 // Greater than 4096 alignment is not supported by some compilers, so 4096 is
 // the maximum specified here.
-BASE_DECL_ALIGNED_MEMORY(1);
-BASE_DECL_ALIGNED_MEMORY(2);
+BASE_DECL_ALIGNED_MEMORY_STATIC(1);
+BASE_DECL_ALIGNED_MEMORY_STATIC(2);
+// Minimum alignment for dynamic allocations is sizeof(void*) which is 4 on 32
+// bit platforms and 8 on 64 bit.  On NaCl sizeof(void*) is always 4.
+#if defined(ARCH_CPU_64_BITS) && !defined(OS_NACL)
+BASE_DECL_ALIGNED_MEMORY_STATIC(4);
+COMPILE_ASSERT(sizeof(void*) == 8, Minimum_Alignment_Should_Be_8);
+#else
+COMPILE_ASSERT(sizeof(void*) == 4, Minimum_Alignment_Should_Be_4);
 BASE_DECL_ALIGNED_MEMORY(4);
+#endif
 BASE_DECL_ALIGNED_MEMORY(8);
 BASE_DECL_ALIGNED_MEMORY(16);
 BASE_DECL_ALIGNED_MEMORY(32);
@@ -71,6 +156,12 @@ BASE_DECL_ALIGNED_MEMORY(1024);
 BASE_DECL_ALIGNED_MEMORY(2048);
 BASE_DECL_ALIGNED_MEMORY(4096);
 
+#undef INTERNAL_ALIGNED_MALLOC
+#undef INTERNAL_ALIGNED_FREE
+#undef BASE_DECL_ALIGNED_MEMORY_STATIC
+#undef BASE_DECL_ALIGNED_MEMORY_DYNAMIC
+#undef BASE_DECL_ALIGNED_MEMORY
+
 }  // base
 
 #endif  // BASE_MEMORY_ALIGNED_MEMORY_H_